[PATCH] ext4: Don't report EOPNOTSUPP errors from discard

[PATCH] ext4: Don't report EOPNOTSUPP errors from discard

[Jan Kara]

When ext4 is mounted without journal, with discard mount option, and on
a device not supporting trim, we print error for each and every freed
extent. This is not only useless but actively harmful. Instead ignore
the EOPNOTSUPP error. Trim is only advisory anyway and when the
filesystem has journal we silently ignore trim error as well.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/mballoc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index e4f7cf9d89c4..aed620cf4d40 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -6488,7 +6488,13 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
 		if (test_opt(sb, DISCARD)) {
 			err = ext4_issue_discard(sb, block_group, bit,
 						 count_clusters, NULL);
-			if (err && err != -EOPNOTSUPP)
+			/*
+			 * Ignore EOPNOTSUPP error. This is consistent with
+			 * what happens when using journal.
+			 */
+			if (err == -EOPNOTSUPP)
+				err = 0;
+			if (err)
 				ext4_msg(sb, KERN_WARNING, "discard request in"
 					 " group:%u block:%d count:%lu failed"
 					 " with %d", block_group, bit, count,
-- 
2.35.3

Re: [PATCH] ext4: Don't report EOPNOTSUPP errors from discard

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1631 bytes --]

On Feb 13, 2024, at 3:16 AM, Jan Kara <jack@suse.cz> wrote:
> 
> When ext4 is mounted without journal, with discard mount option, and on
> a device not supporting trim, we print error for each and every freed
> extent. This is not only useless but actively harmful. Instead ignore
> the EOPNOTSUPP error. Trim is only advisory anyway and when the
> filesystem has journal we silently ignore trim error as well.
> 

> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> fs/ext4/mballoc.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index e4f7cf9d89c4..aed620cf4d40 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -6488,7 +6488,13 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
> 		if (test_opt(sb, DISCARD)) {
> 			err = ext4_issue_discard(sb, block_group, bit,
> 						 count_clusters, NULL);
> -			if (err && err != -EOPNOTSUPP)
> +			/*
> +			 * Ignore EOPNOTSUPP error. This is consistent with
> +			 * what happens when using journal.
> +			 */
> +			if (err == -EOPNOTSUPP)
> +				err = 0;
> +			if (err)

I don't see how this patch is actually changing whether the error message
is printed?  Previously, if "err" was set and err was -EOPNOTSUPP the
message was skipped.  Now it is doing the same thing in a different way?

The "err" value is overwritten 50 lines later on without being used:

        err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);

so the setting "err = 0" doesn't really affect the later code either.
What am I missing?

Cheers, Andreas



[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH] ext4: Don't report EOPNOTSUPP errors from discard

[Jan Kara]

On Wed 14-02-24 16:01:57, Andreas Dilger wrote:
> On Feb 13, 2024, at 3:16 AM, Jan Kara <jack@suse.cz> wrote:
> > 
> > When ext4 is mounted without journal, with discard mount option, and on
> > a device not supporting trim, we print error for each and every freed
> > extent. This is not only useless but actively harmful. Instead ignore
> > the EOPNOTSUPP error. Trim is only advisory anyway and when the
> > filesystem has journal we silently ignore trim error as well.
> > 
> 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> > fs/ext4/mballoc.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> > index e4f7cf9d89c4..aed620cf4d40 100644
> > --- a/fs/ext4/mballoc.c
> > +++ b/fs/ext4/mballoc.c
> > @@ -6488,7 +6488,13 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
> > 		if (test_opt(sb, DISCARD)) {
> > 			err = ext4_issue_discard(sb, block_group, bit,
> > 						 count_clusters, NULL);
> > -			if (err && err != -EOPNOTSUPP)
> > +			/*
> > +			 * Ignore EOPNOTSUPP error. This is consistent with
> > +			 * what happens when using journal.
> > +			 */
> > +			if (err == -EOPNOTSUPP)
> > +				err = 0;
> > +			if (err)
> 
> I don't see how this patch is actually changing whether the error message
> is printed?  Previously, if "err" was set and err was -EOPNOTSUPP the
> message was skipped.  Now it is doing the same thing in a different way?
> 
> The "err" value is overwritten 50 lines later on without being used:
> 
>         err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);
> 
> so the setting "err = 0" doesn't really affect the later code either.
> What am I missing?

Yeah, the code flow is a bit contrived. The error message gets printed by
ext4_std_error() at the end of ext4_mb_clear_bb(). I don't think there's
any ext4_handle_dirty_metadata() call in the current version of
ext4_mb_clear_bb()...

								Honza

> 
> Cheers, Andreas
> 
> 


-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] ext4: Don't report EOPNOTSUPP errors from discard

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2955 bytes --]

On Feb 15, 2024, at 2:46 AM, Jan Kara <jack@suse.cz> wrote:
> 
> On Wed 14-02-24 16:01:57, Andreas Dilger wrote:
>> On Feb 13, 2024, at 3:16 AM, Jan Kara <jack@suse.cz> wrote:
>>> 
>>> When ext4 is mounted without journal, with discard mount option, and on
>>> a device not supporting trim, we print error for each and every freed
>>> extent. This is not only useless but actively harmful. Instead ignore
>>> the EOPNOTSUPP error. Trim is only advisory anyway and when the
>>> filesystem has journal we silently ignore trim error as well.
>>> 
>> 
>>> Signed-off-by: Jan Kara <jack@suse.cz>
>>> ---
>>> fs/ext4/mballoc.c | 8 +++++++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>>> index e4f7cf9d89c4..aed620cf4d40 100644
>>> --- a/fs/ext4/mballoc.c
>>> +++ b/fs/ext4/mballoc.c
>>> @@ -6488,7 +6488,13 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>>> 		if (test_opt(sb, DISCARD)) {
>>> 			err = ext4_issue_discard(sb, block_group, bit,
>>> 						 count_clusters, NULL);
>>> -			if (err && err != -EOPNOTSUPP)
>>> +			/*
>>> +			 * Ignore EOPNOTSUPP error. This is consistent with
>>> +			 * what happens when using journal.
>>> +			 */
>>> +			if (err == -EOPNOTSUPP)
>>> +				err = 0;
>>> +			if (err)
>> 
>> I don't see how this patch is actually changing whether the error message
>> is printed?  Previously, if "err" was set and err was -EOPNOTSUPP the
>> message was skipped.  Now it is doing the same thing in a different way?
>> 
>> The "err" value is overwritten 50 lines later on without being used:
>> 
>>        err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);
>> 
>> so the setting "err = 0" doesn't really affect the later code either.
>> What am I missing?
> 
> Yeah, the code flow is a bit contrived. The error message gets printed by
> ext4_std_error() at the end of ext4_mb_clear_bb(). I don't think there's
> any ext4_handle_dirty_metadata() call in the current version of
> ext4_mb_clear_bb()...

I had meant to reply on this thread sooner...

Does this mean that no error will be returned at all from FSTRIM?
That means userspace will just keep pounding this ioctl indefinitely
and never get a notification that it is the wrong thing to do.

I'd think it would instead be better to also skip the ext4_std_error()
in case of -EOPNOTSUPP but still return the error code to the caller
so that they can make a better decision next time.

A patch recently landed to lib/ext2fs/unix_io.c to do exactly that[1] -
skip calling ioctl(BLKDISCARD) a million times if formatting a filesystem
where the underlying storage doesn't support it, but I could imagine the
same is true for other applications that want the error to be returned.

[1] https://patchwork.ozlabs.org/project/linux-ext4/patch/1683695929-26972-1-git-send-email-adilger@dilger.ca/

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH] ext4: Don't report EOPNOTSUPP errors from discard

[Jan Kara]

On Thu 22-02-24 14:23:38, Andreas Dilger wrote:
> On Feb 15, 2024, at 2:46 AM, Jan Kara <jack@suse.cz> wrote:
> > 
> > On Wed 14-02-24 16:01:57, Andreas Dilger wrote:
> >> On Feb 13, 2024, at 3:16 AM, Jan Kara <jack@suse.cz> wrote:
> >>> 
> >>> When ext4 is mounted without journal, with discard mount option, and on
> >>> a device not supporting trim, we print error for each and every freed
> >>> extent. This is not only useless but actively harmful. Instead ignore
> >>> the EOPNOTSUPP error. Trim is only advisory anyway and when the
> >>> filesystem has journal we silently ignore trim error as well.
> >>> 
> >> 
> >>> Signed-off-by: Jan Kara <jack@suse.cz>
> >>> ---
> >>> fs/ext4/mballoc.c | 8 +++++++-
> >>> 1 file changed, 7 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> >>> index e4f7cf9d89c4..aed620cf4d40 100644
> >>> --- a/fs/ext4/mballoc.c
> >>> +++ b/fs/ext4/mballoc.c
> >>> @@ -6488,7 +6488,13 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
> >>> 		if (test_opt(sb, DISCARD)) {
> >>> 			err = ext4_issue_discard(sb, block_group, bit,
> >>> 						 count_clusters, NULL);
> >>> -			if (err && err != -EOPNOTSUPP)
> >>> +			/*
> >>> +			 * Ignore EOPNOTSUPP error. This is consistent with
> >>> +			 * what happens when using journal.
> >>> +			 */
> >>> +			if (err == -EOPNOTSUPP)
> >>> +				err = 0;
> >>> +			if (err)
> >> 
> >> I don't see how this patch is actually changing whether the error message
> >> is printed?  Previously, if "err" was set and err was -EOPNOTSUPP the
> >> message was skipped.  Now it is doing the same thing in a different way?
> >> 
> >> The "err" value is overwritten 50 lines later on without being used:
> >> 
> >>        err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);
> >> 
> >> so the setting "err = 0" doesn't really affect the later code either.
> >> What am I missing?
> > 
> > Yeah, the code flow is a bit contrived. The error message gets printed by
> > ext4_std_error() at the end of ext4_mb_clear_bb(). I don't think there's
> > any ext4_handle_dirty_metadata() call in the current version of
> > ext4_mb_clear_bb()...
> 
> I had meant to reply on this thread sooner...
> 
> Does this mean that no error will be returned at all from FSTRIM?

This path is not about FITRIM ioctl but about 'discard' mount option. So in
this case there's nobody to return error to.

> That means userspace will just keep pounding this ioctl indefinitely
> and never get a notification that it is the wrong thing to do.
> 
> I'd think it would instead be better to also skip the ext4_std_error()
> in case of -EOPNOTSUPP but still return the error code to the caller
> so that they can make a better decision next time.

I agree this might make sense.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] ext4: Don't report EOPNOTSUPP errors from discard

[Zhang Yi]

On 2024/2/13 18:16, Jan Kara wrote:
> When ext4 is mounted without journal, with discard mount option, and on
> a device not supporting trim, we print error for each and every freed
> extent. This is not only useless but actively harmful. Instead ignore
> the EOPNOTSUPP error. Trim is only advisory anyway and when the
> filesystem has journal we silently ignore trim error as well.
> 

Make sense, call ext4_std_error() since EOPNOTSUPP is really harmful.

Reviewed-by: Zhang Yi <yi.zhang@huawei.com>

> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/mballoc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index e4f7cf9d89c4..aed620cf4d40 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -6488,7 +6488,13 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>  		if (test_opt(sb, DISCARD)) {
>  			err = ext4_issue_discard(sb, block_group, bit,
>  						 count_clusters, NULL);
> -			if (err && err != -EOPNOTSUPP)
> +			/*
> +			 * Ignore EOPNOTSUPP error. This is consistent with
> +			 * what happens when using journal.
> +			 */
> +			if (err == -EOPNOTSUPP)
> +				err = 0;
> +			if (err)
>  				ext4_msg(sb, KERN_WARNING, "discard request in"
>  					 " group:%u block:%d count:%lu failed"
>  					 " with %d", block_group, bit, count,
> 

Re: [PATCH] ext4: Don't report EOPNOTSUPP errors from discard

[Theodore Ts'o]

On Tue, 13 Feb 2024 11:16:01 +0100, Jan Kara wrote:
> When ext4 is mounted without journal, with discard mount option, and on
> a device not supporting trim, we print error for each and every freed
> extent. This is not only useless but actively harmful. Instead ignore
> the EOPNOTSUPP error. Trim is only advisory anyway and when the
> filesystem has journal we silently ignore trim error as well.
> 
> 
> [...]

Applied, thanks!

[1/1] ext4: Don't report EOPNOTSUPP errors from discard
      commit: f3edd83e2e71dd393c0f485be103a4b35f9ef41a

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>