On Mon, Jul 18, 2011 at 9:19 AM, Andreas Dilger <adilger@xxxxxxxxx> wrote:
> On 2011-07-17, at 8:52 PM, Yongqiang Yang wrote:
>> This patch lets ext4_group_add_blocks() return an error code if it fails,
>> so that upper functions can handle error correctly.
>
> This patch is somewhat incorrect, though it seems the existing code is
> also incorrect, which isn't the fault of this patch, but should be
> fixed if this code is being reworked.
>
> When this code was originally written, this code could not fail
> unless the filesystem had been marked read-only or the journal was
> aborted. It only freed blocks using ext3_free_blocks_sb(), which
> only had error paths calling ext3_error().
>
> It was purposely done that way because all of the failure cases
> (e.g. not being able to read the block bitmap) were checked in
> advance, before modifying any of the filesystem metadata.
>
> In the case of this patch, ext4_blocks_count_set() is called to
> add the new blocks to the total in the superblock. If this code
> fails, it doesn't try to reset the total number of blocks in the
> superblock, which can cause e2fsck to be unhappy.
>
> Looking at this code more closely, it seems that ext4_group_add_blocks()
> (formerly ext4_add_groupblocks()) is now only used by the resize code,
> which IMHO is wrong. What it was doing in the past was using existing
> code to "free" the blocks at the end of the block bitmap, as if a file
> had just been deleted. As it is now, it seems to be duplicating a lot
> of what is in ext4_free_blocks().
This is my (git) blame.
Before I had my way with ext4_add_groupblocks() it was practically using
old remains of ext3_free_blocks_sb() code (see commit e73a347b).
To make things work better (without a rw_sem) I copied code from
ext4_free_blocks(), not messing with the latter on purpose.
Now that my changes have proven to work (?), the core freeing of
the blocks can be broken out to a helper function.
>
> It probably makes sense to try and get some consolidated helper
> function that includes nearly all of the code in ext4_free_blocks between
> "do_more:" and "error_return:" (all of the work of verifying the blocks
> to be freed, updating the block bitmap and the buddy bitmap, updates the
> group descriptors and superblock, and marks the blocks dirty in the
> journal), but not the code that checks the writeback mode, updates quota,
> or any of that (since this isn't really using an inode).
>
>> Signed-off-by: Yongqiang Yang <xiaoqiangnk@xxxxxxxxx>
>> ---
>> fs/ext4/ext4.h | 2 +-
>> fs/ext4/mballoc.c | 19 +++++++++++++------
>> fs/ext4/resize.c | 10 +++++++---
>> 3 files changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>> index bbe81db..da7ab48 100644
>> --- a/fs/ext4/ext4.h
>> +++ b/fs/ext4/ext4.h
>> @@ -1799,7 +1799,7 @@ extern void ext4_free_blocks(handle_t *handle, struct inode *inode,
>> unsigned long count, int flags);
>> extern int ext4_mb_add_groupinfo(struct super_block *sb,
>> ext4_group_t i, struct ext4_group_desc *desc);
>> -extern void ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
>> +extern int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
>> ext4_fsblk_t block, unsigned long count);
>> extern int ext4_trim_fs(struct super_block *, struct fstrim_range *);
>>
>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>> index ea80c0b..33c41e6 100644
>> --- a/fs/ext4/mballoc.c
>> +++ b/fs/ext4/mballoc.c
>> @@ -4664,7 +4664,7 @@ error_return:
>> *
>> * This marks the blocks as free in the bitmap and buddy.
>> */
>> -void ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
>> +int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
>> ext4_fsblk_t block, unsigned long count)
>> {
>> struct buffer_head *bitmap_bh = NULL;
>> @@ -4685,15 +4685,21 @@ void ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
>> * Check to see if we are freeing blocks across a group
>> * boundary.
>> */
>> - if (bit + count > EXT4_BLOCKS_PER_GROUP(sb))
>> - goto error_return;
>> + if (bit + count > EXT4_BLOCKS_PER_GROUP(sb)) {
>> + ext4_warning(sb, "too much blocks added to group %u\n",
>> + block_group);
>> + return -EINVAL;
>> + }
>>
>> bitmap_bh = ext4_read_block_bitmap(sb, block_group);
>> if (!bitmap_bh)
>> - goto error_return;
>> + return -EIO;
>> +
>> desc = ext4_get_group_desc(sb, block_group, &gd_bh);
>> - if (!desc)
>> + if (!desc) {
>> + err = -EIO;
>> goto error_return;
>> + }
>>
>> if (in_range(ext4_block_bitmap(sb, desc), block, count) ||
>> in_range(ext4_inode_bitmap(sb, desc), block, count) ||
>> @@ -4703,6 +4709,7 @@ void ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
>> ext4_error(sb, "Adding blocks in system zones - "
>> "Block = %llu, count = %lu",
>> block, count);
>> + err = -EINVAL;
>> goto error_return;
>> }
>>
>> @@ -4771,7 +4778,7 @@ void ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
>> error_return:
>> brelse(bitmap_bh);
>> ext4_std_error(sb, err);
>> - return;
>> + return err;
>> }
>>
>> /**
>> diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
>> index 374fd1c..2e63376 100644
>> --- a/fs/ext4/resize.c
>> +++ b/fs/ext4/resize.c
>> @@ -990,7 +990,7 @@ int ext4_group_extend(struct super_block *sb, struct ext4_super_block *es,
>> ext4_grpblk_t add;
>> struct buffer_head *bh;
>> handle_t *handle;
>> - int err;
>> + int err, err2;
>> ext4_group_t group;
>>
>> o_blocks_count = ext4_blocks_count(es);
>> @@ -1066,11 +1066,15 @@ int ext4_group_extend(struct super_block *sb, struct ext4_super_block *es,
>> ext4_debug("freeing blocks %llu through %llu\n", o_blocks_count,
>> o_blocks_count + add);
>> /* We add the blocks to the bitmap and set the group need init bit */
>> - ext4_group_add_blocks(handle, sb, o_blocks_count, add);
>> + err = ext4_group_add_blocks(handle, sb, o_blocks_count, add);
>> ext4_handle_dirty_super(handle, sb);
>> ext4_debug("freed blocks %llu through %llu\n", o_blocks_count,
>> o_blocks_count + add);
>> - if ((err = ext4_journal_stop(handle)))
>> + err2 = ext4_journal_stop(handle);
>> + if (!err && err2)
>> + err = err2;
>> +
>> + if (err)
>> goto exit_put;
>>
>> if (test_opt(sb, DEBUG))
>> --
>> 1.7.5.1
>>
>
>
> Cheers, Andreas
>
>
>
>
>
> --
> 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
>
--
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
