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

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2024-07-29 at 09:52 -0400, Chuck Lever wrote:
> On Mon, Jul 29, 2024 at 04:39:07PM +0300, Sagi Grimberg wrote:
> > 
> > 
> > 
> > On 29/07/2024 16:10, Jeff Layton wrote:
> > > On Mon, 2024-07-29 at 15:58 +0300, Sagi Grimberg wrote:
> > > > 
> > > > 
> > > > On 29/07/2024 15:10, 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) {
> > > > > > +			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.
> > > > Ah, this is something that I did not fully understand...
> > > > However it looks like st_access_bmap is not something that is
> > > > accounted on the delegation stateid...
> > > > 
> > > > Can I simply set it on the open stateid (stp)?
> > > That would likely fix the leak, but I'm not sure that's the best
> > > approach. You have an NFS4_SHARE_ACCESS_WRITE-only stateid here,
> > > and
> > > that would turn it a NFS4_SHARE_ACCESS_BOTH one, wouldn't it?
> > > 
> > > It wouldn't surprise me if that might break a testcase or two.
> > 
> > Well, if the server handed out a write delegation, isn't it
> > effectively
> > equivalent to
> > NFS4_SHARE_ACCESS_BOTH open?
> 
> It has to be equivalent, since the write delegation gives the client
> carte blanche to perform any open it wants to, locally. The server
> does not know about those local client-side opens, and it has a
> notification set up to fire if anyone else wants to open that file.
> 
> In nfs4_set_delegation(), we have this comment:
> 
> >      
> > /*                                                                 
> >          
> >        * Try for a write delegation first. RFC8881 section 10.4
> > says:             
> >       
> > *                                                                  
> >         
> >        *  "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.                                            
> >        */   
> 
> I think we did it this way only to circumvent the broken test cases.
> 
> A write delegation stateid uses a local O_RDWR OPEN, yes? If so, why
> can't it be used for READ operations already? All that has to be
> done is hand out the write delegation in the correct cases.
> 

It currently doesn't hold a reference to the fi_fds array at all. The
delegation is dependent on the open stateid holding the fi_fds array
open.

Maybe we should have delegations hold references to those too? That
might help us implement the delstid RFC later as well.

> Instead of gating the delegation on the intent presented by the OPEN
> operation, gate it on whether the user who is opening the file has
> both read and write access to the file.
> 

This seems like a reasonable approach.
-- 
Jeff Layton <jlayton@kernel.org>