Re: [RFC PATCH v2] nfsv4: Add support for the birth time attribute

[Benjamin Coddington <bcodding@redhat.com>]

I'm interested in this work, Chen are you still interested in moving this
forward?   I have another question below --

On 1 Sep 2023, at 4:34, Chen Hanxiao wrote:

> nfsd already support btime by commit e377a3e698.
>
> This patch enable nfs to report btime in nfs_getattr.
> If underlying filesystem supports "btime" timestamp,
> statx will report btime for STATX_BTIME.
>
> Signed-off-by: Chen Hanxiao <chenhx.fnst@fujitsu.com>
>
> ---
> v1.1:
> 	minor fix
> v2:
> 	properly set cache validity
>
>  fs/nfs/inode.c          | 28 ++++++++++++++++++++++++----
>  fs/nfs/nfs4proc.c       |  3 +++
>  fs/nfs/nfs4xdr.c        | 23 +++++++++++++++++++++++
>  include/linux/nfs_fs.h  |  2 ++
>  include/linux/nfs_xdr.h |  5 ++++-
>  5 files changed, 56 insertions(+), 5 deletions(-)
>
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 8172dd4135a1..cfdf68b07982 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -196,7 +196,8 @@ void nfs_set_cache_invalid(struct inode *inode, unsigned long flags)
>  		if (!(flags & NFS_INO_REVAL_FORCED))
>  			flags &= ~(NFS_INO_INVALID_MODE |
>  				   NFS_INO_INVALID_OTHER |
> -				   NFS_INO_INVALID_XATTR);
> +				   NFS_INO_INVALID_XATTR |
> +				   NFS_INO_INVALID_BTIME);
>  		flags &= ~(NFS_INO_INVALID_CHANGE | NFS_INO_INVALID_SIZE);
>  	}
>
> @@ -515,6 +516,7 @@ nfs_fhget(struct super_block *sb, struct nfs_fh *fh, struct nfs_fattr *fattr)
>  		memset(&inode->i_atime, 0, sizeof(inode->i_atime));
>  		memset(&inode->i_mtime, 0, sizeof(inode->i_mtime));
>  		memset(&inode->i_ctime, 0, sizeof(inode->i_ctime));
> +		memset(&nfsi->btime, 0, sizeof(nfsi->btime));
>  		inode_set_iversion_raw(inode, 0);
>  		inode->i_size = 0;
>  		clear_nlink(inode);
> @@ -538,6 +540,10 @@ nfs_fhget(struct super_block *sb, struct nfs_fh *fh, struct nfs_fattr *fattr)
>  			inode->i_ctime = fattr->ctime;
>  		else if (fattr_supported & NFS_ATTR_FATTR_CTIME)
>  			nfs_set_cache_invalid(inode, NFS_INO_INVALID_CTIME);
> +		if (fattr->valid & NFS_ATTR_FATTR_BTIME)
> +			nfsi->btime = fattr->btime;
> +		else if (fattr_supported & NFS_ATTR_FATTR_BTIME)
> +			nfs_set_cache_invalid(inode, NFS_INO_INVALID_BTIME);
>  		if (fattr->valid & NFS_ATTR_FATTR_CHANGE)
>  			inode_set_iversion_raw(inode, fattr->change_attr);
>  		else
> @@ -835,6 +841,7 @@ int nfs_getattr(struct mnt_idmap *idmap, const struct path *path,
>  {
>  	struct inode *inode = d_inode(path->dentry);
>  	struct nfs_server *server = NFS_SERVER(inode);
> +	struct nfs_inode *nfsi = NFS_I(inode);
>  	unsigned long cache_validity;
>  	int err = 0;
>  	bool force_sync = query_flags & AT_STATX_FORCE_SYNC;
> @@ -845,7 +852,7 @@ int nfs_getattr(struct mnt_idmap *idmap, const struct path *path,
>
>  	request_mask &= STATX_TYPE | STATX_MODE | STATX_NLINK | STATX_UID |
>  			STATX_GID | STATX_ATIME | STATX_MTIME | STATX_CTIME |
> -			STATX_INO | STATX_SIZE | STATX_BLOCKS |
> +			STATX_INO | STATX_SIZE | STATX_BLOCKS | STATX_BTIME |
>  			STATX_CHANGE_COOKIE;
>
>  	if ((query_flags & AT_STATX_DONT_SYNC) && !force_sync) {
> @@ -920,6 +927,10 @@ int nfs_getattr(struct mnt_idmap *idmap, const struct path *path,
>  		stat->attributes |= STATX_ATTR_CHANGE_MONOTONIC;
>  	if (S_ISDIR(inode->i_mode))
>  		stat->blksize = NFS_SERVER(inode)->dtsize;
> +	if (!(server->fattr_valid & NFS_ATTR_FATTR_BTIME))
> +		stat->result_mask &= ~STATX_BTIME;
> +	else
> +		stat->btime = nfsi->btime;
>  out:
>  	trace_nfs_getattr_exit(inode, err);
>  	return err;
> @@ -1803,7 +1814,7 @@ static int nfs_inode_finish_partial_attr_update(const struct nfs_fattr *fattr,
>  		NFS_INO_INVALID_ATIME | NFS_INO_INVALID_CTIME |
>  		NFS_INO_INVALID_MTIME | NFS_INO_INVALID_SIZE |
>  		NFS_INO_INVALID_BLOCKS | NFS_INO_INVALID_OTHER |
> -		NFS_INO_INVALID_NLINK;
> +		NFS_INO_INVALID_NLINK | NFS_INO_INVALID_BTIME;
>  	unsigned long cache_validity = NFS_I(inode)->cache_validity;
>  	enum nfs4_change_attr_type ctype = NFS_SERVER(inode)->change_attr_type;
>
> @@ -2122,7 +2133,8 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
>  	nfsi->cache_validity &= ~(NFS_INO_INVALID_ATTR
>  			| NFS_INO_INVALID_ATIME
>  			| NFS_INO_REVAL_FORCED
> -			| NFS_INO_INVALID_BLOCKS);
> +			| NFS_INO_INVALID_BLOCKS
> +			| NFS_INO_INVALID_BTIME);
>
>  	/* Do atomic weak cache consistency updates */
>  	nfs_wcc_update_inode(inode, fattr);
> @@ -2161,6 +2173,7 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
>  					| NFS_INO_INVALID_BLOCKS
>  					| NFS_INO_INVALID_NLINK
>  					| NFS_INO_INVALID_MODE
> +					| NFS_INO_INVALID_BTIME
>  					| NFS_INO_INVALID_OTHER;
>  				if (S_ISDIR(inode->i_mode))
>  					nfs_force_lookup_revalidate(inode);
> @@ -2189,6 +2202,12 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
>  		nfsi->cache_validity |=
>  			save_cache_validity & NFS_INO_INVALID_MTIME;
>
> +	if (fattr->valid & NFS_ATTR_FATTR_BTIME) {
> +		nfsi->btime = fattr->btime;
> +	} else if (fattr_supported & NFS_ATTR_FATTR_BTIME)
> +		nfsi->cache_validity |=
> +			save_cache_validity & NFS_INO_INVALID_BTIME;
> +
>  	if (fattr->valid & NFS_ATTR_FATTR_CTIME)
>  		inode->i_ctime = fattr->ctime;
>  	else if (fattr_supported & NFS_ATTR_FATTR_CTIME)
> @@ -2332,6 +2351,7 @@ struct inode *nfs_alloc_inode(struct super_block *sb)
>  #endif /* CONFIG_NFS_V4 */
>  #ifdef CONFIG_NFS_V4_2
>  	nfsi->xattr_cache = NULL;
> +	memset(&nfsi->btime, 0, sizeof(nfsi->btime));


^^ is this redundant if we're going to do it anyway in nfs_fhget for I_NEW?

.. actually, I don't understand why were doing /any/ nfsi member
initialization here.. am I missing something?

Otherwise, this gets

Tested-by: Benjamin Coddington <bcodding@redhat.com>
Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Ben