Re: [PATCH 2/2] NFSD: allow client to use write delegation stateid for READ

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2025-02-13 at 11:37 -0800, Dai Ngo wrote:
> Allow read using write delegation stateid granted on OPENs with
> OPEN4_SHARE_ACCESS_WRITE only, to accommodate clients whose WRITE
> implementation may unavoidably do (e.g., due to buffer cache
> constraints).
> 
> When this condition is detected in nfsd4_encode_read the access
> mode FMODE_READ is temporarily added to the file's f_mode and is
> removed when the read is done.
> 
> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> ---
>  fs/nfsd/nfs4proc.c | 15 ++++++++++++++-
>  fs/nfsd/nfs4xdr.c  |  8 ++++++++
>  fs/nfsd/xdr4.h     |  1 +
>  3 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index f6e06c779d09..be43627bbf78 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -973,7 +973,18 @@ nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	/* check stateid */
>  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
>  					&read->rd_stateid, RD_STATE,
> -					&read->rd_nf, NULL);
> +					&read->rd_nf, &read->rd_wd_stid);
> +	/*
> +	 * rd_wd_stid is needed for nfsd4_encode_read to allow write
> +	 * delegation stateid used for read. Its refcount is decremented
> +	 * by nfsd4_read_release when read is done.
> +	 */
> +	if (!status && read->rd_wd_stid &&
> +		(read->rd_wd_stid->sc_type != SC_TYPE_DELEG ||
> +		delegstateid(read->rd_wd_stid)->dl_type != NFS4_OPEN_DELEGATE_WRITE)) {
> +		nfs4_put_stid(read->rd_wd_stid);
> +		read->rd_wd_stid = NULL;
> +	}
>  
>  	read->rd_rqstp = rqstp;
>  	read->rd_fhp = &cstate->current_fh;
> @@ -984,6 +995,8 @@ nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  static void
>  nfsd4_read_release(union nfsd4_op_u *u)
>  {
> +	if (u->read.rd_wd_stid)
> +		nfs4_put_stid(u->read.rd_wd_stid);
>  	if (u->read.rd_nf)
>  		nfsd_file_put(u->read.rd_nf);
>  	trace_nfsd_read_done(u->read.rd_rqstp, u->read.rd_fhp,
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index e67420729ecd..3996678bab3f 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -4498,6 +4498,7 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
>  	unsigned long maxcount;
>  	__be32 wire_data[2];
>  	struct file *file;
> +	bool wronly = false;
>  
>  	if (nfserr)
>  		return nfserr;
> @@ -4515,10 +4516,17 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
>  	maxcount = min_t(unsigned long, read->rd_length,
>  			 (xdr->buf->buflen - xdr->buf->len));
>  
> +	if (!(file->f_mode & FMODE_READ) && read->rd_wd_stid) {
> +		/* allow READ using write delegation stateid */
> +		wronly = true;
> +		file->f_mode |= FMODE_READ;
> +	}

Is that really OK? Can we just upgrade the f_mode like that?

Also, what happens with more exotic exported filesystems like NFS? 

For example, if I'm reexporting NFS, the backend NFS server may not
allow you to do a READ operation using a OPEN4_SHARE_ACCESS_WRITE only
stateid. Won't this break in that case?

>  	if (file->f_op->splice_read && splice_ok)
>  		nfserr = nfsd4_encode_splice_read(resp, read, file, maxcount);
>  	else
>  		nfserr = nfsd4_encode_readv(resp, read, file, maxcount);
> +	if (wronly)
> +		file->f_mode &= ~FMODE_READ;
>  	if (nfserr) {
>  		xdr_truncate_encode(xdr, eof_offset);
>  		return nfserr;
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index c26ba86dbdfd..2f053beed899 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -426,6 +426,7 @@ struct nfsd4_read {
>  	struct svc_rqst		*rd_rqstp;          /* response */
>  	struct svc_fh		*rd_fhp;            /* response */
>  	u32			rd_eof;             /* response */
> +	struct nfs4_stid	*rd_wd_stid;        /* internal */
>  };
>  
>  struct nfsd4_readdir {

-- 
Jeff Layton <jlayton@kernel.org>