Re: [PATCH v2 2/3] nfsd: Offer write delegations for o_wronly opens

[Jeff Layton <jlayton@kernel.org>]

On Sun, 2024-07-28 at 23:41 +0300, Sagi Grimberg wrote:
> In order to support write delegations with O_WRONLY opens, we need to
> allow the clients to read using the write delegation stateid (Per RFC
> 8881 section 9.1.2. Use of the Stateid and Locking).
> 
> Hence, we check for NFS4_SHARE_ACCESS_WRITE set in open request, and
> in case the share access flag does not set NFS4_SHARE_ACCESS_READ as
> well, we'll open the file locally with O_RDWR in order to allow the
> client to use the write delegation stateid when issuing a read in
> case
> it may choose to.
> 
> Plus, find_rw_file singular call-site is now removed, remove it
> altogether.
> 
> Note: reads using special stateids that conflict with pending write
> delegations are undetected, and will be covered in a follow on patch.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>  fs/nfsd/nfs4proc.c  | 18 +++++++++++++++++-
>  fs/nfsd/nfs4state.c | 42 ++++++++++++++++++++----------------------
>  fs/nfsd/xdr4.h      |  2 ++
>  3 files changed, 39 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 7b70309ad8fb..041bcc3ab5d7 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -979,8 +979,22 @@ 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) {
> +		if (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;
>  	return status;
> @@ -990,6 +1004,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/nfs4state.c b/fs/nfsd/nfs4state.c
> index 0645fccbf122..538b6e1127a2 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -639,18 +639,6 @@ find_readable_file(struct nfs4_file *f)
>  	return ret;
>  }
>  
> -static struct nfsd_file *
> -find_rw_file(struct nfs4_file *f)
> -{
> -	struct nfsd_file *ret;
> -
> -	spin_lock(&f->fi_lock);
> -	ret = nfsd_file_get(f->fi_fds[O_RDWR]);
> -	spin_unlock(&f->fi_lock);
> -
> -	return ret;
> -}
> -
>  struct nfsd_file *
>  find_any_file(struct nfs4_file *f)
>  {
> @@ -5784,15 +5772,11 @@ nfs4_set_delegation(struct nfsd4_open *open,
> struct nfs4_ol_stateid *stp,
>  	 *  "An OPEN_DELEGATE_WRITE delegation allows the client to
> handle,
>  	 *   on its own, all opens."
>  	 *
> -	 * Furthermore the client can use a write delegation for
> most READ
> -	 * operations as well, so we require a O_RDWR file here.
> -	 *
> -	 * Offer a write delegation in the case of a BOTH open, and
> ensure
> -	 * we get the O_RDWR descriptor.
> +	 * Offer a write delegation for WRITE or BOTH access
>  	 */
> -	if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) ==
> NFS4_SHARE_ACCESS_BOTH) {
> -		nf = find_rw_file(fp);
> +	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
>  		dl_type = NFS4_OPEN_DELEGATE_WRITE;
> +		nf = find_writeable_file(fp);
>  	}
>  
>  	/*
> @@ -5934,8 +5918,8 @@ static void nfsd4_open_deleg_none_ext(struct
> nfsd4_open *open, int status)
>   * open or lock state.
>   */
>  static void
> -nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid
> *stp,
> -		     struct svc_fh *currentfh)
> +nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open
> *open,
> +		struct nfs4_ol_stateid *stp, struct svc_fh
> *currentfh)
>  {
>  	struct nfs4_delegation *dp;
>  	struct nfs4_openowner *oo = openowner(stp->st_stateowner);
> @@ -5994,6 +5978,20 @@ nfs4_open_delegation(struct nfsd4_open *open,
> struct nfs4_ol_stateid *stp,
>  		dp->dl_cb_fattr.ncf_cur_fsize = stat.size;
>  		dp->dl_cb_fattr.ncf_initial_cinfo =
>  			nfsd4_change_attribute(&stat,
> d_inode(currentfh->fh_dentry));
> +		if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH)
> != NFS4_SHARE_ACCESS_BOTH) {
> +			struct nfsd_file *nf = NULL;
> +
> +			/* make sure the file is opened locally for
> O_RDWR */
> +			status = nfsd_file_acquire_opened(rqstp,
> currentfh,
> +				nfs4_access_to_access(NFS4_SHARE_ACC
> ESS_BOTH),
> +				open->op_filp, &nf);
> +			if (status) {
> +				nfs4_put_stid(&dp->dl_stid);
> +				destroy_delegation(dp);
> +				goto out_no_deleg;
> +			}
> +			stp->st_stid.sc_file->fi_fds[O_RDWR] = nf;

I have a bit of a concern here. When we go to put access references to
the fi_fds, that's done according to the st_access_bmap. Here though,
you're adding an extra reference for the O_RDWR fd, but I don't see
where you're calling set_access for that on the delegation stateid? Am
I missing where that happens? Not doing that may lead to fd leaks if it
was missed.

> +		}
>  	} else {
>  		open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
>  		trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
> @@ -6123,7 +6121,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp,
> struct svc_fh *current_fh, struct nf
>  	* Attempt to hand out a delegation. No error return, because
> the
>  	* OPEN succeeds even if we fail.
>  	*/
> -	nfs4_open_delegation(open, stp, &resp->cstate.current_fh);
> +	nfs4_open_delegation(rqstp, open, stp, &resp-
> >cstate.current_fh);
>  nodeleg:
>  	status = nfs_ok;
>  	trace_nfsd_open(&stp->st_stid.sc_stateid);
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index fbdd42cde1fa..434973a6a8b1 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -426,6 +426,8 @@ 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>