Re: [PATCH v2 1/2] nfsd: encode stat->mtime for getattr instead of inode->i_mtime

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2018-01-03 at 17:14 +0200, Amir Goldstein wrote:
> The values of stat->mtime and inode->i_mtime may differ for overlayfs
> and stat->mtime is the correct value to use when encoding getattr.
> This is also consistent with the fact that other attr times are also
> encoded from stat values.
> 
> Both callers of lease_get_mtime() already have the value of stat->mtime,
> so the only needed change is that lease_get_mtime() will not overwrite
> this value with inode->i_mtime in case the inode does not have an
> exclusive lease.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/locks.c       | 6 ++----
>  fs/nfsd/nfsxdr.c | 1 +
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 21b4dfa289ee..d6ff4beb70ce 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1554,9 +1554,9 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
>  EXPORT_SYMBOL(__break_lease);
>  
>  /**
> - *	lease_get_mtime - get the last modified time of an inode
> + *	lease_get_mtime - update modified time of an inode with exclusive lease
>   *	@inode: the inode
> - *      @time:  pointer to a timespec which will contain the last modified time
> + *      @time:  pointer to a timespec which contains the last modified time
>   *
>   * This is to force NFS clients to flush their caches for files with
>   * exclusive leases.  The justification is that if someone has an
> @@ -1580,8 +1580,6 @@ void lease_get_mtime(struct inode *inode, struct timespec *time)
>  
>  	if (has_lease)
>  		*time = current_time(inode);
> -	else
> -		*time = inode->i_mtime;
>  }
>  
>  EXPORT_SYMBOL(lease_get_mtime);
> diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
> index 644a0342f0e0..79b6064f8977 100644
> --- a/fs/nfsd/nfsxdr.c
> +++ b/fs/nfsd/nfsxdr.c
> @@ -188,6 +188,7 @@ encode_fattr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp,
>  	*p++ = htonl((u32) stat->ino);
>  	*p++ = htonl((u32) stat->atime.tv_sec);
>  	*p++ = htonl(stat->atime.tv_nsec ? stat->atime.tv_nsec / 1000 : 0);
> +	time = stat->mtime;
>  	lease_get_mtime(d_inode(dentry), &time); 
>  	*p++ = htonl((u32) time.tv_sec);
>  	*p++ = htonl(time.tv_nsec ? time.tv_nsec / 1000 : 0); 

This looks like a clear improvement. AFAICT, in all cases we pass
lease_get_mtime a value from a previous getattr operation. We should
prefer that one to inode->i_mtime.

The lease_get_mtime prototype is now rather awkward though. It might be
nice to clean that up, eventually, but that can be done later.

Reviewed-by: Jeff Layton <jlayton@kernel.org>