| Message ID | 201107282055.p6SKtKp5008047@imap1.linux-foundation.org |
|---|---|
| State | New, archived |
| Headers | show |
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 > -- 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
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
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. > --
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
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
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!
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;