On Tue, 05 Nov 2024, Chuck Lever wrote:
> On Tue, Nov 05, 2024 at 07:30:03AM +1100, NeilBrown wrote:
> > On Tue, 05 Nov 2024, Chuck Lever wrote:
> > > On Mon, Nov 04, 2024 at 03:47:42PM +1100, NeilBrown wrote:
> > > >
> > > > An NFSv4.2 COPY request can explicitly request a synchronous copy. If
> > > > that is not requested then the server is free to perform the copy
> > > > synchronously or asynchronously.
> > > >
> > > > In the Linux implementation an async copy requires more resources than a
> > > > sync copy. If nfsd cannot allocate these resources, the best response
> > > > is to simply perform the copy (or the first 4MB of it) synchronously.
> > > >
> > > > This choice may be debatable if the unavailable resource was due to
> > > > memory allocation failure - when memalloc fails it might be best to
> > > > simply give up as the server is clearly under load. However in the case
> > > > that policy prevents another kthread being created there is no benefit
> > > > and much cost is failing with NFS4ERR_DELAY. In that case it seems
> > > > reasonable to avoid that error in all circumstances.
> > > >
> > > > So change the out_err case to retry as a sync copy.
> > > >
> > > > Fixes: aadc3bbea163 ("NFSD: Limit the number of concurrent async COPY operations")
> > >
> > > Hi Neil,
> > >
> > > Why is a Fixes: tag necessary?
> > >
> > > And why that commit? async copies can fail due to lack of resources
> > > on kernels that don't have aadc3bbea163, AFAICT.
> >
> > I had hoped my commit message would have explained that, though I accept
> > it was not as explicit as it could be.
>
> The problem might be that you and I have different understandings of
> what exactly aadc3bbea163 does.
It might be.
My understanding is that it limits the number of concurrent async
COPY requests to ->sp_nrthreads and once that limit in reached
any further COPY requests that don't explicitly request "synchronous"
are refused with NFS4ERR_DELAY.
>
>
> > kmalloc(GFP_KERNEL) allocation failures aren't interesting. They never
> > happen for smallish sizes, and if they do then the server is so borked
> > that it hardly matter what we do.
> >
> > The fixed commit introduces a new failure mode that COULD easily be hit
> > in practice. It causes the N+1st COPY to wait indefinitely until at
> > least one other copy completes which, as you observed in that commit,
> > could "run for a long time". I don't think that behaviour is necessary
> > or appropriate.
>
> The waiting happens on the client. An async COPY operation always
> completes quickly on the server, in this case with NFS4ERR_DELAY. It
> does not tie up an nfsd thread.
Agreed that it doesn't tie up an nfsd thread. It does tie up a separate
kthread for which there is a limit matching the number of nfsd threads
(in the pool).
Agreed that the waiting happens on the client, but why should there be
any waiting at all? The client doesn't know what it is waiting for, so
will typically wait a few seconds. In that time many megabytes of sync
COPY could have been processed.
>
> By the way, there are two fixes in this area that now appear in
> v6.12-rc6 that you should check out.
I'll try to schedule time to have a look - thanks.
>
>
> > Changing the handling for kmalloc failure was just an irrelevant
> > side-effect for changing the behaviour when then number of COPY requests
> > exceeded the number of configured threads.
>
> aadc3bbea163 checks the number of concurrent /async/ COPY requests,
> which do not tie up nfsd threads, and thus are not limited by the
> svc_thread count, as synchronous COPY operations are by definition.
They are PRECISELY limited by the svc_thread count. ->sp_nrthreads.
+ if (atomic_inc_return(&nn->pending_async_copies) >
+ (int)rqstp->rq_pool->sp_nrthreads) {
>
> I'm still thinking about the ramifications of converting an async
> COPY to a sync COPY in this case. We want to reduce the server
> workload in this case, rather than accommodate an aggressive client.
We are not "converting" an async COPY to a sync COPY. There is no such
thing as an "async COPY" in terms of what the client requests.
The client can request "COPY" which there server may perform sync or
async, or the client can request "COPY synchronous" which the server
must perform synchronously, or refuse to perform.
By tying up a thread temporarily with a sync COPY we do reduce server
workload by potentially increasing latency to the client. I don't
think that "aggressive" is a fair description of the client.
"opportunistic" might be reasonable.
My current thinking is that we should not start extra threads for
handling async copies. We should create a queue of pending copies and
any nfsd thread can dequeue a copy and process 4MB each time through
"The main request loop" just like it calls nfsd_file_net_dispose() to do
a little bit of work. That isn't needed now but I'll need something
like that before my dynamic thread pool work can land.
>
>
> > This came up because CVE-2024-49974 was created so I had to do something
> > about the theoretical DoS vector in SLE kernels. I didn't like the
> > patch so I backported
> >
> > Commit 8d915bbf3926 ("NFSD: Force all NFSv4.2 COPY requests to be synchronous")
> >
> > instead (and wondered why it hadn't gone to stable).
>
> I was conservative about requesting a backport here. However, if a
> CVE has been filed, and if there is no automation behind that
> process, you can explicitly request aadc3bbea163 be backported.
>
> The problem, to me, was less about server resource depletion and
> more about client hangs.
And yet the patch that dealt with the less important server resource
depletion was marked for stable, and the patch that dealt with client
hangs wasn't??
The CVE was for that less important patch, probably because it contained
the magic word "DoS".
I think 8d915bbf3926 should go to stable but I would like to understand
why you felt the need to be conservative.
Thanks,
NeilBrown
>
>
> > Thanks,
> > NeilBrown
> >
> >
> > >
> > >
> > > > Signed-off-by: NeilBrown <neilb@xxxxxxx>
> > > > ---
> > > > fs/nfsd/nfs4proc.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > > index fea171ffed62..06e0d9153ca9 100644
> > > > --- a/fs/nfsd/nfs4proc.c
> > > > +++ b/fs/nfsd/nfs4proc.c
> > > > @@ -1972,6 +1972,7 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > > > wake_up_process(async_copy->copy_task);
> > > > status = nfs_ok;
> > > > } else {
> > > > + retry_sync:
> > > > status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > copy->nf_dst->nf_file, true);
> > > > }
> > > > @@ -1990,8 +1991,7 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > > > }
> > > > if (async_copy)
> > > > cleanup_async_copy(async_copy);
> > > > - status = nfserr_jukebox;
> > > > - goto out;
> > > > + goto retry_sync;
> > > > }
> > > >
> > > > static struct nfsd4_copy *
> > > >
> > > > base-commit: 26e6e693936986309c01e8bb80e318d63fda4a44
> > > > --
> > > > 2.47.0
> > > >
> > >
> > > --
> > > Chuck Lever
> > >
> >
>
> --
> Chuck Lever
>
