On Feb 15, 2012, at 1:37 PM, Myklebust, Trond wrote: > On Wed, 2012-02-15 at 11:06 -0500, Andy Adamson wrote: >> On Wed, Feb 15, 2012 at 10:46 AM, Myklebust, Trond >> <Trond.Myklebust@xxxxxxxxxx> wrote: >>> On Wed, 2012-02-15 at 15:07 +0000, Adamson, Andy wrote: >>>> >>>> Please don't merge nfs4_reset_slot_tables and nfs4_init_slot_tables. It assumes a static array (realloc) which goes away with the dynamic slot table code. Instead, take a look at the patch set I sent on Feb 12 >>> >>> Why not? There is _no_ functional difference between what >>> nfs4_reset_slot_tables() and nfs4_init_slot_tables() need to do. >>> They both need to allocate new slots (conditionally in the case of >>> nfs4_reset_slot_tables, but the condition is compatible with the >>> nfs4_init_slot_tables case), and they both need to initialise those >>> slots to the value '1'. >> >> The reset case also needs to reduce the number of slots. In the >> dynamic slot case, the reset function will not require draining of the >> session. So reset will, in response to a change in the >> target_highest_slotid or a CB_RECALL_SLOT from the server, add or >> remove slots without draining the session. In this case, reset will >> only initialize any new slots sequence numbers to 1, not all slots. >> >> That said, I guess it can all be in one function. >> >>> >>> AFAICS There is no reason for keeping those as 2 separate functions, and >>> I don't see how the dynamic session patches change anything to that >>> conclusion. >>> >>>> [PATCH Version 7 3/3] NFSv4.1 avoid freeing slot when tasks are waiting >>>> [PATCH Version 1 2/3] NFSv4.1 prepare for dyamic session slots >>>> [PATCH Version 7 1/3] NFS4.1 clean up nfs4_alloc_session >>> >>> I need a fix for the 3.3 final... >>> >>> Those patches can be cleaned up and made ready for 3.4 (needs work - >>> they won't apply to the 'nfs-for-next' branch), >> >> I worked against the bugfix branch. I'll use the nfs-for-next branch. >> >>> but right now they're >>> not sufficiently tested nor are they sufficiently reviewed. >> >> Agreed. I want to test some more at connectathon. I did iozone tests >> to show that this dynamic slot implementation performs as well as the >> static array, but not enough testing / review for coding bugs. >> >>> For >>> instance, I'm not happy with the idea of adding a 'tk_private' field in >>> the struct rpc_task. >> >> OK. Do you have a suggestion? > > Yes. Please see how we solve the minor version 0 equivalent problem in > nfs_release_seqid(). > > In the case of minor version 1, I suggest setting up a dedicated linked > list of 'things waiting for a slot', where each entry in the linked list > contains the nfs4_sequence_args/nfs4_sequence_res to initialise, and > then a pointer to the rpc_task to wake up. OK - thanks for the advice. -->Andy > > -- > 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
