[PATCH] Save one stat in update_write_time_on_close()

[Ralph Böhme]

On Tue, Oct 11, 2016 at 11:56:47AM -0700, Jeremy Allison wrote:
> On Tue, Oct 11, 2016 at 05:52:12PM +0200, Ralph Böhme wrote:
> > Hi!
> > 
> > To me it looks like in update_write_time_on_close() we could avoid
> > refreshing stat if we already have some valid stat info in the
> > smb_fname, as the code lines following thereafter don't seem to depend
> > on up2date mtime (or any other stat piece).
> > 
> > This is part of the devilish write time update insanity, so I'm not
> > 100% sure this is correct. At least it passed a private autobuild and
> > iirc we have a set of tests that verify correct behaviour.
> > 
> > What do you think?
> 
> So this will work, but the thing it will lose is the
> accuracy of atime - ...

ah, I see. Wonder whether setting filesystem atime in
update_write_time_on_close() to some stale value from an earlier stat
makes sense at all. Maybe just setting

ft.atime = ft.mtime = fsp->close_write_time;

in update_write_time_on_close() makes more sense? This way we could
also avoid the stat if it's valid. Updated patch attached.

-slow
-------------- next part --------------
From f4b6bdd955c6035fe69cd44a25d784363fd55c4c Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Tue, 11 Oct 2016 13:55:13 +0200
Subject: [PATCH] WIP: s3/smbd: in update_write_time_on_close() only stat() if
 we must

Also set atime to mtime and don't take it from the stat info of the
file. Even better would be using utimesat with atime set to UTIME_OMIT.
---
 source3/smbd/close.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/source3/smbd/close.c b/source3/smbd/close.c
index bc468c7..88328f8 100644
--- a/source3/smbd/close.c
+++ b/source3/smbd/close.c
@@ -546,15 +546,17 @@ static NTSTATUS update_write_time_on_close(struct files_struct *fsp)
 		fsp->close_write_time = timespec_current();
 	}
 
-	/* Ensure we have a valid stat struct for the source. */
-	status = vfs_stat_fsp(fsp);
-	if (!NT_STATUS_IS_OK(status)) {
-		return status;
-	}
-
 	if (!VALID_STAT(fsp->fsp_name->st)) {
-		/* if it doesn't seem to be a real file */
-		return NT_STATUS_OK;
+		/* Ensure we have a valid stat struct for the source. */
+		status = vfs_stat_fsp(fsp);
+		if (!NT_STATUS_IS_OK(status)) {
+			return status;
+		}
+
+		if (!VALID_STAT(fsp->fsp_name->st)) {
+			/* if it doesn't seem to be a real file */
+			return NT_STATUS_OK;
+		}
 	}
 
 	/*
@@ -583,7 +585,7 @@ static NTSTATUS update_write_time_on_close(struct files_struct *fsp)
 		TALLOC_FREE(lck);
 	}
 
-	ft.mtime = fsp->close_write_time;
+	ft.atime = ft.mtime = fsp->close_write_time;
 	/* As this is a close based update, we are not directly changing the
 	   file attributes from a client call, but indirectly from a write. */
 	status = smb_set_file_time(fsp->conn, fsp, fsp->fsp_name, &ft, false);
-- 
2.7.4