Re: [PATCH v3 1/1] NFS: Extend rdirplus mount option with "force|none"

[Trond Myklebust <trondmy@hammerspace.com>]

On Thu, 2025-03-13 at 10:43 -0400, Benjamin Coddington wrote:
> There are certain users that wish to force the NFS client to choose
> READDIRPLUS over READDIR for a particular mount.  Update the
> "rdirplus" mount
> option to optionally accept values.  For "rdirplus=force", the NFS
> client
> will always attempt to use READDDIRPLUS.  The setting of
> "rdirplus=none" is
> aliased to the existing "nordirplus".
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/dir.c              |  2 ++
>  fs/nfs/fs_context.c       | 32 ++++++++++++++++++++++++++++----
>  fs/nfs/super.c            |  1 +
>  include/linux/nfs_fs_sb.h |  1 +
>  4 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 2b04038b0e40..c9de0e474cf5 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -666,6 +666,8 @@ static bool nfs_use_readdirplus(struct inode
> *dir, struct dir_context *ctx,
>  {
>  	if (!nfs_server_capable(dir, NFS_CAP_READDIRPLUS))
>  		return false;
> +	if (NFS_SERVER(dir)->flags && NFS_MOUNT_FORCE_RDIRPLUS)

Bitwise and?

> +		return true;
>  	if (ctx->pos == 0 ||
>  	    cache_hits + cache_misses >
> NFS_READDIR_CACHE_USAGE_THRESHOLD)
>  		return true;
> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
> index b069385eea17..1cabba1231d6 100644
> --- a/fs/nfs/fs_context.c
> +++ b/fs/nfs/fs_context.c
> @@ -72,6 +72,8 @@ enum nfs_param {
>  	Opt_posix,
>  	Opt_proto,
>  	Opt_rdirplus,
> +	Opt_rdirplus_none,
> +	Opt_rdirplus_force,
>  	Opt_rdma,
>  	Opt_resvport,
>  	Opt_retrans,
> @@ -174,7 +176,8 @@ static const struct fs_parameter_spec
> nfs_fs_parameters[] = {
>  	fsparam_u32   ("port",		Opt_port),
>  	fsparam_flag_no("posix",	Opt_posix),
>  	fsparam_string("proto",		Opt_proto),
> -	fsparam_flag_no("rdirplus",	Opt_rdirplus),
> +	fsparam_flag_no("rdirplus", Opt_rdirplus), //
> rdirplus|nordirplus
> +	fsparam_string("rdirplus",  Opt_rdirplus), // rdirplus=...
>  	fsparam_flag  ("rdma",		Opt_rdma),
>  	fsparam_flag_no("resvport",	Opt_resvport),
>  	fsparam_u32   ("retrans",	Opt_retrans),
> @@ -288,6 +291,12 @@ static const struct constant_table
> nfs_xprtsec_policies[] = {
>  	{}
>  };
>  
> +static const struct constant_table nfs_rdirplus_tokens[] = {
> +	{ "none",	Opt_rdirplus_none },
> +	{ "force",	Opt_rdirplus_force },
> +	{}
> +};
> +
>  /*
>   * Sanity-check a server address provided by the mount command.
>   *
> @@ -636,10 +645,25 @@ static int nfs_fs_context_parse_param(struct
> fs_context *fc,
>  			ctx->flags &= ~NFS_MOUNT_NOACL;
>  		break;
>  	case Opt_rdirplus:
> -		if (result.negated)
> +		if (result.negated) {
> +			ctx->flags &= ~NFS_MOUNT_FORCE_RDIRPLUS;
>  			ctx->flags |= NFS_MOUNT_NORDIRPLUS;
> -		else
> -			ctx->flags &= ~NFS_MOUNT_NORDIRPLUS;
> +		} else if (!param->string) {
> +			ctx->flags &= ~(NFS_MOUNT_NORDIRPLUS |
> NFS_MOUNT_FORCE_RDIRPLUS);
> +		} else {
> +			switch (lookup_constant(nfs_rdirplus_tokens,
> param->string, -1)) {
> +			case Opt_rdirplus_none:
> +				ctx->flags &=
> ~NFS_MOUNT_FORCE_RDIRPLUS;
> +				ctx->flags |= NFS_MOUNT_NORDIRPLUS;
> +				break;
> +			case Opt_rdirplus_force:
> +				ctx->flags &= ~NFS_MOUNT_NORDIRPLUS;
> +				ctx->flags |=
> NFS_MOUNT_FORCE_RDIRPLUS;
> +				break;
> +			default:
> +				goto out_invalid_value;
> +			}
> +		}
>  		break;
>  	case Opt_sharecache:
>  		if (result.negated)
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index aeb715b4a690..9a747b224a9d 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -456,6 +456,7 @@ static void nfs_show_mount_options(struct
> seq_file *m, struct nfs_server *nfss,
>  		{ NFS_MOUNT_NORDIRPLUS, ",nordirplus", "" },
>  		{ NFS_MOUNT_UNSHARED, ",nosharecache", "" },
>  		{ NFS_MOUNT_NORESVPORT, ",noresvport", "" },
> +		{ NFS_MOUNT_FORCE_RDIRPLUS, ",rdirplus=force", "" },

Put the above together with the other rdirplus options?

>  		{ 0, NULL, NULL }
>  	};
>  	const struct proc_nfs_info *nfs_infop;
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index f00bfcee7120..3774b2235a1e 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -167,6 +167,7 @@ struct nfs_server {
>  #define NFS_MOUNT_TRUNK_DISCOVERY	0x04000000
>  #define NFS_MOUNT_SHUTDOWN			0x08000000
>  #define NFS_MOUNT_NO_ALIGNWRITE		0x10000000
> +#define NFS_MOUNT_FORCE_RDIRPLUS	0x20000000
>  
>  	unsigned int		fattr_valid;	/* Valid attributes
> */
>  	unsigned int		caps;		/* server
> capabilities */

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com