Received-SPF: pass (google.com: domain of linux-ext4+bounces-1822-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 2 Apr 2024 14:55:55 +0200
From: Jan Kara <jack@suse.cz>
To: "yebin (H)" <yebin10@huawei.com>
Cc: "Darrick J. Wong" <djwong@kernel.org>, tytso@mit.edu,
	adilger.kernel@dilger.ca, linux-ext4@vger.kernel.org,
	linux-kernel@vger.kernel.org, jack@suse.cz
Subject: Re: [PATCH] jbd2: use shrink_type type instead of bool type for
 __jbd2_journal_clean_checkpoint_list()
Message-ID: <20240402125555.kqxsfmzeaeqbsmdp@quack3>
References: <20240401011614.3650958-1-yebin10@huawei.com>
 <20240401024417.GA739535@frogsfrogsfrogs>
 <660A5535.7080005@huawei.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <660A5535.7080005@huawei.com>

On Mon 01-04-24 14:33:25, yebin (H) wrote:
> On 2024/4/1 10:44, Darrick J. Wong wrote:
> > On Mon, Apr 01, 2024 at 09:16:14AM +0800, Ye Bin wrote:
> > > "enum shrink_type" can clearly express the meaning of the parameter of
> > > __jbd2_journal_clean_checkpoint_list(), and there is no need to use the
> > > bool type.
> > > 
> > > Signed-off-by: Ye Bin <yebin10@huawei.com>
> > > ---
> > >   fs/jbd2/checkpoint.c | 9 +++------
> > >   fs/jbd2/commit.c     | 2 +-
> > >   include/linux/jbd2.h | 4 +++-
> > >   3 files changed, 7 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> > > index 1c97e64c4784..d6e8b80a4078 100644
> > > --- a/fs/jbd2/checkpoint.c
> > > +++ b/fs/jbd2/checkpoint.c
> > > @@ -337,8 +337,6 @@ int jbd2_cleanup_journal_tail(journal_t *journal)
> > >   /* Checkpoint list management */
> > > -enum shrink_type {SHRINK_DESTROY, SHRINK_BUSY_STOP, SHRINK_BUSY_SKIP};
> > > -
> > >   /*
> > >    * journal_shrink_one_cp_list
> > >    *
> > > @@ -476,17 +474,16 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,
> > >    *
> > >    * Called with j_list_lock held.
> > >    */
> > > -void __jbd2_journal_clean_checkpoint_list(journal_t *journal, bool destroy)
> > > +void __jbd2_journal_clean_checkpoint_list(journal_t *journal,
> > > +					  enum shrink_type type)

The comment above this function needs updating after this change.

> > >   {
> > >   	transaction_t *transaction, *last_transaction, *next_transaction;
> > > -	enum shrink_type type;
> > >   	bool released;
> > >   	transaction = journal->j_checkpoint_transactions;
> > >   	if (!transaction)
> > >   		return;
> > > -	type = destroy ? SHRINK_DESTROY : SHRINK_BUSY_STOP;
> > What is supposed to happen if the caller passes in SHRINK_BUSY_SKIP?
> > 
> > --D
> 
> If SHRING_BUSY_SKIP is passed, the buffers in use will be skipped during traversal
> and release.Currently, SHRINKING_BUSY_SKIP is used in the memory reclamation process.

I guess Darrick was wondering whether there's some usefulness in calling
__jbd2_journal_clean_checkpoint_list() with SHRINKING_BUSY_SKIP. I agree it
does no harm but as we have seen in the past, it just wastes CPU cycles
scanning the buffer list in some cases so that's why we created
SHRINKING_BUSY_STOP. I also agree the 'bool' argument isn't great and the
enum is actually explaining more so I'm in favor of switching to enum but
perhaps we can have WARN_ON_ONCE(type == SHRINKING_BUSY_SKIP) (perhaps with
a short explanation in the comment above the function) to see if we
accidentally didn't grow unexpected use.

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