Re: [pnfs] [PATCH] nfsd: use nfs client rpc callback program

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, Sep 25, 2008 at 10:30:33PM +0300, Benny Halevy wrote:
> On 09/24/2008 9:49:34 pm +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > On Wed, Sep 24, 2008 at 02:42:25PM -0400, Trond Myklebust wrote:
> >> On Wed, 2008-09-24 at 13:42 -0400, J. Bruce Fields wrote:
> >>
> >>> If by "broken" you mean, "introduces a new kernel bug", I don't see it.
> >> I mean it introduces utterly unnecessary complications that may return
> >> to bite us in the arse at a future time.
> > 
> > OK, that's the answer I was looking for, thanks.
> > 
> >> Remember how we once said that it would never make sense for child
> >> clones to call the portmapper, and so we added the BUG_ON() in
> >> rpcb_getport_async; well guess what, we currently have a bug to fix...
> > 
> > No, I don't remember that.  But yes, I can see how in general this sort
> > of thing could make the code harder to maintain.
> > 
> >> One pretty obvious fix is to simply move the release method so that it
> >> doesn't occur when you release a child. The disadvantage is that a child
> >> may then not change its program to one that requires a release method
> >> (do we need that?).
> > 
> > I doubt we need that, but...
> > 
> >> Another fix would be to add a refcount to the rpc_program structure...
> > 
> > ... a refcount seems more straightforward.  Benny, what do you think?
> 
> I agree.  I'll send a patch hopefully tomorrow.
> Would you like that combined with the one I sent or as a separate one?
> (I'm inclined towards the latter).

That'd be fine.

> One more thing that seems to need fixing is rpc_bind_new_program
> which now uses the passed-in program->cl_stats but doesn't point
> to the passed-in program, but rather it only extracts its
> name, number, and stats.
> Since program->stats may possibly go away with the program
> in the refcounted world I think we should get a reference on the
> program here too.
> 
> That observed, it may also be a good idea to get rid of clnt->cl_stats
> altogether and use clnt->program->stats instead to prevent any
> discrepancy.

Sounds good to me.

--b.