Re: [PATCH] nfs41: Initialize slot->seq_nr at nfs4_init_slot_table()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 2012-02-14 at 19:53 -0500, J. Bruce Fields wrote:
> On Wed, Feb 15, 2012 at 12:43:08AM +0000, Myklebust, Trond wrote:
> > Then we have a problem. The zero initialisation is already in use out
> > there in both commercial and non-commercial versions of Linux. It is too
> > late to change that now.
> > 
> > Furthermore, since none of the servers we've tested against in earlier
> > Bakeathons and Connectathons have complained, I suggest that we rather
> > change the spec with an errata.
> 
> Argh.
> 
> I just noticed that the server was crashing intermittently and traced it
> to incorrect handling of the case where the client sends a one-op
> SEQUENCE compound with seqid 0.  I'm not sure why it started popping up
> just in 3.3--perhaps some change in the way the client uses slots made
> that more likely.
> 
> The server code actually looks like it did assume initial seqid 1, but
> accepted initial seqid 0 (except in this one case) basically by mistake.
> 
> In any case, I think it should be easy enough to teach it just to accept
> any seqid on a previously unused slot, so I'll do that....

Hang on... Looking at 3.2, I think you're correct. We used to initialise
to 1, and now we're initialising to 0.

Ah... I see what happened.

commit aacd5537270a752fe12a9914a207284fc2341c6d (NFSv4.1: cleanup init
and reset of session slot tables) seems to have removed the call to
nfs4_reset_slot_tables() that was doing that initialisation. OK... So
this is a Linux 3.3-rcX only regression, and should be treated as
such...

How about the following instead so that we ensure we fully share the
initialisation code between nfs4_reset_slot_tables and
nfs4_init_slot_tables?

8<-------------------------------------------------------------------------
>From 8ae8d177cc106ecdb3e29b48c78ec37cf76ea549 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
Date: Tue, 14 Feb 2012 20:33:19 -0500
Subject: [PATCH] NFSv4.1: Fix a NFSv4.1 session initialisation regression

Commit aacd553 (NFSv4.1: cleanup init and reset of session slot tables)
introduces a regression in the session initialisation code. New tables
now find their sequence ids initialised to 0, rather than the mandated
value of 1 (see RFC5661).

Reported-by: Vitaliy Gusev <gusev.vitaliy@xxxxxxxxxxx>
Signed-off-by: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
---
 fs/nfs/nfs4proc.c |   58 ++++++++++++++++++++++++++++++++---------------------
 1 files changed, 35 insertions(+), 23 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index d202e04..9f76ce9 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5008,37 +5008,53 @@ int nfs4_proc_get_lease_time(struct nfs_client *clp, struct nfs_fsinfo *fsinfo)
 	return status;
 }
 
+static struct nfs4_slot *nfs4_session_alloc_slots(u32 max_slots, gfp_t gfp_flags)
+{
+	return kcalloc(max_slots, sizeof(struct nfs4_slot), gfp_flags);
+}
+
+static void nfs4_session_add_slots(struct nfs4_slot_table *tbl,
+		struct nfs4_slot *new,
+		u32 max_slots,
+		u32 ivalue)
+{
+	struct nfs4_slot *old = NULL;
+	u32 i;
+
+	spin_lock(&tbl->slot_tbl_lock);
+	if (new) {
+		old = tbl->slots;
+		tbl->slots = new;
+		tbl->max_slots = max_slots;
+	}
+	tbl->highest_used_slotid = -1;	/* no slot is currently used */
+	for (i = 0; i < tbl->max_slots; i++)
+		tbl->slots[i].seq_nr = ivalue;
+	spin_unlock(&tbl->slot_tbl_lock);
+	kfree(old);
+}
+
 /*
  * Reset a slot table
  */
 static int nfs4_reset_slot_table(struct nfs4_slot_table *tbl, u32 max_reqs,
-				 int ivalue)
+				 u32 ivalue)
 {
 	struct nfs4_slot *new = NULL;
-	int i;
-	int ret = 0;
+	int ret = -ENOMEM;
 
 	dprintk("--> %s: max_reqs=%u, tbl->max_slots %d\n", __func__,
 		max_reqs, tbl->max_slots);
 
 	/* Does the newly negotiated max_reqs match the existing slot table? */
 	if (max_reqs != tbl->max_slots) {
-		ret = -ENOMEM;
-		new = kmalloc(max_reqs * sizeof(struct nfs4_slot),
-			      GFP_NOFS);
+		new = nfs4_session_alloc_slots(max_reqs, GFP_NOFS);
 		if (!new)
 			goto out;
-		ret = 0;
-		kfree(tbl->slots);
 	}
-	spin_lock(&tbl->slot_tbl_lock);
-	if (new) {
-		tbl->slots = new;
-		tbl->max_slots = max_reqs;
-	}
-	for (i = 0; i < tbl->max_slots; ++i)
-		tbl->slots[i].seq_nr = ivalue;
-	spin_unlock(&tbl->slot_tbl_lock);
+	ret = 0;
+
+	nfs4_session_add_slots(tbl, new, max_reqs, ivalue);
 	dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
 		tbl, tbl->slots, tbl->max_slots);
 out:
@@ -5064,7 +5080,7 @@ static void nfs4_destroy_slot_tables(struct nfs4_session *session)
  * Initialize slot table
  */
 static int nfs4_init_slot_table(struct nfs4_slot_table *tbl,
-		int max_slots, int ivalue)
+		u32 max_slots, u32 ivalue)
 {
 	struct nfs4_slot *slot;
 	int ret = -ENOMEM;
@@ -5073,16 +5089,12 @@ static int nfs4_init_slot_table(struct nfs4_slot_table *tbl,
 
 	dprintk("--> %s: max_reqs=%u\n", __func__, max_slots);
 
-	slot = kcalloc(max_slots, sizeof(struct nfs4_slot), GFP_NOFS);
+	slot = nfs4_session_alloc_slots(max_slots, GFP_NOFS);
 	if (!slot)
 		goto out;
 	ret = 0;
 
-	spin_lock(&tbl->slot_tbl_lock);
-	tbl->max_slots = max_slots;
-	tbl->slots = slot;
-	tbl->highest_used_slotid = -1;  /* no slot is currently used */
-	spin_unlock(&tbl->slot_tbl_lock);
+	nfs4_session_add_slots(tbl, slot, max_slots, ivalue);
 	dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
 		tbl, tbl->slots, tbl->max_slots);
 out:
-- 
1.7.7.6


-- 
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�����٥



[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux