[CC vfs maintainers] On Fri, Jul 11, 2025 at 6:41 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > On Fri, Jul 11, 2025 at 1:21 AM NeilBrown <neil@xxxxxxxxxx> wrote: > > > > This is a revised set of patches following helpful feedback. There are > > now more patches, but they should be a lot easier to review. > > I confirm that this set was "reviewable" :) > > No major comments on my part, mostly petty nits. > > I would prefer to see parent_lock/unlock helpers in vfs for v3, > but if you prefer to keep the prep patches internal to ovl, that's fine too. > In that case I'd prefer to use ovl_parent_lock/unlock, but if that's too > painful, don't bother. > > Thanks, > Amir. > > > > > These patches are all in a git tree at > > https://github.com/neilbrown/linux/commits/pdirops > > though there a lot more patches there too - demonstrating what is to come. > > 0eaa1c629788 ovl: rename ovl_cleanup_unlocked() to ovl_cleanup() > > is the last in the series posted here. > > > > I welcome further review. > > > > Original description: > > > > This series of patches for overlayfs is primarily focussed on preparing > > for some proposed changes to directory locking. In the new scheme we > > will lock individual dentries in a directory rather than the whole > > directory. > > > > ovl currently will sometimes lock a directory on the upper filesystem > > and do a few different things while holding the lock. This is > > incompatible with the new scheme. > > > > This series narrows the region of code protected by the directory lock, > > taking it multiple times when necessary. This theoretically open up the > > possibilty of other changes happening on the upper filesytem between the > > unlock and the lock. To some extent the patches guard against that by > > checking the dentries still have the expect parent after retaking the > > lock. In general, I think ovl would have trouble if upperfs were being > > changed independantly, and I don't think the changes here increase the > > problem in any important way. > > > > I have tested this with fstests, both generic and unionfs tests. I > > wouldn't be surprised if I missed something though, so please review > > carefully. > > > > After this series (with any needed changes) lands I will resubmit my > > change to vfs_rmdir() behaviour to have it drop the lock on error. ovl > > will be much better positioned to handle that change. It will come with > > the new "lookup_and_lock" API that I am proposing. > > Slightly off topic. As I know how much ovl code currently depends on (perhaps even abuses) the directory inode lock beyond its vfs uses (e.g. to synchronize internal ovl dir cache changes) just an idea that came to my head for your followup patches - Consider adding an assertion in WRAP_DIR_ITER() that disallows i_op->no_dir_lock. Not that any of the current users of WRAP_DIR_ITER() are candidates for parallel dir ops (?), but its an easy assertion to add. Thanks, Amir.
