> On Nov 7, 2022, at 13:45, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Mon, 2022-11-07 at 18:16 +0000, Chuck Lever III wrote:
>>
>>> On Nov 7, 2022, at 12:28 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>>>
>>> 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.
>>>>
>>>> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
>>>> ---
>>>> 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.
>>
>
> True. Looking through the changelog, the dedicated workqueue was added
> by Trond in 9542e6a643fc ("nfsd: Containerise filecache laundrette"). I
> assume he was concerned about reclaim.
It is about separation of concerns. The issue is to ensure that when you have multiple containers each running their own set of knfsd servers, then you won’t end up with one server bottlenecking the cleanup of all the containers. It is of particular interest to do this if one of these containers is actually re-exporting NFS.
>
> Trond, if we keep the dedicated workqueue for the laundrette, does it
> also need WQ_MEM_RECLAIM?
In theory, the files on that list are supposed to have already been flushed, so I don’t think it is needed.
_________________________________
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
