Re: [RFC PATCH v2 5/9] ovl: add redirect dir feature when set redirect xattr to dir

["zhangyi (F)" <yi.zhang@huawei.com>]

On 2018/8/1 19:03, Amir Goldstein Wrote:
> On Tue, Jul 31, 2018 at 12:37 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>> Redirect dir feature is backward incompatible because old kernel (which
>> not support this feature) may corrupt the merge relationship between
>> directories when change the directory which have redirect xattr.
>>
>> This patch check and set the upper layer's redirect dir feature when
>> kernel set redirect xattr to directory in the upper layer.
>>
>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
>> ---
>>  fs/overlayfs/dir.c       | 11 +++++++++++
>>  fs/overlayfs/overlayfs.h |  5 ++++-
>>  2 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
>> index f480b1a2cd2e..238999e5f47b 100644
>> --- a/fs/overlayfs/dir.c
>> +++ b/fs/overlayfs/dir.c
>> @@ -969,6 +969,7 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
>>         bool is_dir = d_is_dir(old);
>>         bool new_is_dir = d_is_dir(new);
>>         bool samedir = olddir == newdir;
>> +       struct ovl_fs* ofs = old->d_sb->s_fs_info;
>>         struct dentry *opaquedir = NULL;
>>         const struct cred *old_cred = NULL;
>>         LIST_HEAD(list);
>> @@ -1061,6 +1062,16 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
>>                 }
>>         }
>>
>> +       if ((is_dir && ovl_type_merge_or_lower(old)) ||
>> +           (!overwrite && new_is_dir && ovl_type_merge_or_lower(new))) {
>> +               /*
>> +                * Set redirect dir feature to the upper root dir if this
>> +                * feature doesn't exist.
>> +                */
>> +               if (!ovl_has_feature_redirect_dir(ofs->upper_layer))
>> +                       err = ovl_set_feature_redirect_dir(ofs);
>> +       }
>> +
> 
> Please just place a call to  ovl_set_feature_redirect_dir(ofs)
> inside ovl_set_redirect(). There is no need to check all those
> conditions.
> Also, there is no need to check ovl_has_feature_redirect_dir()
> as ovl_set_feature() already checks for existing bit.
>

I understand check those condition is not perfect, but place it
to ovl_set_redirect() will lead to inode AA dead lock if the
parent dir of the renamed dir is the upper root dir, because
lock_rename() and vfs_setxattr() will get the same inode lock.

ovl_rename()
  ->lock_rename(upperdir)   //get the upper root inode lock
  ->ovl_set_redirect()
    ->ovl_set_feature_redirect_dir()
      ->vfs_setxattr(upperdir)  //dead lock in inode_lock()

Thanks,
Yi.