Re: [PATCH 10/11] ovl: Introduce read/write barriers around metacopy flag update

[Vivek Goyal <vgoyal@redhat.com>]

On Wed, Oct 18, 2017 at 08:19:50AM +0300, Amir Goldstein wrote:
> On Wed, Oct 18, 2017 at 12:05 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > If a file is copied up metadata only and later when same file is opened
> > for WRITE, then data copy up takes place. We copy up data, remove METACOPY
> > xattr and then clear the METACOPY flag from ovl_entry->flags. While all
> > these operations happen with oi->lock held, read side of oi->flags is
> > lockless. That is another thread on another cpu can check if METACOPY
> > flag is clear or not.
> >
> > So this gives us an ordering requirement w.r.t METACOPY flag. That is, if
> > another cpu sees METACOPY flag clear, then it should be guaranteed that
> > effects of data copy up and remove xattr operations are also visible.
> >
> > For example.
> >
> >         CPU1                            CPU2
> > ovl_copy_up_flags()                     acquire(oi->lock)
> >  ovl_dentry_needs_data_copy_up()          ovl_copy_up_data()
> 
> The relevant lockless execution path of CPU1 is
> ovl_d_real -> ovl_test_flag(OVL_METACOPY) ->  return real
> when real is upper, upper needs to be with correct data
> 

That path is relevant for ordering requirements w.r.t oi->__upperdentry
and next patch does put an smp_rmb() there to cover it.

> The execution path you describe here is going to test the flag
> under overlay inode lock again in  ovl_copy_up_one() so it is irrelevant
> to the barriers patch

It will go into ovl_copy_up_one() only if this cpu saw OVL_METACOPY=1. But
what about the case where OVL_METACOPY is being cleared on CPU2, and
it became visible on CPU1. That means cpu1 will assume file copy up is
complete and it will continue and never both to take oi->lock.

> 
> 
> >    ovl_test_flag(OVL_METACOPY)            vfs_removexattr()
> >                                           ovl_clear_metacopy_flag()
> >                                         release(oi->lock)
> >
> > Say CPU2 is copying up data and in the end clears metacopy flag. But if
> > CPU1 perceives the effects of clearing of metacopy flag but not effects of
> > preceeding operations, that would be a problem.
> >
> > Hence this patch introduces smp_wmb() on metacopy flag clear operation
> > and smp_rmb() on metacopy flag test operation.
> >
> > May be some other lock or barrier is already covering it. But I am not sure
> > what that is and is it obvious enough that we will not break it in future.
> >
> > So hence trying to be safe here and introducing barriers explicitly for
> > metacopy bit.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/copy_up.c |  6 ++++++
> >  fs/overlayfs/util.c    | 12 ++++++++++--
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 99b7be4ff4fc..200ee3b7d397 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -466,6 +466,12 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
> >         if (err)
> >                 return err;
> >
> > +       /*
> > +        * Pairs with smp_rmb() after test_bit(OVL_METACOPY). Make sure
> 
> This comment is meant to refer the reader to the code that this pairs with
> i.e. pairs with smp_rmb() in ovl_d_real() ..

This pairs with smp_rmb() in ovl_dentry_needs_data_copy_up(). Next patch
has smp_rmb() in ovl_d_real() and I will explain that more.

> 
> > +        * if OVL_METACOPY flag reset is visible, then all the write
> > +        * operations before it are visible as well
> > +        */
> > +       smp_wmb();
> >         ovl_clear_flag(OVL_METACOPY, d_inode(c->dentry));
> >         return err;
> >  }
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index 91c542d1a39d..6bca9a960c6d 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -228,6 +228,8 @@ void ovl_dentry_set_upper_alias(struct dentry *dentry)
> >  }
> >
> >  bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags) {
> > +       bool metacopy;
> > +
> >         if (!S_ISREG(d_inode(dentry)->i_mode))
> >                 return false;
> >
> > @@ -237,9 +239,15 @@ bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags) {
> >         if (!(OPEN_FMODE(flags) & FMODE_WRITE))
> >                 return false;
> >
> > -       if (!ovl_test_flag(OVL_METACOPY, d_inode(dentry)))
> > +       metacopy = ovl_test_flag(OVL_METACOPY, d_inode(dentry));
> > +       /*
> > +        * Pairs with smp_wmb() while clearing OVL_METACOPY. Make sure if
> 
> This comment is meant to refer the reader to the code that this pairs with
> i.e. pairs with smp_wmb() in ovl_copy_up_meta_inode_data() ..

Will do. I guess I will retain this which explains what's the intent
and also add funciton name where OVL_METACOPY is being cleared.

> 
> > +        * clearing of OVL_METACOPY is visible, then effects of writes
> > +        * before that are visible too.
> > +        */
> > +       smp_rmb();
> 
> Please move this rmb to ovl_d_real() after re-testing flag,
> which is the only place that matters for inode data access.

But we are testing it atleast at two places.
(ovl_dentry_needs_data_copy_up()).

First call is in.

ovl_d_real()
  ovl_open_need_copy_up()
     ovl_dentry_needs_data_copy_up()

And second call is in ovl_copy_up_flags() further down.

ovl_d_real()
  ovl_open_need_copy_up()
     ovl_dentry_needs_data_copy_up()  <---- First Call
          ovl_copy_up_flags()
	      ovl_dentry_needs_data_copy_up() <--- Second Call

Given we are checking/loading oi->flags at both the places, IIUC, we
need smp_rmb() at both the places. And if that's the case, its better
to keep it inside ovl_dentry_needs_data_copy_up() instead of open
coding it in ovl_open_need_copy_up() and ovl_copy_up_flags().

Thanks
Vivek


> 
> > +       if (!metacopy)
> >                 return false;
> > -
> >         return true;
> >  }
> >
> > --
> > 2.13.5
> >