diff mbox series

[v2,3/3] nfsd: resolve possible conflicts of reads using special stateids and write delegations

Message ID 20240728204104.519041-4-sagi@grimberg.me (mailing list archive)
State New
Headers show
Series Offer write delegations for O_WRONLY opens | expand

Commit Message

Sagi Grimberg July 28, 2024, 8:41 p.m. UTC 
In case a client issues a read using a special (anonymous) stateid, we need
to check if there is a conflicting write delegation already given to a different
client. If it is, recall the delegation before processing the read.

If the client holding the write delegation sends this read, it probably needs
to be fixed, but nonetheless, as the spec allows it, we accept the read, and
don't recall the client delegation.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 fs/nfsd/nfs4proc.c  | 11 +++++++----
 fs/nfsd/nfs4state.c | 47 +++++++++++++++++++++++++++++++++++++++++++++
 fs/nfsd/state.h     |  2 ++
 3 files changed, 56 insertions(+), 4 deletions(-)

Comments

kernel test robot July 29, 2024, 12:35 a.m. UTC | #1 
Hi Sagi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.11-rc1 next-20240726]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sagi-Grimberg/nfsd-don-t-assume-copy-notify-when-preprocessing-the-stateid/20240729-044834
base:   linus/master
patch link:    https://lore.kernel.org/r/20240728204104.519041-4-sagi%40grimberg.me
patch subject: [PATCH v2 3/3] nfsd: resolve possible conflicts of reads using special stateids and write delegations
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20240729/202407290814.7REsmaH7-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240729/202407290814.7REsmaH7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407290814.7REsmaH7-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> fs/nfsd/nfs4state.c:8826: warning: Excess function parameter 'inode' description in 'nfsd4_deleg_read_conflict'
>> fs/nfsd/nfs4state.c:8826: warning: Excess function parameter 'modified' description in 'nfsd4_deleg_read_conflict'
>> fs/nfsd/nfs4state.c:8826: warning: Excess function parameter 'size' description in 'nfsd4_deleg_read_conflict'


vim +8826 fs/nfsd/nfs4state.c

  8805	
  8806	/**
  8807	 * nfsd4_deleg_read_conflict - Recall if read causes conflict
  8808	 * @rqstp: RPC transaction context
  8809	 * @clp: nfs client
  8810	 * @fhp: nfs file handle
  8811	 * @inode: file to be checked for a conflict
  8812	 * @modified: return true if file was modified
  8813	 * @size: new size of file if modified is true
  8814	 *
  8815	 * This function is called when there is a conflict between a write
  8816	 * delegation and a read that is using a special stateid where the
  8817	 * we cannot derive the client stateid exsistence. The server
  8818	 * must recall a conflicting delegation before allowing the read
  8819	 * to continue.
  8820	 *
  8821	 * Returns 0 if there is no conflict; otherwise an nfs_stat
  8822	 * code is returned.
  8823	 */
  8824	__be32 nfsd4_deleg_read_conflict(struct svc_rqst *rqstp,
  8825			struct nfs4_client *clp, struct svc_fh *fhp)
> 8826	{
  8827		struct nfs4_file *fp;
  8828		__be32 status = 0;
  8829	
  8830		fp = nfsd4_file_hash_lookup(fhp);
  8831		if (!fp)
  8832			return nfs_ok;
  8833	
  8834		spin_lock(&state_lock);
  8835		spin_lock(&fp->fi_lock);
  8836		if (!list_empty(&fp->fi_delegations) &&
  8837		    !nfs4_delegation_exists(clp, fp)) {
  8838			/* conflict, recall deleg */
  8839			status = nfserrno(nfsd_open_break_lease(fp->fi_inode,
  8840						NFSD_MAY_READ));
  8841			if (status)
  8842				goto out;
  8843			if (!nfsd_wait_for_delegreturn(rqstp, fp->fi_inode))
  8844				status = nfserr_jukebox;
  8845		}
  8846	out:
  8847		spin_unlock(&fp->fi_lock);
  8848		spin_unlock(&state_lock);
  8849		return status;
  8850	}
  8851
Sagi Grimberg July 29, 2024, 6:22 a.m. UTC | #2 
On 29/07/2024 3:35, kernel test robot wrote:
> Hi Sagi,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on linus/master]
> [also build test WARNING on v6.11-rc1 next-20240726]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Sagi-Grimberg/nfsd-don-t-assume-copy-notify-when-preprocessing-the-stateid/20240729-044834
> base:   linus/master
> patch link:    https://lore.kernel.org/r/20240728204104.519041-4-sagi%40grimberg.me
> patch subject: [PATCH v2 3/3] nfsd: resolve possible conflicts of reads using special stateids and write delegations
> config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20240729/202407290814.7REsmaH7-lkp@intel.com/config)
> compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240729/202407290814.7REsmaH7-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202407290814.7REsmaH7-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
>>> fs/nfsd/nfs4state.c:8826: warning: Excess function parameter 'inode' description in 'nfsd4_deleg_read_conflict'
>>> fs/nfsd/nfs4state.c:8826: warning: Excess function parameter 'modified' description in 'nfsd4_deleg_read_conflict'
>>> fs/nfsd/nfs4state.c:8826: warning: Excess function parameter 'size' description in 'nfsd4_deleg_read_conflict'
>
> vim +8826 fs/nfsd/nfs4state.c
>
>    8805	
>    8806	/**
>    8807	 * nfsd4_deleg_read_conflict - Recall if read causes conflict
>    8808	 * @rqstp: RPC transaction context
>    8809	 * @clp: nfs client
>    8810	 * @fhp: nfs file handle
>    8811	 * @inode: file to be checked for a conflict
>    8812	 * @modified: return true if file was modified
>    8813	 * @size: new size of file if modified is true
>    8814	 *
>    8815	 * This function is called when there is a conflict between a write
>    8816	 * delegation and a read that is using a special stateid where the
>    8817	 * we cannot derive the client stateid exsistence. The server
>    8818	 * must recall a conflicting delegation before allowing the read
>    8819	 * to continue.
>    8820	 *
>    8821	 * Returns 0 if there is no conflict; otherwise an nfs_stat
>    8822	 * code is returned.
>    8823	 */
>    8824	__be32 nfsd4_deleg_read_conflict(struct svc_rqst *rqstp,
>    8825			struct nfs4_client *clp, struct svc_fh *fhp)
>> 8826	{
>    8827		struct nfs4_file *fp;
>    8828		__be32 status = 0;
>    8829	
>    8830		fp = nfsd4_file_hash_lookup(fhp);
>    8831		if (!fp)
>    8832			return nfs_ok;
>    8833	
>    8834		spin_lock(&state_lock);
>    8835		spin_lock(&fp->fi_lock);
>    8836		if (!list_empty(&fp->fi_delegations) &&
>    8837		    !nfs4_delegation_exists(clp, fp)) {
>    8838			/* conflict, recall deleg */
>    8839			status = nfserrno(nfsd_open_break_lease(fp->fi_inode,
>    8840						NFSD_MAY_READ));
>    8841			if (status)
>    8842				goto out;
>    8843			if (!nfsd_wait_for_delegreturn(rqstp, fp->fi_inode))
>    8844				status = nfserr_jukebox;
>    8845		}
>    8846	out:
>    8847		spin_unlock(&fp->fi_lock);
>    8848		spin_unlock(&state_lock);
>    8849		return status;
>    8850	}
>    8851	
>

Its not clear what is the warning about here...
Miguel Ojeda July 29, 2024, 7:55 a.m. UTC | #3 
On Mon, Jul 29, 2024 at 8:23 AM Sagi Grimberg <sagi@grimberg.me> wrote:
>
> Its not clear what is the warning about here...

Yeah, it would be nice if it said from which "step" it came from, i.e.
from the docs. Or if `kernel-doc` prefixed itself there (like
`objtool` does).

`@inode` etc. are not parameters of the function but are documented in
the patch.

Cheers,
Miguel
diff mbox series

Patch

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 041bcc3ab5d7..8a61c3bb2289 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -987,10 +987,13 @@  nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	 * 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)) {
+		if (!read->rd_wd_stid) {
+			/* special stateid? */
+			status = nfsd4_deleg_read_conflict(rqstp, cstate->clp,
+				&cstate->current_fh);
+		} else if (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;
 		}
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 538b6e1127a2..a6c6d813c59c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -8803,6 +8803,53 @@  nfsd4_get_writestateid(struct nfsd4_compound_state *cstate,
 	get_stateid(cstate, &u->write.wr_stateid);
 }
 
+/**
+ * nfsd4_deleg_read_conflict - Recall if read causes conflict
+ * @rqstp: RPC transaction context
+ * @clp: nfs client
+ * @fhp: nfs file handle
+ * @inode: file to be checked for a conflict
+ * @modified: return true if file was modified
+ * @size: new size of file if modified is true
+ *
+ * This function is called when there is a conflict between a write
+ * delegation and a read that is using a special stateid where the
+ * we cannot derive the client stateid exsistence. The server
+ * must recall a conflicting delegation before allowing the read
+ * to continue.
+ *
+ * Returns 0 if there is no conflict; otherwise an nfs_stat
+ * code is returned.
+ */
+__be32 nfsd4_deleg_read_conflict(struct svc_rqst *rqstp,
+		struct nfs4_client *clp, struct svc_fh *fhp)
+{
+	struct nfs4_file *fp;
+	__be32 status = 0;
+
+	fp = nfsd4_file_hash_lookup(fhp);
+	if (!fp)
+		return nfs_ok;
+
+	spin_lock(&state_lock);
+	spin_lock(&fp->fi_lock);
+	if (!list_empty(&fp->fi_delegations) &&
+	    !nfs4_delegation_exists(clp, fp)) {
+		/* conflict, recall deleg */
+		status = nfserrno(nfsd_open_break_lease(fp->fi_inode,
+					NFSD_MAY_READ));
+		if (status)
+			goto out;
+		if (!nfsd_wait_for_delegreturn(rqstp, fp->fi_inode))
+			status = nfserr_jukebox;
+	}
+out:
+	spin_unlock(&fp->fi_lock);
+	spin_unlock(&state_lock);
+	return status;
+}
+
+
 /**
  * nfsd4_deleg_getattr_conflict - Recall if GETATTR causes conflict
  * @rqstp: RPC transaction context
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index ffc217099d19..c1f13b5877c6 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -780,6 +780,8 @@  static inline bool try_to_expire_client(struct nfs4_client *clp)
 	return clp->cl_state == NFSD4_EXPIRABLE;
 }
 
+extern __be32 nfsd4_deleg_read_conflict(struct svc_rqst *rqstp,
+		struct nfs4_client *clp, struct svc_fh *fhp);
 extern __be32 nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp,
 		struct inode *inode, bool *file_modified, u64 *size);
 #endif   /* NFSD4_STATE_H */