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@xxxxxxxxxxx>
> > > > > > ---
> > > > > > 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@xxxxxxxxxx>
