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@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) {
> > 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@xxxxxxxxxx>
