Re: [PATCH 05/12] ext4: let ext4_group_add_blocks return an error code

[Amir Goldstein <amir73il@gmail.com>]

On Mon, Jul 18, 2011 at 9:19 AM, Andreas Dilger <adilger@dilger.ca> 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@gmail.com>
>> ---
>> 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@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