On Mon, 14 Jul 2025, Amir Goldstein wrote: > [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 a sensible suggestion - thanks. Though removing the need for WRAP_DIR_ITER() would be nice too... Not an easy task for course. Thanks, NeilBrown
