mbox series

[0/2] nfsd: fix handling of error from unshare_fs_struct()

Message ID 20240729022126.4450-1-neilb@suse.de (mailing list archive)
Headers show
Series nfsd: fix handling of error from unshare_fs_struct() | expand

Message

NeilBrown July 29, 2024, 2:18 a.m. UTC
These two patches replace my previous patch:
 [PATCH 07/14] Change unshare_fs_struct() to never fail.

I had explored ways to change kthread_create() to avoid the need for
GFP_NOFAIL and concluded that we can do everything we need in the sunrpc
layer.  So the first patch here is a simple cleanup, and the second adds
simple infrastructure for an svc thread to confirm that it has started
up and to report if it was successful in that.

Thanks,
NeilBrown



 [PATCH 1/2] sunrpc: merge svc_rqst_alloc() into svc_prepare_thread()
 [PATCH 2/2] sunrpc: allow svc threads to fail initialisation cleanly

Comments

Jeff Layton July 29, 2024, 12:44 p.m. UTC | #1
On Mon, 2024-07-29 at 12:18 +1000, NeilBrown wrote:
> These two patches replace my previous patch:
>  [PATCH 07/14] Change unshare_fs_struct() to never fail.
> 
> I had explored ways to change kthread_create() to avoid the need for
> GFP_NOFAIL and concluded that we can do everything we need in the
> sunrpc
> layer.  So the first patch here is a simple cleanup, and the second
> adds
> simple infrastructure for an svc thread to confirm that it has
> started
> up and to report if it was successful in that.
> 
> Thanks,
> NeilBrown
> 
> 
> 
>  [PATCH 1/2] sunrpc: merge svc_rqst_alloc() into svc_prepare_thread()
>  [PATCH 2/2] sunrpc: allow svc threads to fail initialisation cleanly

I like this solution better:

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Chuck Lever III July 29, 2024, 2:36 p.m. UTC | #2
On Mon, Jul 29, 2024 at 12:18:19PM +1000, NeilBrown wrote:
> These two patches replace my previous patch:
>  [PATCH 07/14] Change unshare_fs_struct() to never fail.
> 
> I had explored ways to change kthread_create() to avoid the need for
> GFP_NOFAIL and concluded that we can do everything we need in the sunrpc
> layer.  So the first patch here is a simple cleanup, and the second adds
> simple infrastructure for an svc thread to confirm that it has started
> up and to report if it was successful in that.
> 
> Thanks,
> NeilBrown
> 
> 
> 
>  [PATCH 1/2] sunrpc: merge svc_rqst_alloc() into svc_prepare_thread()
>  [PATCH 2/2] sunrpc: allow svc threads to fail initialisation cleanly

This series does not apply to nfsd-next. It looks like 1/2 expects
to see the EXPORT_SYMBOL after svc_rqst_alloc() that you already
removed in "SUNRPC: make various functions static, or not exported."

Also, 1/2 is From: your brown.name account, but the SoB is your
SuSE email. (Maybe that doesn't matter).

Can you rebase and resend?

In 2/2, what is the reason to make svc_thread_init_status() a static
inline rather than an exported function? I don't think this is going
to be a performance hot path, but maybe it becomes one in a future
patch?
NeilBrown July 29, 2024, 9:07 p.m. UTC | #3
On Tue, 30 Jul 2024, Chuck Lever wrote:
> On Mon, Jul 29, 2024 at 12:18:19PM +1000, NeilBrown wrote:
> > These two patches replace my previous patch:
> >  [PATCH 07/14] Change unshare_fs_struct() to never fail.
> > 
> > I had explored ways to change kthread_create() to avoid the need for
> > GFP_NOFAIL and concluded that we can do everything we need in the sunrpc
> > layer.  So the first patch here is a simple cleanup, and the second adds
> > simple infrastructure for an svc thread to confirm that it has started
> > up and to report if it was successful in that.
> > 
> > Thanks,
> > NeilBrown
> > 
> > 
> > 
> >  [PATCH 1/2] sunrpc: merge svc_rqst_alloc() into svc_prepare_thread()
> >  [PATCH 2/2] sunrpc: allow svc threads to fail initialisation cleanly
> 
> This series does not apply to nfsd-next. It looks like 1/2 expects
> to see the EXPORT_SYMBOL after svc_rqst_alloc() that you already
> removed in "SUNRPC: make various functions static, or not exported."
> 
> Also, 1/2 is From: your brown.name account, but the SoB is your
> SuSE email. (Maybe that doesn't matter).

Probably don't matter.  That happened because I wrote that patch on my
notebook instead of my desktop and they have different defaults.  I'll
try to remember that for next time.

> 
> Can you rebase and resend?

I can't see "SUNRPC: make various functions static, or not exported." in
    git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux 
Am I looking in the wrong place? If not, can you push out nfsd-next?
(and if you could delete "for-next" from there, and possibly other old
cruft, that might help too)

Thanks,
NeilBrown


> 
> In 2/2, what is the reason to make svc_thread_init_status() a static
> inline rather than an exported function? I don't think this is going
> to be a performance hot path, but maybe it becomes one in a future
> patch?
> 
> 
> -- 
> Chuck Lever
>
Chuck Lever III July 29, 2024, 9:25 p.m. UTC | #4
> On Jul 29, 2024, at 5:07 PM, NeilBrown <neilb@suse.de> wrote:
> 
> On Tue, 30 Jul 2024, Chuck Lever wrote:
>> On Mon, Jul 29, 2024 at 12:18:19PM +1000, NeilBrown wrote:
>>> These two patches replace my previous patch:
>>> [PATCH 07/14] Change unshare_fs_struct() to never fail.
>>> 
>>> I had explored ways to change kthread_create() to avoid the need for
>>> GFP_NOFAIL and concluded that we can do everything we need in the sunrpc
>>> layer.  So the first patch here is a simple cleanup, and the second adds
>>> simple infrastructure for an svc thread to confirm that it has started
>>> up and to report if it was successful in that.
>>> 
>>> Thanks,
>>> NeilBrown
>>> 
>>> 
>>> 
>>> [PATCH 1/2] sunrpc: merge svc_rqst_alloc() into svc_prepare_thread()
>>> [PATCH 2/2] sunrpc: allow svc threads to fail initialisation cleanly
>> 
>> This series does not apply to nfsd-next. It looks like 1/2 expects
>> to see the EXPORT_SYMBOL after svc_rqst_alloc() that you already
>> removed in "SUNRPC: make various functions static, or not exported."
>> 
>> Also, 1/2 is From: your brown.name account, but the SoB is your
>> SuSE email. (Maybe that doesn't matter).
> 
> Probably don't matter.  That happened because I wrote that patch on my
> notebook instead of my desktop and they have different defaults.  I'll
> try to remember that for next time.

Some patch linting tools like the From: to match the SoB tag.
But again, I'm not sure that matters except to the legally
pedantic.


>> Can you rebase and resend?
> 
> I can't see "SUNRPC: make various functions static, or not exported." in
>    git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux
> Am I looking in the wrong place? If not, can you push out nfsd-next?

For some reason, git keeps pushing my local nfsd-next branch
to cel's "master" branch. I've pushed again to cel's nfsd-next,
and it seems to be showing up now on the web UI as I intended.


> (and if you could delete "for-next" from there, and possibly other old
> cruft, that might help too)

I'm looking at the list of branches, and I don't see a for-next
in the cel git repo. The only branches I can see are:

nfsd-next
nfsd-6.1.y
nfsd-testing
master
nfsd-5.10.y
nfsd-5.15.y
nfsd-fixes
exportfs-next
topic-xdr-tracepoints
topic-rdma-fault-injection


I've been deleting old tags as I notice them, but "git push :branch"
doesn't always seem to work against git.kernel.org.


> Thanks,
> NeilBrown
> 
> 
>> 
>> In 2/2, what is the reason to make svc_thread_init_status() a static
>> inline rather than an exported function? I don't think this is going
>> to be a performance hot path, but maybe it becomes one in a future
>> patch?
>> 
>> 
>> -- 
>> Chuck Lever


--
Chuck Lever
Christoph Hellwig July 29, 2024, 10:01 p.m. UTC | #5
On Tue, Jul 30, 2024 at 07:07:29AM +1000, NeilBrown wrote:
> > Also, 1/2 is From: your brown.name account, but the SoB is your
> > SuSE email. (Maybe that doesn't matter).
> 
> Probably don't matter.  That happened because I wrote that patch on my
> notebook instead of my desktop and they have different defaults.  I'll
> try to remember that for next time.

The signoff is supposed to match the author address.  If you send it
out using a different email account you need to add a separate From:
line to the mail body.  git-send-email will do that automatically for
you.
NeilBrown July 29, 2024, 11:17 p.m. UTC | #6
On Tue, 30 Jul 2024, Chuck Lever III wrote:
> 
> 
> > On Jul 29, 2024, at 5:07 PM, NeilBrown <neilb@suse.de> wrote:
> > 
> 
> > (and if you could delete "for-next" from there, and possibly other old
> > cruft, that might help too)
> 
> I'm looking at the list of branches, and I don't see a for-next
> in the cel git repo. The only branches I can see are:
> 
> nfsd-next
> nfsd-6.1.y
> nfsd-testing
> master
> nfsd-5.10.y
> nfsd-5.15.y
> nfsd-fixes
> exportfs-next
> topic-xdr-tracepoints
> topic-rdma-fault-injection
> 
> 
> I've been deleting old tags as I notice them, but "git push :branch"
> doesn't always seem to work against git.kernel.org.


Ahh...  "git fetch" does discard branched that have been deleted
remotely.
I need
   git remote prune $remote
or to set remote.$remote.prune to true

Thanks,
NeilBrown
NeilBrown July 30, 2024, 12:43 a.m. UTC | #7
On Tue, 30 Jul 2024, Chuck Lever wrote:
> 
> In 2/2, what is the reason to make svc_thread_init_status() a static
> inline rather than an exported function? I don't think this is going
> to be a performance hot path, but maybe it becomes one in a future
> patch?
> 

Sorry - I missed this question the first time.

My reasoning was that it was a tiny function which would be optimised
even smaller in two out of three call sites where the err number is a
constant zero.  Also my original draft didn't even have this as a
function but was open coded.

I don't think it matters much either way.

Thanks,
NeilBrown