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

[Niels de Vos]

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
-------------- next part --------------
>From afc80e99bca7256b9765cffdb049b67847229e9f Mon Sep 17 00:00:00 2001
From: Niels de Vos <ndevos at redhat.com>
Date: Fri, 10 Jan 2014 16:26:18 +0100
Subject: [PATCH] vfs/glusterfs: in case atime is not passed, set it to the current atime

The Linux CIFS client does not pass an updated atime when a write() is
done. This causes the vfs/glusterfs module to set the atime to -1 on the
Gluster backend, resulting in an atime far in the future (year 2106).

Signed-off-by: Niels de Vos <ndevos at redhat.com>
---
 source3/modules/vfs_glusterfs.c |   26 ++++++++++++++++++++++----
 1 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/source3/modules/vfs_glusterfs.c b/source3/modules/vfs_glusterfs.c
index 3262f11..9bcd0cb 100644
--- a/source3/modules/vfs_glusterfs.c
+++ b/source3/modules/vfs_glusterfs.c
@@ -734,10 +734,28 @@ static int vfs_gluster_ntimes(struct vfs_handle_struct *handle,
 {
 	struct timespec times[2];
 
-	times[0].tv_sec = ft->atime.tv_sec;
-	times[0].tv_nsec = ft->atime.tv_nsec;
-	times[1].tv_sec = ft->mtime.tv_sec;
-	times[1].tv_nsec = ft->mtime.tv_nsec;
+	if (null_timespec(ft->atime)) {
+		times[0].tv_sec = smb_fname->st.st_ex_atime.tv_sec;
+		times[0].tv_nsec = smb_fname->st.st_ex_atime.tv_nsec;
+	} else {
+		times[0].tv_sec = ft->atime.tv_sec;
+		times[0].tv_nsec = ft->atime.tv_nsec;
+	}
+
+	if (null_timespec(ft->mtime)) {
+		times[1].tv_sec = smb_fname->st.st_ex_mtime.tv_sec;
+		times[1].tv_nsec = smb_fname->st.st_ex_mtime.tv_nsec;
+	} else {
+		times[1].tv_sec = ft->mtime.tv_sec;
+		times[1].tv_nsec = ft->mtime.tv_nsec;
+	}
+
+	if ((timespec_compare(&times[0],
+			      &smb_fname->st.st_ex_atime) == 0) &&
+	    (timespec_compare(&times[1],
+			      &smb_fname->st.st_ex_mtime) == 0)) {
+		return 0;
+	}
 
 	return glfs_utimens(handle->data, smb_fname->base_name, times);
 }
-- 
1.7.1