Received-SPF: pass (google.com: domain of linux-ext4+bounces-1849-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 4 Apr 2024 11:18:58 +1100
From: Dave Chinner <david@fromorbit.com>
To: Ye Bin <yebin10@huawei.com>
Cc: 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: <Zg3x8qJXrseiiYgU@dread.disaster.area>
References: <20240401011614.3650958-1-yebin10@huawei.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240401011614.3650958-1-yebin10@huawei.com>

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};

So this is a local, internal definition, but ....

> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 971f3e826e15..58a961999d70 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1434,7 +1434,9 @@ void jbd2_update_log_tail(journal_t *journal, tid_t tid, unsigned long block);
>  extern void jbd2_journal_commit_transaction(journal_t *);
>  
>  /* Checkpoint list management */
> -void __jbd2_journal_clean_checkpoint_list(journal_t *journal, bool destroy);
> +enum shrink_type {SHRINK_DESTROY, SHRINK_BUSY_STOP, SHRINK_BUSY_SKIP};

... this exports it to the world. That's a problem, because the
"SHRINK_*" namespace is owned by the memory management subsystem for
controlling memory shrinkers. e.g. SHRINK_STOP and SHRINK_EMPTY are
already defined and in wide use across the kernel in the cache
shrinker infrastructure.

IOWS, these new types needs to be prefixed to indicate they are JBD2
objects. i.e

enum jbd2_shrink_type {JBD2_SHRINK_DESTROY, JBD2_.... };

So that people who are looking at memory shrinker stuff don't get
horribly confused by jbd2 using shrinker namespaces for things that
are completely unrelated to memory reclaim...

-Dave.
-- 
Dave Chinner
david@fromorbit.com