diff mbox series

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

Message ID 20241231023423.656128-2-leo.lilong@huawei.com (mailing list archive)
State New
Headers show
Series xfs: fix two issues regarding mount failures | expand

Commit Message

Long Li Dec. 31, 2024, 2:34 a.m. UTC 
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(-)

Comments

Darrick J. Wong Jan. 6, 2025, 7:52 p.m. UTC | #1 
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
> 
>
Long Li Jan. 7, 2025, 1:11 p.m. UTC | #2 
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
> > 
> > 
>
Darrick J. Wong Jan. 8, 2025, 12:32 a.m. UTC | #3 
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
> > > 
> > > 
> > 
>
Long Li Jan. 8, 2025, 1:26 a.m. UTC | #4 
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
diff mbox series

Patch

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