[PATCH] xfs: Use xfs set and clear mp state helpers

[PATCH] xfs: Use xfs set and clear mp state helpers

[John Garry]

Use the set and clear mp state helpers instead of open-coding.

It is noted that in some instances calls to atomic operation set_bit() and
clear_bit() are being replaced with test_and_set_bit() and
test_and_clear_bit(), respectively, as there is no specific helpers for
set_bit() and clear_bit() only. However should be ok, as we are just
ignoring the returned value from those "test" variants.

Signed-off-by: John Garry <john.g.garry@oracle.com>

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index c211ea2b63c4..3643cc843f62 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -485,7 +485,7 @@ xfs_do_force_shutdown(
 	const char	*why;
 
 
-	if (test_and_set_bit(XFS_OPSTATE_SHUTDOWN, &mp->m_opstate)) {
+	if (xfs_set_shutdown(mp)) {
 		xlog_shutdown_wait(mp->m_log);
 		return;
 	}
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 817ea7e0a8ab..26b2f5887b88 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3495,7 +3495,7 @@ xlog_force_shutdown(
 	 * If this log shutdown also sets the mount shutdown state, issue a
 	 * shutdown warning message.
 	 */
-	if (!test_and_set_bit(XFS_OPSTATE_SHUTDOWN, &log->l_mp->m_opstate)) {
+	if (!xfs_set_shutdown(log->l_mp)) {
 		xfs_alert_tag(log->l_mp, XFS_PTAG_SHUTDOWN_LOGERROR,
 "Filesystem has been shut down due to log error (0x%x).",
 				shutdown_flags);
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 4423dd344239..1a74fe22672e 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1336,7 +1336,7 @@ xlog_find_tail(
 	 * headers if we have a filesystem using non-persistent counters.
 	 */
 	if (clean)
-		set_bit(XFS_OPSTATE_CLEAN, &log->l_mp->m_opstate);
+		xfs_set_clean(log->l_mp);
 
 	/*
 	 * Make sure that there are no blocks in front of the head
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 09eef1721ef4..460f93a9ce00 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -595,7 +595,7 @@ xfs_unmount_flush_inodes(
 	xfs_extent_busy_wait_all(mp);
 	flush_workqueue(xfs_discard_wq);
 
-	set_bit(XFS_OPSTATE_UNMOUNTING, &mp->m_opstate);
+	xfs_set_unmounting(mp);
 
 	xfs_ail_push_all_sync(mp->m_ail);
 	xfs_inodegc_stop(mp);
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 27e9f749c4c7..904e7bf846d7 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -311,9 +311,9 @@ xfs_set_inode_alloc(
 	 * the allocator to accommodate the request.
 	 */
 	if (xfs_has_small_inums(mp) && ino > XFS_MAXINUMBER_32)
-		set_bit(XFS_OPSTATE_INODE32, &mp->m_opstate);
+		xfs_set_inode32(mp);
 	else
-		clear_bit(XFS_OPSTATE_INODE32, &mp->m_opstate);
+		xfs_clear_inode32(mp);
 
 	for (index = 0; index < agcount; index++) {
 		struct xfs_perag	*pag;
@@ -1511,7 +1511,7 @@ xfs_fs_fill_super(
 	 * the newer fsopen/fsconfig API.
 	 */
 	if (fc->sb_flags & SB_RDONLY)
-		set_bit(XFS_OPSTATE_READONLY, &mp->m_opstate);
+		xfs_set_readonly(mp);
 	if (fc->sb_flags & SB_DIRSYNC)
 		mp->m_features |= XFS_FEAT_DIRSYNC;
 	if (fc->sb_flags & SB_SYNCHRONOUS)
@@ -1820,7 +1820,7 @@ xfs_remount_rw(
 		return -EINVAL;
 	}
 
-	clear_bit(XFS_OPSTATE_READONLY, &mp->m_opstate);
+	xfs_clear_readonly(mp);
 
 	/*
 	 * If this is the first remount to writeable state we might have some
@@ -1908,7 +1908,7 @@ xfs_remount_ro(
 	xfs_save_resvblks(mp);
 
 	xfs_log_clean(mp);
-	set_bit(XFS_OPSTATE_READONLY, &mp->m_opstate);
+	xfs_set_readonly(mp);
 
 	return 0;
 }
-- 
2.31.1

Re: [PATCH] xfs: Use xfs set and clear mp state helpers

[Darrick J. Wong]

On Wed, Jul 10, 2024 at 10:31:19AM +0000, John Garry wrote:
> Use the set and clear mp state helpers instead of open-coding.
> 
> It is noted that in some instances calls to atomic operation set_bit() and
> clear_bit() are being replaced with test_and_set_bit() and
> test_and_clear_bit(), respectively, as there is no specific helpers for
> set_bit() and clear_bit() only. However should be ok, as we are just
> ignoring the returned value from those "test" variants.
> 
> Signed-off-by: John Garry <john.g.garry@oracle.com>

Seems pretty straightfoward to me...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> 
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index c211ea2b63c4..3643cc843f62 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -485,7 +485,7 @@ xfs_do_force_shutdown(
>  	const char	*why;
>  
>  
> -	if (test_and_set_bit(XFS_OPSTATE_SHUTDOWN, &mp->m_opstate)) {
> +	if (xfs_set_shutdown(mp)) {
>  		xlog_shutdown_wait(mp->m_log);
>  		return;
>  	}
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 817ea7e0a8ab..26b2f5887b88 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3495,7 +3495,7 @@ xlog_force_shutdown(
>  	 * If this log shutdown also sets the mount shutdown state, issue a
>  	 * shutdown warning message.
>  	 */
> -	if (!test_and_set_bit(XFS_OPSTATE_SHUTDOWN, &log->l_mp->m_opstate)) {
> +	if (!xfs_set_shutdown(log->l_mp)) {
>  		xfs_alert_tag(log->l_mp, XFS_PTAG_SHUTDOWN_LOGERROR,
>  "Filesystem has been shut down due to log error (0x%x).",
>  				shutdown_flags);
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 4423dd344239..1a74fe22672e 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1336,7 +1336,7 @@ xlog_find_tail(
>  	 * headers if we have a filesystem using non-persistent counters.
>  	 */
>  	if (clean)
> -		set_bit(XFS_OPSTATE_CLEAN, &log->l_mp->m_opstate);
> +		xfs_set_clean(log->l_mp);
>  
>  	/*
>  	 * Make sure that there are no blocks in front of the head
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 09eef1721ef4..460f93a9ce00 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -595,7 +595,7 @@ xfs_unmount_flush_inodes(
>  	xfs_extent_busy_wait_all(mp);
>  	flush_workqueue(xfs_discard_wq);
>  
> -	set_bit(XFS_OPSTATE_UNMOUNTING, &mp->m_opstate);
> +	xfs_set_unmounting(mp);
>  
>  	xfs_ail_push_all_sync(mp->m_ail);
>  	xfs_inodegc_stop(mp);
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 27e9f749c4c7..904e7bf846d7 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -311,9 +311,9 @@ xfs_set_inode_alloc(
>  	 * the allocator to accommodate the request.
>  	 */
>  	if (xfs_has_small_inums(mp) && ino > XFS_MAXINUMBER_32)
> -		set_bit(XFS_OPSTATE_INODE32, &mp->m_opstate);
> +		xfs_set_inode32(mp);
>  	else
> -		clear_bit(XFS_OPSTATE_INODE32, &mp->m_opstate);
> +		xfs_clear_inode32(mp);
>  
>  	for (index = 0; index < agcount; index++) {
>  		struct xfs_perag	*pag;
> @@ -1511,7 +1511,7 @@ xfs_fs_fill_super(
>  	 * the newer fsopen/fsconfig API.
>  	 */
>  	if (fc->sb_flags & SB_RDONLY)
> -		set_bit(XFS_OPSTATE_READONLY, &mp->m_opstate);
> +		xfs_set_readonly(mp);
>  	if (fc->sb_flags & SB_DIRSYNC)
>  		mp->m_features |= XFS_FEAT_DIRSYNC;
>  	if (fc->sb_flags & SB_SYNCHRONOUS)
> @@ -1820,7 +1820,7 @@ xfs_remount_rw(
>  		return -EINVAL;
>  	}
>  
> -	clear_bit(XFS_OPSTATE_READONLY, &mp->m_opstate);
> +	xfs_clear_readonly(mp);
>  
>  	/*
>  	 * If this is the first remount to writeable state we might have some
> @@ -1908,7 +1908,7 @@ xfs_remount_ro(
>  	xfs_save_resvblks(mp);
>  
>  	xfs_log_clean(mp);
> -	set_bit(XFS_OPSTATE_READONLY, &mp->m_opstate);
> +	xfs_set_readonly(mp);
>  
>  	return 0;
>  }
> -- 
> 2.31.1
> 
> 

Re: [PATCH] xfs: Use xfs set and clear mp state helpers

[Dave Chinner]

On Wed, Jul 10, 2024 at 10:31:19AM +0000, John Garry wrote:
> Use the set and clear mp state helpers instead of open-coding.
> 
> It is noted that in some instances calls to atomic operation set_bit() and
> clear_bit() are being replaced with test_and_set_bit() and
> test_and_clear_bit(), respectively, as there is no specific helpers for
> set_bit() and clear_bit() only. However should be ok, as we are just
> ignoring the returned value from those "test" variants.
> 
> Signed-off-by: John Garry <john.g.garry@oracle.com>

Nice cleanup.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: Use xfs set and clear mp state helpers

[Chaitanya Kulkarni]

On 7/10/2024 3:31 AM, John Garry wrote:
> Use the set and clear mp state helpers instead of open-coding.
> 
> It is noted that in some instances calls to atomic operation set_bit() and
> clear_bit() are being replaced with test_and_set_bit() and
> test_and_clear_bit(), respectively, as there is no specific helpers for
> set_bit() and clear_bit() only. However should be ok, as we are just
> ignoring the returned value from those "test" variants.
> 
> Signed-off-by: John Garry<john.g.garry@oracle.com>

This patch looks good to me, however formatting of the patch seems
little odd to me, what I meant is section describing the number of flies
changes and lines per file seems to be missing, e.g. (from different
patch) :-

"
---
  fs/xfs/scrub/trace.h |   10 ++++------
  fs/xfs/xfs_trace.h   |   10 ++++------
  2 files changed, 8 insertions(+), 12 deletions(-)
"

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH] xfs: Use xfs set and clear mp state helpers

[John Garry]

On 12/07/2024 18:39, Chaitanya Kulkarni wrote:
>> Signed-off-by: John Garry<john.g.garry@oracle.com>
> This patch looks good to me, however formatting of the patch seems
> little odd to me, what I meant is section describing the number of flies
> changes and lines per file seems to be missing, e.g. (from different
> patch) :-

yeah, I often use -p (no diffstat) for generating single patches. The 
kernel documentation for submitting patches does not mandate a diffstat 
AFAICS, but maybe it's strongly preferred on the xfs mailing list.

> 
> "
> ---
>    fs/xfs/scrub/trace.h |   10 ++++------
>    fs/xfs/xfs_trace.h   |   10 ++++------
>    2 files changed, 8 insertions(+), 12 deletions(-)
> "
> 
> Reviewed-by: Chaitanya Kulkarni<kch@nvidia.com>

thanks.