Re: [PATCH v12 15/17] ovl: Remove redirect when data of a metacopy file is copied up

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?

I think you found a good race situation. 

>  
> 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.

This is a good point. So we need to protect OVL_I(inode)->redirect with
oi->lock mutex as well (atleast for non-dirs). So ovl_rename() will nest
3 locks (which it already does for index case).

parent dir i_mutex.
 oi->lock
   dentry->d_lock().

I will try to write a patch for this and see what issues do I face

Vivek
> 
> Thanks,
> Amir.
--
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



[Index of Archives]     [Linux Filesystems Devel]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux