On Fri, 2009-09-25 at 14:07 -0400, Andy Adamson wrote: > On Sep 25, 2009, at 11:01 AM, Trond Myklebust wrote: > > > On Fri, 2009-09-25 at 07:29 +0300, Benny Halevy wrote: > >> The client cannot reuse a slot while the server's > >> still working on the previous RPC using it. > >> > >> The symptom Bruce saw without this patch was occasional > >> ESERVERFAULT during Connectathon special tests, 30 MB write/read > >> test. > >> > >> Reported-by: J. Bruce Fields <bfields@xxxxxxxxxxxx> > >> Tested-by: J. Bruce Fields <bfields@xxxxxxxxxxxx> > >> Signed-off-by: Benny Halevy <bhalevy@xxxxxxxxxxx> > >> --- > >> fs/nfs/nfs4proc.c | 6 ++++++ > >> 1 files changed, 6 insertions(+), 0 deletions(-) > >> > >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > >> index be6544a..70895d6 100644 > >> --- a/fs/nfs/nfs4proc.c > >> +++ b/fs/nfs/nfs4proc.c > >> @@ -386,6 +386,12 @@ static void nfs41_sequence_done(struct > >> nfs_client *clp, > >> spin_unlock(&clp->cl_lock); > >> return; > >> } > >> + > >> + /* Do not free slot if retried while operation was in progress */ > >> + if (res->sr_status == -NFS4ERR_DELAY) { > >> + dprintk("%s: Operation in progress\n", __func__); > >> + return; > >> + } > >> out: > >> /* The session may be reset by one of the error handlers. */ > >> dprintk("%s: Error %d free the slot \n", __func__, res->sr_status); > > > > OK... Can somebody please tell me how this is supposed to work? As far > > as I can see, most callers of nfs41_sequence_done() are calling > > nfs41_sequence_free_slot() themselves. > > > > IOW: Who is responsible for calling nfs41_sequence_free_slot()? Is it > > nfs41_sequence_done(), or is it the callers, or is it some combination > > of the two (and if so, what determines who calls it)? > > It's a combination. > > nfs41_sequence_free_slot was separated from nfs41_sequence_done so > that the error handling code such as nfs4_async_handle_error or > calling rpc_restart could retry a call using the same session slot - > we didn't want to give up the slot just to wait for a new slot. > > So nfs41_sequence_free_slot is called by the callers of > nfs41_sequence_done when: > 1) the caller either does not try to handle and error or restart. > nfs4_sequence_done_free_slot is used for this, > pnfs_layoutcommit_rpc_done is also an example. > > 2) The restart attempt is also done by the caller. nfs4_close_done is > an example where nfs4_async_handle_error is called in between the > calls to nfs4_sequence_done and nfs4_sequence free_slot. > > nfs41_sequence_free_slot is not called by the caller of > nfs41_sequence_done when: > > 1) the caller does not contain the restart code. The read and write > paths are an example of this. For read, nfs4_sequence_done is called > in nfs4_read_done but the retry code is called in nfs_readpage_retry > which is where the corresponding nfs4_sequence_free_slot is called. > For write, nfs4_sequence_done is called in nfs4_write_done but the > retry code is in nfs_writeback_done where nfs4_sequence_free_slot is > called. So... Look for instance at nfs41_call_sync_done(), which will call nfs41_sequence_free_slot() _twice_ if res->sr_status is not 0 or 1, or if res->sr_slotid == NFS4_MAX_SLOT_TABLE. Ditto in nfs4_get_lease_time_done(), unless task->tk_status happens to be NFS4ERR_DELAY. IOW: Afaics, all errors that happen to the SEQUENCE operation (e.g. NFS4ERR_BADSESSION, NFS4ERR_CONN_NOT_BOUND_TO_SESSION, NFS4ERR_DEADSESSION, ...) will apparently result in a double call to nfs41_sequence_free_slot(). Please could we make a decision here. Either all callers are responsible for freeing the slot, or you have to ensure that they can safely call nfs41_sequence_free_slot twice. -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@xxxxxxxxxx www.netapp.com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html