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.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
