linux-kernel - Re: [PATCH] ext4: add error handling when discarding is fail in FITRIM ioctl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 25 Jan 2012 07:42:51 +0900
From:	Namjae Jeon <linkinjeon@...il.com>
To:	Jan Kara <jack@...e.cz>
Cc:	tytso@....edu, adilger.kernel@...ger.ca,
	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org,
	Amit Sahrawat <amit.sahrawat83@...il.com>
Subject: Re: [PATCH] ext4: add error handling when discarding is fail in
 FITRIM ioctl

2012/1/24 Jan Kara <jack@...e.cz>:
> 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@...il.com>
>> Signed-off-by: Amit Sahrawat <amit.sahrawat83@...il.com>
>> ---
>>  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@...e.cz>
Hi. Jan.
Thanks for your review.~
>
>                                                                Honza
> --
> Jan Kara <jack@...e.cz>
> SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/