[PATCH]loop cleanup in fs/namespace.c - repost

[PATCH]loop cleanup in fs/namespace.c - repost

[Dmitri Vorobiev]

Hi list,

Apparently, the following two C language constructs

while (condition) {
	// DO HARD WORK
}

and

repeat:
	if (condition) {
		// DO HARD WORK
		goto repeat;
	}

are equivalent, but the latter one looks quite cumbersome and is not idiomatic. However, the mntput_no_expire() routine in fs/namespace.c is implemented using the goto-based approach.

The patch given below replaces the goto-loop by a while-based one. Besides, it removes the export for the same routine, because there are no users for it outside of the core VFS code.

Signed-off-by: Dmitri Vorobiev <dmitri.vorobiev@gmail.com>
---
diff --git a/fs/namespace.c b/fs/namespace.c
index 0608388..38b4b4a 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -278,8 +278,7 @@ static inline void __mntput(struct vfsmo
 
 void mntput_no_expire(struct vfsmount *mnt)
 {
-repeat:
-	if (atomic_dec_and_lock(&mnt->mnt_count, &vfsmount_lock)) {
+	while (atomic_dec_and_lock(&mnt->mnt_count, &vfsmount_lock)) {
 		if (likely(!mnt->mnt_pinned)) {
 			spin_unlock(&vfsmount_lock);
 			__mntput(mnt);
@@ -290,12 +289,9 @@ repeat:
 		spin_unlock(&vfsmount_lock);
 		acct_auto_close_mnt(mnt);
 		security_sb_umount_close(mnt);
-		goto repeat;
 	}
 }
 
-EXPORT_SYMBOL(mntput_no_expire);
-
 void mnt_pin(struct vfsmount *mnt)
 {
 	spin_lock(&vfsmount_lock);

Re: [PATCH]loop cleanup in fs/namespace.c - repost

[Zach Brown]

> The patch given below replaces the goto-loop by a while-based one.

That certainly looks fine.  I would also replace the 'return' with
'break', but I guess that's more of a question of personal preference.

> Besides, it removes the export for the same routine, because there are
> no users for it outside of the core VFS code.

This doesn't look fine.  Did you test this?

mntput_no_expire() is called from mntput() which is an inline function
in mount.h.  So lots of callers of mntput() in modules will end up
trying to call mntput_no_expire() from modules.

$ nm fs/fuse/fuse.ko | grep mntput_no_expire
                 U mntput_no_expire

- z

Re: [PATCH]loop cleanup in fs/namespace.c - repost

[Dmitri Vorobiev]

Zach Brown пишет:
>> The patch given below replaces the goto-loop by a while-based one.
> 
> That certainly looks fine.  I would also replace the 'return' with
> 'break', but I guess that's more of a question of personal preference.
> 
>> Besides, it removes the export for the same routine, because there are
>> no users for it outside of the core VFS code.
> 
> This doesn't look fine.  Did you test this?

Oops, my fault. Of course, I tested the patch, but kernel modules are disabled in my test setup, so I missed the error. Thanks for pointing that out, Zach.

Enclosed to this message is a new patch, which replaces the goto-loop by the while-based one, but leaves the EXPORT_SYMBOL macro intact.

And yes, I agree that for this routine, the break statement looks more appropriate than return.

> 
> mntput_no_expire() is called from mntput() which is an inline function
> in mount.h.  So lots of callers of mntput() in modules will end up
> trying to call mntput_no_expire() from modules.
> 
> $ nm fs/fuse/fuse.ko | grep mntput_no_expire
>                  U mntput_no_expire
> 
> - z
> 

Signed-off-by Dmitri Vorobiev <dmitri.vorobiev@gmail.com>
---
diff --git a/fs/namespace.c b/fs/namespace.c
index 0608388..410e766 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -278,19 +278,17 @@ static inline void __mntput(struct vfsmo
 
 void mntput_no_expire(struct vfsmount *mnt)
 {
-repeat:
-	if (atomic_dec_and_lock(&mnt->mnt_count, &vfsmount_lock)) {
+	while (atomic_dec_and_lock(&mnt->mnt_count, &vfsmount_lock)) {
 		if (likely(!mnt->mnt_pinned)) {
 			spin_unlock(&vfsmount_lock);
 			__mntput(mnt);
-			return;
+			break;
 		}
 		atomic_add(mnt->mnt_pinned + 1, &mnt->mnt_count);
 		mnt->mnt_pinned = 0;
 		spin_unlock(&vfsmount_lock);
 		acct_auto_close_mnt(mnt);
 		security_sb_umount_close(mnt);
-		goto repeat;
 	}
 }
 
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]loop cleanup in fs/namespace.c - repost

[Zach Brown]

>> This doesn't look fine.  Did you test this?
> 
> Oops, my fault. Of course, I tested the patch, but kernel modules are
> disabled in my test setup, so I missed the error.

:)

> Enclosed to this message is a new patch, which replaces the goto-loop by
> the while-based one, but leaves the EXPORT_SYMBOL macro intact.

It certainly looks OK to me now, for whatever that's worth.

You probably want to wait 'till the next merge window to get it in,
though.  It's just a cleanup and so shouldn't go in this late in the -rc
line.

Maybe Andrew will be willing to queue it until that time in -mm.

- z

Re: [PATCH] [2.6.24-rc3-mm1] loop cleanup in fs/namespace.c - repost

[Dmitri Vorobiev]

[-- Attachment #1: Type: text/plain, Size: 810 bytes --]

Zach Brown пишет:
>>> This doesn't look fine.  Did you test this?
>> Oops, my fault. Of course, I tested the patch, but kernel modules are
>> disabled in my test setup, so I missed the error.
> 
> :)
> 
>> Enclosed to this message is a new patch, which replaces the goto-loop by
>> the while-based one, but leaves the EXPORT_SYMBOL macro intact.
> 
> It certainly looks OK to me now, for whatever that's worth. 

Zach, thank you for the code review and suggestions.

> 
> You probably want to wait 'till the next merge window to get it in,
> though.  It's just a cleanup and so shouldn't go in this late in the -rc
> line.
> 
> Maybe Andrew will be willing to queue it until that time in -mm.

I am enclosing the patch against current -mm tree and adding Andrew to the Cc: list.

Thanks,

Dmitri

> 
> - z
> 


[-- Attachment #2: loop-cleanup-fs-namespace-mm.diff --]
[-- Type: text/x-patch, Size: 741 bytes --]

Signed-off-by: Dmitri Vorobiev <dmitri.vorobiev@gmail.com>
---
diff --git a/fs/namespace.c b/fs/namespace.c
index 79883fe..b098b63 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -606,19 +606,17 @@ static inline void __mntput(struct vfsmo
 
 void mntput_no_expire(struct vfsmount *mnt)
 {
-repeat:
-	if (atomic_dec_and_lock(&mnt->mnt_count, &vfsmount_lock)) {
+	while (atomic_dec_and_lock(&mnt->mnt_count, &vfsmount_lock)) {
 		if (likely(!mnt->mnt_pinned)) {
 			spin_unlock(&vfsmount_lock);
 			__mntput(mnt);
-			return;
+			break;
 		}
 		atomic_add(mnt->mnt_pinned + 1, &mnt->mnt_count);
 		mnt->mnt_pinned = 0;
 		spin_unlock(&vfsmount_lock);
 		acct_auto_close_mnt(mnt);
 		security_sb_umount_close(mnt);
-		goto repeat;
 	}
 }
 

Re: [PATCH] [2.6.24-rc3-mm1] loop cleanup in fs/namespace.c - repost

[Andrew Morton]

On Thu, 22 Nov 2007 01:49:19 +0300
Dmitri Vorobiev <dmitri.vorobiev@gmail.com> wrote:

> Zach Brown пишет:
> >>> This doesn't look fine.  Did you test this?
> >> Oops, my fault. Of course, I tested the patch, but kernel modules are
> >> disabled in my test setup, so I missed the error.
> > 
> > :)
> > 
> >> Enclosed to this message is a new patch, which replaces the goto-loop by
> >> the while-based one, but leaves the EXPORT_SYMBOL macro intact.
> > 
> > It certainly looks OK to me now, for whatever that's worth. 
> 
> Zach, thank you for the code review and suggestions.
> 
> > 
> > You probably want to wait 'till the next merge window to get it in,
> > though.  It's just a cleanup and so shouldn't go in this late in the -rc
> > line.
> > 
> > Maybe Andrew will be willing to queue it until that time in -mm.
> 
> I am enclosing the patch against current -mm tree and adding Andrew to the Cc: list.
> 
> Thanks,
> 
> Dmitri
> 
> > 
> > - z
> > 
> 
> 
[loop-cleanup-fs-namespace-mm.diff  text/x-patch (742B)]
Signed-off-by: Dmitri Vorobiev <dmitri.vorobiev@gmail.com>
---
diff --git a/fs/namespace.c b/fs/namespace.c
index 79883fe..b098b63 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -606,19 +606,17 @@ static inline void __mntput(struct vfsmo
 
 void mntput_no_expire(struct vfsmount *mnt)
 {
-repeat:
-	if (atomic_dec_and_lock(&mnt->mnt_count, &vfsmount_lock)) {
+	while (atomic_dec_and_lock(&mnt->mnt_count, &vfsmount_lock)) {
 		if (likely(!mnt->mnt_pinned)) {
 			spin_unlock(&vfsmount_lock);
 			__mntput(mnt);
-			return;
+			break;
 		}
 		atomic_add(mnt->mnt_pinned + 1, &mnt->mnt_count);
 		mnt->mnt_pinned = 0;
 		spin_unlock(&vfsmount_lock);
 		acct_auto_close_mnt(mnt);
 		security_sb_umount_close(mnt);
-		goto repeat;
 	}
 }
 
This patch has no changelog which I can use.

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [2.6.24-rc3-mm1] loop cleanup in fs/namespace.c - repost

[Dmitri Vorobiev]

[-- Attachment #1: Type: text/plain, Size: 2237 bytes --]

Andrew Morton пишет:
> On Thu, 22 Nov 2007 01:49:19 +0300
> Dmitri Vorobiev <dmitri.vorobiev@gmail.com> wrote:
> 
>> Zach Brown пишет:
>>>>> This doesn't look fine.  Did you test this?
>>>> Oops, my fault. Of course, I tested the patch, but kernel modules are
>>>> disabled in my test setup, so I missed the error.
>>> :)
>>>
>>>> Enclosed to this message is a new patch, which replaces the goto-loop by
>>>> the while-based one, but leaves the EXPORT_SYMBOL macro intact.
>>> It certainly looks OK to me now, for whatever that's worth. 
>> Zach, thank you for the code review and suggestions.
>>
>>> You probably want to wait 'till the next merge window to get it in,
>>> though.  It's just a cleanup and so shouldn't go in this late in the -rc
>>> line.
>>>
>>> Maybe Andrew will be willing to queue it until that time in -mm.
>> I am enclosing the patch against current -mm tree and adding Andrew to the Cc: list.
>>
>> Thanks,
>>
>> Dmitri
>>
>>> - z
>>>
>>
> [loop-cleanup-fs-namespace-mm.diff  text/x-patch (742B)]
> Signed-off-by: Dmitri Vorobiev <dmitri.vorobiev@gmail.com>
> ---
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 79883fe..b098b63 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -606,19 +606,17 @@ static inline void __mntput(struct vfsmo
>  
>  void mntput_no_expire(struct vfsmount *mnt)
>  {
> -repeat:
> -	if (atomic_dec_and_lock(&mnt->mnt_count, &vfsmount_lock)) {
> +	while (atomic_dec_and_lock(&mnt->mnt_count, &vfsmount_lock)) {
>  		if (likely(!mnt->mnt_pinned)) {
>  			spin_unlock(&vfsmount_lock);
>  			__mntput(mnt);
> -			return;
> +			break;
>  		}
>  		atomic_add(mnt->mnt_pinned + 1, &mnt->mnt_count);
>  		mnt->mnt_pinned = 0;
>  		spin_unlock(&vfsmount_lock);
>  		acct_auto_close_mnt(mnt);
>  		security_sb_umount_close(mnt);
> -		goto repeat;
>  	}
>  }
>  
> This patch has no changelog which I can use.
> 
> 

Andrew, thanks for the quick reply. I believe that a couple of sentences is enough for the changelog entry, so here it goes...

From: Dmitri Vorobiev <dmitri.vorobiev@gmail.com>

The mntput_no_expire() routine implements a simple loop using the goto-based construct. Replace this with an equivalent while-based loop, which looks much cleaner in C code.



[-- Attachment #2: loop-cleanup-fs-namespace-mm.diff --]
[-- Type: text/x-patch, Size: 741 bytes --]

Signed-off-by: Dmitri Vorobiev <dmitri.vorobiev@gmail.com>
---
diff --git a/fs/namespace.c b/fs/namespace.c
index 79883fe..b098b63 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -606,19 +606,17 @@ static inline void __mntput(struct vfsmo
 
 void mntput_no_expire(struct vfsmount *mnt)
 {
-repeat:
-	if (atomic_dec_and_lock(&mnt->mnt_count, &vfsmount_lock)) {
+	while (atomic_dec_and_lock(&mnt->mnt_count, &vfsmount_lock)) {
 		if (likely(!mnt->mnt_pinned)) {
 			spin_unlock(&vfsmount_lock);
 			__mntput(mnt);
-			return;
+			break;
 		}
 		atomic_add(mnt->mnt_pinned + 1, &mnt->mnt_count);
 		mnt->mnt_pinned = 0;
 		spin_unlock(&vfsmount_lock);
 		acct_auto_close_mnt(mnt);
 		security_sb_umount_close(mnt);
-		goto repeat;
 	}
 }
 

Re: [PATCH] [2.6.24-rc3-mm1] loop cleanup in fs/namespace.c - repost

[Al Viro]

On Wed, Nov 21, 2007 at 03:24:33PM -0800, Andrew Morton wrote:
> -repeat:
> -	if (atomic_dec_and_lock(&mnt->mnt_count, &vfsmount_lock)) {
> +	while (atomic_dec_and_lock(&mnt->mnt_count, &vfsmount_lock)) {
>  		if (likely(!mnt->mnt_pinned)) {
>  			spin_unlock(&vfsmount_lock);
>  			__mntput(mnt);
> -			return;
> +			break;
>  		}
>  		atomic_add(mnt->mnt_pinned + 1, &mnt->mnt_count);
>  		mnt->mnt_pinned = 0;
>  		spin_unlock(&vfsmount_lock);
>  		acct_auto_close_mnt(mnt);
>  		security_sb_umount_close(mnt);
> -		goto repeat;
>  	}
>  }
>  
> This patch has no changelog which I can use.

BTW, I have a problem with that one.  The change is misleading; FWIW,
I'm very tempted to turn that into tail recursion, with

	if (!atomic_dec_and_lock(&mnt->mnt_count, &vfsmount_lock))
		return;

in the very beginning (i.e. invert the test and pull the body to top level).