On Thu, Dec 01, 2016 at 05:04:06PM -0800, Eivind Sarto
wrote:> Overwriting an existing file with O_TRUNC will yield an incorrect atime.
> Example:
>
> root at vsan123:~# cp /vmlinuz /nfs
> root at vsan123:~# stat /nfs/vmlinuz
> File: ?/nfs/vmlinuz?
> Size: 6565968 Blocks: 12825 IO Block: 1048576 regular file
> Device: 25h/37d Inode: 10266595591654252930 Links: 1
> Access: (0600/-rw-------) Uid: ( 0/ root) Gid: ( 0/ root)
> Access: 2016-12-01 16:39:48.000000000 -0800
> Modify: 2016-12-01 16:39:48.235275472 -0800
> Change: 2016-12-01 16:39:48.239275472 -0800
> Birth: -
> root at vsan123:~# dd if=/vmlinuz of=/nfs/vmlinuz bs=64k conv=nocreat
> 100+1 records in
> 100+1 records out
> 6565968 bytes (6.6 MB) copied, 0.0271412 s, 242 MB/s
> root at vsan123:~# stat /nfs/vmlinuz
> File: ?/nfs/vmlinuz?
> Size: 6565968 Blocks: 12825 IO Block: 1048576 regular file
> Device: 25h/37d Inode: 10266595591654252930 Links: 1
> Access: (0600/-rw-------) Uid: ( 0/ root) Gid: ( 0/ root)
> Access: 1969-12-31 16:00:00.000000000 -0800
> Modify: 2016-12-01 16:39:57.923275920 -0800
> Change: 2016-12-01 16:39:57.923275920 -0800GF_SET_ATTR_MTIME
> Birth: -
>
> The Access time has been set to zero (or 1970 PST in my case)
>
> This appears to happen in all versions of gluster. I think the problem can
> actually be blamed on posix_setattr(). If one does a FOP_SETATTR and only
> passes the GF_SET_ATTR_MTIME flag, posix_setattr() will blindly call
> lutimes() to modify both atime+mtime and atime gets set to 0.
> I think the solution would be to modify posix_setattr() to write back
> original atime/mtime if one of the flags are not specified.
Thanks for reporting this! Could you file a bug on
https://bugzilla.redhat.com/enter_bug.cgi?product=GlusterFS as well?
Just copy/paste the contents of the email in the report and let us know
the URL of the bug that you created. We'll clone the bug for all
actively maintained versions after that.
The attached patch has been completely untested, but it should resolve
the problem. I'd appreciate it if you can give that a try and report
results.
> I have also seen NFS-Ganesha end up with a zero atime, but it does happen
> not for the example above on Ganesha. Ganesha probably does something
> similar someplace in its code where only GF_SET_ATTR_MTIME is being set.
> But, I cannot remember exactly what caused the zero atime on Ganesha.
NFS-Ganesha uses libgfapi and the library makes it possible to only pass
atime or mtime in glfs_h_setattrs(). Possibly NFS-Ganesha internally
maintains the atime+mtime combination, or the (Linux?) NFS-client
handles it differently over NFSv4. In any case, the attached patch
should also prevent this from happening with NFS-Ganesha.
> All other code in gluster, other than nfsserver, sets both ATIME and MTIME
> when doing an FOP_SETATTR.
Good to check that, it would give one possible explanation why this is
not happening over FUSE mounts.
Thanks again,
Niels
-------------- next part -------------->From b0517be8d11b4363e7a55e349a1efcf604cce96e Mon Sep 17 00:00:00 2001
From: Niels de Vos <ndevos at redhat.com>
Date: Fri, 2 Dec 2016 16:54:12 +0100
Subject: [PATCH] posix: make sure atime and mtime are set when calling
lutimes()
When overwriting an existing file with O_TRUNC, the 'atime' was set to
0, meaning the Epoch (01-Jan-1970 UTC). However, the 'mtime' gets
updated correcty.
In case 'atime' or 'mtime' is not passed in the 'struct
iatt', the time
values passed to the systemcall are taken from the current values are
returned by lstat().
Change-Id: Ibc7c1351c8c2397f8080f667b6eeef18e60b4cc5
Reported-by: Eivind Sarto <eivindsarto at gmail.com>
Signed-off-by: Niels de Vos <ndevos at redhat.com>
---
tests/bugs/nfs/zero-atime.t | 31 +++++++++++++++++++++++++++++++
xlators/storage/posix/src/posix.c | 26 ++++++++++++++++++++------
2 files changed, 51 insertions(+), 6 deletions(-)
create mode 100755 tests/bugs/nfs/zero-atime.t
diff --git a/tests/bugs/nfs/zero-atime.t b/tests/bugs/nfs/zero-atime.t
new file mode 100755
index 0000000..f3b9e61
--- /dev/null
+++ b/tests/bugs/nfs/zero-atime.t
@@ -0,0 +1,31 @@
+#!/bin/bash
+#
+# posix_do_utimes() sets atime and mtime to the values in the passed IATT. If
+# not set, these values are 0 and cause a atime/mtime set to the Epoch.
+#
+
+. $(dirname $0)/../../include.rc
+. $(dirname $0)/../../volume.rc
+. $(dirname $0)/../../nfs.rc
+
+cleanup
+
+TEST glusterd
+TEST pidof glusterd
+
+TEST $CLI volume create $V0 $H0:$B0/$V0
+TEST $CLI volume set $V0 nfs.disable false
+TEST $CLI volume start $V0
+EXPECT_WITHIN $NFS_EXPORT_TIMEOUT "1" is_nfs_export_available;
+TEST mount_nfs $H0:/$V0 $N0 nolock
+
+# create a file for testing
+TEST dd if=/dev/urandom of=$M0/small count=1 bs=1024k
+
+# timezone in UTC results in atime=0 if not set correctly
+TEST TZ=UTC dd if=/dev/urandom of=$M0/small bs=64k conv=nocreat
+TEST [ "$(stat --format=%X $M0/small)" != "0" ]
+
+TEST rm $M0/small
+
+cleanup
diff --git a/xlators/storage/posix/src/posix.c
b/xlators/storage/posix/src/posix.c
index d7b175c..6ade0af 100644
--- a/xlators/storage/posix/src/posix.c
+++ b/xlators/storage/posix/src/posix.c
@@ -375,7 +375,8 @@ posix_do_chown (xlator_t *this,
static int
posix_do_utimes (xlator_t *this,
const char *path,
- struct iatt *stbuf)
+ struct iatt *stbuf,
+ int32_t valid)
{
int32_t ret = -1;
struct timeval tv[2] = {{0,},{0,}};
@@ -392,10 +393,23 @@ posix_do_utimes (xlator_t *this,
if (S_ISLNK (stat.st_mode))
is_symlink = 1;
- tv[0].tv_sec = stbuf->ia_atime;
- tv[0].tv_usec = stbuf->ia_atime_nsec / 1000;
- tv[1].tv_sec = stbuf->ia_mtime;
- tv[1].tv_usec = stbuf->ia_mtime_nsec / 1000;
+ if ((valid & GF_SET_ATTR_ATIME) == GF_SET_ATTR_ATIME) {
+ tv[0].tv_sec = stbuf->ia_atime;
+ tv[0].tv_usec = stbuf->ia_atime_nsec / 1000;
+ } else {
+ /* atime is not given, use current values */
+ tv[0].tv_sec = stat.st_atim.tv_sec;
+ tv[0].tv_usec = ST_ATIM_NSEC (&stat) / 1000;
+ }
+
+ if ((valid & GF_SET_ATTR_MTIME) == GF_SET_ATTR_MTIME) {
+ tv[1].tv_sec = stbuf->ia_mtime;
+ tv[1].tv_usec = stbuf->ia_mtime_nsec / 1000;
+ } else {
+ /* mtime is not given, use current values */
+ tv[1].tv_sec = stat.st_mtim.tv_sec;
+ tv[1].tv_usec = ST_MTIM_NSEC (&stat) / 1000;
+ }
ret = lutimes (path, tv);
if ((ret == -1) && (errno == ENOSYS)) {
@@ -464,7 +478,7 @@ posix_setattr (call_frame_t *frame, xlator_t *this,
}
if (valid & (GF_SET_ATTR_ATIME | GF_SET_ATTR_MTIME)) {
- op_ret = posix_do_utimes (this, real_path, stbuf);
+ op_ret = posix_do_utimes (this, real_path, stbuf, valid);
if (op_ret == -1) {
op_errno = errno;
gf_msg (this->name, GF_LOG_ERROR, errno,
--
2.9.3