Skip to contentSkip to conversation

cleanup inode.c and bug fixes

PATCH In review [email protected] 3 participants 8 messages Jul 6, 2010 – Jul 7, 2010

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

[email protected]

author
#0 lore raw

cover cleanup inode.c and bug fixes

For the pnfs-submit branch 2.6.35-rc3 7-6-2010 tree.

These first three patches simplify the inode.c pnfs initialization calls. 0001-SQUASHME-pnfs-submit-remove-pnfs_init_once.patch 0002-SQUASHME-pnfs-submit-remove-pnfs_alloc_init_inode.patch 0003-SQUASHME-pnfs-submit-remove-pnfs_destroy_inode.patch

These two patches fix bugs in the layoutget and layoutcommit error handling where the calldata->status was not set after the async error handler in the rpc_call_done routines.

0004-SQUASHME-pnfs-submit-set-layoutcommit-status-after-a.patch 0005-SQUASHME-pnfs-submit-set-layoutget-status-after-asyn.patch

Tested:
CONFIG_NFS_V4_1 set:

pNFS mount connectathon tests pass.
pyNFS server with NFS4ERR_DELAY returned by layoutget handles the error in the async error handler, and then the calldata->status is set to NFS4_OK.

CONFIG_NFS_V4_1 not set;

NFSv4.0 mount passes connectathon tests.

-->Andy

author
#1 lore raw

PATCH 1/5 SQUASHME pnfs-submit: remove pnfs_init_once

From: Andy Adamson <[email protected]>

Place all layout initialization in nfs4_init_once

Signed-off-by Andy Adamson <[email protected]>
---
 fs/nfs/inode.c |   17 ++++++-----------
 1 files changed, 6 insertions(+), 11 deletions(-)
fs/nfs/inode.c +6 -11
index 7989dea..231cfa3 100644
@@ -1416,8 +1416,13 @@ void nfs_destroy_inode(struct inode *inode)
 	kmem_cache_free(nfs_inode_cachep, nfsi);
 }
 
-static void pnfs_init_once(struct nfs_inode *nfsi)
+static inline void nfs4_init_once(struct nfs_inode *nfsi)
 {
+#ifdef CONFIG_NFS_V4
+	INIT_LIST_HEAD(&nfsi->open_states);
+	nfsi->delegation = NULL;
+	nfsi->delegation_state = 0;
+	init_rwsem(&nfsi->rwsem);
 #ifdef CONFIG_NFS_V4_1
 	init_waitqueue_head(&nfsi->lo_waitq);
 	seqlock_init(&nfsi->layout.seqlock);

@@ -1426,15 +1431,6 @@ static void pnfs_init_once(struct nfs_inode *nfsi)
 	nfsi->layout.refcount = 0;
 	nfsi->layout.ld_data = NULL;
 #endif /* CONFIG_NFS_V4_1 */
-}
-
-static inline void nfs4_init_once(struct nfs_inode *nfsi)
-{
-#ifdef CONFIG_NFS_V4
-	INIT_LIST_HEAD(&nfsi->open_states);
-	nfsi->delegation = NULL;
-	nfsi->delegation_state = 0;
-	init_rwsem(&nfsi->rwsem);
 #endif
 }
 

@@ -1453,7 +1449,6 @@ static void init_once(void *foo)
 	INIT_HLIST_HEAD(&nfsi->silly_list);
 	init_waitqueue_head(&nfsi->waitqueue);
 	nfs4_init_once(nfsi);
-	pnfs_init_once(nfsi);
 }
 
 static int __init nfs_init_inodecache(void)
Signature · 1 line
1.6.6
author
#2 lore raw

PATCH 2/5 SQUASHME pnfs-submit remove pnfs_alloc_init_inode

From: Andy Adamson <[email protected]>

Place all layout initialization in nfs4_init_once

Signed-off-by Andy Adamson <[email protected]>
---
 fs/nfs/inode.c |   19 ++++++-------------
 1 files changed, 6 insertions(+), 13 deletions(-)
fs/nfs/inode.c +6 -13
index 231cfa3..fa310b1 100644
@@ -1361,18 +1361,6 @@ void nfs4_clear_inode(struct inode *inode)
 }
 #endif
 
-static void pnfs_alloc_init_inode(struct nfs_inode *nfsi)
-{
-#ifdef CONFIG_NFS_V4_1
-	nfsi->layout.pnfs_layout_state = 0;
-	memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
-	nfsi->layout.roc_iomode = 0;
-	nfsi->layout.lo_cred = NULL;
-	nfsi->layout.pnfs_write_begin_pos = 0;
-	nfsi->layout.pnfs_write_end_pos = 0;
-#endif /* CONFIG_NFS_V4_1 */
-}
-
 struct inode *nfs_alloc_inode(struct super_block *sb)
 {
 	struct nfs_inode *nfsi;

@@ -1388,7 +1376,6 @@ struct inode *nfs_alloc_inode(struct super_block *sb)
 #ifdef CONFIG_NFS_V4
 	nfsi->nfs4_acl = NULL;
 #endif /* CONFIG_NFS_V4 */
-	pnfs_alloc_init_inode(nfsi);
 	return &nfsi->vfs_inode;
 }
 

@@ -1430,6 +1417,12 @@ static inline void nfs4_init_once(struct nfs_inode *nfsi)
 	INIT_LIST_HEAD(&nfsi->layout.segs);
 	nfsi->layout.refcount = 0;
 	nfsi->layout.ld_data = NULL;
+	nfsi->layout.pnfs_layout_state = 0;
+	memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
+	nfsi->layout.roc_iomode = 0;
+	nfsi->layout.lo_cred = NULL;
+	nfsi->layout.pnfs_write_begin_pos = 0;
+	nfsi->layout.pnfs_write_end_pos = 0;
 #endif /* CONFIG_NFS_V4_1 */
 #endif
 }
Signature · 1 line
1.6.6
author
#3 lore raw

PATCH 3/5 SQUASHME pnfs-submit: remove pnfs_destroy_inode

From: Andy Adamson <[email protected]>

Move WARN_ONs and list check into pnfs_layout_destroy under the i_lock

Signed-off-by Andy Adamson <[email protected]>
---
 fs/nfs/inode.c |   18 +-----------------
 fs/nfs/pnfs.c  |    9 +++++++++
 fs/nfs/pnfs.h  |    4 ++++
 3 files changed, 14 insertions(+), 17 deletions(-)
fs/nfs/inode.c +1 -17
index fa310b1..229cdab 100644
@@ -1379,27 +1379,11 @@ struct inode *nfs_alloc_inode(struct super_block *sb)
 	return &nfsi->vfs_inode;
 }
 
-static void pnfs_destroy_inode(struct nfs_inode *nfsi)
-{
-#ifdef CONFIG_NFS_V4_1
-	if (!list_empty(&nfsi->layout.segs))
-		pnfs_destroy_layout(nfsi);
-
-	WARN_ON(!list_empty(&nfsi->layout.segs));
-	if (nfsi->layout.refcount)
-		printk("%s: WARNING: layout.refcount %d\n", __func__,
-			nfsi->layout.refcount);
-	WARN_ON(nfsi->layout.refcount);
-	WARN_ON(!list_empty(&nfsi->layout.lo_layouts));
-	WARN_ON(nfsi->layout.ld_data);
-#endif /* CONFIG_NFS_V4_1 */
-}
-
 void nfs_destroy_inode(struct inode *inode)
 {
 	struct nfs_inode *nfsi = NFS_I(inode);
 
-	pnfs_destroy_inode(nfsi);
+	pnfs_destroy_layout(nfsi);
 	kmem_cache_free(nfs_inode_cachep, nfsi);
 }
fs/nfs/pnfs.c +9 -0
index baa3de7..fb9374b 100644
@@ -379,6 +379,15 @@ pnfs_destroy_layout(struct nfs_inode *nfsi)
 	lo = grab_current_layout(nfsi);
 	if (lo) {
 		pnfs_free_layout(lo, &range);
+		WARN_ON(!list_empty(&nfsi->layout.segs));
+		WARN_ON(!list_empty(&nfsi->layout.lo_layouts));
+		WARN_ON(nfsi->layout.ld_data);
+
+		if (nfsi->layout.refcount != 1)
+			printk(KERN_WARNING "%s: layout refcount not=1 %d\n",
+				__func__, nfsi->layout.refcount);
+		WARN_ON(nfsi->layout.refcount != 1);
+
 		put_layout(lo);
 	}
 	spin_unlock(&nfsi->vfs_inode.i_lock);
fs/nfs/pnfs.h +4 -0
index c60eff6..9b0fed4 100644
@@ -190,6 +190,10 @@ static inline void pnfs_update_layout(struct inode *ino,
 
 #else  /* CONFIG_NFS_V4_1 */
 
+static inline void pnfs_destroy_layout(struct nfs_inode *nfsi)
+{
+}
+
 static inline void get_lseg(struct pnfs_layout_segment *lseg)
 {
 }
Signature · 1 line
1.6.6
author
#4 lore raw

PATCH 4/5 SQUASHME pnfs-submit: set layoutcommit status after async error handler

From: Andy Adamson <[email protected]>

Signed-off-by Andy Adamson <[email protected]>
---
 fs/nfs/nfs4proc.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
fs/nfs/nfs4proc.c +2 -2
index 6283996..36de1b3 100644
@@ -5549,14 +5549,14 @@ pnfs_layoutcommit_done(struct rpc_task *task, void *calldata)
 		(struct pnfs_layoutcommit_data *)calldata;
 	struct nfs_server *server = NFS_SERVER(data->args.inode);
 
-	data->status = task->tk_status;
-
 	nfs4_sequence_done(server, &data->res.seq_res, task->tk_status);
 	if (RPC_ASSASSINATED(task))
 		return;
 
 	if (nfs4_async_handle_error(task, server, NULL, NULL) == -EAGAIN)
 		nfs_restart_rpc(task, server->nfs_client);
+
+	data->status = task->tk_status;
 }
 
 static void pnfs_layoutcommit_release(void *lcdata)
Signature · 1 line
1.6.6
author
#5 lore raw

PATCH 5/5 SQUASHME pnfs-submit: set layoutget status after async error handler

From: Andy Adamson <[email protected]>

Signed-off-by Andy Adamson <[email protected]>
---
 fs/nfs/nfs4proc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
fs/nfs/nfs4proc.c +1 -0
index 36de1b3..c5b8a2f 100644
@@ -5450,6 +5450,7 @@ static void nfs4_pnfs_layoutget_done(struct rpc_task *task, void *calldata)
 	if (nfs4_async_handle_error(task, server, NULL, NULL) == -EAGAIN)
 		nfs_restart_rpc(task, server->nfs_client);
 
+	lgp->status = task->tk_status;
 	dprintk("<-- %s\n", __func__);
 }
Signature · 1 line
1.6.6
#6 lore raw

Re: [PATCH 2/5] SQUASHME pnfs-submit remove pnfs_alloc_init_inode

In reply to [email protected]
Quoted 41 lines · On 07/07/2010 12:16 AM, [email protected] wrote:

From: Andy Adamson <[email protected]>

Place all layout initialization in nfs4_init_once

Signed-off-by Andy Adamson <[email protected]>
---
 fs/nfs/inode.c |   19 ++++++-------------
 1 files changed, 6 insertions(+), 13 deletions(-)
fs/nfs/inode.c +0 -13
index 231cfa3..fa310b1 100644
@@ -1361,18 +1361,6 @@ void nfs4_clear_inode(struct inode *inode)
 }
 #endif
 
-static void pnfs_alloc_init_inode(struct nfs_inode *nfsi)
-{
-#ifdef CONFIG_NFS_V4_1
-	nfsi->layout.pnfs_layout_state = 0;
-	memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
-	nfsi->layout.roc_iomode = 0;
-	nfsi->layout.lo_cred = NULL;
-	nfsi->layout.pnfs_write_begin_pos = 0;
-	nfsi->layout.pnfs_write_end_pos = 0;
-#endif /* CONFIG_NFS_V4_1 */
-}
-
 struct inode *nfs_alloc_inode(struct super_block *sb)
 {
 	struct nfs_inode *nfsi;

@@ -1388,7 +1376,6 @@ struct inode *nfs_alloc_inode(struct super_block *sb)
 #ifdef CONFIG_NFS_V4
 	nfsi->nfs4_acl = NULL;
 #endif /* CONFIG_NFS_V4 */
-	pnfs_alloc_init_inode(nfsi);
 	return &nfsi->vfs_inode;
 }

fs/nfs/inode.c static inline void nfs4_init_once(struct nfs_inode *nfsi) · line 1425

Quoted 9 lines
fs/nfs/inode.c +6 -0
@@ -1430,6 +1417,12 @@ static inline void nfs4_init_once(struct nfs_inode *nfsi)
 	INIT_LIST_HEAD(&nfsi->layout.segs);
 	nfsi->layout.refcount = 0;
 	nfsi->layout.ld_data = NULL;
+	nfsi->layout.pnfs_layout_state = 0;
+	memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
+	nfsi->layout.roc_iomode = 0;
+	nfsi->layout.lo_cred = NULL;
+	nfsi->layout.pnfs_write_begin_pos = 0;
+	nfsi->layout.pnfs_write_end_pos = 0;

If we are already here. What are the rules with zeros? It is costume elsewhere in Kernel that at construction points all zeros are just not done, if a kzalloc or memset is guaranteed. Isn't nfs_inode zero_allocated? If not it should. Adding zero initialization to every new member, and an extra remove line to any member remove is just maintenance nightmare.

(This patch could be much more beautiful if it was "only removed lines")

Boaz

Quoted 3 lines
fs/nfs/inode.c +0 -0
 #endif /* CONFIG_NFS_V4_1 */
 #endif
 }
#7 lore raw

Re: [PATCH 2/5] SQUASHME pnfs-submit remove pnfs_alloc_init_inode

On Wed, Jul 7, 2010 at 5:27 AM, Boaz Harrosh <[email protected]> wro= te:

Quoted 18 lines · On 07/07/2010 12:16 AM, [email protected] wrote:
Quoted 17 lines

From: Andy Adamson <[email protected]>

Place all layout initialization in nfs4_init_once

Signed-off-by Andy Adamson <[email protected]>

---
=A0fs/nfs/inode.c | =A0 19 ++++++------------- =A01 files changed, 6 insertions(+), 13 deletions(-)

fs/nfs/inode.c +0 -0
index 231cfa3..fa310b1 100644
@@ -1361,18 +1361,6 @@ void nfs4_clear_inode(struct inode *inode)
 =A0}

=A0#endif

Quoted 16 lines

fs/nfs/inode.c struct inode *nfs_alloc_inode(struct super_blo=

Quoted 16 lines
fs/nfs/inode.c +0 -12
-static void pnfs_alloc_init_inode(struct nfs_inode *nfsi)
-{
-#ifdef CONFIG_NFS_V4_1
- =A0 =A0 nfsi->layout.pnfs_layout_state =3D 0;
- =A0 =A0 memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
- =A0 =A0 nfsi->layout.roc_iomode =3D 0;
- =A0 =A0 nfsi->layout.lo_cred =3D NULL;
- =A0 =A0 nfsi->layout.pnfs_write_begin_pos =3D 0;
- =A0 =A0 nfsi->layout.pnfs_write_end_pos =3D 0;
-#endif /* CONFIG_NFS_V4_1 */
-}
-

=A0struct inode *nfs_alloc_inode(struct super_block *sb) =A0{
=A0 =A0 =A0 struct nfs_inode *nfsi;

fs/nfs/inode.c +0 -0
@@ -1388,7 +1376,6 @@ struct inode *nfs_alloc_inode(struct super_blo=

ck *sb)

Quoted 8 lines

fs/nfs/inode.c static inline void nfs4_init_once(struct nfs_=

Quoted 8 lines

=A0#ifdef CONFIG_NFS_V4
=A0 =A0 =A0 nfsi->nfs4_acl =3D NULL;
=A0#endif /* CONFIG_NFS_V4 */
- =A0 =A0 pnfs_alloc_init_inode(nfsi);
=A0 =A0 =A0 return &nfsi->vfs_inode;
=A0}

fs/nfs/inode.c +0 -0
@@ -1430,6 +1417,12 @@ static inline void nfs4_init_once(struct nfs_=

inode *nfsi)

Quoted 11 lines

fs/nfs/inode.c static inline void nfs4_init_once(struct nfs_= · line 1425

Quoted 9 lines
fs/nfs/inode.c +6 -0
 =A0 =A0 =A0 INIT_LIST_HEAD(&nfsi->layout.segs);
 =A0 =A0 =A0 nfsi->layout.refcount =3D 0;
 =A0 =A0 =A0 nfsi->layout.ld_data =3D NULL;
+ =A0 =A0 nfsi->layout.pnfs_layout_state =3D 0;
+ =A0 =A0 memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
+ =A0 =A0 nfsi->layout.roc_iomode =3D 0;
+ =A0 =A0 nfsi->layout.lo_cred =3D NULL;
+ =A0 =A0 nfsi->layout.pnfs_write_begin_pos =3D 0;
+ =A0 =A0 nfsi->layout.pnfs_write_end_pos =3D 0;

If we are already here. What are the rules with zeros? It is costume =

elsewhere in

Quoted 1 line

Kernel that at construction points all zeros are just not done, if a =

kzalloc or memset

Quoted 1 line

is guaranteed. Isn't nfs_inode zero_allocated? If not it should. Addi=

ng zero initialization

Quoted 1 line

to every new member, and an extra remove line to any member remove is=

just maintenance

Quoted 1 line

nightmare.

Well, the next patch set will remove all but one zero assignment. Second; no, nfs_alloc_inode does not zero out all nfs_inode fields.

Quoted 2 lines

(This patch could be much more beautiful if it was "only removed line=

s")

The end result will be beautiful. I just wanted to break up all the cha= nges.

-->Andy

Quoted 10 lines

Boaz

Quoted 3 lines

=A0#endif /* CONFIG_NFS_V4_1 */
=A0#endif
=A0}

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" =

in

Quoted 3 lines

the body of a message to [email protected]
More majordomo info at =A0http://vger.kernel.org/majordomo-info.html