diff mbox series

[09/14] xfs: fix maxlevels comparisons in the btree staging code

Message ID 163192859919.416199.9790046292707106095.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: support dynamic btree cursor height | expand

Commit Message

Darrick J. Wong Sept. 18, 2021, 1:29 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

The btree geometry computation function has an off-by-one error in that
it does not allow maximally tall btrees (nlevels == XFS_BTREE_MAXLEVELS).
This can result in repairs failing unnecessarily on very fragmented
filesystems.  Subsequent patches to remove MAXLEVELS usage in favor of
the per-btree type computations will make this a much more likely
occurrence.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_btree_staging.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Chandan Babu R Sept. 20, 2021, 9:55 a.m. UTC | #1 
On 18 Sep 2021 at 06:59, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> The btree geometry computation function has an off-by-one error in that
> it does not allow maximally tall btrees (nlevels == XFS_BTREE_MAXLEVELS).
> This can result in repairs failing unnecessarily on very fragmented
> filesystems.  Subsequent patches to remove MAXLEVELS usage in favor of
> the per-btree type computations will make this a much more likely
> occurrence.

Looks good.

Reviewed-by: Chandan Babu R <chandan.babu@oracle.com>

>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_btree_staging.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
>
> diff --git a/fs/xfs/libxfs/xfs_btree_staging.c b/fs/xfs/libxfs/xfs_btree_staging.c
> index 26143297bb7b..cc56efc2b90a 100644
> --- a/fs/xfs/libxfs/xfs_btree_staging.c
> +++ b/fs/xfs/libxfs/xfs_btree_staging.c
> @@ -662,7 +662,7 @@ xfs_btree_bload_compute_geometry(
>  	xfs_btree_bload_ensure_slack(cur, &bbl->node_slack, 1);
>  
>  	bbl->nr_records = nr_this_level = nr_records;
> -	for (cur->bc_nlevels = 1; cur->bc_nlevels < XFS_BTREE_MAXLEVELS;) {
> +	for (cur->bc_nlevels = 1; cur->bc_nlevels <= XFS_BTREE_MAXLEVELS;) {
>  		uint64_t	level_blocks;
>  		uint64_t	dontcare64;
>  		unsigned int	level = cur->bc_nlevels - 1;
> @@ -726,7 +726,7 @@ xfs_btree_bload_compute_geometry(
>  		nr_this_level = level_blocks;
>  	}
>  
> -	if (cur->bc_nlevels == XFS_BTREE_MAXLEVELS)
> +	if (cur->bc_nlevels > XFS_BTREE_MAXLEVELS)
>  		return -EOVERFLOW;
>  
>  	bbl->btree_height = cur->bc_nlevels;
Christoph Hellwig Sept. 21, 2021, 8:56 a.m. UTC | #2 
On Fri, Sep 17, 2021 at 06:29:59PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The btree geometry computation function has an off-by-one error in that
> it does not allow maximally tall btrees (nlevels == XFS_BTREE_MAXLEVELS).
> This can result in repairs failing unnecessarily on very fragmented
> filesystems.  Subsequent patches to remove MAXLEVELS usage in favor of
> the per-btree type computations will make this a much more likely
> occurrence.

Shouldn't this go in first as a fix?

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Darrick J. Wong Sept. 22, 2021, 3:59 p.m. UTC | #3 
On Tue, Sep 21, 2021 at 09:56:02AM +0100, Christoph Hellwig wrote:
> On Fri, Sep 17, 2021 at 06:29:59PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > The btree geometry computation function has an off-by-one error in that
> > it does not allow maximally tall btrees (nlevels == XFS_BTREE_MAXLEVELS).
> > This can result in repairs failing unnecessarily on very fragmented
> > filesystems.  Subsequent patches to remove MAXLEVELS usage in favor of
> > the per-btree type computations will make this a much more likely
> > occurrence.
> 
> Shouldn't this go in first as a fix?

It probably should, though I haven't seen any bug reports about this
fault.  I'll move it to the front of the patchset.

--D

> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_btree_staging.c b/fs/xfs/libxfs/xfs_btree_staging.c
index 26143297bb7b..cc56efc2b90a 100644
--- a/fs/xfs/libxfs/xfs_btree_staging.c
+++ b/fs/xfs/libxfs/xfs_btree_staging.c
@@ -662,7 +662,7 @@  xfs_btree_bload_compute_geometry(
 	xfs_btree_bload_ensure_slack(cur, &bbl->node_slack, 1);
 
 	bbl->nr_records = nr_this_level = nr_records;
-	for (cur->bc_nlevels = 1; cur->bc_nlevels < XFS_BTREE_MAXLEVELS;) {
+	for (cur->bc_nlevels = 1; cur->bc_nlevels <= XFS_BTREE_MAXLEVELS;) {
 		uint64_t	level_blocks;
 		uint64_t	dontcare64;
 		unsigned int	level = cur->bc_nlevels - 1;
@@ -726,7 +726,7 @@  xfs_btree_bload_compute_geometry(
 		nr_this_level = level_blocks;
 	}
 
-	if (cur->bc_nlevels == XFS_BTREE_MAXLEVELS)
+	if (cur->bc_nlevels > XFS_BTREE_MAXLEVELS)
 		return -EOVERFLOW;
 
 	bbl->btree_height = cur->bc_nlevels;