Message ID | 1343561514-8380-1-git-send-email-linkinjeon@gmail.com |
---|---|
State | Accepted, archived |
Headers | show |
On Sun, 29 Jul 2012, Namjae Jeon wrote: > Date: Sun, 29 Jul 2012 07:31:54 -0400 > From: Namjae Jeon <linkinjeon@gmail.com> > To: tytso@mit.edu, sandeen@redhat.com, lczerner@redhat.com, > linux-ext4@vger.kernel.org > Cc: linux-kernel@vger.kernel.org, Namjae Jeon <linkinjeon@gmail.com>, > Amit Sahrawat <amit.sahrawat83@gmail.com> > 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 <linkinjeon@gmail.com> > Signed-off-by: Amit Sahrawat <amit.sahrawat83@gmail.com> > Reviewed-by: Jan Kara <jack@suse.cz> > --- > 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; > } > > /** > -- 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
On Mon, Jul 30, 2012 at 5:01 PM, Lukáš Czerner <lczerner@redhat.com> wrote: > On Sun, 29 Jul 2012, Namjae Jeon wrote: > >> Date: Sun, 29 Jul 2012 07:31:54 -0400 >> From: Namjae Jeon <linkinjeon@gmail.com> >> To: tytso@mit.edu, sandeen@redhat.com, lczerner@redhat.com, >> linux-ext4@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org, Namjae Jeon <linkinjeon@gmail.com>, >> Amit Sahrawat <amit.sahrawat83@gmail.com> >> 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 > -- 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
>> 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 -- 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
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; } /**