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

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20240215094635.p5anw5w36snmqwsh@quack3>
Date: Thu, 15 Feb 2024 10:46:35 +0100
From: Jan Kara <jack@...e.cz>
To: Andreas Dilger <adilger@...ger.ca>
Cc: Jan Kara <jack@...e.cz>, Ted Tso <tytso@....edu>,
	linux-ext4@...r.kernel.org
Subject: Re: [PATCH] ext4: Don't report EOPNOTSUPP errors from discard

On Wed 14-02-24 16:01:57, Andreas Dilger wrote:
> On Feb 13, 2024, at 3:16 AM, Jan Kara <jack@...e.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@...e.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@...e.com>
SUSE Labs, CR