On Mon 30-05-11 10:22:51, Andreas Dilger wrote:
> On 2011-05-30, at 8:57 AM, Jan Kara wrote:
> > On Mon 30-05-11 22:47:15, Akinobu Mita wrote:
> >> s/ext3_set_bit/__test_and_set_bit_le/
> >> s/ext3_clear_bit/__test_and_clear_bit_le/
> >> s/ext3_test_bit/test_bit_le/
> >> s/ext3_find_first_zero_bit/find_first_zero_bit_le/
> >> s/ext3_find_next_zero_bit/find_next_zero_bit_le/
> >>
> >> This change reveals that there are some __test_and_{set,clear}_bit_le()
> >> calls which ignore the return value. These can be replaced with
> >> __{set,clear}_bit_le().
> >
> > OK, after some though this is probably an improvement in readability.
> > Added to my tree.
>
> Really? There are many different varieties of bitops, and once the
> ext3_* bitops are removed it is no longer clear to the developer which
> version is the right one to use for the ext3 operations.
> test_and_set_bit() vs. __test_and_set_bit() vs. test_and_set_bit_lock()
> vs. sync_test_and_set_bit() vs. __test_and_set_bit_le() vs.
> test_and_set_bit_le()? I think it will lead to confusion and bugs being
> added to the code in the future as different types of operations are
> incorrectly used in the code.
On the other hand hiding the real function behind some wrapper obscures
the gurantees the particular function provides. That's why I decided to
include the patch in the end. But I see your point and yes, I can imagine
someone will use e.g. test_bit() instead of test_bit_le(). That's probably
more likely so I've removed the patches.
Honza
> I doubt most kernel developers understand all of the differences between
> those different bitops on different architectures. What works on one
> arch (especially x86) may not work correctly on another arch, and
> corrupt the filesystem for only a small number of users, producing
> hard-to-debug problems.
>
> Note that I'm all in favour of removing the ext2_{set,clear}_bit_atomic()
> variants of the bitops as global bitops functions, and change the
> ext3_{set,clear}_bit() definitions to using test_and_{set,clear}_bit()
> functions directly, but it makes sense to keep the ext3_*() bitops macros
> and their use in the code for consistency, and as a clear guide to the
> programmer about.
>
> There are many examples of helper macro/inline functions to provide
> uniform access to low-level bitmaps, like cpumask_test_and_set_cpu(),
> cpu_test_and_set(), tasklet_try_lock(), zone_test_and_set_flag(),
> node_test_and_set(), etc. These are all in place so that there is a
> single uniform way of accessing and modifying these structures.
>
> >> Signed-off-by: Akinobu Mita <akinobu.mita@xxxxxxxxx>
> >> Cc: Jan Kara <jack@xxxxxxx>
> >> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> >> Cc: Andreas Dilger <adilger.kernel@xxxxxxxxx>
> >> Cc: linux-ext4@xxxxxxxxxxxxxxx
> >> ---
> >> fs/ext3/balloc.c | 20 ++++++++++----------
> >> fs/ext3/ialloc.c | 8 ++++----
> >> fs/ext3/inode.c | 2 +-
> >> fs/ext3/resize.c | 14 +++++++-------
> >> include/linux/ext3_fs.h | 5 -----
> >> 5 files changed, 22 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/fs/ext3/balloc.c b/fs/ext3/balloc.c
> >> index fe52297..8365b3d 100644
> >> --- a/fs/ext3/balloc.c
> >> +++ b/fs/ext3/balloc.c
> >> @@ -112,21 +112,21 @@ static int ext3_valid_block_bitmap(struct super_block *sb,
> >> /* check whether block bitmap block number is set */
> >> bitmap_blk = le32_to_cpu(desc->bg_block_bitmap);
> >> offset = bitmap_blk - group_first_block;
> >> - if (!ext3_test_bit(offset, bh->b_data))
> >> + if (!test_bit_le(offset, bh->b_data))
> >> /* bad block bitmap */
> >> goto err_out;
> >>
> >> /* check whether the inode bitmap block number is set */
> >> bitmap_blk = le32_to_cpu(desc->bg_inode_bitmap);
> >> offset = bitmap_blk - group_first_block;
> >> - if (!ext3_test_bit(offset, bh->b_data))
> >> + if (!test_bit_le(offset, bh->b_data))
> >> /* bad block bitmap */
> >> goto err_out;
> >>
> >> /* check whether the inode table block number is set */
> >> bitmap_blk = le32_to_cpu(desc->bg_inode_table);
> >> offset = bitmap_blk - group_first_block;
> >> - next_zero_bit = ext3_find_next_zero_bit(bh->b_data,
> >> + next_zero_bit = find_next_zero_bit_le(bh->b_data,
> >> offset + EXT3_SB(sb)->s_itb_per_group,
> >> offset);
> >> if (next_zero_bit >= offset + EXT3_SB(sb)->s_itb_per_group)
> >> @@ -722,14 +722,14 @@ static int ext3_test_allocatable(ext3_grpblk_t nr, struct buffer_head *bh)
> >> int ret;
> >> struct journal_head *jh = bh2jh(bh);
> >>
> >> - if (ext3_test_bit(nr, bh->b_data))
> >> + if (test_bit_le(nr, bh->b_data))
> >> return 0;
> >>
> >> jbd_lock_bh_state(bh);
> >> if (!jh->b_committed_data)
> >> ret = 1;
> >> else
> >> - ret = !ext3_test_bit(nr, jh->b_committed_data);
> >> + ret = !test_bit_le(nr, jh->b_committed_data);
> >> jbd_unlock_bh_state(bh);
> >> return ret;
> >> }
> >> @@ -752,14 +752,14 @@ bitmap_search_next_usable_block(ext3_grpblk_t start, struct buffer_head *bh,
> >> struct journal_head *jh = bh2jh(bh);
> >>
> >> while (start < maxblocks) {
> >> - next = ext3_find_next_zero_bit(bh->b_data, maxblocks, start);
> >> + next = find_next_zero_bit_le(bh->b_data, maxblocks, start);
> >> if (next >= maxblocks)
> >> return -1;
> >> if (ext3_test_allocatable(next, bh))
> >> return next;
> >> jbd_lock_bh_state(bh);
> >> if (jh->b_committed_data)
> >> - start = ext3_find_next_zero_bit(jh->b_committed_data,
> >> + start = find_next_zero_bit_le(jh->b_committed_data,
> >> maxblocks, next);
> >> jbd_unlock_bh_state(bh);
> >> }
> >> @@ -798,7 +798,7 @@ find_next_usable_block(ext3_grpblk_t start, struct buffer_head *bh,
> >> ext3_grpblk_t end_goal = (start + 63) & ~63;
> >> if (end_goal > maxblocks)
> >> end_goal = maxblocks;
> >> - here = ext3_find_next_zero_bit(bh->b_data, end_goal, start);
> >> + here = find_next_zero_bit_le(bh->b_data, end_goal, start);
> >> if (here < end_goal && ext3_test_allocatable(here, bh))
> >> return here;
> >> ext3_debug("Bit not found near goal\n");
> >> @@ -845,7 +845,7 @@ claim_block(spinlock_t *lock, ext3_grpblk_t block, struct buffer_head *bh)
> >> if (ext3_set_bit_atomic(lock, block, bh->b_data))
> >> return 0;
> >> jbd_lock_bh_state(bh);
> >> - if (jh->b_committed_data && ext3_test_bit(block,jh->b_committed_data)) {
> >> + if (jh->b_committed_data && test_bit_le(block, jh->b_committed_data)) {
> >> ext3_clear_bit_atomic(lock, block, bh->b_data);
> >> ret = 0;
> >> } else {
> >> @@ -1697,7 +1697,7 @@ allocated:
> >> int i;
> >>
> >> for (i = 0; i < num; i++) {
> >> - if (ext3_test_bit(grp_alloc_blk+i,
> >> + if (test_bit_le(grp_alloc_blk + i,
> >> bh2jh(bitmap_bh)->b_committed_data)) {
> >> printk("%s: block was unexpectedly set in "
> >> "b_committed_data\n", __func__);
> >> diff --git a/fs/ext3/ialloc.c b/fs/ext3/ialloc.c
> >> index bfc2dc4..8796fbc 100644
> >> --- a/fs/ext3/ialloc.c
> >> +++ b/fs/ext3/ialloc.c
> >> @@ -460,7 +460,7 @@ struct inode *ext3_new_inode(handle_t *handle, struct inode * dir,
> >> ino = 0;
> >>
> >> repeat_in_this_group:
> >> - ino = ext3_find_next_zero_bit((unsigned long *)
> >> + ino = find_next_zero_bit_le(
> >> bitmap_bh->b_data, EXT3_INODES_PER_GROUP(sb), ino);
> >> if (ino < EXT3_INODES_PER_GROUP(sb)) {
> >>
> >> @@ -654,7 +654,7 @@ struct inode *ext3_orphan_get(struct super_block *sb, unsigned long ino)
> >> * is a valid orphan (no e2fsck run on fs). Orphans also include
> >> * inodes that were being truncated, so we can't check i_nlink==0.
> >> */
> >> - if (!ext3_test_bit(bit, bitmap_bh->b_data))
> >> + if (!test_bit_le(bit, bitmap_bh->b_data))
> >> goto bad_orphan;
> >>
> >> inode = ext3_iget(sb, ino);
> >> @@ -680,9 +680,9 @@ iget_failed:
> >> bad_orphan:
> >> ext3_warning(sb, __func__,
> >> "bad orphan inode %lu! e2fsck was run?", ino);
> >> - printk(KERN_NOTICE "ext3_test_bit(bit=%d, block=%llu) = %d\n",
> >> + printk(KERN_NOTICE "test_bit_le(bit=%d, block=%llu) = %d\n",
> >> bit, (unsigned long long)bitmap_bh->b_blocknr,
> >> - ext3_test_bit(bit, bitmap_bh->b_data));
> >> + test_bit_le(bit, bitmap_bh->b_data));
> >> printk(KERN_NOTICE "inode=%p\n", inode);
> >> if (inode) {
> >> printk(KERN_NOTICE "is_bad_inode(inode)=%d\n",
> >> diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
> >> index 3451d23..57432a6 100644
> >> --- a/fs/ext3/inode.c
> >> +++ b/fs/ext3/inode.c
> >> @@ -2727,7 +2727,7 @@ static int __ext3_get_inode_loc(struct inode *inode,
> >> for (i = start; i < start + inodes_per_buffer; i++) {
> >> if (i == inode_offset)
> >> continue;
> >> - if (ext3_test_bit(i, bitmap_bh->b_data))
> >> + if (test_bit_le(i, bitmap_bh->b_data))
> >> break;
> >> }
> >> brelse(bitmap_bh);
> >> diff --git a/fs/ext3/resize.c b/fs/ext3/resize.c
> >> index 7916e4c..213a794 100644
> >> --- a/fs/ext3/resize.c
> >> +++ b/fs/ext3/resize.c
> >> @@ -148,7 +148,7 @@ static void mark_bitmap_end(int start_bit, int end_bit, char *bitmap)
> >>
> >> ext3_debug("mark end bits +%d through +%d used\n", start_bit, end_bit);
> >> for (i = start_bit; i < ((start_bit + 7) & ~7UL); i++)
> >> - ext3_set_bit(i, bitmap);
> >> + __test_and_set_bit_le(i, bitmap);
> >> if (i < end_bit)
> >> memset(bitmap + (i >> 3), 0xff, (end_bit - i) >> 3);
> >> }
> >> @@ -222,7 +222,7 @@ static int setup_new_group_blocks(struct super_block *sb,
> >>
> >> if (ext3_bg_has_super(sb, input->group)) {
> >> ext3_debug("mark backup superblock %#04lx (+0)\n", start);
> >> - ext3_set_bit(0, bh->b_data);
> >> + __test_and_set_bit_le(0, bh->b_data);
> >> }
> >>
> >> /* Copy all of the GDT blocks into the backup in this group */
> >> @@ -254,7 +254,7 @@ static int setup_new_group_blocks(struct super_block *sb,
> >> brelse(gdb);
> >> goto exit_bh;
> >> }
> >> - ext3_set_bit(bit, bh->b_data);
> >> + __test_and_set_bit_le(bit, bh->b_data);
> >> brelse(gdb);
> >> }
> >>
> >> @@ -278,15 +278,15 @@ static int setup_new_group_blocks(struct super_block *sb,
> >> brelse(gdb);
> >> goto exit_bh;
> >> }
> >> - ext3_set_bit(bit, bh->b_data);
> >> + __test_and_set_bit_le(bit, bh->b_data);
> >> brelse(gdb);
> >> }
> >> ext3_debug("mark block bitmap %#04x (+%ld)\n", input->block_bitmap,
> >> input->block_bitmap - start);
> >> - ext3_set_bit(input->block_bitmap - start, bh->b_data);
> >> + __test_and_set_bit_le(input->block_bitmap - start, bh->b_data);
> >> ext3_debug("mark inode bitmap %#04x (+%ld)\n", input->inode_bitmap,
> >> input->inode_bitmap - start);
> >> - ext3_set_bit(input->inode_bitmap - start, bh->b_data);
> >> + __test_and_set_bit_le(input->inode_bitmap - start, bh->b_data);
> >>
> >> /* Zero out all of the inode table blocks */
> >> for (i = 0, block = input->inode_table, bit = block - start;
> >> @@ -309,7 +309,7 @@ static int setup_new_group_blocks(struct super_block *sb,
> >> goto exit_bh;
> >> }
> >> brelse(it);
> >> - ext3_set_bit(bit, bh->b_data);
> >> + __test_and_set_bit_le(bit, bh->b_data);
> >> }
> >>
> >> err = extend_or_restart_transaction(handle, 2, bh);
> >> diff --git a/include/linux/ext3_fs.h b/include/linux/ext3_fs.h
> >> index 5e06acf..6caadd0 100644
> >> --- a/include/linux/ext3_fs.h
> >> +++ b/include/linux/ext3_fs.h
> >> @@ -418,13 +418,8 @@ struct ext3_inode {
> >> #define EXT2_MOUNT_DATA_FLAGS EXT3_MOUNT_DATA_FLAGS
> >> #endif
> >>
> >> -#define ext3_set_bit __test_and_set_bit_le
> >> #define ext3_set_bit_atomic ext2_set_bit_atomic
> >> -#define ext3_clear_bit __test_and_clear_bit_le
> >> #define ext3_clear_bit_atomic ext2_clear_bit_atomic
> >> -#define ext3_test_bit test_bit_le
> >> -#define ext3_find_first_zero_bit find_first_zero_bit_le
> >> -#define ext3_find_next_zero_bit find_next_zero_bit_le
> >>
> >> /*
> >> * Maximal mount counts between two filesystem checks
> >> --
> >> 1.7.4.4
> >>
> > --
> > Jan Kara <jack@xxxxxxx>
> > SUSE Labs, CR
>
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
