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

[Vivek Goyal <vgoyal@redhat.com>]

On Thu, Mar 15, 2018 at 10:42:11PM +0200, Amir Goldstein wrote:
> On Thu, Mar 15, 2018 at 8:47 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Wed, Mar 14, 2018 at 03:15:33PM -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@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?
> >>
> >> 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
> >
> > Hi Amir,
> >
> > I am trying to understand better how you are taking oi->lock w.r.t
> > nlink stuff and I am having a hard time.
> >
> > - Why do you keep oi->locked for the duration of operation (link, unlink
> >   etc) using ovl_nlink_start() and ovl_nlink_end().
> 
> As the comment above ovl_nlink_start() says, union nlink may be changed
> by link(), unlink() and copyup. nlink is an overlay inode property, so we need
> to protect its updates with a lock on the inode object, which in this level if
> oi->lock. Also, in ovl_nlink_end() we cleanup the index on last union nlink drop
> and we need to do that also under inode object lock.

Sure. What I don't understand is that why do we have to continue to hold
the lock for the whole duration. Can we drop the lock and re-acquire it
before we cleanup index and change nlink value on ovelray inode?

Thanks
Vivek