On Fri, Aug 3, 2018, 1:52 PM zhangyi (F) <yi.zhang@xxxxxxxxxx> wrote:
On 2018/8/1 19:03, Amir Goldstein Wrote:
> On Tue, Jul 31, 2018 at 12:37 PM, zhangyi (F) <yi.zhang@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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.
