[PATCH v2] vfs/glusterfs: in case atime is not passed, set it to the current atime

[Ira Cooper]

Good pickup,

Reviewed-by: Ira Cooper <ira at samba.org>

-Ira


On Wed, Jan 15, 2014 at 5:17 AM, Niels de Vos <ndevos at redhat.com> wrote:

> On Tue, Jan 14, 2014 at 02:56:16PM -0800, Jeremy Allison wrote:
> > On Mon, Jan 13, 2014 at 11:26:42AM +0100, Niels de Vos wrote:
> > > Hello,
> > >
> > > we were informed that when using Samba+vfs_glusterfs a normal 'write'
> to
> > > a file does not correctly set the 'atime'. In fact, the 'atime' is set
> > > to "2106-02-07 11:58:15.000000000 +0530" according to 'stat'.
> > >
> > > Tracing the network and checking the SMB/SET_FILE_INFO shows that the
> > > Linux CIFS client sends the following date:
> > > - May 28, 60056 07:36:10.955161500 CEST
> > >
> > > This date, in fact, shows as 0xffffffff (32-bit -1) in the packet
> > > capture. The Linux CIFS client sets NO_CHANGE_64 (0xffffffffffffffff,
> > > 64-bit -1) when it intends to not modify the 'atime' (fs/cifs/inode.c).
> > >
> > > Debugging the Samba/vfs_glusterfs module with systemtap, showed that
> the
> > > 'write' that triggers the unexpected 'atime', indeed gets forwarded to
> > > the GlusterFS volume as -1. Obviously, Samba passes a -1 for the
> 'atime'
> > > when it should not get modified. Unfortunately the gfapi library does
> > > not expose a function to set the 'mtime' only, it is always needed to
> > > set the 'atime' as well (like 'utimes()').
> > >
> > > The attached patch fixes setting the 'atime' to a value way in the
> > > future. It resolves to setting the 'atime' to the same value as the
> > > 'mtime', whenever the 'atime' is set to -1.
> > >
> > > Downstream bug report and detailed results of the debugging:
> > > - https://bugzilla.redhat.com/1051226
> >
> > Actually I have to vote NAK on this patch, as it's
> > not doing the expected thing.
> >
> > Sending -1 means no atime change, and the current
> > atime is available to you in the smb_fname->st struct
> > passed into the ntimes VFS call.
> >
> > So I suggest you copy the code inside vfswrap_ntimes()
> > in source3/modules/vfs_default.c and do:
> >
> >         if (null_timespec(ft->atime)) {
> >                 ft->atime= smb_fname->st.st_ex_atime;
> >         }
> >
> >         if (null_timespec(ft->mtime)) {
> >                 ft->mtime = smb_fname->st.st_ex_mtime;
> >         }
> >
> >         if (!null_timespec(ft->create_time)) {
> >                 set_create_timespec_ea(handle->conn,
> >                                        smb_fname,
> >                                        ft->create_time);
> >         }
> >
> >         if ((timespec_compare(&ft->atime,
> >                               &smb_fname->st.st_ex_atime) == 0) &&
> >             (timespec_compare(&ft->mtime,
> >                               &smb_fname->st.st_ex_mtime) == 0)) {
> >                 return 0;
> >         }
> >
> > instead.
>
> That makes perfect sense to me. Thanks for the suggestion! The attached
> patch has been tested successfully too.
>
> Niels
>