Re: [PATCH] ovl: take lower dir inode mutex outside upper sb_writers lock

[Miklos Szeredi <miklos@szeredi.hu>]

On Wed, Nov 22, 2017 at 6:46 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Nov 10, 2017 at 1:18 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> The functions ovl_lower_positive() and ovl_check_empty_dir() both take
>> inode mutex on the real lower dir under ovl_want_write() which takes
>> the upper_mnt sb_writers lock.
>>
>> While this is not a clear locking order or layering violation, it creates
>> an undesired lock dependency between two unrelated layers for no good
>> reason.
>>
>> This lock dependency materializes to a false(?) positive lockdep warning
>> when calling rmdir() on a nested overlayfs, where both nested and
>> underlying overlayfs both use the same fs type as upper layer.
>>
>> rmdir() on the nested overlayfs creates the lock chain:
>>   sb_writers of upper_mnt (e.g. tmpfs) in ovl_do_remove()
>>   ovl_i_mutex_dir_key[] of lower overlay dir in ovl_lower_positive()
>>
>> rmdir() on the underlying overlayfs creates the lock chain in
>> reverse order:
>>   ovl_i_mutex_dir_key[] of lower overlay dir in vfs_rmdir()
>>   sb_writers of nested upper_mnt (e.g. tmpfs) in ovl_do_remove()
>>
>> To rid of the unneeded locking dependency, move both ovl_lower_positive()
>> and ovl_check_empty_dir() to before ovl_want_write() in rmdir() and
>> rename() implementation.
>>
>> This change spreads the pieces of ovl_check_empty_and_clear() directly
>> inside the rmdir()/rename() implementations so the helper is no longer
>> needed and removed.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>
>> Miklos,
>>
>> Please consider this patch for v4.15.
>>
>> I bumped into the lockdep warning while testing file handles on nested
>> overlay and had to work around it by using different type of fs for the
>> lower-upper(xfs) and the upper-upper(tmpfs).
>>
>> The workaround doesn't suffice if there is another test mounting an overlay
>> with the same fs type used for upper-upper (e.g. unionmount-testsuite)
>> at the same machine boot, which happens on my automated testing.
>>
>> So fixing this removes a false positive from my automated testing.
>>
>
> Miklos,
>
> Any objections to this change?
>
> Thanks,
> Amir.

No objection, just need time to review.  Will do.

Thanks,
Miklos