[patch 1/1] ext4: use proper little-endian bitops

[patch 1/1] ext4: use proper little-endian bitops

[akpm]

From: Akinobu Mita <akinobu.mita@gmail.com>

ext4_{set,clear}_bit() is defined as __test_and_{set,clear}_bit_le() for
ext4.  Only two ext4_{set,clear}_bit() calls check the return value.  The
rest of calls ignore the return value and they can be replaced with
__{set,clear}_bit_le().

This changes ext4_{set,clear}_bit() from __test_and_{set,clear}_bit_le()
to __{set,clear}_bit_le() and introduces ext4_test_and_{set,clear}_bit()
for the two places where old bit needs to be returned.

This ext4_{set,clear}_bit() change is considered safe, because if someone
uses these macros without noticing the change, new ext4_{set,clear}_bit
don't have return value and causes compiler errors where the return value
is used.

This also removes unused ext4_find_first_zero_bit().

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andreas Dilger <adilger.kernel@dilger.ca>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/ext4/ext4.h   |    7 ++++---
 fs/ext4/ialloc.c |    4 ++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff -puN fs/ext4/ext4.h~ext4-use-proper-little-endian-bitops fs/ext4/ext4.h
--- a/fs/ext4/ext4.h~ext4-use-proper-little-endian-bitops
+++ a/fs/ext4/ext4.h
@@ -931,12 +931,13 @@ struct ext4_inode_info {
 #define test_opt2(sb, opt)		(EXT4_SB(sb)->s_mount_opt2 & \
 					 EXT4_MOUNT2_##opt)
 
-#define ext4_set_bit			__test_and_set_bit_le
+#define ext4_test_and_set_bit		__test_and_set_bit_le
+#define ext4_set_bit			__set_bit_le
 #define ext4_set_bit_atomic		ext2_set_bit_atomic
-#define ext4_clear_bit			__test_and_clear_bit_le
+#define ext4_test_and_clear_bit		__test_and_clear_bit_le
+#define ext4_clear_bit			__clear_bit_le
 #define ext4_clear_bit_atomic		ext2_clear_bit_atomic
 #define ext4_test_bit			test_bit_le
-#define ext4_find_first_zero_bit	find_first_zero_bit_le
 #define ext4_find_next_zero_bit		find_next_zero_bit_le
 #define ext4_find_next_bit		find_next_bit_le
 
diff -puN fs/ext4/ialloc.c~ext4-use-proper-little-endian-bitops fs/ext4/ialloc.c
--- a/fs/ext4/ialloc.c~ext4-use-proper-little-endian-bitops
+++ a/fs/ext4/ialloc.c
@@ -252,7 +252,7 @@ void ext4_free_inode(handle_t *handle, s
 		fatal = ext4_journal_get_write_access(handle, bh2);
 	}
 	ext4_lock_group(sb, block_group);
-	cleared = ext4_clear_bit(bit, bitmap_bh->b_data);
+	cleared = ext4_test_and_clear_bit(bit, bitmap_bh->b_data);
 	if (fatal || !cleared) {
 		ext4_unlock_group(sb, block_group);
 		goto out;
@@ -729,7 +729,7 @@ static int ext4_claim_inode(struct super
 	 */
 	down_read(&grp->alloc_sem);
 	ext4_lock_group(sb, group);
-	if (ext4_set_bit(ino, inode_bitmap_bh->b_data)) {
+	if (ext4_test_and_set_bit(ino, inode_bitmap_bh->b_data)) {
 		/* not a free inode */
 		retval = 1;
 		goto err_ret;
_

Re: [patch 1/1] ext4: use proper little-endian bitops

[Lukas Czerner]

On Thu, 28 Jul 2011, akpm@linux-foundation.org wrote:

> From: Akinobu Mita <akinobu.mita@gmail.com>
> 
> ext4_{set,clear}_bit() is defined as __test_and_{set,clear}_bit_le() for
> ext4.  Only two ext4_{set,clear}_bit() calls check the return value.  The
> rest of calls ignore the return value and they can be replaced with
> __{set,clear}_bit_le().
> 
> This changes ext4_{set,clear}_bit() from __test_and_{set,clear}_bit_le()
> to __{set,clear}_bit_le() and introduces ext4_test_and_{set,clear}_bit()
> for the two places where old bit needs to be returned.
> 
> This ext4_{set,clear}_bit() change is considered safe, because if someone
> uses these macros without noticing the change, new ext4_{set,clear}_bit
> don't have return value and causes compiler errors where the return value
> is used.
> 
> This also removes unused ext4_find_first_zero_bit().

Hi Anikobu,

I have some comments bellow.

> 
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  fs/ext4/ext4.h   |    7 ++++---
>  fs/ext4/ialloc.c |    4 ++--
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff -puN fs/ext4/ext4.h~ext4-use-proper-little-endian-bitops fs/ext4/ext4.h
> --- a/fs/ext4/ext4.h~ext4-use-proper-little-endian-bitops
> +++ a/fs/ext4/ext4.h
> @@ -931,12 +931,13 @@ struct ext4_inode_info {
>  #define test_opt2(sb, opt)		(EXT4_SB(sb)->s_mount_opt2 & \
>  					 EXT4_MOUNT2_##opt)
>  
> -#define ext4_set_bit			__test_and_set_bit_le
> +#define ext4_test_and_set_bit		__test_and_set_bit_le
> +#define ext4_set_bit			__set_bit_le
>  #define ext4_set_bit_atomic		ext2_set_bit_atomic
We can remove this since it is not used anywhere and it is just a macro
for test_and_set_bit_le() anyway.

> -#define ext4_clear_bit			__test_and_clear_bit_le
> +#define ext4_test_and_clear_bit		__test_and_clear_bit_le
> +#define ext4_clear_bit			__clear_bit_le
>  #define ext4_clear_bit_atomic		ext2_clear_bit_atomic
Not used as well.

>  #define ext4_test_bit			test_bit_le
> -#define ext4_find_first_zero_bit	find_first_zero_bit_le
>  #define ext4_find_next_zero_bit		find_next_zero_bit_le
>  #define ext4_find_next_bit		find_next_bit_le

Other than that it looks good. You can add my 

Reviewed-by: Lukas Czerner <lczerner@redhat.com>

Thanks!
-Lukas

>  
> diff -puN fs/ext4/ialloc.c~ext4-use-proper-little-endian-bitops fs/ext4/ialloc.c
> --- a/fs/ext4/ialloc.c~ext4-use-proper-little-endian-bitops
> +++ a/fs/ext4/ialloc.c
> @@ -252,7 +252,7 @@ void ext4_free_inode(handle_t *handle, s
>  		fatal = ext4_journal_get_write_access(handle, bh2);
>  	}
>  	ext4_lock_group(sb, block_group);
> -	cleared = ext4_clear_bit(bit, bitmap_bh->b_data);
> +	cleared = ext4_test_and_clear_bit(bit, bitmap_bh->b_data);
>  	if (fatal || !cleared) {
>  		ext4_unlock_group(sb, block_group);
>  		goto out;
> @@ -729,7 +729,7 @@ static int ext4_claim_inode(struct super
>  	 */
>  	down_read(&grp->alloc_sem);
>  	ext4_lock_group(sb, group);
> -	if (ext4_set_bit(ino, inode_bitmap_bh->b_data)) {
> +	if (ext4_test_and_set_bit(ino, inode_bitmap_bh->b_data)) {
>  		/* not a free inode */
>  		retval = 1;
>  		goto err_ret;
> _
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [patch 1/1] ext4: use proper little-endian bitops

[Akinobu Mita]

2011/7/29 Lukas Czerner <lczerner@redhat.com>:

>> diff -puN fs/ext4/ext4.h~ext4-use-proper-little-endian-bitops fs/ext4/ext4.h
>> --- a/fs/ext4/ext4.h~ext4-use-proper-little-endian-bitops
>> +++ a/fs/ext4/ext4.h
>> @@ -931,12 +931,13 @@ struct ext4_inode_info {
>>  #define test_opt2(sb, opt)           (EXT4_SB(sb)->s_mount_opt2 & \
>>                                        EXT4_MOUNT2_##opt)
>>
>> -#define ext4_set_bit                 __test_and_set_bit_le
>> +#define ext4_test_and_set_bit                __test_and_set_bit_le
>> +#define ext4_set_bit                 __set_bit_le
>>  #define ext4_set_bit_atomic          ext2_set_bit_atomic
> We can remove this since it is not used anywhere and it is just a macro
> for test_and_set_bit_le() anyway.

Amir Goldstein requested not to remove it because ext4 snapshot patches is
using ext4_set_bit_atomic(), although I really don't know about the status of
mainline inclusion.

> Other than that it looks good. You can add my
>
> Reviewed-by: Lukas Czerner <lczerner@redhat.com>

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

Re: [patch 1/1] ext4: use proper little-endian bitops

[Lukas Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1447 bytes --]

On Fri, 29 Jul 2011, Akinobu Mita wrote:

> 2011/7/29 Lukas Czerner <lczerner@redhat.com>:
> 
> >> diff -puN fs/ext4/ext4.h~ext4-use-proper-little-endian-bitops fs/ext4/ext4.h
> >> --- a/fs/ext4/ext4.h~ext4-use-proper-little-endian-bitops
> >> +++ a/fs/ext4/ext4.h
> >> @@ -931,12 +931,13 @@ struct ext4_inode_info {
> >>  #define test_opt2(sb, opt)           (EXT4_SB(sb)->s_mount_opt2 & \
> >>                                        EXT4_MOUNT2_##opt)
> >>
> >> -#define ext4_set_bit                 __test_and_set_bit_le
> >> +#define ext4_test_and_set_bit                __test_and_set_bit_le
> >> +#define ext4_set_bit                 __set_bit_le
> >>  #define ext4_set_bit_atomic          ext2_set_bit_atomic
> > We can remove this since it is not used anywhere and it is just a macro
> > for test_and_set_bit_le() anyway.
> 
> Amir Goldstein requested not to remove it because ext4 snapshot patches is
> using ext4_set_bit_atomic(), although I really don't know about the status of
> mainline inclusion.

It is not anywhere near inclusion. Moreover it is using
__test_and_set_bit_le, but the name does not really imply *test*.
So please, just remove it and when Amir is going to
need it someday he might add proper define with the proper name using
the proper set, or test_and_set functions.

Thanks!
-Lukas

> 
> > Other than that it looks good. You can add my
> >
> > Reviewed-by: Lukas Czerner <lczerner@redhat.com>
> 
> Thanks.
> 

-- 

Re: [patch 1/1] ext4: use proper little-endian bitops

[Amir Goldstein]

On Fri, Jul 29, 2011 at 2:51 PM, Lukas Czerner <lczerner@redhat.com> wrote:
> On Fri, 29 Jul 2011, Akinobu Mita wrote:
>
>> 2011/7/29 Lukas Czerner <lczerner@redhat.com>:
>>
>> >> diff -puN fs/ext4/ext4.h~ext4-use-proper-little-endian-bitops fs/ext4/ext4.h
>> >> --- a/fs/ext4/ext4.h~ext4-use-proper-little-endian-bitops
>> >> +++ a/fs/ext4/ext4.h
>> >> @@ -931,12 +931,13 @@ struct ext4_inode_info {
>> >>  #define test_opt2(sb, opt)           (EXT4_SB(sb)->s_mount_opt2 & \
>> >>                                        EXT4_MOUNT2_##opt)
>> >>
>> >> -#define ext4_set_bit                 __test_and_set_bit_le
>> >> +#define ext4_test_and_set_bit                __test_and_set_bit_le
>> >> +#define ext4_set_bit                 __set_bit_le
>> >>  #define ext4_set_bit_atomic          ext2_set_bit_atomic
>> > We can remove this since it is not used anywhere and it is just a macro
>> > for test_and_set_bit_le() anyway.
>>
>> Amir Goldstein requested not to remove it because ext4 snapshot patches is
>> using ext4_set_bit_atomic(), although I really don't know about the status of
>> mainline inclusion.
>
> It is not anywhere near inclusion. Moreover it is using
> __test_and_set_bit_le, but the name does not really imply *test*.
> So please, just remove it and when Amir is going to
> need it someday he might add proper define with the proper name using
> the proper set, or test_and_set functions.
>

Yongqiang,

Please refresh my memory.
Are we still using those macros?
Didn't we say will need to switch to using mballoc's ext4_set_bits() instead?
I think you modified the calls in some of the execution paths and not
all of them
(i.e. not in MOW), which is probably a bug.

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

Re: [patch 1/1] ext4: use proper little-endian bitops

[Akinobu Mita]

2011/7/29 Lukas Czerner <lczerner@redhat.com>:
> On Fri, 29 Jul 2011, Akinobu Mita wrote:
>
>> 2011/7/29 Lukas Czerner <lczerner@redhat.com>:
>>
>> >> diff -puN fs/ext4/ext4.h~ext4-use-proper-little-endian-bitops fs/ext4/ext4.h
>> >> --- a/fs/ext4/ext4.h~ext4-use-proper-little-endian-bitops
>> >> +++ a/fs/ext4/ext4.h
>> >> @@ -931,12 +931,13 @@ struct ext4_inode_info {
>> >>  #define test_opt2(sb, opt)           (EXT4_SB(sb)->s_mount_opt2 & \
>> >>                                        EXT4_MOUNT2_##opt)
>> >>
>> >> -#define ext4_set_bit                 __test_and_set_bit_le
>> >> +#define ext4_test_and_set_bit                __test_and_set_bit_le
>> >> +#define ext4_set_bit                 __set_bit_le
>> >>  #define ext4_set_bit_atomic          ext2_set_bit_atomic
>> > We can remove this since it is not used anywhere and it is just a macro
>> > for test_and_set_bit_le() anyway.
>>
>> Amir Goldstein requested not to remove it because ext4 snapshot patches is
>> using ext4_set_bit_atomic(), although I really don't know about the status of
>> mainline inclusion.
>
> It is not anywhere near inclusion. Moreover it is using
> __test_and_set_bit_le, but the name does not really imply *test*.

BTW, do you think of anything more preferable name?  Because ext[23] and
nilfs2 are still using them, so renaming can avoid confusion.

> So please, just remove it and when Amir is going to
> need it someday he might add proper define with the proper name using
> the proper set, or test_and_set functions.

OK, I'll remove if no one has strong objection against it.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 1/1] ext4: use proper little-endian bitops

[Lukas Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2035 bytes --]

On Fri, 29 Jul 2011, Akinobu Mita wrote:

> 2011/7/29 Lukas Czerner <lczerner@redhat.com>:
> > On Fri, 29 Jul 2011, Akinobu Mita wrote:
> >
> >> 2011/7/29 Lukas Czerner <lczerner@redhat.com>:
> >>
> >> >> diff -puN fs/ext4/ext4.h~ext4-use-proper-little-endian-bitops fs/ext4/ext4.h
> >> >> --- a/fs/ext4/ext4.h~ext4-use-proper-little-endian-bitops
> >> >> +++ a/fs/ext4/ext4.h
> >> >> @@ -931,12 +931,13 @@ struct ext4_inode_info {
> >> >>  #define test_opt2(sb, opt)           (EXT4_SB(sb)->s_mount_opt2 & \
> >> >>                                        EXT4_MOUNT2_##opt)
> >> >>
> >> >> -#define ext4_set_bit                 __test_and_set_bit_le
> >> >> +#define ext4_test_and_set_bit                __test_and_set_bit_le
> >> >> +#define ext4_set_bit                 __set_bit_le
> >> >>  #define ext4_set_bit_atomic          ext2_set_bit_atomic
> >> > We can remove this since it is not used anywhere and it is just a macro
> >> > for test_and_set_bit_le() anyway.
> >>
> >> Amir Goldstein requested not to remove it because ext4 snapshot patches is
> >> using ext4_set_bit_atomic(), although I really don't know about the status of
> >> mainline inclusion.
> >
> > It is not anywhere near inclusion. Moreover it is using
> > __test_and_set_bit_le, but the name does not really imply *test*.
> 
> BTW, do you think of anything more preferable name?  Because ext[23] and
> nilfs2 are still using them, so renaming can avoid confusion.

Well, it is the same situation as you are fixing with this ext4 patch.
So ext2_test_and_set_bit_atomic seems ok, but I really wonder if we need
to call it ext2_* since it is not ext2 specific.

Maybe it would be nice to rename it, as well as move it into different
file, or just rename the file. But it is not *very* important I guess.

> 
> > So please, just remove it and when Amir is going to
> > need it someday he might add proper define with the proper name using
> > the proper set, or test_and_set functions.
> 
> OK, I'll remove if no one has strong objection against it.
> 

Thanks!

[patch 1/1] ext4: use proper little-endian bitops

[akpm]

From: Akinobu Mita <akinobu.mita@gmail.com>
Subject: ext4: use proper little-endian bitops

ext4_{set,clear}_bit() is defined as __test_and_{set,clear}_bit_le() for
ext4.  Only two ext4_{set,clear}_bit() calls check the return value.  The
rest of calls ignore the return value and they can be replaced with
__{set,clear}_bit_le().

This changes ext4_{set,clear}_bit() from __test_and_{set,clear}_bit_le()
to __{set,clear}_bit_le() and introduces ext4_test_and_{set,clear}_bit()
for the two places where old bit needs to be returned.

This ext4_{set,clear}_bit() change is considered safe, because if someone
uses these macros without noticing the change, new ext4_{set,clear}_bit
don't have return value and causes compiler errors where the return value
is used.

This also removes unused ext4_find_first_zero_bit().

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andreas Dilger <adilger.kernel@dilger.ca>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/ext4/ext4.h   |    7 ++++---
 fs/ext4/ialloc.c |    4 ++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff -puN fs/ext4/ext4.h~ext4-use-proper-little-endian-bitops fs/ext4/ext4.h
--- a/fs/ext4/ext4.h~ext4-use-proper-little-endian-bitops
+++ a/fs/ext4/ext4.h
@@ -957,12 +957,13 @@ struct ext4_inode_info {
 #define test_opt2(sb, opt)		(EXT4_SB(sb)->s_mount_opt2 & \
 					 EXT4_MOUNT2_##opt)
 
-#define ext4_set_bit			__test_and_set_bit_le
+#define ext4_test_and_set_bit		__test_and_set_bit_le
+#define ext4_set_bit			__set_bit_le
 #define ext4_set_bit_atomic		ext2_set_bit_atomic
-#define ext4_clear_bit			__test_and_clear_bit_le
+#define ext4_test_and_clear_bit		__test_and_clear_bit_le
+#define ext4_clear_bit			__clear_bit_le
 #define ext4_clear_bit_atomic		ext2_clear_bit_atomic
 #define ext4_test_bit			test_bit_le
-#define ext4_find_first_zero_bit	find_first_zero_bit_le
 #define ext4_find_next_zero_bit		find_next_zero_bit_le
 #define ext4_find_next_bit		find_next_bit_le
 
diff -puN fs/ext4/ialloc.c~ext4-use-proper-little-endian-bitops fs/ext4/ialloc.c
--- a/fs/ext4/ialloc.c~ext4-use-proper-little-endian-bitops
+++ a/fs/ext4/ialloc.c
@@ -252,7 +252,7 @@ void ext4_free_inode(handle_t *handle, s
 		fatal = ext4_journal_get_write_access(handle, bh2);
 	}
 	ext4_lock_group(sb, block_group);
-	cleared = ext4_clear_bit(bit, bitmap_bh->b_data);
+	cleared = ext4_test_and_clear_bit(bit, bitmap_bh->b_data);
 	if (fatal || !cleared) {
 		ext4_unlock_group(sb, block_group);
 		goto out;
@@ -618,7 +618,7 @@ static int ext4_claim_inode(struct super
 	 */
 	down_read(&grp->alloc_sem);
 	ext4_lock_group(sb, group);
-	if (ext4_set_bit(ino, inode_bitmap_bh->b_data)) {
+	if (ext4_test_and_set_bit(ino, inode_bitmap_bh->b_data)) {
 		/* not a free inode */
 		retval = 1;
 		goto err_ret;
_

Re: [patch 1/1] ext4: use proper little-endian bitops

[Ted Ts'o]

On Tue, Nov 15, 2011 at 02:56:06PM -0800, akpm@linux-foundation.org wrote:
> From: Akinobu Mita <akinobu.mita@gmail.com>
> Subject: ext4: use proper little-endian bitops
> 
> ext4_{set,clear}_bit() is defined as __test_and_{set,clear}_bit_le() for
> ext4.  Only two ext4_{set,clear}_bit() calls check the return value.  The
> rest of calls ignore the return value and they can be replaced with
> __{set,clear}_bit_le().
> 
> This changes ext4_{set,clear}_bit() from __test_and_{set,clear}_bit_le()
> to __{set,clear}_bit_le() and introduces ext4_test_and_{set,clear}_bit()
> for the two places where old bit needs to be returned.
> 
> This ext4_{set,clear}_bit() change is considered safe, because if someone
> uses these macros without noticing the change, new ext4_{set,clear}_bit
> don't have return value and causes compiler errors where the return value
> is used.
> 
> This also removes unused ext4_find_first_zero_bit().
> 
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Applied.

						- Ted