Re: [PATCH 08/15] knfsd: spawn kernel thread to probe callbackchannel

[J. Bruce Fields]

On Tue, Aug 28, 2007 at 09:26:36AM +1000, Neil Brown wrote:
> On Monday August 27, bfields@xxxxxxxxxxxx wrote:
> >  
> > +/* Reference counting, callback cleanup, etc., all look racy as heck.
> > + * And why is cb_set an atomic? */
> 
> Agreed.... so do we really want this code in mainline?  is the old
> code so bad that this is better?

The code in question is just moved, not rewritten, so those are
preexisting problems that I was just adding a whiny comment about in
hopes it would remind me some more patches were probably needed.
(Thanks for the reminder....)

>  - cb_set should not be atomic.
>  - This looks like a job for async-rpc rather than a kernel thread

The problem isn't the wait for the rpc itself, it's the creation of the
new cred that's needed in the gss case.  That could also be made
asynchronous, but as there's several other delicate changes in the
pipeline (keyring stuff, etc.), I'd rather not touch it for now.

>  - If you do use a thread, you at least want __module_get before
>    starting the thread, and module_put_and_exit to terminate the
>    thread.
>  - Can you just use 'cb_client' rather than cb_set?  If you move
>    rpc_create into the thread, you don't need to set cb_client until
>    the callback is successful.  Then add a 'cb_active' flag bit
>    so that you don't have two callbacks at the same time, and it
>    should be less racy..

Yep, probably so.

--b.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/