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