[pnfs] Server sessions slot table patches
William A. (Andy) Adamson
andros at citi.umich.edu
Wed Apr 4 09:15:18 EDT 2007
hi benny
On 4/4/07, Benny Halevy <bhalevy at panasas.com> wrote:
>
> Andy, my comments in "=>"
> Benny
>
> 0003-Declare-server-sessionid_t-and-rename-server-session.patch:
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> @@ -369,15 +362,15 @@ add_to_sessionid_hashtbl(clientid_t *clientid,
> sessionid_t *sessionid)
> if (!new)
> goto out;
>
> - memcpy(&new->clientid, clientid, sizeof(*clientid));
> - memcpy(&new->sessionid, *sessionid, sizeof(sessionid_t));
> + memcpy(&new->se_clientid, clientid, sizeof(*clientid));
> + memcpy(&new->se_sessionid, *sessionid, sizeof(sessionid_t));
>
> => (isn't dereferencing sessionid a bug here?)
> + memcpy(&new->se_sessionid, sessionid, sizeof(sessionid_t));
sessionid is allocated as is new->se_sessionid. why do you see a bug?
0007-Slot-table-sequence-number-processing-for-server.patch:
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> @@ -2181,17 +2200,47 @@ int
> nfsd4_sequence(struct svc_rqst *r, sessionid_t *current_sid, struct
> nfsd4_sequence *seq)
> {
> struct nfs41_session *elem;
> - elem = find_in_sessionid_hashtbl(&seq->sessionid);
> + struct nfs41_slot *slot;
> + int status = nfserr_stale_clientid;
>
> + nfs4_lock_state();
> + elem = find_in_sessionid_hashtbl(&seq->sessionid);
>
> => replace leading spaces with tab
+ elem = find_in_sessionid_hashtbl(&seq->sessionid);
>
> @@ -2181,17 +2200,47 @@ int
> nfsd4_sequence(struct svc_rqst *r, sessionid_t *current_sid, struct
> nfsd4_sequence *seq)
> {
> struct nfs41_session *elem;
> - elem = find_in_sessionid_hashtbl(&seq->sessionid);
> + struct nfs41_slot *slot;
> + int status = nfserr_stale_clientid;
> => int status = nfserr_badsession;
+ nfs4_lock_state();
> + elem = find_in_sessionid_hashtbl(&seq->sessionid);
> if (!elem) {
> printk(KERN_EMERG "stale clientid supplied to
> sequence!!\n");
> => printk(KERN_EMERG "bad sessionid supplied in
> sequence!!\n");
> => dump_sessionid(__FUNCTION__, &seq->sessionid);
> return nfserr_stale_clientid;
> => goto out;
> (need to unlock state)
good catch - thanks!
}
> + if (STALE_CLIENTID(&elem->se_client->cl_clientid))
> => {
> => printk(KERN_EMERG "stale clientid supplied to
> sequence!!\n");
> => status = nfserr_stale_clientid;
> + goto out;
> => }
the coding style here is:
(no tabs because gmail doesn' t let me use them!)
status = nfserr_stale_clientid;
if (STALE_CLIENTID)&elem->se_clietn->cl_clientid))
goto out;
+
> + /* XXX status = nfserr_badslot; */
> + status = nfserr_inval;
> + if (seq->slotid >= elem->se_fnumslots)
> + goto out;
> +
> + slot = &elem->se_slots[seq->slotid];
> + dprintk("%s slotid %d \n", __FUNCTION__, seq->slotid);
> => dprintk("%s slotid %d\n", __FUNCTION__, seq->slotid);
> + status = check_slot_seqid(seq->seqid, slot);
> + if (status == NFSERR_REPLAY_ME)
> + goto replay;
> + else if (status)
> + goto out;
> +
> + /* Success! bump slot seqid */
> + slot->sl_seqid++;
>
> memcpy(current_sid, &seq->sessionid, sizeof(sessionid_t));
> renew_client(elem->se_client);
> - return nfs_ok;
> + status = nfs_ok;
> +out:
> + dprintk("%s returns %d\n", __FUNCTION__, status);
> + nfs4_unlock_state();
> + return status;
> +replay:
> + dprintk("%s REPLAY - AKKKK! no code yet! return STALE CLIENTID\n",
> + __FUNCTION__);
> + status = nfserr_stale_clientid;
> => status = nfserr_badsession;
> (stale_client_id is too strong, restarting the session should be good
> enough here)
> + goto out;
ok.
}
>
> 0008-Expand-current_sid-to-include-the-current-slot.patch:
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>
> => this is not directly related but the calls to kfree() can be
> conditional
> with no additional cost
and indeed they should be. thanks!
@@ -968,8 +968,17 @@ out:
> if (current_fh)
> fh_put(current_fh);
> kfree(current_fh);
> => if (current_fh) {
> => fh_put(current_fh);
> => kfree(current_fh);
> => }
> if (save_fh)
> fh_put(save_fh);
> => if (save_fh) {
> => fh_put(save_fh);
> => kfree(save_fh);
> => }
> - if (current_sid)
> - kfree(current_sid);
> + if (current_ses) {
> + if (current_ses->cs_slot) {
> + if (op && op->status != nfserr_dropit) {
> + dprintk("%s SET SLOT STATE TO
> AVAILABLE\n",
> + __FUNCTION__);
> + current_ses->cs_slot->sl_state =
> NFS4_SLOT_AVAILABLE;
> + }
> +
> nfs41_put_session(current_ses->cs_slot->sl_session);
> + }
> + kfree(current_ses);
> + }
> kfree(save_fh);
> => kfree(save_fh) moved into the "if (save_fh) {}" block, where it belongs
> return status;
> }
>
> 0009-Server-exchange_id-to-draft-ietf-nfsv4-minorversion1.patch:
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> @@ -443,16 +446,28 @@ check:
> return elem;
> }
>
> -void
> -release_session(struct nfs41_session *ses)
> +static void
> +nfs41_free_session(struct kref *kref)
> {
> + struct nfs41_session *ses = container_of(kref, struct
> nfs41_session, se_ref);
> +
> list_del(&ses->se_hash);
> list_del(&ses->se_perclnt);
> - if (ses->se_slots)
> - kfree(ses->se_slots);
> => I don't get it, this was added in patch 0008 and removed here,
> => who frees se_slots then?
i added reference counting on the nfs41_session struct, and used the kref
architecture (used several places in the v4 server code).
look at the kref_put() function. it calls the registered free function when
the reference count goes from 1->0.
kfree(ses);
> }
>
>
> William A. (Andy) Adamson wrote:
> > hi
> >
> > here are 9 patches that implement the server slot table management.
> there is
> > not DRC yet, but this code has been developed with the DRC in mind. This
> > code almost passes the connectathon tests on the 4.1-sessions branch
> with
> > the client patches i just sent out.
> >
> > it fails the general connectathon tests with an NFS4ERR_SEQ_MISORDERED.
> i
> > note that the client mostly uses slot 0, and occasionally uses slot 1.
> the
> > slot 1 sequence is out of order - i don't know if it is the client or
> > server.
> >
> > please review the patches...
> >
> > thanks
> >
> > -->Andy
> >
> >
> >
> > ------------------------------------------------------------------------
> >
> > _______________________________________________
> > pNFS mailing list
> > pNFS at linux-nfs.org
> > http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://linux-nfs.org/pipermail/pnfs/attachments/20070404/e7a31115/attachment-0001.htm
More information about the pNFS
mailing list