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

[Ira Cooper]

Good pickup,

Reviewed-by: Ira Cooper <address@hidden>

-Ira

On Wed, Jan 15, 2014 at 5:17 AM, Niels de Vos <address@hidden> 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