nfsd: open file caching for v2/3

Currently, preprocess_stateid_op calls nfs4_check_olstateid which verifies that the open stateid corresponds to the current_fh in the call by calling nfs4_check_fh.

If the stateid is a NFS4_DELEG_STID however, then no such check is done. Move the call to nfs4_check_fh into nfs4_check_file instead so that it can be done for all stateid types.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfsd/nfs4state.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

fs/nfsd/nfs4state.c +6 -5

index cd8c33186e26..75f617a052cf 100644
@@ -4406,9 +4406,9 @@ laundromat_main(struct work_struct *laundry)
 	queue_delayed_work(laundry_wq, &nn->laundromat_work, t*HZ);
 }
 
-static inline __be32 nfs4_check_fh(struct svc_fh *fhp, struct nfs4_ol_stateid *stp)
+static inline __be32 nfs4_check_fh(struct svc_fh *fhp, struct nfs4_stid *stp)
 {
-	if (!fh_match(&fhp->fh_handle, &stp->st_stid.sc_file->fi_fhandle))
+	if (!fh_match(&fhp->fh_handle, &stp->sc_file->fi_fhandle))
 		return nfserr_bad_stateid;
 	return nfs_ok;
 }

@@ -4611,9 +4611,6 @@ nfs4_check_olstateid(struct svc_fh *fhp, struct nfs4_ol_stateid *ols, int flags)
 {
 	__be32 status;
 
-	status = nfs4_check_fh(fhp, ols);
-	if (status)
-		return status;
 	status = nfsd4_check_openowner_confirmed(ols);
 	if (status)
 		return status;

@@ -4628,6 +4625,10 @@ nfs4_check_file(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfs4_stid *s,
 	struct file *file;
 	__be32 status;
 
+	status = nfs4_check_fh(fhp, s);
+	if (status)
+		return status;
+
 	file = nfs4_find_file(s, flags);
 	if (file) {
 		status = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,

Quoted 5 lines

fs/nfsd/nfs4state.c +0 -0

 		return status;

@@ -4628,6 +4625,10 @@ nfs4_check_file(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfs4_stid *s,
 	struct file *file;
 	__be32 status;
 

fs/nfsd/nfs4state.c nfs4_check_file(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfs4_stid *s, · line 4631

Quoted 4 lines

fs/nfsd/nfs4state.c +4 -0

+	status = nfs4_check_fh(fhp, s);
+	if (status)
+		return status;
+

This means we check the file handle for all stateids now, not just open and lock stateids. That seems reasonable to me but should be mentioned in the changelog.

So, this just adds this check to delegation stateids (which is necessary I think). That is mentioned in the changelog though. Do you think it needs more elaboration or is that sufficient?

Currently, nfsd uses a singlethread workqueue for this, but that's probably not necessary. If we have multiple namespaces, then there's no need to serialize the laundromat runs. They can run in parallel.

Also, create_singlethread_workqueue adds the WQ_MEM_RECLAIM flag, which doesn't really seem to be necessary. The laundromat jobs are always kicked off via a timer, and not from memory reclaim paths. There's no need for a rescuer thread.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfsd/nfs4state.c | 23 +++++------------------
 fs/nfsd/nfsd.h      |  1 +
 fs/nfsd/nfssvc.c    | 14 +++++++++++++-
 3 files changed, 19 insertions(+), 19 deletions(-)

fs/nfsd/nfs4state.c +5 -18

index 79795c898dd1..977a8aee9122 100644
@@ -4376,7 +4376,6 @@ nfs4_laundromat(struct nfsd_net *nn)
 	return new_timeo;
 }
 
-static struct workqueue_struct *laundry_wq;
 static void laundromat_main(struct work_struct *);
 
 static void

@@ -4390,7 +4389,7 @@ laundromat_main(struct work_struct *laundry)
 
 	t = nfs4_laundromat(nn);
 	dprintk("NFSD: laundromat_main - sleeping for %ld seconds\n", t);
-	queue_delayed_work(laundry_wq, &nn->laundromat_work, t*HZ);
+	queue_delayed_work(nfsd_laundry_wq, &nn->laundromat_work, t*HZ);
 }
 
 static inline __be32 nfs4_check_fh(struct svc_fh *fhp, struct nfs4_stid *stp)

@@ -6587,7 +6586,8 @@ nfs4_state_start_net(struct net *net)
 	nfsd4_client_tracking_init(net);
 	printk(KERN_INFO "NFSD: starting %ld-second grace period (net %p)\n",
 	       nn->nfsd4_grace, net);
-	queue_delayed_work(laundry_wq, &nn->laundromat_work, nn->nfsd4_grace * HZ);
+	queue_delayed_work(nfsd_laundry_wq, &nn->laundromat_work,
+				nn->nfsd4_grace * HZ);
 	return 0;
 }
 

@@ -6601,22 +6601,10 @@ nfs4_state_start(void)
 	ret = set_callback_cred();
 	if (ret)
 		return -ENOMEM;
-	laundry_wq = create_singlethread_workqueue("nfsd4");
-	if (laundry_wq == NULL) {
-		ret = -ENOMEM;
-		goto out_recovery;
-	}
 	ret = nfsd4_create_callback_queue();
-	if (ret)
-		goto out_free_laundry;
-
-	set_max_delegations();
-
-	return 0;
+	if (!ret)
+		set_max_delegations();
 
-out_free_laundry:
-	destroy_workqueue(laundry_wq);
-out_recovery:
 	return ret;
 }
 

@@ -6653,7 +6641,6 @@ nfs4_state_shutdown_net(struct net *net)
 void
 nfs4_state_shutdown(void)
 {
-	destroy_workqueue(laundry_wq);
 	nfsd4_destroy_callback_queue();
 }
 

fs/nfsd/nfsd.h +1 -0

index cf980523898b..0199415344ff 100644
@@ -62,6 +62,7 @@ struct readdir_cd {
 extern struct svc_program	nfsd_program;
 extern struct svc_version	nfsd_version2, nfsd_version3,
 				nfsd_version4;
+extern struct workqueue_struct	*nfsd_laundry_wq;
 extern struct mutex		nfsd_mutex;
 extern spinlock_t		nfsd_drc_lock;
 extern unsigned long		nfsd_drc_max_mem;

fs/nfsd/nfssvc.c +13 -1

index ad4e2377dd63..ced9944201a0 100644
@@ -17,6 +17,7 @@
 #include <linux/lockd/bind.h>
 #include <linux/nfsacl.h>
 #include <linux/seq_file.h>
+#include <linux/workqueue.h>
 #include <net/net_namespace.h>
 #include "nfsd.h"
 #include "cache.h"

@@ -28,6 +29,9 @@
 extern struct svc_program	nfsd_program;
 static int			nfsd(void *vrqstp);
 
+/* A workqueue for nfsd-related cleanup tasks */
+struct workqueue_struct		*nfsd_laundry_wq;
+
 /*
  * nfsd_mutex protects nn->nfsd_serv -- both the pointer itself and the members
  * of the svc_serv struct. In particular, ->sv_nrthreads but also to some

@@ -224,11 +228,19 @@ static int nfsd_startup_generic(int nrservs)
 	if (ret)
 		goto dec_users;
 
+	ret = -ENOMEM;
+	nfsd_laundry_wq = alloc_workqueue("%s", WQ_UNBOUND, 0, "nfsd-laundry");
+	if (!nfsd_laundry_wq)
+		goto out_racache;
+
 	ret = nfs4_state_start();
 	if (ret)
-		goto out_racache;
+		goto out_wq;
 	return 0;
 
+out_wq:
+	destroy_workqueue(nfsd_laundry_wq);
+	nfsd_laundry_wq = NULL;
 out_racache:
 	nfsd_racache_shutdown();
 dec_users:

Signature · 1 line

2.4.3

Quoted 3 lines · Jeff Layton

Currently, nfsd uses a singlethread workqueue for this, but that's probably not necessary. If we have multiple namespaces, then there's no need to serialize the laundromat runs. They can run in parallel.

Running a singled-threaded workqueue for each namespace is one thing, but if we're allowing multiple work items for one namespace to run simultaneously then this would need more of an argument.

However... I guess we only requeue this from the end of the work itself, so there's no risk of concurrent execution here, OK, fine.

--b.

Quoted 134 lines

Also, create_singlethread_workqueue adds the WQ_MEM_RECLAIM flag, which doesn't really seem to be necessary. The laundromat jobs are always kicked off via a timer, and not from memory reclaim paths. There's no need for a rescuer thread.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfsd/nfs4state.c | 23 +++++------------------
 fs/nfsd/nfsd.h      |  1 +
 fs/nfsd/nfssvc.c    | 14 +++++++++++++-
 3 files changed, 19 insertions(+), 19 deletions(-)

fs/nfsd/nfs4state.c +5 -18

index 79795c898dd1..977a8aee9122 100644
@@ -4376,7 +4376,6 @@ nfs4_laundromat(struct nfsd_net *nn)
 	return new_timeo;
 }
 
-static struct workqueue_struct *laundry_wq;
 static void laundromat_main(struct work_struct *);
 
 static void

@@ -4390,7 +4389,7 @@ laundromat_main(struct work_struct *laundry)
 
 	t = nfs4_laundromat(nn);
 	dprintk("NFSD: laundromat_main - sleeping for %ld seconds\n", t);
-	queue_delayed_work(laundry_wq, &nn->laundromat_work, t*HZ);
+	queue_delayed_work(nfsd_laundry_wq, &nn->laundromat_work, t*HZ);
 }
 
 static inline __be32 nfs4_check_fh(struct svc_fh *fhp, struct nfs4_stid *stp)

@@ -6587,7 +6586,8 @@ nfs4_state_start_net(struct net *net)
 	nfsd4_client_tracking_init(net);
 	printk(KERN_INFO "NFSD: starting %ld-second grace period (net %p)\n",
 	       nn->nfsd4_grace, net);
-	queue_delayed_work(laundry_wq, &nn->laundromat_work, nn->nfsd4_grace * HZ);
+	queue_delayed_work(nfsd_laundry_wq, &nn->laundromat_work,
+				nn->nfsd4_grace * HZ);
 	return 0;
 }
 

@@ -6601,22 +6601,10 @@ nfs4_state_start(void)
 	ret = set_callback_cred();
 	if (ret)
 		return -ENOMEM;
-	laundry_wq = create_singlethread_workqueue("nfsd4");
-	if (laundry_wq == NULL) {
-		ret = -ENOMEM;
-		goto out_recovery;
-	}
 	ret = nfsd4_create_callback_queue();
-	if (ret)
-		goto out_free_laundry;
-
-	set_max_delegations();
-
-	return 0;
+	if (!ret)
+		set_max_delegations();
 
-out_free_laundry:
-	destroy_workqueue(laundry_wq);
-out_recovery:
 	return ret;
 }
 

@@ -6653,7 +6641,6 @@ nfs4_state_shutdown_net(struct net *net)
 void
 nfs4_state_shutdown(void)
 {
-	destroy_workqueue(laundry_wq);
 	nfsd4_destroy_callback_queue();
 }
 

fs/nfsd/nfsd.h +1 -0

index cf980523898b..0199415344ff 100644
@@ -62,6 +62,7 @@ struct readdir_cd {
 extern struct svc_program	nfsd_program;
 extern struct svc_version	nfsd_version2, nfsd_version3,
 				nfsd_version4;
+extern struct workqueue_struct	*nfsd_laundry_wq;
 extern struct mutex		nfsd_mutex;
 extern spinlock_t		nfsd_drc_lock;
 extern unsigned long		nfsd_drc_max_mem;

fs/nfsd/nfssvc.c +13 -1

index ad4e2377dd63..ced9944201a0 100644
@@ -17,6 +17,7 @@
 #include <linux/lockd/bind.h>
 #include <linux/nfsacl.h>
 #include <linux/seq_file.h>
+#include <linux/workqueue.h>
 #include <net/net_namespace.h>
 #include "nfsd.h"
 #include "cache.h"

@@ -28,6 +29,9 @@
 extern struct svc_program	nfsd_program;
 static int			nfsd(void *vrqstp);
 
+/* A workqueue for nfsd-related cleanup tasks */
+struct workqueue_struct		*nfsd_laundry_wq;
+
 /*
  * nfsd_mutex protects nn->nfsd_serv -- both the pointer itself and the members
  * of the svc_serv struct. In particular, ->sv_nrthreads but also to some

@@ -224,11 +228,19 @@ static int nfsd_startup_generic(int nrservs)
 	if (ret)
 		goto dec_users;
 
+	ret = -ENOMEM;
+	nfsd_laundry_wq = alloc_workqueue("%s", WQ_UNBOUND, 0, "nfsd-laundry");
+	if (!nfsd_laundry_wq)
+		goto out_racache;
+
 	ret = nfs4_state_start();
 	if (ret)
-		goto out_racache;
+		goto out_wq;
 	return 0;
 
+out_wq:
+	destroy_workqueue(nfsd_laundry_wq);
+	nfsd_laundry_wq = NULL;
 out_racache:
 	nfsd_racache_shutdown();
 dec_users:

Signature · 1 line

2.4.3

We'll want to reuse some of this for common open file caching infrastructure.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfsd/filecache.h | 25 +++++++++++++++++++++++++
 fs/nfsd/nfs4state.c | 17 ++---------------
 2 files changed, 27 insertions(+), 15 deletions(-)
 create mode 100644 fs/nfsd/filecache.h

fs/nfsd/filecache.h new file +10 -0

new file mode 100644
index 000000000000..9051ee54faa3
@@ -0,0 +1,25 @@
+#ifndef _FS_NFSD_FILECACHE_H
+#define _FS_NFSD_FILECACHE_H
+
+#include <linux/jhash.h>
+#include <linux/sunrpc/xdr.h>
+
+#include "export.h"
+
+/* hash table for nfs4_file */
+#define NFSD_FILE_HASH_BITS                   8

fs/nfsd/filecache.h new file +5 -0

+{
+	return nfsd_fh_hashval(fh) & (NFSD_FILE_HASH_SIZE - 1);
+}
+
+#endif /* _FS_NFSD_FILECACHE_H */

fs/nfsd/nfs4state.c +2 -15

index 43903abd6189..b3b306bd1830 100644
@@ -49,6 +49,7 @@
 
 #include "netns.h"
 #include "pnfs.h"
+#include "filecache.h"
 
 #define NFSDDBG_FACILITY                NFSDDBG_PROC
 

@@ -381,21 +382,7 @@ static unsigned int ownerstr_hashval(struct xdr_netobj *ownername)
 	return ret & OWNER_HASH_MASK;
 }
 
-/* hash table for nfs4_file */
-#define FILE_HASH_BITS                   8
-#define FILE_HASH_SIZE                  (1 << FILE_HASH_BITS)
-
-static unsigned int nfsd_fh_hashval(struct knfsd_fh *fh)
-{
-	return jhash2(fh->fh_base.fh_pad, XDR_QUADLEN(fh->fh_size), 0);
-}
-
-static unsigned int file_hashval(struct knfsd_fh *fh)
-{
-	return nfsd_fh_hashval(fh) & (FILE_HASH_SIZE - 1);
-}
-
-static struct hlist_head file_hashtbl[FILE_HASH_SIZE];
+static struct hlist_head file_hashtbl[NFSD_FILE_HASH_SIZE];
 
 static void
 __nfs4_file_get_access(struct nfs4_file *fp, u32 access)

Quoted 26 lines · Jeff Layton

We'll want to reuse some of this for common open file caching infrastructure.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfsd/filecache.h | 25 +++++++++++++++++++++++++
 fs/nfsd/nfs4state.c | 17 ++---------------
 2 files changed, 27 insertions(+), 15 deletions(-)
 create mode 100644 fs/nfsd/filecache.h

fs/nfsd/filecache.h new file +10 -0

new file mode 100644
index 000000000000..9051ee54faa3
@@ -0,0 +1,25 @@
+#ifndef _FS_NFSD_FILECACHE_H
+#define _FS_NFSD_FILECACHE_H
+
+#include <linux/jhash.h>
+#include <linux/sunrpc/xdr.h>
+
+#include "export.h"
+
+/* hash table for nfs4_file */
+#define NFSD_FILE_HASH_BITS                   8

fs/nfsd/filecache.h line 20

Quoted 10 lines

fs/nfsd/filecache.h new file +10 -0

+#define NFSD_FILE_HASH_SIZE                  (1 << NFSD_FILE_HASH_BITS)
+
+static inline unsigned int
+nfsd_fh_hashval(struct knfsd_fh *fh)
+{
+	return jhash2(fh->fh_base.fh_pad, XDR_QUADLEN(fh->fh_size), 0);
+}
+
+static inline unsigned int
+file_hashval(struct knfsd_fh *fh)

Update to nfsd_file_hashval maybe better ?

thanks,
Kinglong Mee

Quoted 41 lines

fs/nfsd/filecache.h new file +5 -0

+{
+	return nfsd_fh_hashval(fh) & (NFSD_FILE_HASH_SIZE - 1);
+}
+
+#endif /* _FS_NFSD_FILECACHE_H */

fs/nfsd/nfs4state.c +2 -15

index 43903abd6189..b3b306bd1830 100644
@@ -49,6 +49,7 @@
 
 #include "netns.h"
 #include "pnfs.h"
+#include "filecache.h"
 
 #define NFSDDBG_FACILITY                NFSDDBG_PROC
 

@@ -381,21 +382,7 @@ static unsigned int ownerstr_hashval(struct xdr_netobj *ownername)
 	return ret & OWNER_HASH_MASK;
 }
 
-/* hash table for nfs4_file */
-#define FILE_HASH_BITS                   8
-#define FILE_HASH_SIZE                  (1 << FILE_HASH_BITS)
-
-static unsigned int nfsd_fh_hashval(struct knfsd_fh *fh)
-{
-	return jhash2(fh->fh_base.fh_pad, XDR_QUADLEN(fh->fh_size), 0);
-}
-
-static unsigned int file_hashval(struct knfsd_fh *fh)
-{
-	return nfsd_fh_hashval(fh) & (FILE_HASH_SIZE - 1);
-}
-
-static struct hlist_head file_hashtbl[FILE_HASH_SIZE];
+static struct hlist_head file_hashtbl[NFSD_FILE_HASH_SIZE];
 
 static void
 __nfs4_file_get_access(struct nfs4_file *fp, u32 access)

Currently, nfsd uses a singlethread workqueue for this, but that's not necessary. If we have multiple namespaces, then there's no need to serialize the laundromat runs since they are only requeued at the end of the work itself.

Also, create_singlethread_workqueue adds the WQ_MEM_RECLAIM flag, which doesn't really seem to be necessary. The laundromat jobs are always kicked off via a timer, and not from memory reclaim paths. There's no need for a rescuer thread.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfsd/nfs4state.c | 23 +++++------------------
 fs/nfsd/nfsd.h      |  1 +
 fs/nfsd/nfssvc.c    | 14 +++++++++++++-
 3 files changed, 19 insertions(+), 19 deletions(-)

fs/nfsd/nfs4state.c +5 -18

index b3b306bd1830..c94859122e6f 100644
@@ -4366,7 +4366,6 @@ nfs4_laundromat(struct nfsd_net *nn)
 	return new_timeo;
 }
 
-static struct workqueue_struct *laundry_wq;
 static void laundromat_main(struct work_struct *);
 
 static void

@@ -4380,7 +4379,7 @@ laundromat_main(struct work_struct *laundry)
 
 	t = nfs4_laundromat(nn);
 	dprintk("NFSD: laundromat_main - sleeping for %ld seconds\n", t);
-	queue_delayed_work(laundry_wq, &nn->laundromat_work, t*HZ);
+	queue_delayed_work(nfsd_laundry_wq, &nn->laundromat_work, t*HZ);
 }
 
 static inline __be32 nfs4_check_fh(struct svc_fh *fhp, struct nfs4_stid *stp)

@@ -6569,7 +6568,8 @@ nfs4_state_start_net(struct net *net)
 	nfsd4_client_tracking_init(net);
 	printk(KERN_INFO "NFSD: starting %ld-second grace period (net %p)\n",
 	       nn->nfsd4_grace, net);
-	queue_delayed_work(laundry_wq, &nn->laundromat_work, nn->nfsd4_grace * HZ);
+	queue_delayed_work(nfsd_laundry_wq, &nn->laundromat_work,
+				nn->nfsd4_grace * HZ);
 	return 0;
 }
 

@@ -6583,22 +6583,10 @@ nfs4_state_start(void)
 	ret = set_callback_cred();
 	if (ret)
 		return -ENOMEM;
-	laundry_wq = create_singlethread_workqueue("nfsd4");
-	if (laundry_wq == NULL) {
-		ret = -ENOMEM;
-		goto out_recovery;
-	}
 	ret = nfsd4_create_callback_queue();
-	if (ret)
-		goto out_free_laundry;
-
-	set_max_delegations();
-
-	return 0;
+	if (!ret)
+		set_max_delegations();
 
-out_free_laundry:
-	destroy_workqueue(laundry_wq);
-out_recovery:
 	return ret;
 }
 

@@ -6635,7 +6623,6 @@ nfs4_state_shutdown_net(struct net *net)
 void
 nfs4_state_shutdown(void)
 {
-	destroy_workqueue(laundry_wq);
 	nfsd4_destroy_callback_queue();
 }
 

fs/nfsd/nfsd.h +1 -0

index cf980523898b..0199415344ff 100644
@@ -62,6 +62,7 @@ struct readdir_cd {
 extern struct svc_program	nfsd_program;
 extern struct svc_version	nfsd_version2, nfsd_version3,
 				nfsd_version4;
+extern struct workqueue_struct	*nfsd_laundry_wq;
 extern struct mutex		nfsd_mutex;
 extern spinlock_t		nfsd_drc_lock;
 extern unsigned long		nfsd_drc_max_mem;

fs/nfsd/nfssvc.c +9 -0

index ad4e2377dd63..ced9944201a0 100644
@@ -17,6 +17,7 @@
 #include <linux/lockd/bind.h>
 #include <linux/nfsacl.h>
 #include <linux/seq_file.h>
+#include <linux/workqueue.h>
 #include <net/net_namespace.h>
 #include "nfsd.h"
 #include "cache.h"

@@ -28,6 +29,9 @@
 extern struct svc_program	nfsd_program;
 static int			nfsd(void *vrqstp);
 
+/* A workqueue for nfsd-related cleanup tasks */
+struct workqueue_struct		*nfsd_laundry_wq;
+
 /*
  * nfsd_mutex protects nn->nfsd_serv -- both the pointer itself and the members
  * of the svc_serv struct. In particular, ->sv_nrthreads but also to some

@@ -224,11 +228,19 @@ static int nfsd_startup_generic(int nrservs)
 	if (ret)
 		goto dec_users;
 
+	ret = -ENOMEM;
+	nfsd_laundry_wq = alloc_workqueue("%s", WQ_UNBOUND, 0, "nfsd-laundry");
+	if (!nfsd_laundry_wq)
+		goto out_racache;
+
 	ret = nfs4_state_start();
 	if (ret)

Quoted 126 lines · Jeff Layton

Currently, nfsd uses a singlethread workqueue for this, but that's not necessary. If we have multiple namespaces, then there's no need to serialize the laundromat runs since they are only requeued at the end of the work itself.

Also, create_singlethread_workqueue adds the WQ_MEM_RECLAIM flag, which doesn't really seem to be necessary. The laundromat jobs are always kicked off via a timer, and not from memory reclaim paths. There's no need for a rescuer thread.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfsd/nfs4state.c | 23 +++++------------------
 fs/nfsd/nfsd.h      |  1 +
 fs/nfsd/nfssvc.c    | 14 +++++++++++++-
 3 files changed, 19 insertions(+), 19 deletions(-)

fs/nfsd/nfs4state.c +5 -18

index b3b306bd1830..c94859122e6f 100644
@@ -4366,7 +4366,6 @@ nfs4_laundromat(struct nfsd_net *nn)
 	return new_timeo;
 }
 
-static struct workqueue_struct *laundry_wq;
 static void laundromat_main(struct work_struct *);
 
 static void

@@ -4380,7 +4379,7 @@ laundromat_main(struct work_struct *laundry)
 
 	t = nfs4_laundromat(nn);
 	dprintk("NFSD: laundromat_main - sleeping for %ld seconds\n", t);
-	queue_delayed_work(laundry_wq, &nn->laundromat_work, t*HZ);
+	queue_delayed_work(nfsd_laundry_wq, &nn->laundromat_work, t*HZ);
 }
 
 static inline __be32 nfs4_check_fh(struct svc_fh *fhp, struct nfs4_stid *stp)

@@ -6569,7 +6568,8 @@ nfs4_state_start_net(struct net *net)
 	nfsd4_client_tracking_init(net);
 	printk(KERN_INFO "NFSD: starting %ld-second grace period (net %p)\n",
 	       nn->nfsd4_grace, net);
-	queue_delayed_work(laundry_wq, &nn->laundromat_work, nn->nfsd4_grace * HZ);
+	queue_delayed_work(nfsd_laundry_wq, &nn->laundromat_work,
+				nn->nfsd4_grace * HZ);
 	return 0;
 }
 

@@ -6583,22 +6583,10 @@ nfs4_state_start(void)
 	ret = set_callback_cred();
 	if (ret)
 		return -ENOMEM;
-	laundry_wq = create_singlethread_workqueue("nfsd4");
-	if (laundry_wq == NULL) {
-		ret = -ENOMEM;
-		goto out_recovery;
-	}
 	ret = nfsd4_create_callback_queue();
-	if (ret)
-		goto out_free_laundry;
-
-	set_max_delegations();
-
-	return 0;
+	if (!ret)
+		set_max_delegations();
 
-out_free_laundry:
-	destroy_workqueue(laundry_wq);
-out_recovery:
 	return ret;
 }
 

@@ -6635,7 +6623,6 @@ nfs4_state_shutdown_net(struct net *net)
 void
 nfs4_state_shutdown(void)
 {
-	destroy_workqueue(laundry_wq);
 	nfsd4_destroy_callback_queue();
 }
 

fs/nfsd/nfsd.h +1 -0

index cf980523898b..0199415344ff 100644
@@ -62,6 +62,7 @@ struct readdir_cd {
 extern struct svc_program	nfsd_program;
 extern struct svc_version	nfsd_version2, nfsd_version3,
 				nfsd_version4;
+extern struct workqueue_struct	*nfsd_laundry_wq;
 extern struct mutex		nfsd_mutex;
 extern spinlock_t		nfsd_drc_lock;
 extern unsigned long		nfsd_drc_max_mem;

fs/nfsd/nfssvc.c +9 -0

index ad4e2377dd63..ced9944201a0 100644
@@ -17,6 +17,7 @@
 #include <linux/lockd/bind.h>
 #include <linux/nfsacl.h>
 #include <linux/seq_file.h>
+#include <linux/workqueue.h>
 #include <net/net_namespace.h>
 #include "nfsd.h"
 #include "cache.h"

@@ -28,6 +29,9 @@
 extern struct svc_program	nfsd_program;
 static int			nfsd(void *vrqstp);
 
+/* A workqueue for nfsd-related cleanup tasks */
+struct workqueue_struct		*nfsd_laundry_wq;
+
 /*
  * nfsd_mutex protects nn->nfsd_serv -- both the pointer itself and the members
  * of the svc_serv struct. In particular, ->sv_nrthreads but also to some

@@ -224,11 +228,19 @@ static int nfsd_startup_generic(int nrservs)
 	if (ret)
 		goto dec_users;
 
+	ret = -ENOMEM;
+	nfsd_laundry_wq = alloc_workqueue("%s", WQ_UNBOUND, 0, "nfsd-laundry");
+	if (!nfsd_laundry_wq)
+		goto out_racache;
+
 	ret = nfs4_state_start();
 	if (ret)

fs/nfsd/nfssvc.c static int nfsd_startup_generic(int nrservs) · line 246

Quoted 10 lines

fs/nfsd/nfssvc.c +4 -1

-		goto out_racache;
+		goto out_wq;
 	return 0;
 
+out_wq:
+	destroy_workqueue(nfsd_laundry_wq);
+	nfsd_laundry_wq = NULL;
 out_racache:
 	nfsd_racache_shutdown();
 dec_users:

Is a destroy_workqueue(nfsd_laundry_wq) needed in nfsd_shutdown_generic() ?

thanks,
Kinglong Mee

Currently, nfsd uses a singlethread workqueue for this, but that's not necessary. If we have multiple namespaces, then there's no need to serialize the laundromat runs since they are only requeued at the end of the work itself.

Also, create_singlethread_workqueue adds the WQ_MEM_RECLAIM flag, which doesn't really seem to be necessary. The laundromat jobs are always kicked off via a timer, and not from memory reclaim paths. There's no need for a rescuer thread.

Quoted 5 lines · Jeff Layton

Currently, nfsd uses a singlethread workqueue for this, but that's not necessary. If we have multiple namespaces, then there's no need to serialize the laundromat runs since they are only requeued at the end of the work itself.

Quoted 4 lines

Also, create_singlethread_workqueue adds the WQ_MEM_RECLAIM flag, which doesn't really seem to be necessary. The laundromat jobs are always kicked off via a timer, and not from memory reclaim paths. There's no need for a rescuer thread.

Why would you change it to an unbound WQ? I'd really prefer to split the change of workqueue semantics and making it globall available, too.

create_singlethread_workqueue already makes an unbound workqueue. This patch just lifts the "max_active" value to the default, and removes the WQ_MEM_RECLAIM flag.

We certainly could turn this into a bound workqueue, but given the sort of job that the laundromat runs I'm not sure we'd benefit much from the locality.

...and sure, I can turn this into two patches if you'd prefer.

Quoted 9 lines · Jeff Layton

create_singlethread_workqueue already makes an unbound workqueue. This patch just lifts the "max_active" value to the default, and removes the WQ_MEM_RECLAIM flag.

We certainly could turn this into a bound workqueue, but given the sort of job that the laundromat runs I'm not sure we'd benefit much from the locality.

...and sure, I can turn this into two patches if you'd prefer.

The patch was just rather confusing to me. Do you want the existing laundromat to scale better with lots of namespaces? Sounds reasonable, but I don't really see the use case.

Looking at the later patches I now see you're overloading a totally different job to it. I don't think there's a point given how cheap workqueues are these days. Even more it seems like you really should use the mm/list_lru.c infrastructure and a shrinker for a your file cache.

I did consider a shrinker and LRU list for this. The problem there is that shrinkers are triggered on memory pressure. Keeping these files open after they've been idle for a long period of time would prevent the kernel from handing out leases on them, so closing them after a reasonable idle period seemed like the right thing to do.

I suppose however we could use a shrinker/LRU _and_ add a mechanism that would cause the kernel to close idle nfsd_files for an inode when there is an attempt to do a F_SETLEASE. That would probably work, unless I'm missing other reasons that keeping unused files open might be problematic. Are there any?

Quoted 5 lines · Jeff Layton

I did consider a shrinker and LRU list for this. The problem there is that shrinkers are triggered on memory pressure. Keeping these files open after they've been idle for a long period of time would prevent the kernel from handing out leases on them, so closing them after a reasonable idle period seemed like the right thing to do.

True.

Quoted 5 lines

I suppose however we could use a shrinker/LRU _and_ add a mechanism that would cause the kernel to close idle nfsd_files for an inode when there is an attempt to do a F_SETLEASE. That would probably work, unless I'm missing other reasons that keeping unused files open might be problematic. Are there any?

That seems reasonable. Keepign the file open also will prevent unmounting the file system, although currently any NFS export already causes that as well.

Quoted 15 lines · On Mon, 10 Aug 2015 05:10:35 -0700 Christoph Hellwig <[email protected]> wrote:

Quoted 5 lines · Jeff Layton

I did consider a shrinker and LRU list for this. The problem there is that shrinkers are triggered on memory pressure. Keeping these files open after they've been idle for a long period of time would prevent the kernel from handing out leases on them, so closing them after a reasonable idle period seemed like the right thing to do.

True.

Quoted 5 lines

I suppose however we could use a shrinker/LRU _and_ add a mechanism that would cause the kernel to close idle nfsd_files for an inode when there is an attempt to do a F_SETLEASE. That would probably work, unless I'm missing other reasons that keeping unused files open might be problematic. Are there any?

Quoted 3 lines

That seems reasonable. Keepign the file open also will prevent unmounting the file system, although currently any NFS export already causes that as well.

Yes, though that's the reason for the new ->flush hook in the sunrpc cache code. On any export table change, we'll clean out the nfsd_file cache to help ensure that you'll be able to unmount soon after unexporting a filesystem.

In any case, I'll look at the shrinker/lru thing for the next respin and see whether adding a hook into the setlease code might be reasonable.

Thanks for the review so far,

Signature · 1 line

Jeff Layton <[email protected]>

Currently, NFSv2/3 reads and writes have to open a file, do the read or write and then close it again for each RPC. This is highly inefficient, especially when the underlying filesystem has a relatively slow open routine.

This patch adds a new open file cache to knfsd. Rather than doing an open for each RPC, the read/write handlers can call into this cache to see if there is one already there for the correct filehandle and NFS_MAY_READ/WRITE flags.

If there isn't an entry, then we create a new one and attempt to perform the open. If there is, then we wait until the entry is fully instantiated and return it if it is at the end of the wait. If it's not, then we attempt to take over construction.

Since the main goal is to speed up NFSv2/3 I/O, we don't want to close these files on last put of these objects. We need to keep them around for a little while since we never know when the next READ/WRITE will come in.

Cache entries have a hardcoded 1s timeout, and we have a recurring workqueue job that walks the cache and purges any entries that have expired.

Signed-off-by Jeff Layton <[email protected]>

fs/nfsd/filecache.h +1 -0

index 9051ee54faa3..adf7e78b8e43 100644
@@ -4,6 +4,7 @@
 #include <linux/jhash.h>
 #include <linux/sunrpc/xdr.h>
 
+#include "nfsfh.h"
 #include "export.h"
 
 /* hash table for nfs4_file */

fs/nfsd/filecache.h +13 -0

+#define NFSD_FILE_PENDING	(1)
+	unsigned long		nf_flags;
+	struct knfsd_fh		nf_handle;
+	unsigned int		nf_hashval;
+	atomic_t		nf_ref;
+	unsigned char		nf_may;
+};
+
+int nfsd_file_cache_init(void);
+void nfsd_file_cache_shutdown(void);
+void nfsd_file_put(struct nfsd_file *nf);
+__be32 nfsd_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
+		  unsigned int may_flags, struct nfsd_file **nfp);
 #endif /* _FS_NFSD_FILECACHE_H */

fs/nfsd/nfssvc.c +9 -1

index ced9944201a0..0572441e23ec 100644
@@ -23,6 +23,7 @@
 #include "cache.h"
 #include "vfs.h"
 #include "netns.h"
+#include "filecache.h"
 
 #define NFSDDBG_FACILITY	NFSDDBG_SVC
 

@@ -233,11 +234,17 @@ static int nfsd_startup_generic(int nrservs)
 	if (!nfsd_laundry_wq)
 		goto out_racache;
 
-	ret = nfs4_state_start();
+	ret = nfsd_file_cache_init();
 	if (ret)
 		goto out_wq;
+
+	ret = nfs4_state_start();
+	if (ret)
+		goto out_nfsd_file;
 	return 0;
 
+out_nfsd_file:
+	nfsd_file_cache_shutdown();
 out_wq:
 	destroy_workqueue(nfsd_laundry_wq);
 	nfsd_laundry_wq = NULL;

@@ -254,6 +261,7 @@ static void nfsd_shutdown_generic(void)
 		return;
 
 	nfs4_state_shutdown();
+	nfsd_file_cache_shutdown();
 	nfsd_racache_shutdown();
 }
 

Quoted 25 lines · Jeff Layton

Currently, NFSv2/3 reads and writes have to open a file, do the read or write and then close it again for each RPC. This is highly inefficient, especially when the underlying filesystem has a relatively slow open routine.

This patch adds a new open file cache to knfsd. Rather than doing an open for each RPC, the read/write handlers can call into this cache to see if there is one already there for the correct filehandle and NFS_MAY_READ/WRITE flags.

If there isn't an entry, then we create a new one and attempt to perform the open. If there is, then we wait until the entry is fully instantiated and return it if it is at the end of the wait. If it's not, then we attempt to take over construction.

Since the main goal is to speed up NFSv2/3 I/O, we don't want to close these files on last put of these objects. We need to keep them around for a little while since we never know when the next READ/WRITE will come in.

Cache entries have a hardcoded 1s timeout, and we have a recurring workqueue job that walks the cache and purges any entries that have expired.

Signed-off-by Jeff Layton <[email protected]>

... snip ...

Quoted 16 lines

fs/nfsd/filecache.h +1 -0

index 9051ee54faa3..adf7e78b8e43 100644
@@ -4,6 +4,7 @@
 #include <linux/jhash.h>
 #include <linux/sunrpc/xdr.h>
 
+#include "nfsfh.h"
 #include "export.h"
 
 /* hash table for nfs4_file */

@@ -22,4 +23,24 @@ file_hashval(struct knfsd_fh *fh)
 	return nfsd_fh_hashval(fh) & (NFSD_FILE_HASH_SIZE - 1);
 }
 

fs/nfsd/filecache.h file_hashval(struct knfsd_fh *fh) · line 26

Quoted 1 line

fs/nfsd/filecache.h +1 -0

+struct nfsd_file {

There is a nfs4_file in nfsd, it's not easy to distinguish them for a new folk. More comments or a meaningful name is better.

fs/nfsd/filecache.h file_hashval(struct knfsd_fh *fh) · line 32

Quoted 6 lines

fs/nfsd/filecache.h +6 -0

+	struct hlist_node	nf_node;
+	struct list_head	nf_dispose;
+	struct rcu_head		nf_rcu;
+	struct file		*nf_file;
+	unsigned long		nf_time;
+#define NFSD_FILE_HASHED	(0)

Why not using hlist_unhashed()?

Quoted 54 lines

fs/nfsd/filecache.h +13 -0

+#define NFSD_FILE_PENDING	(1)
+	unsigned long		nf_flags;
+	struct knfsd_fh		nf_handle;
+	unsigned int		nf_hashval;
+	atomic_t		nf_ref;
+	unsigned char		nf_may;
+};
+
+int nfsd_file_cache_init(void);
+void nfsd_file_cache_shutdown(void);
+void nfsd_file_put(struct nfsd_file *nf);
+__be32 nfsd_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
+		  unsigned int may_flags, struct nfsd_file **nfp);
 #endif /* _FS_NFSD_FILECACHE_H */

fs/nfsd/nfssvc.c +9 -1

index ced9944201a0..0572441e23ec 100644
@@ -23,6 +23,7 @@
 #include "cache.h"
 #include "vfs.h"
 #include "netns.h"
+#include "filecache.h"
 
 #define NFSDDBG_FACILITY	NFSDDBG_SVC
 

@@ -233,11 +234,17 @@ static int nfsd_startup_generic(int nrservs)
 	if (!nfsd_laundry_wq)
 		goto out_racache;
 
-	ret = nfs4_state_start();
+	ret = nfsd_file_cache_init();
 	if (ret)
 		goto out_wq;
+
+	ret = nfs4_state_start();
+	if (ret)
+		goto out_nfsd_file;
 	return 0;
 
+out_nfsd_file:
+	nfsd_file_cache_shutdown();
 out_wq:
 	destroy_workqueue(nfsd_laundry_wq);
 	nfsd_laundry_wq = NULL;

@@ -254,6 +261,7 @@ static void nfsd_shutdown_generic(void)
 		return;
 
 	nfs4_state_shutdown();
+	nfsd_file_cache_shutdown();
 	nfsd_racache_shutdown();
 }
 

Quoted 43 lines · On Fri, 7 Aug 2015 23:28:41 +0800 Kinglong Mee <[email protected]> wrote:

Quoted 25 lines · Jeff Layton

Currently, NFSv2/3 reads and writes have to open a file, do the read or write and then close it again for each RPC. This is highly inefficient, especially when the underlying filesystem has a relatively slow open routine.

This patch adds a new open file cache to knfsd. Rather than doing an open for each RPC, the read/write handlers can call into this cache to see if there is one already there for the correct filehandle and NFS_MAY_READ/WRITE flags.

If there isn't an entry, then we create a new one and attempt to perform the open. If there is, then we wait until the entry is fully instantiated and return it if it is at the end of the wait. If it's not, then we attempt to take over construction.

Since the main goal is to speed up NFSv2/3 I/O, we don't want to close these files on last put of these objects. We need to keep them around for a little while since we never know when the next READ/WRITE will come in.

Cache entries have a hardcoded 1s timeout, and we have a recurring workqueue job that walks the cache and purges any entries that have expired.

Signed-off-by Jeff Layton <[email protected]>

... snip ...

Quoted 16 lines

fs/nfsd/filecache.h +1 -0

index 9051ee54faa3..adf7e78b8e43 100644
@@ -4,6 +4,7 @@
 #include <linux/jhash.h>
 #include <linux/sunrpc/xdr.h>
 
+#include "nfsfh.h"
 #include "export.h"
 
 /* hash table for nfs4_file */

@@ -22,4 +23,24 @@ file_hashval(struct knfsd_fh *fh)
 	return nfsd_fh_hashval(fh) & (NFSD_FILE_HASH_SIZE - 1);
 }
 

Quoted 5 lines

fs/nfsd/filecache.h file_hashval(struct knfsd_fh *fh) · line 26

Quoted 1 line

fs/nfsd/filecache.h +1 -0

+struct nfsd_file {

There is a nfs4_file in nfsd, it's not easy to distinguish them for a new folk. More comments or a meaningful name is better.

Maybe. Again, any suggestions? My hope was that eventually we can unify the two caches somehow but maybe that's naive.

Quoted 10 lines

fs/nfsd/filecache.h file_hashval(struct knfsd_fh *fh) · line 32

Quoted 7 lines

fs/nfsd/filecache.h +6 -0

+	struct hlist_node	nf_node;
+	struct list_head	nf_dispose;
+	struct rcu_head		nf_rcu;
+	struct file		*nf_file;
+	unsigned long		nf_time;
+#define NFSD_FILE_HASHED	(0)

Why not using hlist_unhashed()?

These entries are removed from the list using hlist_del_rcu(), and hlist_unhashed will not return true after that.

Quoted 56 lines

Quoted 56 lines

fs/nfsd/filecache.h +13 -0

+#define NFSD_FILE_PENDING	(1)
+	unsigned long		nf_flags;
+	struct knfsd_fh		nf_handle;
+	unsigned int		nf_hashval;
+	atomic_t		nf_ref;
+	unsigned char		nf_may;
+};
+
+int nfsd_file_cache_init(void);
+void nfsd_file_cache_shutdown(void);
+void nfsd_file_put(struct nfsd_file *nf);
+__be32 nfsd_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
+		  unsigned int may_flags, struct nfsd_file **nfp);
 #endif /* _FS_NFSD_FILECACHE_H */

fs/nfsd/nfssvc.c +9 -1

index ced9944201a0..0572441e23ec 100644
@@ -23,6 +23,7 @@
 #include "cache.h"
 #include "vfs.h"
 #include "netns.h"
+#include "filecache.h"
 
 #define NFSDDBG_FACILITY	NFSDDBG_SVC
 

@@ -233,11 +234,17 @@ static int nfsd_startup_generic(int nrservs)
 	if (!nfsd_laundry_wq)
 		goto out_racache;
 
-	ret = nfs4_state_start();
+	ret = nfsd_file_cache_init();
 	if (ret)
 		goto out_wq;
+
+	ret = nfs4_state_start();
+	if (ret)
+		goto out_nfsd_file;
 	return 0;
 
+out_nfsd_file:
+	nfsd_file_cache_shutdown();
 out_wq:
 	destroy_workqueue(nfsd_laundry_wq);
 	nfsd_laundry_wq = NULL;

@@ -254,6 +261,7 @@ static void nfsd_shutdown_generic(void)
 		return;
 
 	nfs4_state_shutdown();
+	nfsd_file_cache_shutdown();
 	nfsd_racache_shutdown();
 }
 

Quoted 46 lines · Jeff Layton

Quoted 43 lines · On Fri, 7 Aug 2015 23:28:41 +0800 Kinglong Mee <[email protected]> wrote:

Quoted 25 lines · Jeff Layton

Currently, NFSv2/3 reads and writes have to open a file, do the read or write and then close it again for each RPC. This is highly inefficient, especially when the underlying filesystem has a relatively slow open routine.

This patch adds a new open file cache to knfsd. Rather than doing an open for each RPC, the read/write handlers can call into this cache to see if there is one already there for the correct filehandle and NFS_MAY_READ/WRITE flags.

If there isn't an entry, then we create a new one and attempt to perform the open. If there is, then we wait until the entry is fully instantiated and return it if it is at the end of the wait. If it's not, then we attempt to take over construction.

Since the main goal is to speed up NFSv2/3 I/O, we don't want to close these files on last put of these objects. We need to keep them around for a little while since we never know when the next READ/WRITE will come in.

Cache entries have a hardcoded 1s timeout, and we have a recurring workqueue job that walks the cache and purges any entries that have expired.

Signed-off-by Jeff Layton <[email protected]>

... snip ...

Quoted 16 lines

fs/nfsd/filecache.h +1 -0

index 9051ee54faa3..adf7e78b8e43 100644
@@ -4,6 +4,7 @@
 #include <linux/jhash.h>
 #include <linux/sunrpc/xdr.h>
 
+#include "nfsfh.h"
 #include "export.h"
 
 /* hash table for nfs4_file */

@@ -22,4 +23,24 @@ file_hashval(struct knfsd_fh *fh)
 	return nfsd_fh_hashval(fh) & (NFSD_FILE_HASH_SIZE - 1);
 }
 

Quoted 8 lines

Quoted 5 lines

fs/nfsd/filecache.h file_hashval(struct knfsd_fh *fh) · line 26

Quoted 1 line

fs/nfsd/filecache.h +1 -0

+struct nfsd_file {

There is a nfs4_file in nfsd, it's not easy to distinguish them for a new folk. More comments or a meaningful name is better.

Maybe. Again, any suggestions? My hope was that eventually we can unify the two caches somehow but maybe that's naive.

I cannot find a better name for the new file cache. More comments.

I don't agree with merging those two into one cache.

nfsv4's file cache is a state resource of client which will exist since close or lease expire. But nfsd_file cache is a temporary resource for nfsv2/v3 client without state.

Also, for nfsv4's conflict checking, should we check the temporary file cache for nfsv2/v3 too?

Quoted 11 lines

Quoted 10 lines

fs/nfsd/filecache.h file_hashval(struct knfsd_fh *fh) · line 32

Quoted 7 lines

fs/nfsd/filecache.h +6 -0

+	struct hlist_node	nf_node;
+	struct list_head	nf_dispose;
+	struct rcu_head		nf_rcu;
+	struct file		*nf_file;
+	unsigned long		nf_time;
+#define NFSD_FILE_HASHED	(0)

Why not using hlist_unhashed()?

Quoted 2 lines

These entries are removed from the list using hlist_del_rcu(), and hlist_unhashed will not return true after that.

As I understand, NFSD_FILE_HASHED means the file cache is added to nfsd_file_hashtbl, and increased the global file count.

With calling hlist_del_rcu, file cache has be deleted from the hashtbl, and clear the NFSD_FILE_HASHED bit.

As using in the codes, the bit and hlist_unhashed have the same meaning. Any mistake of my understand?

fs/nfsd/filecache.c new file +8 -0

+static void
+nfsd_file_unhash(struct nfsd_file *nf)
+{
+	if (test_and_clear_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
+		hlist_del_rcu(&nf->nf_node);
+		nfsd_file_count_dec();
+	}
+}

thanks,
Kinglong Mee

Quoted 58 lines

Quoted 56 lines

Quoted 56 lines

fs/nfsd/filecache.h +13 -0

+#define NFSD_FILE_PENDING	(1)
+	unsigned long		nf_flags;
+	struct knfsd_fh		nf_handle;
+	unsigned int		nf_hashval;
+	atomic_t		nf_ref;
+	unsigned char		nf_may;
+};
+
+int nfsd_file_cache_init(void);
+void nfsd_file_cache_shutdown(void);
+void nfsd_file_put(struct nfsd_file *nf);
+__be32 nfsd_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
+		  unsigned int may_flags, struct nfsd_file **nfp);
 #endif /* _FS_NFSD_FILECACHE_H */

fs/nfsd/nfssvc.c +9 -1

index ced9944201a0..0572441e23ec 100644
@@ -23,6 +23,7 @@
 #include "cache.h"
 #include "vfs.h"
 #include "netns.h"
+#include "filecache.h"
 
 #define NFSDDBG_FACILITY	NFSDDBG_SVC
 

@@ -233,11 +234,17 @@ static int nfsd_startup_generic(int nrservs)
 	if (!nfsd_laundry_wq)
 		goto out_racache;
 
-	ret = nfs4_state_start();
+	ret = nfsd_file_cache_init();
 	if (ret)
 		goto out_wq;
+
+	ret = nfs4_state_start();
+	if (ret)
+		goto out_nfsd_file;
 	return 0;
 
+out_nfsd_file:
+	nfsd_file_cache_shutdown();
 out_wq:
 	destroy_workqueue(nfsd_laundry_wq);
 	nfsd_laundry_wq = NULL;

@@ -254,6 +261,7 @@ static void nfsd_shutdown_generic(void)
 		return;
 
 	nfs4_state_shutdown();
+	nfsd_file_cache_shutdown();
 	nfsd_racache_shutdown();
 }
 

Quoted 56 lines · On Sat, 8 Aug 2015 08:14:14 +0800 Kinglong Mee <[email protected]> wrote:

Quoted 46 lines · Jeff Layton

Quoted 43 lines · On Fri, 7 Aug 2015 23:28:41 +0800 Kinglong Mee <[email protected]> wrote:

Quoted 25 lines · Jeff Layton

Currently, NFSv2/3 reads and writes have to open a file, do the read or write and then close it again for each RPC. This is highly inefficient, especially when the underlying filesystem has a relatively slow open routine.

This patch adds a new open file cache to knfsd. Rather than doing an open for each RPC, the read/write handlers can call into this cache to see if there is one already there for the correct filehandle and NFS_MAY_READ/WRITE flags.

If there isn't an entry, then we create a new one and attempt to perform the open. If there is, then we wait until the entry is fully instantiated and return it if it is at the end of the wait. If it's not, then we attempt to take over construction.

Since the main goal is to speed up NFSv2/3 I/O, we don't want to close these files on last put of these objects. We need to keep them around for a little while since we never know when the next READ/WRITE will come in.

Cache entries have a hardcoded 1s timeout, and we have a recurring workqueue job that walks the cache and purges any entries that have expired.

Signed-off-by Jeff Layton <[email protected]>

... snip ...

Quoted 16 lines

fs/nfsd/filecache.h +1 -0

index 9051ee54faa3..adf7e78b8e43 100644
@@ -4,6 +4,7 @@
 #include <linux/jhash.h>
 #include <linux/sunrpc/xdr.h>
 
+#include "nfsfh.h"
 #include "export.h"
 
 /* hash table for nfs4_file */

@@ -22,4 +23,24 @@ file_hashval(struct knfsd_fh *fh)
 	return nfsd_fh_hashval(fh) & (NFSD_FILE_HASH_SIZE - 1);
 }
 

Quoted 8 lines

Quoted 5 lines

fs/nfsd/filecache.h file_hashval(struct knfsd_fh *fh) · line 26

Quoted 1 line

fs/nfsd/filecache.h +1 -0

+struct nfsd_file {

There is a nfs4_file in nfsd, it's not easy to distinguish them for a new folk. More comments or a meaningful name is better.

Maybe. Again, any suggestions? My hope was that eventually we can unify the two caches somehow but maybe that's naive.

Quoted 8 lines

I cannot find a better name for the new file cache. More comments.

I don't agree with merging those two into one cache.

nfsv4's file cache is a state resource of client which will exist since close or lease expire. But nfsd_file cache is a temporary resource for nfsv2/v3 client without state.

You're probably right here. It was idle thought from when I first started this work. What we probably would want to do however is to layer the nfs4_file cache on top of this cache instead of having it manage filps on its own.

I tried to design this cache so that it can handle O_RDWR opens, even though the current callers don't actually ever request those. It should be possible to hook up the nfs4_file cache to it, though I'd prefer to wait until we have this code in place first and then do that later.

Quoted 3 lines

Also, for nfsv4's conflict checking, should we check the temporary file cache for nfsv2/v3 too?

You mean for share/deny modes? We traditionally have not done that, and I don't see a compelling reason to start now. That would be a separate project in its own right, IMO. We'd need to lift the share/deny mode handling into this new cache.

There's also the problem of there not being any protocol support for that in NFSv2/3. What would we return to the client if there's a deny mode conflict when it's trying to do (e.g.) a READ?

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

fs/nfsd/filecache.h file_hashval(struct knfsd_fh *fh) · line 32

Quoted 8 lines

fs/nfsd/filecache.h +6 -0

+	struct hlist_node	nf_node;
+	struct list_head	nf_dispose;
+	struct rcu_head		nf_rcu;
+	struct file		*nf_file;
+	unsigned long		nf_time;
+#define NFSD_FILE_HASHED	(0)

Why not using hlist_unhashed()?

Quoted 9 lines

Quoted 2 lines

These entries are removed from the list using hlist_del_rcu(), and hlist_unhashed will not return true after that.

As I understand, NFSD_FILE_HASHED means the file cache is added to nfsd_file_hashtbl, and increased the global file count.

With calling hlist_del_rcu, file cache has be deleted from the hashtbl, and clear the NFSD_FILE_HASHED bit.

That's correct.

Quoted 3 lines

As using in the codes, the bit and hlist_unhashed have the same meaning. Any mistake of my understand?

hlist_unhashed() won't work here:

static inline int hlist_unhashed(const struct hlist_node *h)

{
        return !h->pprev;
}

...but:

static inline void hlist_del_rcu(struct hlist_node *n)

{
        __hlist_del(n);
        n->pprev = LIST_POISON2;
}

Currently, NFSv2/3 reads and writes have to open a file, do the read or write and then close it again for each RPC. This is highly inefficient, especially when the underlying filesystem has a relatively slow open routine.

Quoted 4 lines · Jeff Layton

Currently, NFSv2/3 reads and writes have to open a file, do the read or write and then close it again for each RPC. This is highly inefficient, especially when the underlying filesystem has a relatively slow open routine.

.. as do many NFSv4 reads and writes as they often get special stateids passed from the client. Seems a little odd that we take more care of this for the legacy protocols than the current one.

I think I'd rather see a nfs4_file cache and turn v2/3 into something like the special stateid case. Did you consider that?

I started looking at extending the nfs4_file cache to NFSv2/3, but it's actually rather difficult...

We traditionally haven't dealt with share/deny modes in NFSv3, so we'd need a mechanism to bypass all of that stuff for legacy protocols. We'd also have to convert that cache from one that frees objects when the last one is put to one that keeps them around for a bit.

That also means that the v2/3 opens have to keep around extra fields that aren't really needed, and deal with the really godawful locking of the NFSv4 code.

I really think this approach is a better one. We can still use this cache from the NFSv4 code and wiring it in shouldn't be too hard. It's mostly a matter of plumbing in struct nfsd_file objects where that code is passing around struct file objects now.

Quoted 12 lines

I started looking at extending the nfs4_file cache to NFSv2/3, but it's actually rather difficult...

We traditionally haven't dealt with share/deny modes in NFSv3, so we'd need a mechanism to bypass all of that stuff for legacy protocols. We'd also have to convert that cache from one that frees objects when the last one is put to one that keeps them around for a bit.

That also means that the v2/3 opens have to keep around extra fields that aren't really needed, and deal with the really godawful locking of the NFSv4 code.

Quoted 4 lines

I really think this approach is a better one. We can still use this cache from the NFSv4 code and wiring it in shouldn't be too hard. It's mostly a matter of plumbing in struct nfsd_file objects where that code is passing around struct file objects now.

Ok. I'd still like to it wired up to NFSv4 - with my changes to make all temporary file opens happens from nfs4_preprocess_stateid_op it should be fairly simple.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfsd/vfs.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

fs/nfsd/vfs.c +0 -2

index 59234d1d8d8e..fd688c86af66 100644
@@ -980,20 +980,14 @@ out_nfserr:
 __be32 nfsd_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	loff_t offset, struct kvec *vec, int vlen, unsigned long *count)
 {
-	struct file *file;
-	struct raparms	*ra;

fs/nfsd/vfs.c +7 -1

-	fput(file);
+	__be32			err;
+	struct nfsd_file	*nf;
 
+	err = nfsd_file_acquire(rqstp, fhp, NFSD_MAY_READ, &nf);
+	if (err == nfs_ok)
+		err = nfsd_vfs_read(rqstp, nf->nf_file, offset, vec, vlen,
+					count);
+	nfsd_file_put(nf);
 	return err;
 }
 

Quoted 19 lines · Jeff Layton

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfsd/vfs.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

fs/nfsd/vfs.c +0 -6

index 59234d1d8d8e..fd688c86af66 100644
@@ -980,20 +980,14 @@ out_nfserr:
 __be32 nfsd_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	loff_t offset, struct kvec *vec, int vlen, unsigned long *count)
 {
-	struct file *file;
-	struct raparms	*ra;
-	__be32 err;
-
-	err = nfsd_open(rqstp, fhp, S_IFREG, NFSD_MAY_READ, &file);
-	if (err)

fs/nfsd/vfs.c out_nfserr: · line 994

Quoted 6 lines

fs/nfsd/vfs.c +0 -6

-		return err;
-
-	ra = nfsd_init_raparms(file);
-	err = nfsd_vfs_read(rqstp, file, offset, vec, vlen, count);
-	if (ra)
-		nfsd_put_raparams(file, ra);

Drop the raparms here ?

Quoted 13 lines

fs/nfsd/vfs.c +7 -1

-	fput(file);
+	__be32			err;
+	struct nfsd_file	*nf;
 
+	err = nfsd_file_acquire(rqstp, fhp, NFSD_MAY_READ, &nf);
+	if (err == nfs_ok)
+		err = nfsd_vfs_read(rqstp, nf->nf_file, offset, vec, vlen,
+					count);
+	nfsd_file_put(nf);
 	return err;
 }
 

Quoted 21 lines · On Fri, 7 Aug 2015 23:29:52 +0800 Kinglong Mee <[email protected]> wrote:

Quoted 19 lines · Jeff Layton

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfsd/vfs.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

fs/nfsd/vfs.c +0 -6

index 59234d1d8d8e..fd688c86af66 100644
@@ -980,20 +980,14 @@ out_nfserr:
 __be32 nfsd_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	loff_t offset, struct kvec *vec, int vlen, unsigned long *count)
 {
-	struct file *file;
-	struct raparms	*ra;
-	__be32 err;
-
-	err = nfsd_open(rqstp, fhp, S_IFREG, NFSD_MAY_READ, &file);
-	if (err)

Quoted 10 lines

fs/nfsd/vfs.c out_nfserr: · line 994

Quoted 6 lines

fs/nfsd/vfs.c +0 -6

-		return err;
-
-	ra = nfsd_init_raparms(file);
-	err = nfsd_vfs_read(rqstp, file, offset, vec, vlen, count);
-	if (ra)
-		nfsd_put_raparams(file, ra);

Drop the raparms here ?

I'm not sure I understand your question. Are you asking why I dropped the raparms from this code?

If so, the reason is that we shouldn't need it any longer. We only keep that cache now because we do an open for every READ RPC. With this, a streaming read should end up using the same struct file, at least as long as there's not _too_ long a delay between READ RPCs. The normal vfs readahead machinery should work properly with this change.

I'd like to eventually have us hook this up to the nfs4_file cache as well. I stopped short of that here since I didn't need that immediately for what I'm working on, but it should be possible to make the nfs4_file cache use the nfsd_file cache instead of calling dentry_open directly. Once we do that, then I don't think we'll need the raparms cache at all anymore.

Quoted 14 lines

Quoted 14 lines

fs/nfsd/vfs.c +7 -1

-	fput(file);
+	__be32			err;
+	struct nfsd_file	*nf;
 
+	err = nfsd_file_acquire(rqstp, fhp, NFSD_MAY_READ, &nf);
+	if (err == nfs_ok)
+		err = nfsd_vfs_read(rqstp, nf->nf_file, offset, vec, vlen,
+					count);
+	nfsd_file_put(nf);
 	return err;
 }