misc fixes

misc fixes

[Christoph Hellwig]

Hi Carlos,

a few misc fixes for the zoned code infrastructure updated by it.

Diffstat:
 xfs_mru_cache.c  |    4 ----
 xfs_super.c      |    5 ++---
 xfs_zone_alloc.c |   42 +++++++++++++++++++++---------------------
 3 files changed, 23 insertions(+), 28 deletions(-)

[PATCH 1/4] xfs: check for shutdown before going to sleep in xfs_select_zone

[Christoph Hellwig]

Ensure the file system hasn't been shut down before waiting for a free
zone to become available, because that won't happen on a shut down
file system.  Without this processes can occasionally get stuck in
the allocator wait loop when racing with a file system shutdown.
This sporadically happens when running generic/388 or generic/475.

Fixes: 4e4d52075577 ("xfs: add the zoned space allocator")
Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_zone_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_zone_alloc.c b/fs/xfs/xfs_zone_alloc.c
index 80add26c0111..0de6f64b3169 100644
--- a/fs/xfs/xfs_zone_alloc.c
+++ b/fs/xfs/xfs_zone_alloc.c
@@ -727,7 +727,7 @@ xfs_select_zone(
 	for (;;) {
 		prepare_to_wait(&zi->zi_zone_wait, &wait, TASK_UNINTERRUPTIBLE);
 		oz = xfs_select_zone_nowait(mp, write_hint, pack_tight);
-		if (oz)
+		if (oz || xfs_is_shutdown(mp))
 			break;
 		schedule();
 	}
-- 
2.47.2

Re: [PATCH 1/4] xfs: check for shutdown before going to sleep in xfs_select_zone

[Shinichiro Kawasaki]

On Jun 05, 2025 / 08:16, Christoph Hellwig wrote:
> Ensure the file system hasn't been shut down before waiting for a free
> zone to become available, because that won't happen on a shut down
> file system.  Without this processes can occasionally get stuck in
> the allocator wait loop when racing with a file system shutdown.
> This sporadically happens when running generic/388 or generic/475.
> 
> Fixes: 4e4d52075577 ("xfs: add the zoned space allocator")
> Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Thanks for the fix. I ran fstests twice on each of various zoned block devices
and confirmed the hang did not happen.

Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

Re: [PATCH 1/4] xfs: check for shutdown before going to sleep in xfs_select_zone

[Hans Holmberg]

On 05/06/2025 08:16, Christoph Hellwig wrote:
> Ensure the file system hasn't been shut down before waiting for a free
> zone to become available, because that won't happen on a shut down
> file system.  Without this processes can occasionally get stuck in
> the allocator wait loop when racing with a file system shutdown.
> This sporadically happens when running generic/388 or generic/475.
> 
> Fixes: 4e4d52075577 ("xfs: add the zoned space allocator")
> Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_zone_alloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_zone_alloc.c b/fs/xfs/xfs_zone_alloc.c
> index 80add26c0111..0de6f64b3169 100644
> --- a/fs/xfs/xfs_zone_alloc.c
> +++ b/fs/xfs/xfs_zone_alloc.c
> @@ -727,7 +727,7 @@ xfs_select_zone(
>  	for (;;) {
>  		prepare_to_wait(&zi->zi_zone_wait, &wait, TASK_UNINTERRUPTIBLE);
>  		oz = xfs_select_zone_nowait(mp, write_hint, pack_tight);
> -		if (oz)
> +		if (oz || xfs_is_shutdown(mp))
>  			break;
>  		schedule();
>  	}

Looks good to me.

Reviewed-by: Hans Holmberg <hans.holmberg@wdc.com>

[PATCH 2/4] xfs: remove NULL pointer checks in xfs_mru_cache_insert

[Christoph Hellwig]

Remove the check for a NULL mru or mru->list in xfs_mru_cache_insert
as this API misused lead to a direct NULL pointer dereference on first
use and is not user triggerable.  As a smatch run by Dan points out
with the recent cleanup it would otherwise try to free the object we
just determined to be NULL for this impossible to reach case.

Fixes: 70b95cb86513 ("xfs: free the item in xfs_mru_cache_insert on failure")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_mru_cache.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/xfs/xfs_mru_cache.c b/fs/xfs/xfs_mru_cache.c
index 08443ceec329..c95401de8397 100644
--- a/fs/xfs/xfs_mru_cache.c
+++ b/fs/xfs/xfs_mru_cache.c
@@ -425,10 +425,6 @@ xfs_mru_cache_insert(
 {
 	int			error = -EINVAL;
 
-	ASSERT(mru && mru->lists);
-	if (!mru || !mru->lists)
-		goto out_free;
-
 	error = -ENOMEM;
 	if (radix_tree_preload(GFP_KERNEL))
 		goto out_free;
-- 
2.47.2

Re: [PATCH 2/4] xfs: remove NULL pointer checks in xfs_mru_cache_insert

[Hans Holmberg]

On 05/06/2025 08:17, Christoph Hellwig wrote:
> Remove the check for a NULL mru or mru->list in xfs_mru_cache_insert
> as this API misused lead to a direct NULL pointer dereference on first
> use and is not user triggerable.  As a smatch run by Dan points out
> with the recent cleanup it would otherwise try to free the object we
> just determined to be NULL for this impossible to reach case.
> 
> Fixes: 70b95cb86513 ("xfs: free the item in xfs_mru_cache_insert on failure")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_mru_cache.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_mru_cache.c b/fs/xfs/xfs_mru_cache.c
> index 08443ceec329..c95401de8397 100644
> --- a/fs/xfs/xfs_mru_cache.c
> +++ b/fs/xfs/xfs_mru_cache.c
> @@ -425,10 +425,6 @@ xfs_mru_cache_insert(
>  {
>  	int			error = -EINVAL;
>  
> -	ASSERT(mru && mru->lists);
> -	if (!mru || !mru->lists)
> -		goto out_free;
> -
>  	error = -ENOMEM;
>  	if (radix_tree_preload(GFP_KERNEL))
>  		goto out_free;

Looks good to me.

Reviewed-by: Hans Holmberg <hans.holmberg@wdc.com>

[PATCH 3/4] xfs: use xfs_readonly_buftarg in xfs_remount_rw

[Christoph Hellwig]

Use xfs_readonly_buftarg instead of open coding it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_super.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 0bc4b5489078..bb0a82635a77 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -2020,14 +2020,13 @@ xfs_remount_rw(
 	int error;
 
 	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp &&
-	    bdev_read_only(mp->m_logdev_targp->bt_bdev)) {
+	    xfs_readonly_buftarg(mp->m_logdev_targp)) {
 		xfs_warn(mp,
 			"ro->rw transition prohibited by read-only logdev");
 		return -EACCES;
 	}
 
-	if (mp->m_rtdev_targp &&
-	    bdev_read_only(mp->m_rtdev_targp->bt_bdev)) {
+	if (mp->m_rtdev_targp && xfs_readonly_buftarg(mp->m_rtdev_targp)) {
 		xfs_warn(mp,
 			"ro->rw transition prohibited by read-only rtdev");
 		return -EACCES;
-- 
2.47.2

Re: [PATCH 3/4] xfs: use xfs_readonly_buftarg in xfs_remount_rw

[John Garry]

On 05/06/2025 07:16, Christoph Hellwig wrote:
> Use xfs_readonly_buftarg instead of open coding it.
> 
> Signed-off-by: Christoph Hellwig<hch@lst.de>

FWIW:

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

Re: [PATCH 3/4] xfs: use xfs_readonly_buftarg in xfs_remount_rw

[Hans Holmberg]

On 05/06/2025 08:17, Christoph Hellwig wrote:
> Use xfs_readonly_buftarg instead of open coding it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_super.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 0bc4b5489078..bb0a82635a77 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -2020,14 +2020,13 @@ xfs_remount_rw(
>  	int error;
>  
>  	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp &&
> -	    bdev_read_only(mp->m_logdev_targp->bt_bdev)) {
> +	    xfs_readonly_buftarg(mp->m_logdev_targp)) {
>  		xfs_warn(mp,
>  			"ro->rw transition prohibited by read-only logdev");
>  		return -EACCES;
>  	}
>  
> -	if (mp->m_rtdev_targp &&
> -	    bdev_read_only(mp->m_rtdev_targp->bt_bdev)) {
> +	if (mp->m_rtdev_targp && xfs_readonly_buftarg(mp->m_rtdev_targp)) {
>  		xfs_warn(mp,
>  			"ro->rw transition prohibited by read-only rtdev");
>  		return -EACCES;

Looks good to me.

Reviewed-by: Hans Holmberg <hans.holmberg@wdc.com>

[PATCH 4/4] xfs: move xfs_submit_zoned_bio a bit

[Christoph Hellwig]

Commit f3e2e53823b9 ("xfs: add inode to zone caching for data placement")
add the new code right between xfs_submit_zoned_bio and
xfs_zone_alloc_and_submit which implement the main zoned write path.
Move xfs_submit_zoned_bio down to keep it together again.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_zone_alloc.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/fs/xfs/xfs_zone_alloc.c b/fs/xfs/xfs_zone_alloc.c
index 0de6f64b3169..01315ed75502 100644
--- a/fs/xfs/xfs_zone_alloc.c
+++ b/fs/xfs/xfs_zone_alloc.c
@@ -777,26 +777,6 @@ xfs_mark_rtg_boundary(
 		ioend->io_flags |= IOMAP_IOEND_BOUNDARY;
 }
 
-static void
-xfs_submit_zoned_bio(
-	struct iomap_ioend	*ioend,
-	struct xfs_open_zone	*oz,
-	bool			is_seq)
-{
-	ioend->io_bio.bi_iter.bi_sector = ioend->io_sector;
-	ioend->io_private = oz;
-	atomic_inc(&oz->oz_ref); /* for xfs_zoned_end_io */
-
-	if (is_seq) {
-		ioend->io_bio.bi_opf &= ~REQ_OP_WRITE;
-		ioend->io_bio.bi_opf |= REQ_OP_ZONE_APPEND;
-	} else {
-		xfs_mark_rtg_boundary(ioend);
-	}
-
-	submit_bio(&ioend->io_bio);
-}
-
 /*
  * Cache the last zone written to for an inode so that it is considered first
  * for subsequent writes.
@@ -891,6 +871,26 @@ xfs_zone_cache_create_association(
 	xfs_mru_cache_insert(mp->m_zone_cache, ip->i_ino, &item->mru);
 }
 
+static void
+xfs_submit_zoned_bio(
+	struct iomap_ioend	*ioend,
+	struct xfs_open_zone	*oz,
+	bool			is_seq)
+{
+	ioend->io_bio.bi_iter.bi_sector = ioend->io_sector;
+	ioend->io_private = oz;
+	atomic_inc(&oz->oz_ref); /* for xfs_zoned_end_io */
+
+	if (is_seq) {
+		ioend->io_bio.bi_opf &= ~REQ_OP_WRITE;
+		ioend->io_bio.bi_opf |= REQ_OP_ZONE_APPEND;
+	} else {
+		xfs_mark_rtg_boundary(ioend);
+	}
+
+	submit_bio(&ioend->io_bio);
+}
+
 void
 xfs_zone_alloc_and_submit(
 	struct iomap_ioend	*ioend,
-- 
2.47.2

Re: [PATCH 4/4] xfs: move xfs_submit_zoned_bio a bit

[Hans Holmberg]

On 05/06/2025 08:17, Christoph Hellwig wrote:
> Commit f3e2e53823b9 ("xfs: add inode to zone caching for data placement")
> add the new code right between xfs_submit_zoned_bio and
> xfs_zone_alloc_and_submit which implement the main zoned write path.
> Move xfs_submit_zoned_bio down to keep it together again.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_zone_alloc.c | 40 ++++++++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/xfs/xfs_zone_alloc.c b/fs/xfs/xfs_zone_alloc.c
> index 0de6f64b3169..01315ed75502 100644
> --- a/fs/xfs/xfs_zone_alloc.c
> +++ b/fs/xfs/xfs_zone_alloc.c
> @@ -777,26 +777,6 @@ xfs_mark_rtg_boundary(
>  		ioend->io_flags |= IOMAP_IOEND_BOUNDARY;
>  }
>  
> -static void
> -xfs_submit_zoned_bio(
> -	struct iomap_ioend	*ioend,
> -	struct xfs_open_zone	*oz,
> -	bool			is_seq)
> -{
> -	ioend->io_bio.bi_iter.bi_sector = ioend->io_sector;
> -	ioend->io_private = oz;
> -	atomic_inc(&oz->oz_ref); /* for xfs_zoned_end_io */
> -
> -	if (is_seq) {
> -		ioend->io_bio.bi_opf &= ~REQ_OP_WRITE;
> -		ioend->io_bio.bi_opf |= REQ_OP_ZONE_APPEND;
> -	} else {
> -		xfs_mark_rtg_boundary(ioend);
> -	}
> -
> -	submit_bio(&ioend->io_bio);
> -}
> -
>  /*
>   * Cache the last zone written to for an inode so that it is considered first
>   * for subsequent writes.
> @@ -891,6 +871,26 @@ xfs_zone_cache_create_association(
>  	xfs_mru_cache_insert(mp->m_zone_cache, ip->i_ino, &item->mru);
>  }
>  
> +static void
> +xfs_submit_zoned_bio(
> +	struct iomap_ioend	*ioend,
> +	struct xfs_open_zone	*oz,
> +	bool			is_seq)
> +{
> +	ioend->io_bio.bi_iter.bi_sector = ioend->io_sector;
> +	ioend->io_private = oz;
> +	atomic_inc(&oz->oz_ref); /* for xfs_zoned_end_io */
> +
> +	if (is_seq) {
> +		ioend->io_bio.bi_opf &= ~REQ_OP_WRITE;
> +		ioend->io_bio.bi_opf |= REQ_OP_ZONE_APPEND;
> +	} else {
> +		xfs_mark_rtg_boundary(ioend);
> +	}
> +
> +	submit_bio(&ioend->io_bio);
> +}
> +
>  void
>  xfs_zone_alloc_and_submit(
>  	struct iomap_ioend	*ioend,

Looks good to me.

Reviewed-by: Hans Holmberg <hans.holmberg@wdc.com>

Re: [PATCH 4/4] xfs: move xfs_submit_zoned_bio a bit

[Carlos Maiolino]

On Thu, Jun 05, 2025 at 08:16:30AM +0200, Christoph Hellwig wrote:
> Commit f3e2e53823b9 ("xfs: add inode to zone caching for data placement")
> add the new code right between xfs_submit_zoned_bio and
> xfs_zone_alloc_and_submit which implement the main zoned write path.
> Move xfs_submit_zoned_bio down to keep it together again.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good,
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> ---
>  fs/xfs/xfs_zone_alloc.c | 40 ++++++++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/xfs/xfs_zone_alloc.c b/fs/xfs/xfs_zone_alloc.c
> index 0de6f64b3169..01315ed75502 100644
> --- a/fs/xfs/xfs_zone_alloc.c
> +++ b/fs/xfs/xfs_zone_alloc.c
> @@ -777,26 +777,6 @@ xfs_mark_rtg_boundary(
>  		ioend->io_flags |= IOMAP_IOEND_BOUNDARY;
>  }
> 
> -static void
> -xfs_submit_zoned_bio(
> -	struct iomap_ioend	*ioend,
> -	struct xfs_open_zone	*oz,
> -	bool			is_seq)
> -{
> -	ioend->io_bio.bi_iter.bi_sector = ioend->io_sector;
> -	ioend->io_private = oz;
> -	atomic_inc(&oz->oz_ref); /* for xfs_zoned_end_io */
> -
> -	if (is_seq) {
> -		ioend->io_bio.bi_opf &= ~REQ_OP_WRITE;
> -		ioend->io_bio.bi_opf |= REQ_OP_ZONE_APPEND;
> -	} else {
> -		xfs_mark_rtg_boundary(ioend);
> -	}
> -
> -	submit_bio(&ioend->io_bio);
> -}
> -
>  /*
>   * Cache the last zone written to for an inode so that it is considered first
>   * for subsequent writes.
> @@ -891,6 +871,26 @@ xfs_zone_cache_create_association(
>  	xfs_mru_cache_insert(mp->m_zone_cache, ip->i_ino, &item->mru);
>  }
> 
> +static void
> +xfs_submit_zoned_bio(
> +	struct iomap_ioend	*ioend,
> +	struct xfs_open_zone	*oz,
> +	bool			is_seq)
> +{
> +	ioend->io_bio.bi_iter.bi_sector = ioend->io_sector;
> +	ioend->io_private = oz;
> +	atomic_inc(&oz->oz_ref); /* for xfs_zoned_end_io */
> +
> +	if (is_seq) {
> +		ioend->io_bio.bi_opf &= ~REQ_OP_WRITE;
> +		ioend->io_bio.bi_opf |= REQ_OP_ZONE_APPEND;
> +	} else {
> +		xfs_mark_rtg_boundary(ioend);
> +	}
> +
> +	submit_bio(&ioend->io_bio);
> +}
> +
>  void
>  xfs_zone_alloc_and_submit(
>  	struct iomap_ioend	*ioend,
> --
> 2.47.2
> 
> 

Re: misc fixes

[Carlos Maiolino]

On Thu, 05 Jun 2025 08:16:26 +0200, Christoph Hellwig wrote:
> a few misc fixes for the zoned code infrastructure updated by it.
> 
> Diffstat:
>  xfs_mru_cache.c  |    4 ----
>  xfs_super.c      |    5 ++---
>  xfs_zone_alloc.c |   42 +++++++++++++++++++++---------------------
>  3 files changed, 23 insertions(+), 28 deletions(-)
> 
> [...]

Applied to for-next, thanks!

[1/4] xfs: check for shutdown before going to sleep in xfs_select_zone
      commit: b0f77d301eb2b4e1fc816f33ade8519ae7f894f4
[2/4] xfs: remove NULL pointer checks in xfs_mru_cache_insert
      commit: a593c89ac5a417605b165cbc9768b3663ab4d8ad
[3/4] xfs: use xfs_readonly_buftarg in xfs_remount_rw
      commit: df3b7e2b56d271f93e2d1f395c13235a1a277639
[4/4] xfs: move xfs_submit_zoned_bio a bit
      commit: 0989dfa61f438150c4f1110604ba0787856fe8b0

Best regards,
-- 
Carlos Maiolino <cem@kernel.org>