Re: [PATCH v4 00/27] add block layout driver to pnfs client

[Trond Myklebust <Trond.Myklebust@netapp.com>]

On Mon, 2011-08-01 at 19:11 -0400, Trond Myklebust wrote: 
> On Mon, 2011-08-01 at 18:57 -0400, Andy Adamson wrote: 
> > On Aug 1, 2011, at 6:35 PM, Trond Myklebust wrote:
> > > /*
> > > @@ -433,26 +436,32 @@ __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
> > > 			      struct cb_sequenceres *res,
> > > 			      struct cb_process_state *cps)
> > > {
> > > +	struct nfs4_slot_table *tbl;
> > > 	struct nfs_client *clp;
> > > 	int i;
> > > 	__be32 status = htonl(NFS4ERR_BADSESSION);
> > > 
> > > -	cps->clp = NULL;
> > > -
> > > 	clp = nfs4_find_client_sessionid(args->csa_addr, &args->csa_sessionid);
> > > 	if (clp == NULL)
> > > 		goto out;
> > > 
> > > +	tbl = &clp->cl_session->bc_slot_table;
> > > +
> > > +	spin_lock(&tbl->slot_tbl_lock);
> > > 	/* state manager is resetting the session */
> > > 	if (test_bit(NFS4_SESSION_DRAINING, &clp->cl_session->session_state)) {
> > > 		status = NFS4ERR_DELAY;
> >                                  
> > status = htonl(NFS4ERR_DELAY);
> 
> Yep. I'll fix that too... 

Looking at this again, I'm not sure that the above is safe. If you
return NFS4ERR_DELAY on the SEQUENCE operation, then that has a very
specific meaning of "I'm executing your command, but it will take a
while to complete.".
It therefore seems to me that if we return NFS4ERR_DELAY, then we are
basically setting ourselves up for a deadlock: 
      * On the one hand, we're trying to destroy the session 
      * On the other hand, we're telling the server that 'your callback
        is being processed'.
In that situation, it seems to me that it should be perfectly valid for
the server to reject our DESTROY_SESSION call with a
NFS4ERR_BACK_CHAN_BUSY until we process the callback.

Comments?

Cheers
  Trond

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com