[PATCH] xfs: revert "xfs: factor rmap btree size into the indlen calculations"

[PATCH] xfs: revert "xfs: factor rmap btree size into the indlen calculations"

[Darrick J. Wong]

In commit fd26a88093ba we added a worst case estimate for rmapbt blocks
needed to satisfy the block mapping request.  Since then, we added the
ability to reserve enough space in each AG such that we should never run
out of blocks to grow the rmapbt, which makes this calculation
unnecessary.  Revert the commit because it makes the extra delalloc
indlen accounting unnecessary and incorrect.

Reported-by: Eryu Guan <eguan@redhat.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_bmap.c |   17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 459f4b4f0..044a363 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -49,7 +49,6 @@
 #include "xfs_rmap.h"
 #include "xfs_ag_resv.h"
 #include "xfs_refcount.h"
-#include "xfs_rmap_btree.h"
 #include "xfs_icache.h"
 
 
@@ -192,12 +191,8 @@ xfs_bmap_worst_indlen(
 	int		maxrecs;	/* maximum record count at this level */
 	xfs_mount_t	*mp;		/* mount structure */
 	xfs_filblks_t	rval;		/* return value */
-	xfs_filblks_t   orig_len;
 
 	mp = ip->i_mount;
-
-	/* Calculate the worst-case size of the bmbt. */
-	orig_len = len;
 	maxrecs = mp->m_bmap_dmxr[0];
 	for (level = 0, rval = 0;
 	     level < XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK);
@@ -205,20 +200,12 @@ xfs_bmap_worst_indlen(
 		len += maxrecs - 1;
 		do_div(len, maxrecs);
 		rval += len;
-		if (len == 1) {
-			rval += XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK) -
+		if (len == 1)
+			return rval + XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK) -
 				level - 1;
-			break;
-		}
 		if (level == 0)
 			maxrecs = mp->m_bmap_dmxr[1];
 	}
-
-	/* Calculate the worst-case size of the rmapbt. */
-	if (xfs_sb_version_hasrmapbt(&mp->m_sb))
-		rval += 1 + xfs_rmapbt_calc_size(mp, orig_len) +
-				mp->m_rmap_maxlevels;
-
 	return rval;
 }
 

Re: [PATCH] xfs: revert "xfs: factor rmap btree size into the indlen calculations"

[Brian Foster]

On Sat, Sep 23, 2017 at 12:11:42AM -0700, Darrick J. Wong wrote:
> In commit fd26a88093ba we added a worst case estimate for rmapbt blocks
> needed to satisfy the block mapping request.  Since then, we added the
> ability to reserve enough space in each AG such that we should never run
> out of blocks to grow the rmapbt, which makes this calculation
> unnecessary.  Revert the commit because it makes the extra delalloc
> indlen accounting unnecessary and incorrect.
> 
> Reported-by: Eryu Guan <eguan@redhat.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_bmap.c |   17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 459f4b4f0..044a363 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -49,7 +49,6 @@
>  #include "xfs_rmap.h"
>  #include "xfs_ag_resv.h"
>  #include "xfs_refcount.h"
> -#include "xfs_rmap_btree.h"
>  #include "xfs_icache.h"
>  
>  
> @@ -192,12 +191,8 @@ xfs_bmap_worst_indlen(
>  	int		maxrecs;	/* maximum record count at this level */
>  	xfs_mount_t	*mp;		/* mount structure */
>  	xfs_filblks_t	rval;		/* return value */
> -	xfs_filblks_t   orig_len;
>  
>  	mp = ip->i_mount;
> -
> -	/* Calculate the worst-case size of the bmbt. */
> -	orig_len = len;
>  	maxrecs = mp->m_bmap_dmxr[0];
>  	for (level = 0, rval = 0;
>  	     level < XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK);
> @@ -205,20 +200,12 @@ xfs_bmap_worst_indlen(
>  		len += maxrecs - 1;
>  		do_div(len, maxrecs);
>  		rval += len;
> -		if (len == 1) {
> -			rval += XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK) -
> +		if (len == 1)
> +			return rval + XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK) -
>  				level - 1;
> -			break;
> -		}
>  		if (level == 0)
>  			maxrecs = mp->m_bmap_dmxr[1];
>  	}
> -
> -	/* Calculate the worst-case size of the rmapbt. */
> -	if (xfs_sb_version_hasrmapbt(&mp->m_sb))
> -		rval += 1 + xfs_rmapbt_calc_size(mp, orig_len) +
> -				mp->m_rmap_maxlevels;
> -
>  	return rval;
>  }
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs: revert "xfs: factor rmap btree size into the indlen calculations"

[Christoph Hellwig]

On Sat, Sep 23, 2017 at 12:11:42AM -0700, Darrick J. Wong wrote:
> In commit fd26a88093ba we added a worst case estimate for rmapbt blocks
> needed to satisfy the block mapping request.  Since then, we added the
> ability to reserve enough space in each AG such that we should never run
> out of blocks to grow the rmapbt, which makes this calculation
> unnecessary.  Revert the commit because it makes the extra delalloc
> indlen accounting unnecessary and incorrect.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH] xfs: revert "xfs: factor rmap btree size into the indlen calculations"

[Darrick J. Wong]

In commit fd26a88093ba we added a worst case estimate for rmapbt blocks
needed to satisfy the block mapping request.  Since then, we added the
ability to reserve enough space in each AG such that we should never run
out of blocks to grow the rmapbt, which makes this calculation
unnecessary.  Revert the commit because it makes the extra delalloc
indlen accounting unnecessary and incorrect.

Reported-by: Eryu Guan <eguan@redhat.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_bmap.c |   17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 459f4b4f0..044a363 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -49,7 +49,6 @@
 #include "xfs_rmap.h"
 #include "xfs_ag_resv.h"
 #include "xfs_refcount.h"
-#include "xfs_rmap_btree.h"
 #include "xfs_icache.h"
 
 
@@ -192,12 +191,8 @@ xfs_bmap_worst_indlen(
 	int		maxrecs;	/* maximum record count at this level */
 	xfs_mount_t	*mp;		/* mount structure */
 	xfs_filblks_t	rval;		/* return value */
-	xfs_filblks_t   orig_len;
 
 	mp = ip->i_mount;
-
-	/* Calculate the worst-case size of the bmbt. */
-	orig_len = len;
 	maxrecs = mp->m_bmap_dmxr[0];
 	for (level = 0, rval = 0;
 	     level < XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK);
@@ -205,20 +200,12 @@ xfs_bmap_worst_indlen(
 		len += maxrecs - 1;
 		do_div(len, maxrecs);
 		rval += len;
-		if (len == 1) {
-			rval += XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK) -
+		if (len == 1)
+			return rval + XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK) -
 				level - 1;
-			break;
-		}
 		if (level == 0)
 			maxrecs = mp->m_bmap_dmxr[1];
 	}
-
-	/* Calculate the worst-case size of the rmapbt. */
-	if (xfs_sb_version_hasrmapbt(&mp->m_sb))
-		rval += 1 + xfs_rmapbt_calc_size(mp, orig_len) +
-				mp->m_rmap_maxlevels;
-
 	return rval;
 }
 

Re: [PATCH] xfs: revert "xfs: factor rmap btree size into the indlen calculations"

[Christoph Hellwig]

On Sat, Sep 02, 2017 at 10:06:26AM -0700, Darrick J. Wong wrote:
> In commit fd26a88093ba we added a worst case estimate for rmapbt blocks
> needed to satisfy the block mapping request.  Since then, we added the
> ability to reserve enough space in each AG such that we should never run
> out of blocks to grow the rmapbt, which makes this calculation
> unnecessary.  Revert the commit because it makes the extra delalloc
> indlen accounting unnecessary and incorrect.

Do you remember any details of why we added it and what is supposed
to fix it?  I have memories of various issues in this area, but I
can't remember the details.

Re: [PATCH] xfs: revert "xfs: factor rmap btree size into the indlen calculations"

[Darrick J. Wong]

On Sun, Sep 03, 2017 at 12:43:58AM -0700, Christoph Hellwig wrote:
> On Sat, Sep 02, 2017 at 10:06:26AM -0700, Darrick J. Wong wrote:
> > In commit fd26a88093ba we added a worst case estimate for rmapbt blocks
> > needed to satisfy the block mapping request.  Since then, we added the
> > ability to reserve enough space in each AG such that we should never run
> > out of blocks to grow the rmapbt, which makes this calculation
> > unnecessary.  Revert the commit because it makes the extra delalloc
> > indlen accounting unnecessary and incorrect.
> 
> Do you remember any details of why we added it and what is supposed
> to fix it?  I have memories of various issues in this area, but I
> can't remember the details.

We'd fill the fs up with delalloc reservations until there wasn't any
space, and once the fs fragmented badly then we suddenly needed more
than just the indlen to satisfy bmbt + rmapbt expansion.  This indlen
patch was a hack to try to ENOSPC out of write_begin/page_mkwrite before
we ran the fs totally out of blocks back when we were still trying to
cram the bmbt and rmap updates into a single huge transaction.  Deferred
ops broke that, and perag reservations made it unnecesary, so now we can
rip it out.

--D

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs: revert "xfs: factor rmap btree size into the indlen calculations"

[Brian Foster]

On Sun, Sep 03, 2017 at 08:40:50AM -0700, Darrick J. Wong wrote:
> On Sun, Sep 03, 2017 at 12:43:58AM -0700, Christoph Hellwig wrote:
> > On Sat, Sep 02, 2017 at 10:06:26AM -0700, Darrick J. Wong wrote:
> > > In commit fd26a88093ba we added a worst case estimate for rmapbt blocks
> > > needed to satisfy the block mapping request.  Since then, we added the
> > > ability to reserve enough space in each AG such that we should never run
> > > out of blocks to grow the rmapbt, which makes this calculation
> > > unnecessary.  Revert the commit because it makes the extra delalloc
> > > indlen accounting unnecessary and incorrect.
> > 
> > Do you remember any details of why we added it and what is supposed
> > to fix it?  I have memories of various issues in this area, but I
> > can't remember the details.
> 
> We'd fill the fs up with delalloc reservations until there wasn't any
> space, and once the fs fragmented badly then we suddenly needed more
> than just the indlen to satisfy bmbt + rmapbt expansion.  This indlen
> patch was a hack to try to ENOSPC out of write_begin/page_mkwrite before
> we ran the fs totally out of blocks back when we were still trying to
> cram the bmbt and rmap updates into a single huge transaction.  Deferred
> ops broke that, and perag reservations made it unnecesary, so now we can
> rip it out.
> 

It's not totally clear to me what the original patch would have done,
since we technically could return all of the indlen on delay -> real
extent conversion before the rmap operation would have even run. Perhaps
it could hide a problem by reserving blocks only to release them just
before we'd actually need to allocate rmapbt blocks..?

Anyways, the patch seems fine to me:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> --D
> 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html