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.
--
Jeff Layton <jlayton@xxxxxxxxxx>
