Received-SPF: pass (google.com: domain of linux-ext4+bounces-1372-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Andreas Dilger <adilger@dilger.ca>
Message-Id: <AF20B4C9-57F1-40B6-B095-621B59D9B2E7@dilger.ca>
Content-Type: multipart/signed;
 boundary="Apple-Mail=_8E572A28-6950-4690-8384-ABC81C02BA86";
 protocol="application/pgp-signature"; micalg=pgp-sha256
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: [PATCH] ext4: Don't report EOPNOTSUPP errors from discard
Date: Thu, 22 Feb 2024 14:23:38 -0700
In-Reply-To: <20240215094635.p5anw5w36snmqwsh@quack3>
Cc: Ted Tso <tytso@mit.edu>,
 linux-ext4@vger.kernel.org
To: Jan Kara <jack@suse.cz>
References: <20240213101601.17463-1-jack@suse.cz>
 <4AC7AEC3-25FC-4147-9C62-2CE5A1686199@dilger.ca>
 <20240215094635.p5anw5w36snmqwsh@quack3>


--Apple-Mail=_8E572A28-6950-4690-8384-ABC81C02BA86
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

On Feb 15, 2024, at 2:46 AM, Jan Kara <jack@suse.cz> wrote:
>=20
> 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:
>>>=20
>>> 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.
>>>=20
>>=20
>>> Signed-off-by: Jan Kara <jack@suse.cz>
>>> ---
>>> fs/ext4/mballoc.c | 8 +++++++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>=20
>>> 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 =3D ext4_issue_discard(sb, block_group, bit,
>>> 						 count_clusters, NULL);
>>> -			if (err && err !=3D -EOPNOTSUPP)
>>> +			/*
>>> +			 * Ignore EOPNOTSUPP error. This is consistent =
with
>>> +			 * what happens when using journal.
>>> +			 */
>>> +			if (err =3D=3D -EOPNOTSUPP)
>>> +				err =3D 0;
>>> +			if (err)
>>=20
>> 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?
>>=20
>> The "err" value is overwritten 50 lines later on without being used:
>>=20
>>        err =3D ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);
>>=20
>> so the setting "err =3D 0" doesn't really affect the later code =
either.
>> What am I missing?
>=20
> 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-g=
it-send-email-adilger@dilger.ca/

Cheers, Andreas


--Apple-Mail=_8E572A28-6950-4690-8384-ABC81C02BA86
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename=signature.asc
Content-Type: application/pgp-signature;
	name=signature.asc
Content-Description: Message signed with OpenPGP

-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - http://gpgtools.org

iQIzBAEBCAAdFiEEDb73u6ZejP5ZMprvcqXauRfMH+AFAmXXu1oACgkQcqXauRfM
H+C2tA//XspdwACyIsMa0ByNXZjjGXTv7h2jiMrz7SMN1nF9xFQAHhfsi5GA5w3E
wa/4XxLB1FiLsgFTISIEt6+RRL/5C38CKrNvLyL8SR/GDOht6DZVLF6fpV7yfFF0
XLQtoeM7mEL05i/xvVriLIeRgtahcdswmnzL66UhC0WZ/0YxeVTgZVvSA21gAGWY
DRAgXcwaeMgoqCp9S/0IrZV6TyFdJhf79Q2wHLuPcGwAbMFaCuAKCbJ6sN75IEWK
Hcob+56o6vKjZf8ihlzPg4b86Pu8TpLfINYk/GOTAujYpXHCRZlcGdi2TapNTGZJ
PmRLv9gOmyWXaFSGepXK6hAY4MgYTc4/COqYp9oqKWeiCG9pe23MBt+nR1JKWpcJ
o1caj0AyRlkBNqEhctCH7p/UnXtYKMLi250Q0YSH8fpIAdG03enIGZYN647yxyhU
/ldehyZ1W4OrhlmdYIKd8nvfL39iyAqIZk+gCh7zrvHuFcnK94Fgdpb1ZaFeKWR/
PA6DrEzgxQziYJrOyq6pz9uRymyMDLPvq9oMDfo1XNG3ZuwdJDsdmRMmgh6wm/1k
AMGzqC0lh5ohJC8s7yKSH2wp0jj7J75L1lc+SDnmq+s+oVnneKiAv9a/h6tMTOmt
zYgOoNRYx04KSzwou0/aPc+IxBL3tGqklswZkDEI0KGsvmD8OxA=
=FZeN
-----END PGP SIGNATURE-----

--Apple-Mail=_8E572A28-6950-4690-8384-ABC81C02BA86--