dounmount(): Acquire covered vnode lock after draining mount point references
Needs ReviewPublic
Actions

Authored by olce on Jul 3 2023, 12:53 PM.

Details

Reviewers

mjg
kib
markj
jah

Summary

This is the first step of a series of commits whose goal is to remove the
VV_CROSSLOCK vnode flag and logic and fix a timing-dependent deadlock between
lookup and unmount.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 52397
Build 49288: arc lint + arc unit

Event Timeline

olce created this revision.Jul 3 2023, 12:53 PM

Herald added a subscriber: imp. · View Herald TranscriptJul 3 2023, 12:53 PM

olce requested review of this revision.Jul 3 2023, 12:53 PM

Harbormaster completed remote builds in B52397: Diff 124117.Jul 3 2023, 12:53 PM

This is exactly the same patch as in https://reviews.freebsd.org/D40600, but only for vfs_mount.c.

olce added a child revision: D40848: VFS lookup: compute_lk_cnflags(): Remove unused argument 'cnflags'; Rename.Jul 3 2023, 1:03 PM

olce mentioned this in D40719: Generic code.Jul 3 2023, 1:37 PM

olce mentioned this in D40720: Remove VV_CROSSLOCK.

olce removed a child revision: D40848: VFS lookup: compute_lk_cnflags(): Remove unused argument 'cnflags'; Rename.Jul 7 2023, 6:58 AM

olce added a child revision: D40850: VFS lookup: New vn_cross_single_mount() and vn_cross_mounts().

emaste added a subscriber: emaste.Jul 17 2023, 2:23 PM

Submitted by: jah@ (Jason Harmening)

This change ought to have @jah as the author and you in a Co-authored-by: trailer (or vice-versa, depending on who is primarily responsible). Submitted by is deprecated.

In D40847#934799, @emaste wrote:

Submitted by: jah@ (Jason Harmening)

This change ought to have @jah as the author and you in a Co-authored-by: trailer (or vice-versa, depending on who is primarily responsible). Submitted by is deprecated.

Sure. It's just that I took only part of the original revision (D40600) from @jah to build this series, and I didn't think I could set the author of a revision I created to someone else, but it seems possible, going to try. All code here is from @jah.

olce foisted this revision upon jah.Jul 17 2023, 4:14 PM

olce edited reviewers, added: olce; removed: jah.

olce edited the summary of this revision. (Show Details)

olce added a reviewer: olce.

At the same time, I'm "responsible" for this series, so maybe it's best that I keep the revision. I could add Co-authored-by, but then really I just made suggestions in D46000 and didn't actually provide code (or very little). Maybe the simplest way is rather to fix the author at commit time by setting the git author to @jah.

In D40847#934819, @olce.freebsd_certner.fr wrote:

At the same time, I'm "responsible" for this series, so maybe it's best that I keep the revision. I could add Co-authored-by, but then really I just made suggestions in D46000 and didn't actually provide code (or very little). Maybe the simplest way is rather to fix the author at commit time by setting the git author to @jah.

Sorry I wasn't clear, I meant specifically the Git Author data. Indeed, Phabricator doesn't handle the concept of uploading a review on someone else's behalf that well. I think you should keep this revision.

(FWIW I have a review open in D41060 to add Co-authored-by to the committer's guide)

jah mentioned this in D40850: VFS lookup: New vn_cross_single_mount() and vn_cross_mounts().Feb 12 2024, 12:32 AM