Re: [PATCH] ext4: add error handling when discarding is fail inFITRIM ioctl

[Namjae Jeon]

Hi. Ted.

Would you check this patch ? maybe, this patch was posted during your vacation.

Thanks.

2012/1/25 Namjae Jeon <linkinjeon@xxxxxxxxx>:
> 2012/1/24 Jan Kara <jack@xxxxxxx>:
>> On Sat 21-01-12 02:56:12, Namjae Jeon wrote:
>>> Although free extents is proper not trimmed(mmc driver return error code while sending trim command), currently FITRIM ioctl return success.
>>> I tried to add exception routine to inform user error code.
>>>
>>> #> ./fitrim_test
>>> end_request: I/O error, dev mmcblk0, sector 27232
>>> EXT4-fs warning (device mmcblk0): ext4_trim_all_free:4857: Discard command returned error -5
>>> #>
>>>
>>> Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxx>
>>> Signed-off-by: Amit Sahrawat <amit.sahrawat83@xxxxxxxxx>
>>> ---
>>> Âfs/ext4/mballoc.c | Â 22 +++++++++++++++-------
>>> Â1 files changed, 15 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>>> index cb990b2..09cc09a 100644
>>> --- a/fs/ext4/mballoc.c
>>> +++ b/fs/ext4/mballoc.c
>>> @@ -4901,10 +4901,11 @@ error_return:
>>> Â * one will allocate those blocks, mark it as used in buddy bitmap. This must
>>> Â * be called with under the group lock.
>>> Â */
>>> -static void ext4_trim_extent(struct super_block *sb, int start, int count,
>>> +static int ext4_trim_extent(struct super_block *sb, int start, int count,
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Âext4_group_t group, struct ext4_buddy *e4b)
>>> Â{
>>> Â Â Â struct ext4_free_extent ex;
>>> + Â Â int err;
>>>
>>> Â Â Â trace_ext4_trim_extent(sb, group, start, count);
>>>
>>> @@ -4920,9 +4921,10 @@ static void ext4_trim_extent(struct super_block *sb, int start, int count,
>>> Â Â Â Â*/
>>> Â Â Â mb_mark_used(e4b, &ex);
>>> Â Â Â ext4_unlock_group(sb, group);
>>> - Â Â ext4_issue_discard(sb, group, start, count);
>>> + Â Â err = ext4_issue_discard(sb, group, start, count);
>>> Â Â Â ext4_lock_group(sb, group);
>>> Â Â Â mb_free_blocks(NULL, e4b, start, ex.fe_len);
>>> + Â Â return err;
>>> Â}
>>>
>>> Â/**
>>> @@ -4978,15 +4980,21 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
>>> Â Â Â Â Â Â Â next = mb_find_next_bit(bitmap, max, start);
>>>
>>> Â Â Â Â Â Â Â if ((next - start) >= minblocks) {
>>> - Â Â Â Â Â Â Â Â Â Â ext4_trim_extent(sb, start,
>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Ânext - start, group, &e4b);
>>> - Â Â Â Â Â Â Â Â Â Â count += next - start;
>>> + Â Â Â Â Â Â Â Â Â Â ret = ext4_trim_extent(sb, start,
>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â next - start, group, &e4b);
>>> + Â Â Â Â Â Â Â Â Â Â if (ret < 0) {
>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (ret != -EOPNOTSUPP)
>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ext4_warning(sb, "Discard command "
>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â"returned error %d\n", ret);
>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â break;
>>> + Â Â Â Â Â Â Â Â Â Â } else
>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â count += next - start;
>>> Â Â Â Â Â Â Â }
>>> Â Â Â Â Â Â Â free_count += next - start;
>>> Â Â Â Â Â Â Â start = next + 1;
>>>
>>> Â Â Â Â Â Â Â if (fatal_signal_pending(current)) {
>>> - Â Â Â Â Â Â Â Â Â Â count = -ERESTARTSYS;
>>> + Â Â Â Â Â Â Â Â Â Â ret = -ERESTARTSYS;
>>> Â Â Â Â Â Â Â Â Â Â Â break;
>>> Â Â Â Â Â Â Â }
>>>
>>> @@ -5009,7 +5017,7 @@ out:
>>> Â Â Â ext4_debug("trimmed %d blocks in the group %d\n",
>>> Â Â Â Â Â Â Â count, group);
>>>
>>> - Â Â return count;
>>> + Â Â return (ret < 0) ? ret : count;
>> ÂI think you need to initialize ret to 0 at the beginning of the function.
>> Otherwise you could use it uninitialized. Otherwise the patch looks good to
>> me and makes things consistent with ext3 so after fixing that bug you can add:
>> ÂReviewed-by: Jan Kara <jack@xxxxxxx>
> Hi. Jan.
> Thanks for your review.~
>>
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂHonza
>> --
>> Jan Kara <jack@xxxxxxx>
>> SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/