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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 






On 29/07/2024 16:17, Jeff Layton wrote:
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.

From the amount of non-trivial work this hunk is doing, I'm starting to think perhaps
we need something simpler?

Maybe call nfs4_get_vfs_file() instead? and modify the open->op_share_access before calling it?
Although I'm not fully sure if things will blow up or not...




[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux