[PATCH] Save one stat in update_write_time_on_close()

[Jeremy Allison]

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 - if the underlying system actually
keeps track of atime (strictatime option on Linux) as
the set_file_time() calls map to UNIX calls that set
both atime and mtime together.

The more pertinent question is do we care about this ?

:-).

Linux by default mounts as relatime (only update atime
if previous was earlier than current mtime or ctime)
and we don't ever check the atime semantics (code that
depends on them will be broken on almost ever modern
UNIX system).

Under the covers on Linux we do have a call utimensat()
which allows individual setting of atime OR mtime
(set the tv_nsec value to UTIME_OMIT to cause it to ignore)
but we're currently not using it.

Can I think about this some more before deciding ?

Cheers,

Jeremy.

> From 2d0941718a685204491b445d7800aedbee8a829d 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] s3/smbd: in update_write_time_on_close() only stat() if we
>  must
> 
> No need to update fsp->fsp_name->st via a stat call, the following code
> doesn't look into it.
> 
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
>  source3/smbd/close.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/source3/smbd/close.c b/source3/smbd/close.c
> index 22bd361..3278d1d 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;
> +		}
>  	}
>  
>  	/*
> -- 
> 2.7.4
>