Re: [PATCH 07/11] vfs: copy_file_range should update file timestamps

[Dave Chinner <david@fromorbit.com>]

On Mon, Dec 03, 2018 at 12:47:39PM +0200, Amir Goldstein wrote:
> On Mon, Dec 3, 2018 at 10:34 AM Dave Chinner <david@fromorbit.com> wrote:
> >
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > Timestamps are not updated right now, so programs looking for
> > timestamp updates for file modifications (like rsync) will not
> > detect that files have changed. We are also accessing the source
> > data when doing a copy (but not when cloning) so we need to update
> > atime on the source file as well.
> >
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/read_write.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/fs/read_write.c b/fs/read_write.c
> > index 3b101183ea19..3288db1d5f21 100644
> > --- a/fs/read_write.c
> > +++ b/fs/read_write.c
> > @@ -1576,6 +1576,16 @@ static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in,
> >  {
> >         ssize_t ret;
> >
> > +       /* Update source timestamps, because we are accessing file data */
> > +       file_accessed(file_in);
> > +
> > +       /* Update destination timestamps, since we can alter file contents. */
> > +       if (!(file_out->f_mode & FMODE_NOCMTIME)) {
> > +               ret = file_update_time(file_out);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> 
> If there is a consistency about who is responsible of calling file_accessed()
> and file_update_time() it eludes me. grep tells me that they are mostly
> handled by filesystem code or generic helpers called by filesystem code
> and not in the vfs helpers.

This isn't the "vfs helper" - this is the code that executes a data
copy. We have to do these timestamp updates regardless of the copy
mechanism used so it makes no real sense to force every
implementation to do it, and then also have to ensure the generic
fallback does it as well. Do it once for everyone, then nobody else
needs to care about it.

> FMODE_NOCMTIME seems like an xfs specific flag (for DMAPI?), which

It's a generic VFS flag that originally only XFS used. We check it
in places where data IO to XFS files might be done. Given that we
have vfs functions doing write on behalf of XFS filesystems (such as
remap_file_range() and copy_file_range() the timestamp updates need
to check this flag.

> most generic callers of file_update_time() completely ignore.

Because most cases don't get called from a context that can have
FMODE_NOCMTIME set. If more filesystems start to use FMODE_NOCMTIME
then it will have to be more widely checked.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com