On Fri, Aug 3, 2018, 1:52 PM zhangyi (F) <yi.zhang@huawei.com> wrote:

> 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()
>

Very well, so you can move ovl_set_redirect before lock_rename()

I'm pretty sure that vivek already posted a patch like this in one of the
revisions of metacopy, but dropped it because it wasn't a must.

Thanks,
Amir.