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 <[email protected]>
---
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);
> 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
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 <[email protected]>
---
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;
}
}
>> 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
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
>
On Thu, 22 Nov 2007 01:49:19 +0300
Dmitri Vorobiev <[email protected]> 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 <[email protected]>
---
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 Morton пишет:
> On Thu, 22 Nov 2007 01:49:19 +0300
> Dmitri Vorobiev <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
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.
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).