Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly handle lockfiles without writes #1391

Merged
merged 1 commit into from
Oct 17, 2022

Conversation

alexlarsson
Copy link
Contributor

A newly created lock-file is of size zero, which happens for example if you reset the storage state. In this state, lockfile.Modified() always returns true, because it thinks any short read means something changed.

I ran into this with the image store, which was empty (using images from a different store), and it keept re-reading the image store all the time even if nothing changed (i.e. both lock file reads returned zero bytes).

The proper way to handle this seems to be to compare partial reads also.

Signed-off-by: Alexander Larsson alexl@redhat.com

A newly created lock-file is of size zero, which happens for example
if you reset the storage state. In this state, lockfile.Modified()
always returns true, because it thinks any short read means something
changed.

I ran into this with the image store, which was empty (using images
from a different store), and it keept re-reading the image store all
the time even if nothing changed (i.e. both lock file reads returned
zero bytes).

The proper way to handle this seems to be to compare partial reads
also.

Signed-off-by: Alexander Larsson <alexl@redhat.com>
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rhatdan
Copy link
Member

rhatdan commented Oct 17, 2022

LGTM

@rhatdan rhatdan merged commit 54663e7 into containers:main Oct 17, 2022
@mtrmac
Copy link
Collaborator

mtrmac commented Oct 17, 2022

This does look like an improvement, but I can’t see that this is quite correct: createLockerForPath creates an object with a new random lw value that matches nothing, so the first use of a lock is always going to return Modified() == true.

So, callers like layerStore are, AFAICS going to load the state in every case (because they are hard-coded to do so on creation), and then re-read it on the first actual use (via ReloadIfChangedModified).

Without examining all of the codebase in detail I’m not comfortable with just changing this behavior of Modified — but longer term, ultimately, it seems to me that the right model is to read the lw value from the lock before reading the data (= manually inspect all users), and having Modified return false on the first call.

@nalind
Copy link
Member

nalind commented Oct 18, 2022

This appears to be causing test failures at containers/podman#16211.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants