Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@kernel.org>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] nfsd: remove dedicated nfsd_filecache_wq
Thread-Topic: [PATCH] nfsd: remove dedicated nfsd_filecache_wq
Thread-Index: AQHY8svsfDGR9gO670+nD/CKFPOHBq4ztxuAgAANfgA=
Date:   Mon, 7 Nov 2022 18:16:47 +0000
Message-ID: <CDEA2A36-B0EC-426B-8489-2BB524C6266A@oracle.com>
References: <20221107171056.64564-1-jlayton@kernel.org>
 <61876142ab0115a7bf39556e5caebfd1a635f945.camel@kernel.org>
In-Reply-To: <61876142ab0115a7bf39556e5caebfd1a635f945.camel@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?x2WaqJeVgdianeqcFQaZ+qpArHJLWsMT56RFMmXel3tt4uSyUy6h5XwdITce?=
 =?us-ascii?Q?gsTSX9duBwB26SjgeF5mVS7YvH/oacZB/l02vVLi0jtJl8s6mGbRJ84Zhvm7?=
 =?us-ascii?Q?6crI6LzkZqGZYDriIAAey3JkfauRU7pswwvfAia4beduwwXa6+Jozfc6MRT9?=
 =?us-ascii?Q?HYudkbYi0Z+1j+H/cAR9AGySr+2yFgp5gaI78w6urtepTZmNtyitWrguOz9M?=
 =?us-ascii?Q?mgv0zfyTfk7MN5LKCLmr8YbCWyPf2esV+TKU3Byxy/HCad3OTzLav6tG7Ldn?=
 =?us-ascii?Q?U2kvtUKQkBp1tEb+z80YSjZUvTNsq08IwGIeBARWIrBGrePvpkcLTDGtYg6b?=
 =?us-ascii?Q?Qd+Krsk/g2D2JT1WjxENWCuM4uACTVpGy3uCnO1Q4IcqaROwXawba27eK2tI?=
 =?us-ascii?Q?0tgqIHi7KCHC//bAWO3f/JpFMACabuFqd7ZSaRJvjQu3cVd6hC2iSFGgW949?=
 =?us-ascii?Q?hlbN5OQWSe4TSH1IxcKKq5WY4zVvlPmmpk0Di6SZWRhcW7fBkI7LnZ0u4u6X?=
 =?us-ascii?Q?bferIIFFlwConisNlYdyhWqkGTQXwiCVPKfaWaAg9ABOvbSC0f/MrZkCQBHc?=
 =?us-ascii?Q?CMXuq6cUCbQv8ejoXLtvV1vdn3nGPrQ4HLOBybXcGazFUepMeBcjjfX9LLXq?=
 =?us-ascii?Q?v6MLjD/vtp6q8Syy6y48HUdHKl/JxZJ9nEGrYcU6kwkyF0ZHRDkbgSzb28LD?=
 =?us-ascii?Q?PXJr5A84v8NvGDu7f2P2bns4sH/3FtDPw48ocjxvKczt2yOpSCNSlV2GsOPS?=
 =?us-ascii?Q?XpMy8VkmJQCHhJEVN3UOpHFqAn4GlEo9EhjRuJSmT9Ja8n7/FShlMA3TroE1?=
 =?us-ascii?Q?wKsJlfYRixvP7LlJXKTb1z562u1z7My4PA5m+y/ZtaigAnXmS5f5fT5wQM8b?=
 =?us-ascii?Q?SXMmOAjWmLmIISXMhvjT0eAnrpR/sHxi3YiIIO0xT6l0IOOeuCOwlWH/fYiP?=
 =?us-ascii?Q?fr+5mx1X47bsB8lbNUcaGgbDFbdep/SjzMurjW6rwHwGqjUQZ16s1oJ2KZsN?=
 =?us-ascii?Q?jaCuKH0FkegOzshhec+57H57VH2GFJj9SZ2kfEVGVfVbqkY+99YcMfiT0igZ?=
 =?us-ascii?Q?Bw3MFdih2h2G3hEBfokVyPy5eLc6+fx4D34eJfWy6WDkGV+1RCzd1Q4cIVI3?=
 =?us-ascii?Q?+mqZhdecRzuD8kgDfqQ4pMUeyOhlA90ZpdJ4VgPSCP0/d4HPmVmVhXnxtsQa?=
 =?us-ascii?Q?XyNF6TNVcxwEHm73c83nSWnAyJhMEzCk9i9xKx5iW6hbm3TpxOVv7XXmaCqw?=
 =?us-ascii?Q?y30QWsflkIjUB2Lv3rRj3H8KDBkCwsml1bHIEVxjxqR/JGKj2UZAM+4cKIOI?=
 =?us-ascii?Q?zm/ScQR+Uqaj/W8PavzAZmbP2NvdS5Tf93easwRHZVL4zs7vjXz9MNsIXuLR?=
 =?us-ascii?Q?k7/Pn7Coj7OYjduUs+jb7Sn0mACVtegjPQUNevwoW3bf0PYP8zvjBP9yLAYb?=
 =?us-ascii?Q?YLV5pXnEYgTntmtpv3ZNp1vIu8AnBl1n56FePp7+FEgqIRQwpukVA5ZOM5qx?=
 =?us-ascii?Q?87e89lW3k46DUva0LLEU6IdYos3lH1nfRvwFmbkOdqLZHN9gQIgrpqEWUmtN?=
 =?us-ascii?Q?PUNWHYn6BpxwbYva9eTmjSGb1DcVdIV+gWhZKa3v+v66bzPvBPFB9u9uuaZd?=
 =?us-ascii?Q?2w=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <17634EEC2C80F745BB2ECAA8B6ED7B65@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0e6d96f7-1da7-461a-ef82-08dac0ec3b95
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Nov 2022 18:16:47.0531
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 9ZnOce+ibAyZ8pwdAgzBCghlPbR712r7Gt/rlRiE+PzEKtUN4fuGQZxIuFGnkJE4etywCRYbO2NBI6U5gptwuQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB5247
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-11-07_09,2022-11-07_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 spamscore=0 malwarescore=0
 adultscore=0 mlxscore=0 mlxlogscore=999 phishscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2211070146
X-Proofpoint-GUID: 9RMpziMlhg0-Z1NpT-kUbkUO4L19DuDn
X-Proofpoint-ORIG-GUID: 9RMpziMlhg0-Z1NpT-kUbkUO4L19DuDn
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Nov 7, 2022, at 12:28 PM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Mon, 2022-11-07 at 12:10 -0500, Jeff Layton wrote:
>> 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.
>>=20
>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> ---
>> fs/nfsd/filecache.c | 13 +------------
>> 1 file changed, 1 insertion(+), 12 deletions(-)
>>=20
>=20
> I'm playing with this and it seems to be ok, but reading further into
> the workqueue doc, it says this:
>=20
> * 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.
>=20
> These jobs are involved in mem reclaim however, due to the shrinker.
> OTOH, the existing nfsd_filecache_wq doesn't set WQ_MEM_RECLAIM.
>=20
> 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.


>> 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;
>> };
>>=20
>> -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);
>> }
>>=20
>> static void
>> @@ -855,11 +853,6 @@ nfsd_file_cache_init(void)
>> 	if (ret)
>> 		return ret;
>>=20
>> -	ret =3D -ENOMEM;
>> -	nfsd_filecache_wq =3D alloc_workqueue("nfsd_filecache", 0, 0);
>> -	if (!nfsd_filecache_wq)
>> -		goto out;
>> -
>> 	nfsd_file_slab =3D 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 =3D NULL;
>> 	kmem_cache_destroy(nfsd_file_mark_slab);
>> 	nfsd_file_mark_slab =3D NULL;
>> -	destroy_workqueue(nfsd_filecache_wq);
>> -	nfsd_filecache_wq =3D 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 =3D NULL;
>> -	destroy_workqueue(nfsd_filecache_wq);
>> -	nfsd_filecache_wq =3D NULL;
>> 	rhashtable_destroy(&nfsd_file_rhash_tbl);
>>=20
>> 	for_each_possible_cpu(i) {
>=20
> --=20
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever