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;
Message-ID: <69d215a099a5c9bfab7cb9f8e1c30186c78a9b76.camel@kernel.org>
Subject: Re: [PATCH] nfsd: remove dedicated nfsd_filecache_wq
From:   Jeff Layton <jlayton@kernel.org>
To:     Trond Myklebust <trondmy@hammerspace.com>
Cc:     Charles Edward Lever <chuck.lever@oracle.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Date:   Mon, 07 Nov 2022 15:32:31 -0500
In-Reply-To: <53271EA7-326E-427C-B9D4-3AB75924508C@hammerspace.com>
References: <20221107171056.64564-1-jlayton@kernel.org>
         <61876142ab0115a7bf39556e5caebfd1a635f945.camel@kernel.org>
         <CDEA2A36-B0EC-426B-8489-2BB524C6266A@oracle.com>
         <0dffb0a8508511229880545245948c3f512a374a.camel@kernel.org>
         <53271EA7-326E-427C-B9D4-3AB75924508C@hammerspace.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2022-11-07 at 19:06 +0000, Trond Myklebust wrote:
>=20
> > On Nov 7, 2022, at 13:45, Jeff Layton <jlayton@kernel.org> wrote:
> >=20
> > On Mon, 2022-11-07 at 18:16 +0000, Chuck Lever III wrote:
> > >=20
> > > > 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 th=
e
> > > > > 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_rhas=
h_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 in=
to
> > > > 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 w=
q
> > > > 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?
> > >=20
> > > 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.
> > >=20
> > > That might explain the dual WQ arrangement in the current code.
> > >=20
> >=20
> > 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.
>=20
> 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=E2=80=99t end up with one server bottlenecking the cleanup of all t=
he containers. It is of particular interest to do this if one of these cont=
ainers is actually re-exporting NFS.
>=20

I'm not sure that having a separate workqueue really prevents that these
days though, does it? They all use the same kthreads. We're not using
flush_workqueue. What scenario do we prevent by having a dedicated
workqueue?

> >=20
> > Trond, if we keep the dedicated workqueue for the laundrette, does it
> > also need WQ_MEM_RECLAIM?
>=20
> In theory, the files on that list are supposed to have already been flush=
ed, so I don=E2=80=99t think it is needed.
>=20

Ok.

--=20
Jeff Layton <jlayton@kernel.org>