[PATCH 0/2] xfs: fix two issues regarding mount failures

[PATCH 0/2] xfs: fix two issues regarding mount failures

[Long Li]

This patch set fix two issue regarding mount failures, the patch 1
fixes the issue where the current kernel cannot mount a xfs disk
without realtime subvolume; the patch 2 resolves the problem of the
mount thread getting hang after a failure.

Long Li (2):
  xfs: correct the sb_rgcount when the disk not support rt volume
  xfs: fix mount hang during primary superblock recovery failure

 fs/xfs/libxfs/xfs_sb.c        | 2 +-
 fs/xfs/xfs_buf_item_recover.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

-- 
2.39.2

[PATCH 1/2] xfs: correct the sb_rgcount when the disk not support rt volume

[Long Li]

When mounting an xfs disk that incompat with metadir and has no realtime
subvolume, if CONFIG_XFS_RT is not enabled in the kernel, the mount will
fail. During superblock log recovery, since mp->m_sb.sb_rgcount is greater
than 0, updating the last rtag in-core is required, however, without
CONFIG_XFS_RT enabled, xfs_update_last_rtgroup_size() always returns
-EOPNOTSUPP, leading to mount failure.

Initializing sb_rgcount as 1 is incorrect in this scenario. If no
realtime subvolume exists, the value of sb_rgcount should be set
to zero. Fix it by initializing sb_rgcount based on the actual number
of realtime blocks.

Fixes: 87fe4c34a383 ("xfs: create incore realtime group structures")
Signed-off-by: Long Li <leo.lilong@huawei.com>
---
 fs/xfs/libxfs/xfs_sb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 3b5623611eba..1ea28f04b75a 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -830,7 +830,7 @@ __xfs_sb_from_disk(
 		to->sb_rsumino = NULLFSINO;
 	} else {
 		to->sb_metadirino = NULLFSINO;
-		to->sb_rgcount = 1;
+		to->sb_rgcount = to->sb_rblocks > 0 ? 1 : 0;
 		to->sb_rgextents = 0;
 	}
 }
-- 
2.39.2

Re: [PATCH 1/2] xfs: correct the sb_rgcount when the disk not support rt volume

[Darrick J. Wong]

On Tue, Dec 31, 2024 at 10:34:22AM +0800, Long Li wrote:
> When mounting an xfs disk that incompat with metadir and has no realtime
> subvolume, if CONFIG_XFS_RT is not enabled in the kernel, the mount will
> fail. During superblock log recovery, since mp->m_sb.sb_rgcount is greater
> than 0, updating the last rtag in-core is required, however, without
> CONFIG_XFS_RT enabled, xfs_update_last_rtgroup_size() always returns
> -EOPNOTSUPP, leading to mount failure.

Didn't we fix the xfs_update_last_rtgroup_size stub to return 0?

--D

> Initializing sb_rgcount as 1 is incorrect in this scenario. If no
> realtime subvolume exists, the value of sb_rgcount should be set
> to zero. Fix it by initializing sb_rgcount based on the actual number
> of realtime blocks.
> 
> Fixes: 87fe4c34a383 ("xfs: create incore realtime group structures")
> Signed-off-by: Long Li <leo.lilong@huawei.com>
> ---
>  fs/xfs/libxfs/xfs_sb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 3b5623611eba..1ea28f04b75a 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -830,7 +830,7 @@ __xfs_sb_from_disk(
>  		to->sb_rsumino = NULLFSINO;
>  	} else {
>  		to->sb_metadirino = NULLFSINO;
> -		to->sb_rgcount = 1;
> +		to->sb_rgcount = to->sb_rblocks > 0 ? 1 : 0;
>  		to->sb_rgextents = 0;
>  	}
>  }
> -- 
> 2.39.2
> 
> 

Re: [PATCH 1/2] xfs: correct the sb_rgcount when the disk not support rt volume

[Long Li]

On Mon, Jan 06, 2025 at 11:52:20AM -0800, Darrick J. Wong wrote:
> On Tue, Dec 31, 2024 at 10:34:22AM +0800, Long Li wrote:
> > When mounting an xfs disk that incompat with metadir and has no realtime
> > subvolume, if CONFIG_XFS_RT is not enabled in the kernel, the mount will
> > fail. During superblock log recovery, since mp->m_sb.sb_rgcount is greater
> > than 0, updating the last rtag in-core is required, however, without
> > CONFIG_XFS_RT enabled, xfs_update_last_rtgroup_size() always returns
> > -EOPNOTSUPP, leading to mount failure.
> 
> Didn't we fix the xfs_update_last_rtgroup_size stub to return 0?
> 
> --D

Indeed, when CONFIG_XFS_RT is not enabled, xfs_update_last_rtgroup_size() should
return 0, as returning an error is meaningless.

1) For kernels without CONFIG_XFS_RT, mounting an image with realtime subvolume will
fail at xfs_rtmount_init().

2) For kernels without CONFIG_XFS_RT, mounting an image without realtime subvolume
should succeed.

However, in the current scenario, should sb_rgcount be initialized to 0 ? it will 
consistent with metadir feature is enabled. The xfs-documentation [1] describes 
sb_rgcount as follows:

"Count of realtime groups in the filesystem, if the XFS_SB_FEAT_RO_INCOMPAT_METADIR
feature is enabled. If no realtime subvolume exists, this value will be zero."

[1] https://git.kernel.org/pub/scm/fs/xfs/xfs-documentation.git/tree/design/XFS_Filesystem_Structure/superblock.asciidoc

Thanks,
Long Li

> 
> > Initializing sb_rgcount as 1 is incorrect in this scenario. If no
> > realtime subvolume exists, the value of sb_rgcount should be set
> > to zero. Fix it by initializing sb_rgcount based on the actual number
> > of realtime blocks.
> > 
> > Fixes: 87fe4c34a383 ("xfs: create incore realtime group structures")
> > Signed-off-by: Long Li <leo.lilong@huawei.com>
> > ---
> >  fs/xfs/libxfs/xfs_sb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index 3b5623611eba..1ea28f04b75a 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -830,7 +830,7 @@ __xfs_sb_from_disk(
> >  		to->sb_rsumino = NULLFSINO;
> >  	} else {
> >  		to->sb_metadirino = NULLFSINO;
> > -		to->sb_rgcount = 1;
> > +		to->sb_rgcount = to->sb_rblocks > 0 ? 1 : 0;
> >  		to->sb_rgextents = 0;
> >  	}
> >  }
> > -- 
> > 2.39.2
> > 
> > 
> 

Re: [PATCH 1/2] xfs: correct the sb_rgcount when the disk not support rt volume

[Darrick J. Wong]

On Tue, Jan 07, 2025 at 09:11:23PM +0800, Long Li wrote:
> On Mon, Jan 06, 2025 at 11:52:20AM -0800, Darrick J. Wong wrote:
> > On Tue, Dec 31, 2024 at 10:34:22AM +0800, Long Li wrote:
> > > When mounting an xfs disk that incompat with metadir and has no realtime
> > > subvolume, if CONFIG_XFS_RT is not enabled in the kernel, the mount will
> > > fail. During superblock log recovery, since mp->m_sb.sb_rgcount is greater
> > > than 0, updating the last rtag in-core is required, however, without
> > > CONFIG_XFS_RT enabled, xfs_update_last_rtgroup_size() always returns
> > > -EOPNOTSUPP, leading to mount failure.
> > 
> > Didn't we fix the xfs_update_last_rtgroup_size stub to return 0?
> > 
> > --D
> 
> Indeed, when CONFIG_XFS_RT is not enabled, xfs_update_last_rtgroup_size() should
> return 0, as returning an error is meaningless.
> 
> 1) For kernels without CONFIG_XFS_RT, mounting an image with realtime subvolume will
> fail at xfs_rtmount_init().
> 
> 2) For kernels without CONFIG_XFS_RT, mounting an image without realtime subvolume
> should succeed.
> 
> However, in the current scenario, should sb_rgcount be initialized to 0 ? it will 
> consistent with metadir feature is enabled. The xfs-documentation [1] describes 
> sb_rgcount as follows:
> 
> "Count of realtime groups in the filesystem, if the XFS_SB_FEAT_RO_INCOMPAT_METADIR
> feature is enabled. If no realtime subvolume exists, this value will be zero."
> 
> [1] https://git.kernel.org/pub/scm/fs/xfs/xfs-documentation.git/tree/design/XFS_Filesystem_Structure/superblock.asciidoc

Ah, I see your point finally -- if there's no realtime section, then
there's no need to allocate any incore rtgroups, nor is there any point
to set sb_rgcount==1.

That said, I think the correct tags here are:
Cc: <stable@vger.kernel.org> # v6.13-rc1
Fixes: 96768e91511bfc ("xfs: define the format of rt groups")

because 96768e91511bfc is the commit that actually added "to->sb_rgcount
= 1;".

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D


> Thanks,
> Long Li
> 
> > 
> > > Initializing sb_rgcount as 1 is incorrect in this scenario. If no
> > > realtime subvolume exists, the value of sb_rgcount should be set
> > > to zero. Fix it by initializing sb_rgcount based on the actual number
> > > of realtime blocks.
> > > 
> > > Fixes: 87fe4c34a383 ("xfs: create incore realtime group structures")
> > > Signed-off-by: Long Li <leo.lilong@huawei.com>
> > > ---
> > >  fs/xfs/libxfs/xfs_sb.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > > index 3b5623611eba..1ea28f04b75a 100644
> > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > @@ -830,7 +830,7 @@ __xfs_sb_from_disk(
> > >  		to->sb_rsumino = NULLFSINO;
> > >  	} else {
> > >  		to->sb_metadirino = NULLFSINO;
> > > -		to->sb_rgcount = 1;
> > > +		to->sb_rgcount = to->sb_rblocks > 0 ? 1 : 0;
> > >  		to->sb_rgextents = 0;
> > >  	}
> > >  }
> > > -- 
> > > 2.39.2
> > > 
> > > 
> > 
> 

Re: [PATCH 1/2] xfs: correct the sb_rgcount when the disk not support rt volume

[Long Li]

On Tue, Jan 07, 2025 at 04:32:06PM -0800, Darrick J. Wong wrote:
> On Tue, Jan 07, 2025 at 09:11:23PM +0800, Long Li wrote:
> > On Mon, Jan 06, 2025 at 11:52:20AM -0800, Darrick J. Wong wrote:
> > > On Tue, Dec 31, 2024 at 10:34:22AM +0800, Long Li wrote:
> > > > When mounting an xfs disk that incompat with metadir and has no realtime
> > > > subvolume, if CONFIG_XFS_RT is not enabled in the kernel, the mount will
> > > > fail. During superblock log recovery, since mp->m_sb.sb_rgcount is greater
> > > > than 0, updating the last rtag in-core is required, however, without
> > > > CONFIG_XFS_RT enabled, xfs_update_last_rtgroup_size() always returns
> > > > -EOPNOTSUPP, leading to mount failure.
> > > 
> > > Didn't we fix the xfs_update_last_rtgroup_size stub to return 0?
> > > 
> > > --D
> > 
> > Indeed, when CONFIG_XFS_RT is not enabled, xfs_update_last_rtgroup_size() should
> > return 0, as returning an error is meaningless.
> > 
> > 1) For kernels without CONFIG_XFS_RT, mounting an image with realtime subvolume will
> > fail at xfs_rtmount_init().
> > 
> > 2) For kernels without CONFIG_XFS_RT, mounting an image without realtime subvolume
> > should succeed.
> > 
> > However, in the current scenario, should sb_rgcount be initialized to 0 ? it will 
> > consistent with metadir feature is enabled. The xfs-documentation [1] describes 
> > sb_rgcount as follows:
> > 
> > "Count of realtime groups in the filesystem, if the XFS_SB_FEAT_RO_INCOMPAT_METADIR
> > feature is enabled. If no realtime subvolume exists, this value will be zero."
> > 
> > [1] https://git.kernel.org/pub/scm/fs/xfs/xfs-documentation.git/tree/design/XFS_Filesystem_Structure/superblock.asciidoc
> 
> Ah, I see your point finally -- if there's no realtime section, then
> there's no need to allocate any incore rtgroups, nor is there any point
> to set sb_rgcount==1.
> 
> That said, I think the correct tags here are:
> Cc: <stable@vger.kernel.org> # v6.13-rc1
> Fixes: 96768e91511bfc ("xfs: define the format of rt groups")
> 
> because 96768e91511bfc is the commit that actually added "to->sb_rgcount
> = 1;".
> 

Ok, thanks for point out that, In fact, xfs_grow_last_rtg() needs to be modified together,
otherwise the growfs may be problematic. Therefore, I will release a new version.

Long Li

Re: [PATCH 1/2] xfs: correct the sb_rgcount when the disk not support rt volume

[Christoph Hellwig]

sb_rgcount for file system without a RT subvolume and without the
metadir/rtgroup feature should be 1.  That is because we have an implicit
default rtgroup that points to the global bitmap and summary inodes,
which exist even with zero rtblocks.  Now for a kernel without
CONFIG_XFS_RT that probably does not matter, but I'd prefer to keep the
value consistent for CONFIG_XFS_RT vs !CONFIG_XFS_RT.

Re: [PATCH 1/2] xfs: correct the sb_rgcount when the disk not support rt volume

[Long Li]

On Tue, Jan 07, 2025 at 10:58:02PM -0800, Christoph Hellwig wrote:
> sb_rgcount for file system without a RT subvolume and without the
> metadir/rtgroup feature should be 1.  That is because we have an implicit
> default rtgroup that points to the global bitmap and summary inodes,
> which exist even with zero rtblocks.  Now for a kernel without
> CONFIG_XFS_RT that probably does not matter, but I'd prefer to keep the
> value consistent for CONFIG_XFS_RT vs !CONFIG_XFS_RT.
> 
> 

Your explanation seems reasonable to me, thanks for your replay.

Long Li

Re: [PATCH 1/2] xfs: correct the sb_rgcount when the disk not support rt volume

[Christoph Hellwig]

On Mon, Jan 06, 2025 at 11:52:20AM -0800, Darrick J. Wong wrote:
> On Tue, Dec 31, 2024 at 10:34:22AM +0800, Long Li wrote:
> > When mounting an xfs disk that incompat with metadir and has no realtime
> > subvolume, if CONFIG_XFS_RT is not enabled in the kernel, the mount will
> > fail. During superblock log recovery, since mp->m_sb.sb_rgcount is greater
> > than 0, updating the last rtag in-core is required, however, without
> > CONFIG_XFS_RT enabled, xfs_update_last_rtgroup_size() always returns
> > -EOPNOTSUPP, leading to mount failure.
> 
> Didn't we fix the xfs_update_last_rtgroup_size stub to return 0?

Hmm, looks like the patch did not get merged.  I'll send a ping.

Re: [PATCH 1/2] xfs: correct the sb_rgcount when the disk not support rt volume

[Long Li]

On Tue, Jan 07, 2025 at 10:55:46PM -0800, Christoph Hellwig wrote:
> On Mon, Jan 06, 2025 at 11:52:20AM -0800, Darrick J. Wong wrote:
> > On Tue, Dec 31, 2024 at 10:34:22AM +0800, Long Li wrote:
> > > When mounting an xfs disk that incompat with metadir and has no realtime
> > > subvolume, if CONFIG_XFS_RT is not enabled in the kernel, the mount will
> > > fail. During superblock log recovery, since mp->m_sb.sb_rgcount is greater
> > > than 0, updating the last rtag in-core is required, however, without
> > > CONFIG_XFS_RT enabled, xfs_update_last_rtgroup_size() always returns
> > > -EOPNOTSUPP, leading to mount failure.
> > 
> > Didn't we fix the xfs_update_last_rtgroup_size stub to return 0?
> 
> Hmm, looks like the patch did not get merged.  I'll send a ping.
> 

Oh, I didn't notice you fixed this before!

[PATCH 2/2] xfs: fix mount hang during primary superblock recovery failure

[Long Li]

When mounting an image containing a log with sb modifications that require
log replay, the mount process hang all the time and stack as follows:

  [root@localhost ~]# cat /proc/557/stack
  [<0>] xfs_buftarg_wait+0x31/0x70
  [<0>] xfs_buftarg_drain+0x54/0x350
  [<0>] xfs_mountfs+0x66e/0xe80
  [<0>] xfs_fs_fill_super+0x7f1/0xec0
  [<0>] get_tree_bdev_flags+0x186/0x280
  [<0>] get_tree_bdev+0x18/0x30
  [<0>] xfs_fs_get_tree+0x1d/0x30
  [<0>] vfs_get_tree+0x2d/0x110
  [<0>] path_mount+0xb59/0xfc0
  [<0>] do_mount+0x92/0xc0
  [<0>] __x64_sys_mount+0xc2/0x160
  [<0>] x64_sys_call+0x2de4/0x45c0
  [<0>] do_syscall_64+0xa7/0x240
  [<0>] entry_SYSCALL_64_after_hwframe+0x76/0x7e

During log recovery, while updating the in-memory superblock from the
primary SB buffer, if an error is encountered, such as superblock
corruption occurs or some other reasons, we will proceed to out_release
and release the xfs_buf. However, this is insufficient because the
xfs_buf's log item has already been initialized and the xfs_buf is held
by the buffer log item as follows, the xfs_buf will not be released,
causing the mount thread to hang.

  xlog_recover_do_primary_sb_buffer
    xlog_recover_do_reg_buffer
      xlog_recover_validate_buf_type
        xfs_buf_item_init(bp, mp)

The solution is straightforward: we simply need to allow it to be
handled by the normal buffer write process. The filesystem will be
shutdown before the submission of buffer_list in xlog_do_recovery_pass(),
ensuring the correct release of the xfs_buf.

Fixes: 6a18765b54e2 ("xfs: update the file system geometry after recoverying superblock buffers")
Signed-off-by: Long Li <leo.lilong@huawei.com>
---
 fs/xfs/xfs_buf_item_recover.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
index 3d0c6402cb36..ec2a42ef66ff 100644
--- a/fs/xfs/xfs_buf_item_recover.c
+++ b/fs/xfs/xfs_buf_item_recover.c
@@ -1079,7 +1079,7 @@ xlog_recover_buf_commit_pass2(
 		error = xlog_recover_do_primary_sb_buffer(mp, item, bp, buf_f,
 				current_lsn);
 		if (error)
-			goto out_release;
+			goto out_writebuf;
 
 		/* Update the rt superblock if we have one. */
 		if (xfs_has_rtsb(mp) && mp->m_rtsb_bp) {
@@ -1096,6 +1096,7 @@ xlog_recover_buf_commit_pass2(
 		xlog_recover_do_reg_buffer(mp, item, bp, buf_f, current_lsn);
 	}
 
+out_writebuf:
 	/*
 	 * Perform delayed write on the buffer.  Asynchronous writes will be
 	 * slower when taking into account all the buffers to be flushed.
-- 
2.39.2

Re: [PATCH 2/2] xfs: fix mount hang during primary superblock recovery failure

[Darrick J. Wong]

On Tue, Dec 31, 2024 at 10:34:23AM +0800, Long Li wrote:
> When mounting an image containing a log with sb modifications that require
> log replay, the mount process hang all the time and stack as follows:
> 
>   [root@localhost ~]# cat /proc/557/stack
>   [<0>] xfs_buftarg_wait+0x31/0x70
>   [<0>] xfs_buftarg_drain+0x54/0x350
>   [<0>] xfs_mountfs+0x66e/0xe80
>   [<0>] xfs_fs_fill_super+0x7f1/0xec0
>   [<0>] get_tree_bdev_flags+0x186/0x280
>   [<0>] get_tree_bdev+0x18/0x30
>   [<0>] xfs_fs_get_tree+0x1d/0x30
>   [<0>] vfs_get_tree+0x2d/0x110
>   [<0>] path_mount+0xb59/0xfc0
>   [<0>] do_mount+0x92/0xc0
>   [<0>] __x64_sys_mount+0xc2/0x160
>   [<0>] x64_sys_call+0x2de4/0x45c0
>   [<0>] do_syscall_64+0xa7/0x240
>   [<0>] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> 
> During log recovery, while updating the in-memory superblock from the
> primary SB buffer, if an error is encountered, such as superblock
> corruption occurs or some other reasons, we will proceed to out_release
> and release the xfs_buf. However, this is insufficient because the
> xfs_buf's log item has already been initialized and the xfs_buf is held
> by the buffer log item as follows, the xfs_buf will not be released,
> causing the mount thread to hang.
> 
>   xlog_recover_do_primary_sb_buffer
>     xlog_recover_do_reg_buffer
>       xlog_recover_validate_buf_type
>         xfs_buf_item_init(bp, mp)
> 
> The solution is straightforward: we simply need to allow it to be
> handled by the normal buffer write process. The filesystem will be
> shutdown before the submission of buffer_list in xlog_do_recovery_pass(),

What shuts it down?  If xlog_recover_do_primary_sb_buffer trips over
something like "mp->m_sb.sb_rgcount < orig_rgcount" then we haven't shut
anything down yet.  Am I missing something? <confused>

--D

> ensuring the correct release of the xfs_buf.
> 
> Fixes: 6a18765b54e2 ("xfs: update the file system geometry after recoverying superblock buffers")
> Signed-off-by: Long Li <leo.lilong@huawei.com>
> ---
>  fs/xfs/xfs_buf_item_recover.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
> index 3d0c6402cb36..ec2a42ef66ff 100644
> --- a/fs/xfs/xfs_buf_item_recover.c
> +++ b/fs/xfs/xfs_buf_item_recover.c
> @@ -1079,7 +1079,7 @@ xlog_recover_buf_commit_pass2(
>  		error = xlog_recover_do_primary_sb_buffer(mp, item, bp, buf_f,
>  				current_lsn);
>  		if (error)
> -			goto out_release;
> +			goto out_writebuf;
>  
>  		/* Update the rt superblock if we have one. */
>  		if (xfs_has_rtsb(mp) && mp->m_rtsb_bp) {
> @@ -1096,6 +1096,7 @@ xlog_recover_buf_commit_pass2(
>  		xlog_recover_do_reg_buffer(mp, item, bp, buf_f, current_lsn);
>  	}
>  
> +out_writebuf:
>  	/*
>  	 * Perform delayed write on the buffer.  Asynchronous writes will be
>  	 * slower when taking into account all the buffers to be flushed.
> -- 
> 2.39.2
> 
> 

Re: [PATCH 2/2] xfs: fix mount hang during primary superblock recovery failure

[Long Li]

On Mon, Jan 06, 2025 at 11:55:41AM -0800, Darrick J. Wong wrote:
> On Tue, Dec 31, 2024 at 10:34:23AM +0800, Long Li wrote:
> > When mounting an image containing a log with sb modifications that require
> > log replay, the mount process hang all the time and stack as follows:
> > 
> >   [root@localhost ~]# cat /proc/557/stack
> >   [<0>] xfs_buftarg_wait+0x31/0x70
> >   [<0>] xfs_buftarg_drain+0x54/0x350
> >   [<0>] xfs_mountfs+0x66e/0xe80
> >   [<0>] xfs_fs_fill_super+0x7f1/0xec0
> >   [<0>] get_tree_bdev_flags+0x186/0x280
> >   [<0>] get_tree_bdev+0x18/0x30
> >   [<0>] xfs_fs_get_tree+0x1d/0x30
> >   [<0>] vfs_get_tree+0x2d/0x110
> >   [<0>] path_mount+0xb59/0xfc0
> >   [<0>] do_mount+0x92/0xc0
> >   [<0>] __x64_sys_mount+0xc2/0x160
> >   [<0>] x64_sys_call+0x2de4/0x45c0
> >   [<0>] do_syscall_64+0xa7/0x240
> >   [<0>] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > 
> > During log recovery, while updating the in-memory superblock from the
> > primary SB buffer, if an error is encountered, such as superblock
> > corruption occurs or some other reasons, we will proceed to out_release
> > and release the xfs_buf. However, this is insufficient because the
> > xfs_buf's log item has already been initialized and the xfs_buf is held
> > by the buffer log item as follows, the xfs_buf will not be released,
> > causing the mount thread to hang.
> > 
> >   xlog_recover_do_primary_sb_buffer
> >     xlog_recover_do_reg_buffer
> >       xlog_recover_validate_buf_type
> >         xfs_buf_item_init(bp, mp)
> > 
> > The solution is straightforward: we simply need to allow it to be
> > handled by the normal buffer write process. The filesystem will be
> > shutdown before the submission of buffer_list in xlog_do_recovery_pass(),
> 
> What shuts it down?  If xlog_recover_do_primary_sb_buffer trips over
> something like "mp->m_sb.sb_rgcount < orig_rgcount" then we haven't shut
> anything down yet.  Am I missing something? <confused>
> 
> --D
> 

Hi Darrick,

Sorry for being unclear. I was referring to the shutdown in xlog_do_recovery_pass().
Here's the specific flow after the fix: 

  xlog_do_recovery_pass
   error = xlog_recover_process
     xlog_recover_process_data
       xlog_recover_process_ophdr
         xlog_recovery_process_trans
           ...
             xlog_recover_buf_commit_pass2
               error = xlog_recover_do_primary_sb_buffer
                 //Encounter error and return
               if (error)
                 goto out_writebuf
               ...
out_writebuf:
               xfs_buf_delwri_queue(bp, buffer_list)  //add bp to buffer_list
               return  error
           ...
   if (!list_empty(&buffer_list))
     if (error)
       xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);  //log shutdown first
     xfs_buf_delwri_submit(&buffer_list);
       __xfs_buf_submit
         if (bp->b_mount->m_log && xlog_is_shutdown(bp->b_mount->m_log))
           xfs_buf_ioend_fail(bp)  //release bp correctly

It might be clearer to put this process into a commit message.

Thanks,
Long Li

Re: [PATCH 2/2] xfs: fix mount hang during primary superblock recovery failure

[Darrick J. Wong]

On Tue, Jan 07, 2025 at 09:39:10PM +0800, Long Li wrote:
> On Mon, Jan 06, 2025 at 11:55:41AM -0800, Darrick J. Wong wrote:
> > On Tue, Dec 31, 2024 at 10:34:23AM +0800, Long Li wrote:
> > > When mounting an image containing a log with sb modifications that require
> > > log replay, the mount process hang all the time and stack as follows:
> > > 
> > >   [root@localhost ~]# cat /proc/557/stack
> > >   [<0>] xfs_buftarg_wait+0x31/0x70
> > >   [<0>] xfs_buftarg_drain+0x54/0x350
> > >   [<0>] xfs_mountfs+0x66e/0xe80
> > >   [<0>] xfs_fs_fill_super+0x7f1/0xec0
> > >   [<0>] get_tree_bdev_flags+0x186/0x280
> > >   [<0>] get_tree_bdev+0x18/0x30
> > >   [<0>] xfs_fs_get_tree+0x1d/0x30
> > >   [<0>] vfs_get_tree+0x2d/0x110
> > >   [<0>] path_mount+0xb59/0xfc0
> > >   [<0>] do_mount+0x92/0xc0
> > >   [<0>] __x64_sys_mount+0xc2/0x160
> > >   [<0>] x64_sys_call+0x2de4/0x45c0
> > >   [<0>] do_syscall_64+0xa7/0x240
> > >   [<0>] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > > 
> > > During log recovery, while updating the in-memory superblock from the
> > > primary SB buffer, if an error is encountered, such as superblock
> > > corruption occurs or some other reasons, we will proceed to out_release
> > > and release the xfs_buf. However, this is insufficient because the
> > > xfs_buf's log item has already been initialized and the xfs_buf is held
> > > by the buffer log item as follows, the xfs_buf will not be released,
> > > causing the mount thread to hang.
> > > 
> > >   xlog_recover_do_primary_sb_buffer
> > >     xlog_recover_do_reg_buffer
> > >       xlog_recover_validate_buf_type
> > >         xfs_buf_item_init(bp, mp)
> > > 
> > > The solution is straightforward: we simply need to allow it to be
> > > handled by the normal buffer write process. The filesystem will be
> > > shutdown before the submission of buffer_list in xlog_do_recovery_pass(),
> > 
> > What shuts it down?  If xlog_recover_do_primary_sb_buffer trips over
> > something like "mp->m_sb.sb_rgcount < orig_rgcount" then we haven't shut
> > anything down yet.  Am I missing something? <confused>
> > 
> > --D
> > 
> 
> Hi Darrick,
> 
> Sorry for being unclear. I was referring to the shutdown in xlog_do_recovery_pass().
> Here's the specific flow after the fix: 
> 
>   xlog_do_recovery_pass
>    error = xlog_recover_process
>      xlog_recover_process_data
>        xlog_recover_process_ophdr
>          xlog_recovery_process_trans
>            ...
>              xlog_recover_buf_commit_pass2
>                error = xlog_recover_do_primary_sb_buffer
>                  //Encounter error and return
>                if (error)
>                  goto out_writebuf
>                ...
> out_writebuf:
>                xfs_buf_delwri_queue(bp, buffer_list)  //add bp to buffer_list
>                return  error
>            ...
>    if (!list_empty(&buffer_list))
>      if (error)
>        xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);  //log shutdown first
>      xfs_buf_delwri_submit(&buffer_list);
>        __xfs_buf_submit
>          if (bp->b_mount->m_log && xlog_is_shutdown(bp->b_mount->m_log))
>            xfs_buf_ioend_fail(bp)  //release bp correctly
> 
> It might be clearer to put this process into a commit message.

Yes please, put that in a code comment.  That was too subtle for me to
figure out. :/

--D

> Thanks,
> Long Li
>