Re: [PATCH v8 3/7] NFSD: add io_cache_read controls to debugfs interface

[Chuck Lever <chuck.lever@oracle.com>]

On 9/3/25 12:02 PM, Mike Snitzer wrote:
> On Wed, Sep 03, 2025 at 11:07:29AM -0400, Mike Snitzer wrote:
>> On Wed, Sep 03, 2025 at 10:38:45AM -0400, Chuck Lever wrote:
> 
>>>> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
>>>> index 1cd0bed57bc2f..6ef799405145f 100644
>>>> --- a/fs/nfsd/nfsd.h
>>>> +++ b/fs/nfsd/nfsd.h
>>>> @@ -153,6 +153,15 @@ static inline void nfsd_debugfs_exit(void) {}
>>>>  
>>>>  extern bool nfsd_disable_splice_read __read_mostly;
>>>>  
>>>> +enum {
>>>> +	NFSD_IO_UNSPECIFIED = 0,
>>>> +	NFSD_IO_BUFFERED,
>>>> +	NFSD_IO_DONTCACHE,
>>>> +	NFSD_IO_DIRECT,
>>>> +};
>>>> +
>>>> +extern u64 nfsd_io_cache_read __read_mostly;
>>>
>>> And then here, initialize nfsd_io_cache_read to reflect the default
>>> behavior. That would be NFSD_IO_BUFFERED for now... then later we might
>>> want to change it to NFSD_IO_DIRECT, for instance.
>>>
>>> Same suggestion for 4/7.
>>
>> Ah ok, I can see the way forward to default to NFSD_IO_BUFFERED but
>> _not_ default to it when erroring (if the user specified some unknown
>> value).
>>
>> I'll run with that (despite just asking Jeff's opinion above, I'm the
>> one who came up with the awkward UNSPECIFIED state when honoring
>> Jeff's early feedback).
> 
> Here is the incremental diff (these changes will be folded into
> appropriate patches in v9):
> 
> diff --git a/fs/nfsd/debugfs.c b/fs/nfsd/debugfs.c
> index 8878c3519b30c..173032a04cdec 100644
> --- a/fs/nfsd/debugfs.c
> +++ b/fs/nfsd/debugfs.c
> @@ -43,11 +43,10 @@ DEFINE_DEBUGFS_ATTRIBUTE(nfsd_dsr_fops, nfsd_dsr_get, nfsd_dsr_set, "%llu\n");
>   * /sys/kernel/debug/nfsd/io_cache_read
>   *
>   * Contents:
> - *   %1: NFS READ will use buffered IO
> - *   %2: NFS READ will use dontcache (buffered IO w/ dropbehind)
> - *   %3: NFS READ will use direct IO
> + *   %0: NFS READ will use buffered IO
> + *   %1: NFS READ will use dontcache (buffered IO w/ dropbehind)
> + *   %2: NFS READ will use direct IO
>   *
> - * The default value of this setting is zero (UNSPECIFIED).
>   * This setting takes immediate effect for all NFS versions,
>   * all exports, and in all NFSD net namespaces.
>   */
> @@ -90,11 +89,10 @@ DEFINE_DEBUGFS_ATTRIBUTE(nfsd_io_cache_read_fops, nfsd_io_cache_read_get,
>   * /sys/kernel/debug/nfsd/io_cache_write
>   *
>   * Contents:
> - *   %1: NFS WRITE will use buffered IO
> - *   %2: NFS WRITE will use dontcache (buffered IO w/ dropbehind)
> - *   %3: NFS WRITE will use direct IO
> + *   %0: NFS WRITE will use buffered IO
> + *   %1: NFS WRITE will use dontcache (buffered IO w/ dropbehind)
> + *   %2: NFS WRITE will use direct IO
>   *
> - * The default value of this setting is zero (UNSPECIFIED).
>   * This setting takes immediate effect for all NFS versions,
>   * all exports, and in all NFSD net namespaces.
>   */
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index fe935b4cda538..412a1e9a2a876 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -154,8 +154,7 @@ static inline void nfsd_debugfs_exit(void) {}
>  extern bool nfsd_disable_splice_read __read_mostly;
>  
>  enum {
> -	NFSD_IO_UNSPECIFIED = 0,
> -	NFSD_IO_BUFFERED,
> +	NFSD_IO_BUFFERED = 0,

Thanks, this LGTM. Two additional remarks:

1. I think that the "= 0" is unneeded here because C enumerators always
start at 0.

2. I'm wondering if this enum definition should be moved to a uapi
header. Thoughts? This is experimental, and not a fixed API. So maybe
it needs to stay in fs/nfsd/nfsd.h.

(I'm probably going over ground that has already been covered.)


>  	NFSD_IO_DONTCACHE,
>  	NFSD_IO_DIRECT,
>  };
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 5e700a0d6b12e..403076443573f 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -50,8 +50,8 @@
>  #define NFSDDBG_FACILITY		NFSDDBG_FILEOP
>  
>  bool nfsd_disable_splice_read __read_mostly;
> -u64 nfsd_io_cache_read __read_mostly;
> -u64 nfsd_io_cache_write __read_mostly;
> +u64 nfsd_io_cache_read __read_mostly = NFSD_IO_BUFFERED;
> +u64 nfsd_io_cache_write __read_mostly = NFSD_IO_BUFFERED;
>  
>  /**
>   * nfserrno - Map Linux errnos to NFS errnos
> @@ -1272,8 +1272,7 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		break;
>  	case NFSD_IO_DONTCACHE:
>  		kiocb.ki_flags = IOCB_DONTCACHE;
> -		fallthrough;
> -	case NFSD_IO_UNSPECIFIED:
> +		break;
>  	case NFSD_IO_BUFFERED:
>  		break;
>  	}
> @@ -1605,8 +1604,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		break;
>  	case NFSD_IO_DONTCACHE:
>  		kiocb.ki_flags |= IOCB_DONTCACHE;
> -		fallthrough;
> -	case NFSD_IO_UNSPECIFIED:
> +		fallthrough; /* must call nfsd_issue_write_buffered */

Right. In this case, the NFSD_IO_BUFFERED arm is more than just a
"break;" so, not as brittle as the nfsd_iter_read() switch statement.
The comment is helpful, though; I'm not suggesting a change, just
observing.


>  	case NFSD_IO_BUFFERED:
>  		host_err = nfsd_issue_write_buffered(rqstp, file,
>  						nvecs, cnt, &kiocb);


-- 
Chuck Lever