On Mon, Jul 29, 2024 at 10:12:46AM -0400, Jeff Layton wrote:
> On Mon, 2024-07-29 at 16:39 +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_W
> > > > > > RITE
> > > > > > )) {
> > > > > > + 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_S
> > > > > > HARE
> > > > > > _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?
>
> For the delegation, yes, but you're proposing to change an open stateid
> that was explicitly requested to be ACCESS_WRITE into ACCESS_BOTH.
>
> Given that you're doing this for a write delegation, that sort of
> implies that no other client has it open. Maybe it's OK in that case,
> but I think that if you do that then you'd need to convert the open
> stateid back into being ACCESS_WRITE when the delegation goes away.
>
> Otherwise you wouldn't (for instance) be able to set a DENY_READ on the
> file after doing an O_WRONLY open on it.
>
> Thinking about this a bit more though, I wonder if we ought to consider
> reworking the nfs4_file access altogether, especially in light of the
> recent delstid draft. Today, the open stateids hold all of the access
> refs, so if those go away while there is still an outstanding
> delegation, there's no guarantee we'll consider the file open at all
> anymore (AFAICS).
>
> Eventually we want to implement delstid support, in which case we'll
> need to support the situation where we may give out a delegation with
> no open stateid. It might be better to rework things along those lines
> instead.
Dai tells me that I assumed incorrectly that each delegation
stateid is backed by its own open file descriptor on the server.
So, if it's the case that delegation stateids are just references to
the file descriptor backing the open stateid, maybe we need to
address that first. Once a write delegation stateid is always backed
by a local O_RDWR open, I think READ with a write delegation stateid
will fall out naturally.
And, what you said here above suggests there could be additional
benefits to that approach.
--
Chuck Lever
