On Nov 7, 2022, at 12:28 PM, Jeff Layton [off-list ref] wrote:

On Mon, 2022-11-07 at 12:10 -0500, Jeff Layton wrote:

quoted

There's no clear benefit to allocating our own over just using the
system_wq. This also fixes a minor bug in nfsd_file_cache_init(). In the
current code, if allocating the wq fails, then the nfsd_file_rhash_tbl
is leaked.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
fs/nfsd/filecache.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)

I'm playing with this and it seems to be ok, but reading further into
the workqueue doc, it says this:

* A wq serves as a domain for forward progress guarantee
 (``WQ_MEM_RECLAIM``, flush and work item attributes.  Work items
 which are not involved in memory reclaim and don't need to be
 flushed as a part of a group of work items, and don't require any
 special attribute, can use one of the system wq.  There is no
 difference in execution characteristics between using a dedicated wq
 and a system wq.

These jobs are involved in mem reclaim however, due to the shrinker.
OTOH, the existing nfsd_filecache_wq doesn't set WQ_MEM_RECLAIM.

In any case, we aren't flushing the work or anything as part of mem
reclaim, so maybe the above bullet point doesn't apply here?

In the steady state, deferring writeback seems like the right
thing to do, and I don't see the need for a special WQ for that
case -- hence nfsd_file_schedule_laundrette() can use the
system_wq.

That might explain the dual WQ arrangement in the current code.

But I'd feel better if the shrinker skipped files that require
writeback to avoid a potential deadlock scenario for some
filesystems.

quoted

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 1e76b0d3b83a..59e06d68d20c 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c

@@ -66,8 +66,6 @@ struct nfsd_fcache_disposal {

	struct list_head freeme;
};

-static struct workqueue_struct *nfsd_filecache_wq __read_mostly;
-
static struct kmem_cache		*nfsd_file_slab;
static struct kmem_cache		*nfsd_file_mark_slab;
static struct list_lru			nfsd_file_lru;

@@ -564,7 +562,7 @@ nfsd_file_list_add_disposal(struct list_head *files, struct net *net)

	spin_lock(&l->lock);
	list_splice_tail_init(files, &l->freeme);
	spin_unlock(&l->lock);
-	queue_work(nfsd_filecache_wq, &l->work);
+	queue_work(system_wq, &l->work);
}

static void

@@ -855,11 +853,6 @@ nfsd_file_cache_init(void)

	if (ret)
		return ret;

-	ret = -ENOMEM;
-	nfsd_filecache_wq = alloc_workqueue("nfsd_filecache", 0, 0);
-	if (!nfsd_filecache_wq)
-		goto out;
-
	nfsd_file_slab = kmem_cache_create("nfsd_file",
				sizeof(struct nfsd_file), 0, 0, NULL);
	if (!nfsd_file_slab) {

@@ -917,8 +910,6 @@ nfsd_file_cache_init(void)

	nfsd_file_slab = NULL;
	kmem_cache_destroy(nfsd_file_mark_slab);
	nfsd_file_mark_slab = NULL;
-	destroy_workqueue(nfsd_filecache_wq);
-	nfsd_filecache_wq = NULL;
	rhashtable_destroy(&nfsd_file_rhash_tbl);
	goto out;
}

@@ -1034,8 +1025,6 @@ nfsd_file_cache_shutdown(void)

	fsnotify_wait_marks_destroyed();
	kmem_cache_destroy(nfsd_file_mark_slab);
	nfsd_file_mark_slab = NULL;
-	destroy_workqueue(nfsd_filecache_wq);
-	nfsd_filecache_wq = NULL;
	rhashtable_destroy(&nfsd_file_rhash_tbl);

	for_each_possible_cpu(i) {

-- 
Jeff Layton [off-list ref]

--
Chuck Lever