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

Remove locker.Locker from drivers/overlay #1214

Merged
merged 1 commit into from Apr 22, 2022

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Apr 20, 2022

In all call paths, the layerStore owning the driver is expected to be locked, so, so this seems redundant.

See also #1140 (comment) .

@rhatdan
Copy link
Member

rhatdan commented Apr 20, 2022

LGTM
@vrothberg @nalind @giuseppe @saschagrunert PTAL

@mtrmac
Copy link
Collaborator Author

mtrmac commented Apr 20, 2022

There are two other users of locker.Locker in drivers/aufs, driver/devmapper. It might make sense to remove them both as well (and then drop the locker package??).


Actually… c/storage/drivers.New is, in principle, available to external callers, and they would invoke the driver without the protection of layerStore’s lock. Do we need to worry about that?? At least a quick search can’t find any external code doing that…

@mtrmac mtrmac changed the title Remove locker.Locker fromd drivers/overlay Remove locker.Locker from drivers/overlay Apr 20, 2022
In all call paths, the layerStore owning the driver
is expected to be locked, so, so this seems redundant.

See also containers#1140 (comment) .

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@flouthoc
Copy link
Collaborator

flouthoc commented Apr 20, 2022

@mtrmac [I could be wrong] Not a review at all , will try to review this in morning but just a comment if someone reviews this

  • Could this open a race where d.Remove( can still be invoked while d.Put( is being invoked or vice-versa.
  • Or a d.Get( can be performed while d.Put( / d,Remove( is being invoked.

I guess if layerstore is locked we might not have to worry but just leaving a comment here.

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

@mtrmac
Copy link
Collaborator Author

mtrmac commented Apr 21, 2022

  • Could this open a race where d.Remove( can still be invoked while d.Put( is being invoked or vice-versa.
  • Or a d.Get( can be performed while d.Put( / d,Remove( is being invoked.

Sure. The question if there is any path how that could happen. I.e.

  • is there any call stack from c/storage.Store that doesn’t exclusively lock the layerStore lock) — I couldn’t find a path but I could well have overlooked something
  • is it supported/possible to call the driver code without going through c/storage.Store. I wouldn’t know.

@rhatdan
Copy link
Member

rhatdan commented Apr 21, 2022

is it supported/possible to call the driver code without going through c/storage.Store. I wouldn’t know.

I would say no, we don't support this.

@flouthoc
Copy link
Collaborator

ah I see I don't think this is an issue then since if layerstore is locked then there might be no issue for any race.

Copy link
Collaborator

@flouthoc flouthoc 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 rhatdan merged commit 8996869 into containers:main Apr 22, 2022
@mtrmac mtrmac deleted the overlay-locker branch April 22, 2022 14:03
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

4 participants