[PATCH 0/5] cleanup inode.c and bug fixes

[PATCH 0/5] cleanup inode.c and bug fixes

[andros]

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

[PATCH 1/5] SQUASHME pnfs-submit: remove pnfs_init_once

[andros]

From: Andy Adamson <andros@netapp.com>

Place all layout initialization in nfs4_init_once

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/inode.c |   17 ++++++-----------
 1 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 7989dea..231cfa3 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -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)
-- 
1.6.6

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

[andros]

From: Andy Adamson <andros@netapp.com>

Place all layout initialization in nfs4_init_once

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/inode.c |   19 ++++++-------------
 1 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 231cfa3..fa310b1 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -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
 }
-- 
1.6.6

[PATCH 3/5] SQUASHME pnfs-submit: remove pnfs_destroy_inode

[andros]

From: Andy Adamson <andros@netapp.com>

Move WARN_ONs and list check into pnfs_layout_destroy under the i_lock

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/inode.c |   18 +-----------------
 fs/nfs/pnfs.c  |    9 +++++++++
 fs/nfs/pnfs.h  |    4 ++++
 3 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index fa310b1..229cdab 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -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);
 }
 
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index baa3de7..fb9374b 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -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);
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index c60eff6..9b0fed4 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -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)
 {
 }
-- 
1.6.6

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

[andros]

From: Andy Adamson <andros@netapp.com>

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/nfs4proc.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 6283996..36de1b3 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -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)
-- 
1.6.6

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

[andros]

From: Andy Adamson <andros@netapp.com>

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/nfs4proc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 36de1b3..c5b8a2f 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -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__);
 }
 
-- 
1.6.6

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

[Boaz Harrosh]

On 07/07/2010 12:16 AM, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> Place all layout initialization in nfs4_init_once
> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfs/inode.c |   19 ++++++-------------
>  1 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 231cfa3..fa310b1 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -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;

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


>  #endif /* CONFIG_NFS_V4_1 */
>  #endif
>  }

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

[William A. (Andy) Adamson]

On Wed, Jul 7, 2010 at 5:27 AM, Boaz Harrosh <bharrosh@panasas.com> wro=
te:
> On 07/07/2010 12:16 AM, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> Place all layout initialization in nfs4_init_once
>>
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>> =A0fs/nfs/inode.c | =A0 19 ++++++-------------
>> =A01 files changed, 6 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>> index 231cfa3..fa310b1 100644
>> --- a/fs/nfs/inode.c
>> +++ b/fs/nfs/inode.c
>> @@ -1361,18 +1361,6 @@ void nfs4_clear_inode(struct inode *inode)
>> =A0}
>> =A0#endif
>>
>> -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;
>> @@ -1388,7 +1376,6 @@ struct inode *nfs_alloc_inode(struct super_blo=
ck *sb)
>> =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}
>>
>> @@ -1430,6 +1417,12 @@ static inline void nfs4_init_once(struct nfs_=
inode *nfsi)
>> =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
> 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. Addi=
ng zero initialization
> to every new member, and an extra remove line to any member remove is=
 just maintenance
> 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.

>
> (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
>
> Boaz
>
>
>> =A0#endif /* CONFIG_NFS_V4_1 */
>> =A0#endif
>> =A0}
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" =
in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html
>