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

[Vivek Goyal <vgoyal@redhat.com>]

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@redhat.com> 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@redhat.com>
> > ---
> >  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.

Quick question on this. For non-dir files, will ovl_inode->redirect be
protected by VFS locking. Atleast for our use case. We seem to be touching
ovl_inode->redirect in rename, link and lookup path.

VFS rename locks both source and destination inodes and link path will
lock source and destination as well. So I think a rename and link can
not make progress in parallel if any or the source or target is files
are common. If that's the case two redirect writers can't stomp over
each other.

Lookup path will only set ovl_inode->redirect only if inode state is
I_NEW and that means other vfs operations on same inode could not be
making any progress.

Now comes question of copy up path when we remove redirect xattr and
modify ovl_inode->redirect. I don't know if we can assume that
VFS is holding inode lock when copy up happens. If this assumption is
valid, then it means rename and link can't make progress and it is
safe to modify ovl_inode->redirect.

I guess this assumption might not be valid, and that's why we need
ovl_inode->lock. Not sure though. Can you throw some light at it.

Thanks
Vivek