Re: [PATCH 2/7] ovl: During copy up, first copy up metadata and then data

[Vivek Goyal <vgoyal@redhat.com>]

On Mon, Oct 02, 2017 at 10:13:24PM +0300, Amir Goldstein wrote:
> On Mon, Oct 2, 2017 at 4:40 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > This just helps with later patches where after copying up metadata, we
> > skip data copying step, if needed.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/copy_up.c | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index f1b15e5c37d3..cdc4d79a1249 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -445,18 +445,6 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> >  {
> >         int err;
> >
> > -       if (S_ISREG(c->stat.mode)) {
> > -               struct path upperpath;
> > -
> > -               ovl_path_upper(c->dentry, &upperpath);
> > -               BUG_ON(upperpath.dentry != NULL);
> > -               upperpath.dentry = temp;
> > -
> > -               err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
> > -               if (err)
> > -                       return err;
> > -       }
> > -
> >         err = ovl_copy_xattr(c->lowerpath.dentry, temp);
> >         if (err)
> >                 return err;
> > @@ -480,6 +468,18 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> >                         return err;
> >         }
> >
> > +       if (S_ISREG(c->stat.mode)) {
> > +               struct path upperpath;
> > +
> > +               ovl_path_upper(c->dentry, &upperpath);
> > +               BUG_ON(upperpath.dentry != NULL);
> > +               upperpath.dentry = temp;
> > +
> > +               err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
> > +               if (err)
> > +                       return err;
> > +       }
> > +
> 
> You moved copy_up_data to after ovl_set_attr() -> ovl_set_timestamps()
> Does this break the copied up mtime? I'm not sure.
> I didn't see that do_splice_direct() or vfs_clone_file_range() change
> mtime directly, but inside file system, they might.

Hi Amir,

That's a good point. I will move ovl_set_attr() call after data copy.

Thanks
Vivek