Re: [PATCH 00/20 v2] ovl: narrow regions protected by i_rw_sem

[Amir Goldstein <amir73il@gmail.com>]

On Fri, Jul 11, 2025 at 1:21 AM NeilBrown <neil@brown.name> 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.
>
> Thanks,
> NeilBrown
>
>
>  [PATCH 01/20] ovl: simplify an error path in ovl_copy_up_workdir()
>  [PATCH 02/20] ovl: change ovl_create_index() to take write and dir
>  [PATCH 03/20] ovl: Call ovl_create_temp() without lock held.
>  [PATCH 04/20] ovl: narrow the locked region in ovl_copy_up_workdir()
>  [PATCH 05/20] ovl: narrow locking in ovl_create_upper()
>  [PATCH 06/20] ovl: narrow locking in ovl_clear_empty()
>  [PATCH 07/20] ovl: narrow locking in ovl_create_over_whiteout()
>  [PATCH 08/20] ovl: narrow locking in ovl_rename()
>  [PATCH 09/20] ovl: narrow locking in ovl_cleanup_whiteouts()
>  [PATCH 10/20] ovl: narrow locking in ovl_cleanup_index()
>  [PATCH 11/20] ovl: narrow locking in ovl_workdir_create()
>  [PATCH 12/20] ovl: narrow locking in ovl_indexdir_cleanup()
>  [PATCH 13/20] ovl: narrow locking in ovl_workdir_cleanup_recurse()
>  [PATCH 14/20] ovl: change ovl_workdir_cleanup() to take dir lock as
>  [PATCH 15/20] ovl: narrow locking on ovl_remove_and_whiteout()
>  [PATCH 16/20] ovl: change ovl_cleanup_and_whiteout() to take rename
>  [PATCH 17/20] ovl: narrow locking in ovl_whiteout()
>  [PATCH 18/20] ovl: narrow locking in ovl_check_rename_whiteout()
>  [PATCH 19/20] ovl: change ovl_create_real() to receive dentry parent
>  [PATCH 20/20] ovl: rename ovl_cleanup_unlocked() to ovl_cleanup()