Re: [PATCH v2] NFSv4: Always ask for type with READDIR

From: Jeff Layton <jlayton@kernel.org>
To: Benjamin Coddington <bcodding@redhat.com>,
	trond.myklebust@hammerspace.com, anna@kernel.org
Cc: linux-nfs@vger.kernel.org
Subject: Re: [PATCH v2] NFSv4: Always ask for type with READDIR
Date: Wed, 30 Aug 2023 16:10:06 -0400	[thread overview]
Message-ID: <f793a08ed0db7bbe292c8aa6ec7241178c111cab.camel@kernel.org> (raw)
In-Reply-To: <82d1908e4f835a2f16a509a11b62b9d93ccb6cdf.1693424491.git.bcodding@redhat.com>

On Wed, 2023-08-30 at 15:42 -0400, Benjamin Coddington wrote:
> Again we have claimed regressions for walking a directory tree, this time
> with the "find" utility which always tries to optimize away asking for any
> attributes until it has a complete list of entries.  This behavior makes
> the readdir plus heuristic do the wrong thing, which causes a storm of
> GETATTRs to determine each entry's type in order to continue the walk.
> 
> For v4 add the type attribute to each READDIR request to include it no
> matter the heuristic.  This allows a simple `find` command to proceed
> quickly through a directory tree.
> 

The important bit here is that with v4, we can fill out d_type even when
"plus" is false, at little cost. The downside is that non-plus READDIR
replies will now be a bit larger on the wire. I think it's a worthwhile
tradeoff though.

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

> Suggested-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> 
> --
> On v2: Don't add the type attribute twice
> ---
>  fs/nfs/nfs4xdr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index deec76cf5afe..7200d6f7cd7b 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -1602,7 +1602,7 @@ static void encode_read(struct xdr_stream *xdr, const struct nfs_pgio_args *args
>  static void encode_readdir(struct xdr_stream *xdr, const struct nfs4_readdir_arg *readdir, struct rpc_rqst *req, struct compound_hdr *hdr)
>  {
>  	uint32_t attrs[3] = {
> -		FATTR4_WORD0_RDATTR_ERROR,
> +		FATTR4_WORD0_TYPE|FATTR4_WORD0_RDATTR_ERROR,
>  		FATTR4_WORD1_MOUNTED_ON_FILEID,
>  	};
>  	uint32_t dircount = readdir->count;
> @@ -1612,7 +1612,7 @@ static void encode_readdir(struct xdr_stream *xdr, const struct nfs4_readdir_arg
>  	unsigned int i;
>  
>  	if (readdir->plus) {
> -		attrs[0] |= FATTR4_WORD0_TYPE|FATTR4_WORD0_CHANGE|FATTR4_WORD0_SIZE|
> +		attrs[0] |= FATTR4_WORD0_CHANGE|FATTR4_WORD0_SIZE|
>  			FATTR4_WORD0_FSID|FATTR4_WORD0_FILEHANDLE|FATTR4_WORD0_FILEID;
>  		attrs[1] |= FATTR4_WORD1_MODE|FATTR4_WORD1_NUMLINKS|FATTR4_WORD1_OWNER|
>  			FATTR4_WORD1_OWNER_GROUP|FATTR4_WORD1_RAWDEV|