Received-SPF: pass (google.com: domain of linux-ext4+bounces-1246-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 15 Feb 2024 10:46:35 +0100
From: Jan Kara <jack@suse.cz>
To: Andreas Dilger <adilger@dilger.ca>
Cc: Jan Kara <jack@suse.cz>, Ted Tso <tytso@mit.edu>,
	linux-ext4@vger.kernel.org
Subject: Re: [PATCH] ext4: Don't report EOPNOTSUPP errors from discard
Message-ID: <20240215094635.p5anw5w36snmqwsh@quack3>
References: <20240213101601.17463-1-jack@suse.cz>
 <4AC7AEC3-25FC-4147-9C62-2CE5A1686199@dilger.ca>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4AC7AEC3-25FC-4147-9C62-2CE5A1686199@dilger.ca>

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