On Tue, Mar 20, 2018 at 10:35 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> On Tue, Mar 20, 2018 at 03:26:19PM -0400, Vivek Goyal wrote:
>> On Wed, Mar 07, 2018 at 10:21:30AM +0200, Amir Goldstein wrote:
>> > On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
>> > > When a metacopy file is no longer a metacopy and data has been copied up,
>> > > remove REDIRECT xattr. Its not needed anymore.
>> > >
>> > > Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
>> > > ---
>> > > fs/overlayfs/copy_up.c | 9 +++++++++
>> > > 1 file changed, 9 insertions(+)
>> > >
>> > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> > > index 0c8d2755bd25..704febd2e2fa 100644
>> > > --- a/fs/overlayfs/copy_up.c
>> > > +++ b/fs/overlayfs/copy_up.c
>> > > @@ -775,6 +775,15 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
>> > > if (err)
>> > > return err;
>> > >
>> > > + /*
>> > > + * A metacopy files does not need redirect xattr once data has
>> > > + * been copied up.
>> > > + */
>> > > + err = vfs_removexattr(upperpath.dentry, OVL_XATTR_REDIRECT);
>> > > + if (err && err != -ENODATA && err != -EOPNOTSUPP)
>> > > + return err;
>> > > +
>> > > + err = 0;
>> > > ovl_set_upperdata(d_inode(c->dentry));
>> > > return err;
>> >
>> > By intuition, I would say that removing redirect should be done after setting
>> > upperdata flag. Not sure if it really matters in real life.
>> > Maybe when racing a lookup of a metacopy hardlink and copy up data of
>> > an upper alias?
>> >
>> > Also, it would make sense to also ovl_dentry_set_redirect(c->dentry, NULL)
>> > probably use a helper ovl_clear_redirect() for the locking.
>> >
>> > But that highlights a serious problem with current patches -
>> > Access to OVL_I(inode)->redirect is protected with parent mutex in ovl_lookup()
>> > and additionally with dentry->d_lock in ovl_rename()
>> > That is sufficient for directories which can only have a single dentry
>> > alias to an
>> > inode but not at all sufficient for non-directories.
>>
>> Hi Amir,
>>
>> I think I have got the locking of ovl_inode right now. Given this
>> situation, I am looking at ovl_lookup() and wondering, why certain
>> properties which belong to inode are still being initialized during
>> dentry initialization (That too without any lock). That sounds wrong.
>>
>> For example.
>>
>> ovl_lookup() {
>> if (index)
>> ovl_set_flag(OVL_INDEX, inode);
>> }
>>
>> This is ovl_inode() property and should be initialized when inode is
>> created first time in cache? And updated later in copy up path as index
>> is created later. What's the reason to update inode property in
>> ovl_lookup() if inode is alreday in cache.
Historic. It was there before index was passed in to ovl_get_inode()
>
> IOW, will a patch as below fly?
Yes, looks good.
>
> ---
> fs/overlayfs/export.c | 3 ---
> fs/overlayfs/inode.c | 3 +++
> fs/overlayfs/namei.c | 2 --
> 3 files changed, 3 insertions(+), 5 deletions(-)
>
> Index: rhvgoyal-linux/fs/overlayfs/inode.c
> ===================================================================
> --- rhvgoyal-linux.orig/fs/overlayfs/inode.c 2018-03-20 16:32:30.137452244 -0400
> +++ rhvgoyal-linux/fs/overlayfs/inode.c 2018-03-20 16:32:56.000452244 -0400
> @@ -760,6 +760,9 @@ struct inode *ovl_get_inode(struct super
> if (upperdentry && ovl_is_impuredir(upperdentry))
> ovl_set_flag(OVL_IMPURE, inode);
>
> + if (index)
> + ovl_set_flag(OVL_INDEX, inode);
> +
> /* Check for non-merge dir that may have whiteouts */
> if (is_dir) {
> if (((upperdentry && lowerdentry) || numlower > 1) ||
> Index: rhvgoyal-linux/fs/overlayfs/namei.c
> ===================================================================
> --- rhvgoyal-linux.orig/fs/overlayfs/namei.c 2018-03-20 16:32:30.137452244 -0400
> +++ rhvgoyal-linux/fs/overlayfs/namei.c 2018-03-20 16:32:56.005452244 -0400
> @@ -985,8 +985,6 @@ struct dentry *ovl_lookup(struct inode *
> goto out_free_oe;
>
> OVL_I(inode)->redirect = upperredirect;
> - if (index)
> - ovl_set_flag(OVL_INDEX, inode);
> }
>
> revert_creds(old_cred);
> Index: rhvgoyal-linux/fs/overlayfs/export.c
> ===================================================================
> --- rhvgoyal-linux.orig/fs/overlayfs/export.c 2018-03-20 16:32:54.067452244 -0400
> +++ rhvgoyal-linux/fs/overlayfs/export.c 2018-03-20 16:33:37.893452244 -0400
> @@ -311,9 +311,6 @@ static struct dentry *ovl_obtain_alias(s
> return ERR_CAST(inode);
> }
>
> - if (index)
> - ovl_set_flag(OVL_INDEX, inode);
> -
> dentry = d_find_any_alias(inode);
> if (!dentry) {
> dentry = d_alloc_anon(inode->i_sb);
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
