| 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 |
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 > >
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 > > > > >
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 > > > > > > > > >
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
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.
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.
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!
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
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; } }
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(-)