On Tue, Dec 06, 2011 at 04:30:25PM +0200, Benny Halevy wrote:
> On 2011-12-06 14:40, J. Bruce Fields wrote:
> > On Tue, Dec 06, 2011 at 01:26:05PM +0200, Benny Halevy wrote:
> >> On 2011-12-06 04:08, J. Bruce Fields wrote:
> >>> On Sun, Dec 04, 2011 at 02:42:11PM +0200, Benny Halevy wrote:
> >>>> On 2011-12-04 14:03, tigran.mkrtchyan@xxxxxxx wrote:
> >>>>> From: Tigran Mkrtchyan <tigran.mkrtchyan@xxxxxxx>
> >>>>>
> >>>>>
> >>>>> Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@xxxxxxx>
> >>>>> ---
> >>>>> fs/nfsd/nfs4proc.c | 6 ++++++
> >>>>> fs/nfsd/nfs4state.c | 26 ++++++++++++++++++++------
> >>>>> 2 files changed, 26 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >>>>> index fa38336..535aed2 100644
> >>>>> --- a/fs/nfsd/nfs4proc.c
> >>>>> +++ b/fs/nfsd/nfs4proc.c
> >>>>> @@ -400,6 +400,12 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >>>>> */
> >>>>> status = nfsd4_process_open2(rqstp, &cstate->current_fh, open);
> >>>>> WARN_ON(status && open->op_created);
> >>>>> +
> >>>>> + if(status)
> >>>>> + goto out;
> >>>>> +
> >>>>> + /* set current state id */
> >>>>> + memcpy(&cstate->current_stateid, &open->op_stateid, sizeof(stateid_t));
> >>>
> >>> That comment is a bit redundant.
> >>>
> >>>> Since this should be done for all stateid-returning operations
> >>>> I think that a cleaner approach could be to mark those as such in
> >>>> nfsd4_ops by providing a per-op function to return the operation's
> >>>> stateid. You can then call this method from nfsd4_proc_compound()
> >>>> after the call to nfsd4_encode_operation() and when status == 0.
> >>>
> >>> So the choice is between
> >>>
> >>> + memcpy(&cstate->current_stateid, &open->op_stateid,
> >>> sizeof(stateid_t));
> >>>
> >>> and
> >>>
> >>> + static void get_open_stateid(stateid_t *s)
> >>> + {
> >>> + memcpy(s, open->op_stateid);
> >>> + }
> >>> +
> >>> + [OP_OPEN] = {
> >>> + ...
> >>> + .op_get_stateid = get_open_stateid,
> >>> + ...
> >>> + }
> >>>
> >>> ?
> >>>
> >>> I'm not so sure.
> >>
> >> The point is to copy the result stateid into the current_stateid
> >> in a centralized place: nfsd4_proc_compound() and do that for all
> >> stateid-modifying operations.
> >>
> >>>
> >>> Anyway, thanks Tigran for looking at this.
> >>>
> >>> Do we want to guarantee that the client can't expire as long as a
> >>> compound references the stateid? I think that's the case.
> >>
> >> The client can't time out while the 4.1 compound is in progress, see commit d768298.
> >
> > OK, you're right, and presumably it would be a bug for a compound to use
> > a session from one client and a stateid from another, so this is taken
> > care of. (Except--I think we need to check for that case. On a quick
> > skim I don't see the current code doing that.)
> >
> > Thanks!
> >
> >> Are you thinking of explicit expiration of the client?
> >> We may unhash the client and keep using it while it's referenced
> >> so that's not a problem. As far as the stateid goes, we're copying the
> >> value of the stateid, not pointing to any stateid structure. If the
> >> actual state was destroyed, we will detect that when the current_stateid
> >> is used by any successive operation and we cannot find the state using
> >> the stateid.
> >
> > I *think* the concensus of the working group was that explicit
> > destruction of a client should wait on in-progress compounds referencing
> > any of the client's sessions:
> >
> > http://www.ietf.org/mail-archive/web/nfsv4/current/msg08584.html
>
> I'm a bit confused, since the rfc5661 says that:
>
> DESTROY_CLIENTID:
> If there are
> sessions (both idle and non-idle), opens, locks, delegations,
> layouts, and/or wants (Section 18.49) associated with the unexpired
> lease of the client ID, the server MUST return NFS4ERR_CLIENTID_BUSY.
Hm. The case Mike Eisler mentions in the message referenced above is
EXCHANGE_ID/CREATE_SESSION.
But he also proposes some text for DESTROY_CLIENTID that contradicts the
text you quote above.
I guess that's intentional--he's worried about not being able to shut
down a client cleanly after a cluster node goes down.
Still, whatever behavior we decide to implement on the server, we may
want to run it by the ietf list before we're done to make sure there's
an agreement here....
> DESTROY_SESSION:
> Locks, delegations, layouts, wants, and the lease, which
> are all tied to the client ID, are not affected by DESTROY_SESSION.
But that doesn't explain what to do about in-progress operations. So if
you the DESTROY_SESSION comes while we're in the middle of processing a
rename using that session, what do we do?
In any case, Tigran's work doesn't need to block on any of this.
--b.
>
> >
> > So we should probably fix this. But we can fix it at the session level.
> >
> > So, OK, I can't see any practical objection to doing as Tigran as and
> > just passing the value of stateid instead of a reference to some object.
> >
> > Well, except for performance--it seems unfortunate to have to redo the
> > lookup on each use. As long as there's no impact on the existing cases
> > (so we're only doing the lookup when a client actually uses the current
> > stateid), I can live with that until somebody actually demonstrates some
> > harm.
>
> Great. I'm glad we're in agreement!
>
> Benny
>
> >
> > --b.
> > --
> > 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
--
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
