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

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2024-07-29 at 15:59 +0300, Sagi Grimberg wrote:
> 
> 
> 
> On 29/07/2024 15:26, Jeff Layton wrote:
> > 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) {
> > More comments on this part:
> > 
> > 
> > nit: You've already tested for NFS4_SHARE_ACCESS_WRITE here, and
> > this
> > seems easier to read:
> > 
> > 		if (!(open->op_share_access &
> > NFS4_SHARE_ACCESS_READ))
> > 
> > 
> > 
> > > +			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
> > > _ACCESS_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;
> > How do you know that this fd isn't already set? Also, this field is
> > protected by the sc_file->fi_lock and that's not held here.
> 
> Something like this?
> --
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index a6c6d813c59c..ee0c65ff1940 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5978,19 +5978,24 @@ nfs4_open_delegation(struct svc_rqst *rqstp, 
> struct nfsd4_open *open,
>                  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) {
> +               if (!(open->op_share_access &
> NFS4_SHARE_ACCESS_READ)) {
> +                       u32 access = 
> nfs4_access_to_access(NFS4_SHARE_ACCESS_BOTH);
> +                       struct nfs4_file *fp = stp->st_stid.sc_file;
>                          struct nfsd_file *nf = NULL;
> 
>                          /* make sure the file is opened locally for 
> O_RDWR */
> +                       set_access(access, stp);
>                          status = nfsd_file_acquire_opened(rqstp,
> currentfh,
> - nfs4_access_to_access(NFS4_SHARE_ACCESS_BOTH),
> -                               open->op_filp, &nf);
> +                                       access, 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;
> +                       spin_lock(&fp->fi_lock);
> +                       if (!fp->fi_fds[O_RDWR])
> +                               fp->fi_fds[O_RDWR] = nf;
> +                       spin_lock(&fp->fi_lock);
>                  }
>          } else {
> --
> 

Your MUA mangled it a bit, but that probably would work. You do also
need to put the nf reference though if you don't assign
fp->fi_fds[O_RDWR] here.

-- 
Jeff Layton <jlayton@kernel.org>