Although free extents is proper not trimmed(mmc driver return error code
while sending trim command), currently FITRIM ioctl return success.
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 <[email protected]>
Signed-off-by: Amit Sahrawat <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
---
fs/ext4/mballoc.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 8eae947..07569b6 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -4852,10 +4852,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);
@@ -4871,9 +4872,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;
}
/**
@@ -4902,7 +4904,7 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
void *bitmap;
ext4_grpblk_t next, count = 0, free_count = 0;
struct ext4_buddy e4b;
- int ret;
+ int ret = 0;
trace_ext4_trim_all_free(sb, group, start, max);
@@ -4929,15 +4931,22 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
next = mb_find_next_bit(bitmap, max + 1, 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;
}
@@ -4960,7 +4969,7 @@ out:
ext4_debug("trimmed %d blocks in the group %d\n",
count, group);
- return count;
+ return (ret < 0) ? ret : count;
}
/**
--
1.7.9.5
On Sun, 29 Jul 2012, Namjae Jeon wrote:
> Date: Sun, 29 Jul 2012 07:31:54 -0400
> From: Namjae Jeon <[email protected]>
> To: [email protected], [email protected], [email protected],
> [email protected]
> Cc: [email protected], Namjae Jeon <[email protected]>,
> Amit Sahrawat <[email protected]>
> Subject: [PATCH] ext4: add error handling when discard cmd is fail in FITRIM
>
> Although free extents is proper not trimmed(mmc driver return error code
> while sending trim command), currently FITRIM ioctl return success.
> 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
> #>
Well, by this change you're actually reverting commit
d9f34504e6952e909a6932c5b2d1857716606380
ext4: ignore errors when issuing discards
which effectively reverts a30eec2a8.
Now I think that the way it is now is actually better than your
proposal for the reasons mentioned in the commit
d9f34504e6952e909a6932c5b2d1857716606380. However I think that the
discard errors should be logged nevertheless but not at the file
system level, but rather on block layer level if it is not done
already.
Thanks!
-Lukas
>
> Signed-off-by: Namjae Jeon <[email protected]>
> Signed-off-by: Amit Sahrawat <[email protected]>
> Reviewed-by: Jan Kara <[email protected]>
> ---
> fs/ext4/mballoc.c | 25 +++++++++++++++++--------
> 1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 8eae947..07569b6 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -4852,10 +4852,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);
>
> @@ -4871,9 +4872,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;
> }
>
> /**
> @@ -4902,7 +4904,7 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
> void *bitmap;
> ext4_grpblk_t next, count = 0, free_count = 0;
> struct ext4_buddy e4b;
> - int ret;
> + int ret = 0;
>
> trace_ext4_trim_all_free(sb, group, start, max);
>
> @@ -4929,15 +4931,22 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
> next = mb_find_next_bit(bitmap, max + 1, 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;
> }
>
> @@ -4960,7 +4969,7 @@ out:
> ext4_debug("trimmed %d blocks in the group %d\n",
> count, group);
>
> - return count;
> + return (ret < 0) ? ret : count;
> }
>
> /**
>
On Mon, Jul 30, 2012 at 5:01 PM, Lukáš Czerner <[email protected]> wrote:
> On Sun, 29 Jul 2012, Namjae Jeon wrote:
>
>> Date: Sun, 29 Jul 2012 07:31:54 -0400
>> From: Namjae Jeon <[email protected]>
>> To: [email protected], [email protected], [email protected],
>> [email protected]
>> Cc: [email protected], Namjae Jeon <[email protected]>,
>> Amit Sahrawat <[email protected]>
>> Subject: [PATCH] ext4: add error handling when discard cmd is fail in FITRIM
>>
>> Although free extents is proper not trimmed(mmc driver return error code
>> while sending trim command), currently FITRIM ioctl return success.
>> 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
>> #>
>
> Well, by this change you're actually reverting commit
>
> d9f34504e6952e909a6932c5b2d1857716606380
> ext4: ignore errors when issuing discards
>
> which effectively reverts a30eec2a8.
>
> Now I think that the way it is now is actually better than your
> proposal for the reasons mentioned in the commit
> d9f34504e6952e909a6932c5b2d1857716606380. However I think that the
How about instead of propagating the error to user and breaking out of
the discard, just print a warning message like:
ext4_warning(sb, "error %d while trimming group block from %d to
%d\n",ret, start, next);
> discard errors should be logged nevertheless but not at the file
> system level, but rather on block layer level if it is not done
> already.
>
> Thanks!
> -Lukas
>
>> How about instead of propagating the error to user and breaking out of
>> the discard, just print a warning message like:
>> ext4_warning(sb, "error %d while trimming group block from %d to
>> %d\n",ret, start, next);
>
> That's what I said. I think that those errors should be logged, but
> I am not sure that doing it on fs level is such a good idea. Do you
> see any errors in the logs when the discard command fails ?
>
Ok, I understand your point. Discard errors should be logged at block
layer. Reasonable too.
This is already happening. In case of discard error:
#> ./fitrim_test
end_request: I/O error, dev mmcblk0, sector 27232
#>
#> echo $?
0
Even though error happend during discard, the return value is success.
I think this is little confusing.
Thanks,
Ashish