Message-id: <170121362397.7109.17858114692838122621@noble.neil.brown.name>
Date:   Wed, 29 Nov 2023 10:20:23 +1100
From:   "NeilBrown" <neilb@...e.de>
To:     "Christian Brauner" <brauner@...nel.org>
Cc:     "Al Viro" <viro@...iv.linux.org.uk>,
        "Christian Brauner" <brauner@...nel.org>,
        "Jeff Layton" <jlayton@...nel.org>, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-nfs@...r.kernel.org
Subject: Re: [PATCH/RFC] core/nfsd: allow kernel threads to use task_work.

On Wed, 29 Nov 2023, Christian Brauner wrote:
> [Reusing the trimmed Cc]
> 
> On Tue, Nov 28, 2023 at 11:16:06AM +1100, NeilBrown wrote:
> > On Tue, 28 Nov 2023, Chuck Lever wrote:
> > > On Tue, Nov 28, 2023 at 09:05:21AM +1100, NeilBrown wrote:
> > > > 
> > > > I have evidence from a customer site of 256 nfsd threads adding files to
> > > > delayed_fput_lists nearly twice as fast they are retired by a single
> > > > work-queue thread running delayed_fput().  As you might imagine this
> > > > does not end well (20 million files in the queue at the time a snapshot
> > > > was taken for analysis).
> > > > 
> > > > While this might point to a problem with the filesystem not handling the
> > > > final close efficiently, such problems should only hurt throughput, not
> > > > lead to memory exhaustion.
> > > 
> > > I have this patch queued for v6.8:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/commit/?h=nfsd-next&id=c42661ffa58acfeaf73b932dec1e6f04ce8a98c0
> > > 
> > 
> > Thanks....
> > I think that change is good, but I don't think it addresses the problem
> > mentioned in the description, and it is not directly relevant to the
> > problem I saw ... though it is complicated.
> > 
> > The problem "workqueue ...  hogged cpu..." probably means that
> > nfsd_file_dispose_list() needs a cond_resched() call in the loop.
> > That will stop it from hogging the CPU whether it is tied to one CPU or
> > free to roam.
> > 
> > Also that work is calling filp_close() which primarily calls
> > filp_flush().
> > It also calls fput() but that does minimal work.  If there is much work
> > to do then that is offloaded to another work-item.  *That* is the
> > workitem that I had problems with.
> > 
> > The problem I saw was with an older kernel which didn't have the nfsd
> > file cache and so probably is calling filp_close more often.  So maybe
> > my patch isn't so important now.  Particularly as nfsd now isn't closing
> > most files in-task but instead offloads that to another task.  So the
> > final fput will not be handled by the nfsd task either.
> > 
> > But I think there is room for improvement.  Gathering lots of files
> > together into a list and closing them sequentially is not going to be as
> > efficient as closing them in parallel.
> > 
> > > 
> > > > For normal threads, the thread that closes the file also calls the
> > > > final fput so there is natural rate limiting preventing excessive growth
> > > > in the list of delayed fputs.  For kernel threads, and particularly for
> > > > nfsd, delayed in the final fput do not impose any throttling to prevent
> > > > the thread from closing more files.
> > > 
> > > I don't think we want to block nfsd threads waiting for files to
> > > close. Won't that be a potential denial of service?
> > 
> > Not as much as the denial of service caused by memory exhaustion due to
> > an indefinitely growing list of files waiting to be closed by a single
> > thread of workqueue.
> 
> It seems less likely that you run into memory exhausting than a DOS
> because nfsd() is busy closing fds. Especially because you default to
> single nfsd thread afaict.

An nfsd thread would not end up being busy closing fds any more than it
can already be busy reading data or busy syncing out changes or busying
renaming a file.
Which it is say: of course it can be busy doing this, but doing this sort
of thing is its whole purpose in life.

If an nfsd thread only completes the close that it initiated the close
on (which is what I am currently proposing) then there would be at most
one, or maybe 2, fds to close after handling each request.  While that
is certainly a non-zero burden, I can't see how it can realistically be
called a DOS.

> 
> > I think it is perfectly reasonable that when handling an NFSv4 CLOSE,
> > the nfsd thread should completely handle that request including all the
> > flush and ->release etc.  If that causes any denial of service, then
> > simple increase the number of nfsd threads.
> 
> But isn't that a significant behavioral change? So I would expect to
> make this at configurable via a module- or Kconfig option?

Not really.  Certainly not more than the change to introduce the
filecache to nfsd in v5.4.

> 
> > For NFSv3 it is more complex.  On the kernel where I saw a problem the
> > filp_close happen after each READ or WRITE (though I think the customer
> > was using NFSv4...).  With the file cache there is no thread that is
> > obviously responsible for the close.
> > To get the sort of throttling that I think is need, we could possibly
> > have each "nfsd_open" check if there are pending closes, and to wait for
> > some small amount of progress.
> > 
> > But don't think it is reasonable for the nfsd threads to take none of
> > the burden of closing files as that can result in imbalance.
> 
> It feels that this really needs to be tested under a similar workload in
> question to see whether this is a viable solution.
> 

Creating that workload might be a challenge.  I know it involved
accessing 10s of millions of files with a server that was somewhat
memory constrained.  I don't know anything about the access pattern.

Certainly I'll try to reproduce something similar by inserting delays in
suitable places.  This will help exercise the code, but won't really
replicate the actual workload.

Thanks,
NeilBrown

Powered by blists - more mailing lists