[PATCH 0/2] xfs_db: fixes to the btheight command

[PATCH 0/2] xfs_db: fixes to the btheight command

[Darrick J. Wong]

Hi all,

Fix some problems that Coverity found with the new btheight command and
enhance it to do calculations for the incore extent tree.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=btheight-fixes

[PATCH 1/2] xfs_db: btheight should check geometry more carefully

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The btheight command needs to check user-supplied geometry more
carefully so that we don't hit floating point exceptions.

Coverity-id: 1453661, 1453659
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/btheight.c |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 82 insertions(+), 6 deletions(-)


diff --git a/db/btheight.c b/db/btheight.c
index 289e5d84..e2c9759f 100644
--- a/db/btheight.c
+++ b/db/btheight.c
@@ -138,6 +138,10 @@ construct_records_per_block(
 		perror(p);
 		goto out;
 	}
+	if (record_size == 0) {
+		fprintf(stderr, _("%s: record size cannot be zero.\n"), tag);
+		goto out;
+	}
 
 	p = strtok(NULL, ":");
 	if (!p) {
@@ -149,6 +153,10 @@ construct_records_per_block(
 		perror(p);
 		goto out;
 	}
+	if (key_size == 0) {
+		fprintf(stderr, _("%s: key size cannot be zero.\n"), tag);
+		goto out;
+	}
 
 	p = strtok(NULL, ":");
 	if (!p) {
@@ -160,6 +168,10 @@ construct_records_per_block(
 		perror(p);
 		goto out;
 	}
+	if (ptr_size == 0) {
+		fprintf(stderr, _("%s: pointer size cannot be zero.\n"), tag);
+		goto out;
+	}
 
 	p = strtok(NULL, ":");
 	if (!p) {
@@ -180,6 +192,27 @@ construct_records_per_block(
 		goto out;
 	}
 
+	if (record_size > blocksize) {
+		fprintf(stderr,
+			_("%s: record size must be less than %u bytes.\n"),
+			tag, blocksize);
+		goto out;
+	}
+
+	if (key_size > blocksize) {
+		fprintf(stderr,
+			_("%s: key size must be less than %u bytes.\n"),
+			tag, blocksize);
+		goto out;
+	}
+
+	if (ptr_size > blocksize) {
+		fprintf(stderr,
+			_("%s: pointer size must be less than %u bytes.\n"),
+			tag, blocksize);
+		goto out;
+	}
+
 	p = strtok(NULL, ":");
 	if (p) {
 		fprintf(stderr,
@@ -211,13 +244,24 @@ report(
 	int			ret;
 
 	ret = construct_records_per_block(tag, blocksize, records_per_block);
-	if (ret) {
-		printf(_("%s: Unable to determine records per block.\n"),
-				tag);
+	if (ret)
 		return;
-	}
 
 	if (report_what & REPORT_MAX) {
+		if (records_per_block[0] < 2) {
+			fprintf(stderr,
+_("%s: cannot calculate best case scenario due to leaf geometry underflow.\n"),
+				tag);
+			return;
+		}
+
+		if (records_per_block[1] < 4) {
+			fprintf(stderr,
+_("%s: cannot calculate best case scenario due to node geometry underflow.\n"),
+				tag);
+			return;
+		}
+
 		printf(
 _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
 				tag, blocksize, records_per_block[0],
@@ -230,6 +274,20 @@ _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
 		records_per_block[0] /= 2;
 		records_per_block[1] /= 2;
 
+		if (records_per_block[0] < 1) {
+			fprintf(stderr,
+_("%s: cannot calculate worst case scenario due to leaf geometry underflow.\n"),
+				tag);
+			return;
+		}
+
+		if (records_per_block[1] < 2) {
+			fprintf(stderr,
+_("%s: cannot calculate worst case scenario due to node geometry underflow.\n"),
+				tag);
+			return;
+		}
+
 		printf(
 _("%s: worst case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
 				tag, blocksize, records_per_block[0],
@@ -284,8 +342,26 @@ btheight_f(
 		}
 	}
 
-	if (argc == optind || blocksize <= 0 || blocksize > INT_MAX ||
-	    nr_records == 0) {
+	if (nr_records == 0) {
+		fprintf(stderr,
+_("Number of records must be greater than zero.\n"));
+		return 0;
+	}
+
+	if (blocksize > INT_MAX) {
+		fprintf(stderr,
+_("The largest block size this command will consider is %u bytes.\n"),
+			INT_MAX);
+		return 0;
+	}
+
+	if (blocksize < 128) {
+		fprintf(stderr,
+_("The smallest block size this command will consider is 128 bytes.\n"));
+		return 0;
+	}
+
+	if (argc == optind) {
 		btheight_help();
 		return 0;
 	}

[PATCH 2/2] xfs_db: calculate iext tree geometry in btheight command

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

(Ab)use the btheight command to calculate the geometry of the incore
extent tree.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/btheight.c |   87 +++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 60 insertions(+), 27 deletions(-)


diff --git a/db/btheight.c b/db/btheight.c
index e2c9759f..be604ebc 100644
--- a/db/btheight.c
+++ b/db/btheight.c
@@ -22,18 +22,37 @@ static int rmap_maxrecs(struct xfs_mount *mp, int blocklen, int leaf)
 	return libxfs_rmapbt_maxrecs(blocklen, leaf);
 }
 
+static int iext_maxrecs(struct xfs_mount *mp, int blocklen, int leaf)
+{
+	blocklen -= 2 * sizeof(void *);
+
+	return blocklen / sizeof(struct xfs_bmbt_rec);
+}
+
+static int disk_blocksize(struct xfs_mount *mp)
+{
+	return mp->m_sb.sb_blocksize;
+}
+
+static int iext_blocksize(struct xfs_mount *mp)
+{
+	return 256;
+}
+
 struct btmap {
 	const char	*tag;
 	int		(*maxrecs)(struct xfs_mount *mp, int blocklen,
 				   int leaf);
+	int		(*default_blocksize)(struct xfs_mount *mp);
 } maps[] = {
-	{"bnobt", libxfs_allocbt_maxrecs},
-	{"cntbt", libxfs_allocbt_maxrecs},
-	{"inobt", libxfs_inobt_maxrecs},
-	{"finobt", libxfs_inobt_maxrecs},
-	{"bmapbt", libxfs_bmbt_maxrecs},
-	{"refcountbt", refc_maxrecs},
-	{"rmapbt", rmap_maxrecs},
+	{"bnobt", libxfs_allocbt_maxrecs, disk_blocksize},
+	{"cntbt", libxfs_allocbt_maxrecs, disk_blocksize},
+	{"inobt", libxfs_inobt_maxrecs, disk_blocksize},
+	{"finobt", libxfs_inobt_maxrecs, disk_blocksize},
+	{"bmapbt", libxfs_bmbt_maxrecs, disk_blocksize},
+	{"refcountbt", refc_maxrecs, disk_blocksize},
+	{"rmapbt", rmap_maxrecs, disk_blocksize},
+	{"iext", iext_maxrecs, iext_blocksize},
 };
 
 static void
@@ -108,7 +127,7 @@ calc_height(
 static int
 construct_records_per_block(
 	char		*tag,
-	int		blocksize,
+	int		*blocksize,
 	unsigned int	*records_per_block)
 {
 	char		*toktag;
@@ -119,8 +138,10 @@ construct_records_per_block(
 
 	for (i = 0, m = maps; i < ARRAY_SIZE(maps); i++, m++) {
 		if (!strcmp(m->tag, tag)) {
-			records_per_block[0] = m->maxrecs(mp, blocksize, 1);
-			records_per_block[1] = m->maxrecs(mp, blocksize, 0);
+			if (*blocksize < 0)
+				*blocksize = m->default_blocksize(mp);
+			records_per_block[0] = m->maxrecs(mp, *blocksize, 1);
+			records_per_block[1] = m->maxrecs(mp, *blocksize, 0);
 			return 0;
 		}
 	}
@@ -178,38 +199,50 @@ construct_records_per_block(
 		fprintf(stderr, _("%s: header type not found.\n"), tag);
 		goto out;
 	}
-	if (!strcmp(p, "short"))
+	if (!strcmp(p, "short")) {
+		if (*blocksize < 0)
+			*blocksize = mp->m_sb.sb_blocksize;
 		blocksize -= XFS_BTREE_SBLOCK_LEN;
-	else if (!strcmp(p, "shortcrc"))
+	} else if (!strcmp(p, "shortcrc")) {
+		if (*blocksize < 0)
+			*blocksize = mp->m_sb.sb_blocksize;
 		blocksize -= XFS_BTREE_SBLOCK_CRC_LEN;
-	else if (!strcmp(p, "long"))
+	} else if (!strcmp(p, "long")) {
+		if (*blocksize < 0)
+			*blocksize = mp->m_sb.sb_blocksize;
 		blocksize -= XFS_BTREE_LBLOCK_LEN;
-	else if (!strcmp(p, "longcrc"))
+	} else if (!strcmp(p, "longcrc")) {
+		if (*blocksize < 0)
+			*blocksize = mp->m_sb.sb_blocksize;
 		blocksize -= XFS_BTREE_LBLOCK_CRC_LEN;
-	else {
+	} else if (!strcmp(p, "iext")) {
+		if (*blocksize < 0)
+			*blocksize = 256;
+		blocksize -= 2 * sizeof(void *);
+	} else {
 		fprintf(stderr, _("%s: unrecognized btree header type."),
 				p);
 		goto out;
 	}
 
-	if (record_size > blocksize) {
+	if (record_size > *blocksize) {
 		fprintf(stderr,
 			_("%s: record size must be less than %u bytes.\n"),
-			tag, blocksize);
+			tag, *blocksize);
 		goto out;
 	}
 
-	if (key_size > blocksize) {
+	if (key_size > *blocksize) {
 		fprintf(stderr,
 			_("%s: key size must be less than %u bytes.\n"),
-			tag, blocksize);
+			tag, *blocksize);
 		goto out;
 	}
 
-	if (ptr_size > blocksize) {
+	if (ptr_size > *blocksize) {
 		fprintf(stderr,
 			_("%s: pointer size must be less than %u bytes.\n"),
-			tag, blocksize);
+			tag, *blocksize);
 		goto out;
 	}
 
@@ -221,8 +254,8 @@ construct_records_per_block(
 		goto out;
 	}
 
-	records_per_block[0] = blocksize / record_size;
-	records_per_block[1] = blocksize / (key_size + ptr_size);
+	records_per_block[0] = *blocksize / record_size;
+	records_per_block[1] = *blocksize / (key_size + ptr_size);
 	ret = 0;
 out:
 	free(toktag);
@@ -238,12 +271,12 @@ report(
 	char			*tag,
 	unsigned int		report_what,
 	unsigned long long	nr_records,
-	unsigned int		blocksize)
+	int			blocksize)
 {
 	unsigned int		records_per_block[2];
 	int			ret;
 
-	ret = construct_records_per_block(tag, blocksize, records_per_block);
+	ret = construct_records_per_block(tag, &blocksize, records_per_block);
 	if (ret)
 		return;
 
@@ -302,7 +335,7 @@ btheight_f(
 	int		argc,
 	char		**argv)
 {
-	long long	blocksize = mp->m_sb.sb_blocksize;
+	long long	blocksize = -1;
 	uint64_t	nr_records = 0;
 	int		report_what = REPORT_DEFAULT;
 	int		i, c;
@@ -355,7 +388,7 @@ _("The largest block size this command will consider is %u bytes.\n"),
 		return 0;
 	}
 
-	if (blocksize < 128) {
+	if (blocksize >= 0 && blocksize < 128) {
 		fprintf(stderr,
 _("The smallest block size this command will consider is 128 bytes.\n"));
 		return 0;

Re: [PATCH 1/2] xfs_db: btheight should check geometry more carefully

[Carlos Maiolino]

On Wed, Sep 25, 2019 at 02:40:53PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The btheight command needs to check user-supplied geometry more
> carefully so that we don't hit floating point exceptions.
> 
> Coverity-id: 1453661, 1453659
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  

Patch looks good, but.


> +	if (record_size > blocksize) {
> +		fprintf(stderr,
> +			_("%s: record size must be less than %u bytes.\n"),

	Couldn't this message maybe be better saying "less than current blocksize"?
	Saying it is less than X bytes sounds kind of meaningless, requiring a
	trip to the code to understand what exactly 'bytes' mean here.

	Maybe something like:

	_("%s: record size must be less than current block size (%u).\n"),


Same for the next two.

> +			tag, blocksize);
> +		goto out;
> +	}
> +
> +	if (key_size > blocksize) {
> +		fprintf(stderr,
> +			_("%s: key size must be less than %u bytes.\n"),
> +			tag, blocksize);
> +		goto out;
> +	}
> +
> +	if (ptr_size > blocksize) {
> +		fprintf(stderr,
> +			_("%s: pointer size must be less than %u bytes.\n"),
> +			tag, blocksize);
> +		goto out;
> +	}
> +
>  	p = strtok(NULL, ":");
>  	if (p) {
>  		fprintf(stderr,
> @@ -211,13 +244,24 @@ report(
>  	int			ret;
>  
>  	ret = construct_records_per_block(tag, blocksize, records_per_block);
> -	if (ret) {
> -		printf(_("%s: Unable to determine records per block.\n"),
> -				tag);
> +	if (ret)
>  		return;
> -	}
>  
>  	if (report_what & REPORT_MAX) {
> +		if (records_per_block[0] < 2) {
> +			fprintf(stderr,
> +_("%s: cannot calculate best case scenario due to leaf geometry underflow.\n"),
> +				tag);
> +			return;
> +		}
> +
> +		if (records_per_block[1] < 4) {
> +			fprintf(stderr,
> +_("%s: cannot calculate best case scenario due to node geometry underflow.\n"),
> +				tag);
> +			return;
> +		}
> +
>  		printf(
>  _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
>  				tag, blocksize, records_per_block[0],
> @@ -230,6 +274,20 @@ _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
>  		records_per_block[0] /= 2;
>  		records_per_block[1] /= 2;
>  
> +		if (records_per_block[0] < 1) {
> +			fprintf(stderr,
> +_("%s: cannot calculate worst case scenario due to leaf geometry underflow.\n"),
> +				tag);
> +			return;
> +		}
> +
> +		if (records_per_block[1] < 2) {
> +			fprintf(stderr,
> +_("%s: cannot calculate worst case scenario due to node geometry underflow.\n"),
> +				tag);
> +			return;
> +		}
> +
>  		printf(
>  _("%s: worst case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
>  				tag, blocksize, records_per_block[0],
> @@ -284,8 +342,26 @@ btheight_f(
>  		}
>  	}
>  
> -	if (argc == optind || blocksize <= 0 || blocksize > INT_MAX ||
> -	    nr_records == 0) {
> +	if (nr_records == 0) {
> +		fprintf(stderr,
> +_("Number of records must be greater than zero.\n"));
> +		return 0;
> +	}
> +
> +	if (blocksize > INT_MAX) {
> +		fprintf(stderr,
> +_("The largest block size this command will consider is %u bytes.\n"),
> +			INT_MAX);
> +		return 0;
> +	}
> +
> +	if (blocksize < 128) {
> +		fprintf(stderr,
> +_("The smallest block size this command will consider is 128 bytes.\n"));
> +		return 0;
> +	}
> +
> +	if (argc == optind) {
>  		btheight_help();
>  		return 0;
>  	}
> 

-- 
Carlos

Re: [PATCH 2/2] xfs_db: calculate iext tree geometry in btheight command

[Carlos Maiolino]

On Wed, Sep 25, 2019 at 02:40:59PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> (Ab)use the btheight command to calculate the geometry of the incore
> extent tree.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  db/btheight.c |   87 +++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 60 insertions(+), 27 deletions(-)
> 
> 
> diff --git a/db/btheight.c b/db/btheight.c
> index e2c9759f..be604ebc 100644
> --- a/db/btheight.c
> +++ b/db/btheight.c
> @@ -22,18 +22,37 @@ static int rmap_maxrecs(struct xfs_mount *mp, int blocklen, int leaf)
>  	return libxfs_rmapbt_maxrecs(blocklen, leaf);
>  }
>  
> +static int iext_maxrecs(struct xfs_mount *mp, int blocklen, int leaf)
> +{
> +	blocklen -= 2 * sizeof(void *);
> +
> +	return blocklen / sizeof(struct xfs_bmbt_rec);
> +}
> +
> +static int disk_blocksize(struct xfs_mount *mp)

This naming looks confusing to me, disk_blocksize sounds to me like
'sector size', maybe fs_blocksize() or get_fs_blocksize()?
> +{
> +	return mp->m_sb.sb_blocksize;
> +}
> +

Otherwise looks good.


> +static int iext_blocksize(struct xfs_mount *mp)
> +{
> +	return 256;
> +}
> +
>  struct btmap {
>  	const char	*tag;
>  	int		(*maxrecs)(struct xfs_mount *mp, int blocklen,
>  				   int leaf);
> +	int		(*default_blocksize)(struct xfs_mount *mp);
>  } maps[] = {
> -	{"bnobt", libxfs_allocbt_maxrecs},
> -	{"cntbt", libxfs_allocbt_maxrecs},
> -	{"inobt", libxfs_inobt_maxrecs},
> -	{"finobt", libxfs_inobt_maxrecs},
> -	{"bmapbt", libxfs_bmbt_maxrecs},
> -	{"refcountbt", refc_maxrecs},
> -	{"rmapbt", rmap_maxrecs},
> +	{"bnobt", libxfs_allocbt_maxrecs, disk_blocksize},
> +	{"cntbt", libxfs_allocbt_maxrecs, disk_blocksize},
> +	{"inobt", libxfs_inobt_maxrecs, disk_blocksize},
> +	{"finobt", libxfs_inobt_maxrecs, disk_blocksize},
> +	{"bmapbt", libxfs_bmbt_maxrecs, disk_blocksize},
> +	{"refcountbt", refc_maxrecs, disk_blocksize},
> +	{"rmapbt", rmap_maxrecs, disk_blocksize},
> +	{"iext", iext_maxrecs, iext_blocksize},
>  };
>  
>  static void
> @@ -108,7 +127,7 @@ calc_height(
>  static int
>  construct_records_per_block(
>  	char		*tag,
> -	int		blocksize,
> +	int		*blocksize,
>  	unsigned int	*records_per_block)
>  {
>  	char		*toktag;
> @@ -119,8 +138,10 @@ construct_records_per_block(
>  
>  	for (i = 0, m = maps; i < ARRAY_SIZE(maps); i++, m++) {
>  		if (!strcmp(m->tag, tag)) {
> -			records_per_block[0] = m->maxrecs(mp, blocksize, 1);
> -			records_per_block[1] = m->maxrecs(mp, blocksize, 0);
> +			if (*blocksize < 0)
> +				*blocksize = m->default_blocksize(mp);
> +			records_per_block[0] = m->maxrecs(mp, *blocksize, 1);
> +			records_per_block[1] = m->maxrecs(mp, *blocksize, 0);
>  			return 0;
>  		}
>  	}
> @@ -178,38 +199,50 @@ construct_records_per_block(
>  		fprintf(stderr, _("%s: header type not found.\n"), tag);
>  		goto out;
>  	}
> -	if (!strcmp(p, "short"))
> +	if (!strcmp(p, "short")) {
> +		if (*blocksize < 0)
> +			*blocksize = mp->m_sb.sb_blocksize;
>  		blocksize -= XFS_BTREE_SBLOCK_LEN;
> -	else if (!strcmp(p, "shortcrc"))
> +	} else if (!strcmp(p, "shortcrc")) {
> +		if (*blocksize < 0)
> +			*blocksize = mp->m_sb.sb_blocksize;
>  		blocksize -= XFS_BTREE_SBLOCK_CRC_LEN;
> -	else if (!strcmp(p, "long"))
> +	} else if (!strcmp(p, "long")) {
> +		if (*blocksize < 0)
> +			*blocksize = mp->m_sb.sb_blocksize;
>  		blocksize -= XFS_BTREE_LBLOCK_LEN;
> -	else if (!strcmp(p, "longcrc"))
> +	} else if (!strcmp(p, "longcrc")) {
> +		if (*blocksize < 0)
> +			*blocksize = mp->m_sb.sb_blocksize;
>  		blocksize -= XFS_BTREE_LBLOCK_CRC_LEN;
> -	else {
> +	} else if (!strcmp(p, "iext")) {
> +		if (*blocksize < 0)
> +			*blocksize = 256;
> +		blocksize -= 2 * sizeof(void *);
> +	} else {
>  		fprintf(stderr, _("%s: unrecognized btree header type."),
>  				p);
>  		goto out;
>  	}
>  
> -	if (record_size > blocksize) {
> +	if (record_size > *blocksize) {
>  		fprintf(stderr,
>  			_("%s: record size must be less than %u bytes.\n"),
> -			tag, blocksize);
> +			tag, *blocksize);
>  		goto out;
>  	}
>  
> -	if (key_size > blocksize) {
> +	if (key_size > *blocksize) {
>  		fprintf(stderr,
>  			_("%s: key size must be less than %u bytes.\n"),
> -			tag, blocksize);
> +			tag, *blocksize);
>  		goto out;
>  	}
>  
> -	if (ptr_size > blocksize) {
> +	if (ptr_size > *blocksize) {
>  		fprintf(stderr,
>  			_("%s: pointer size must be less than %u bytes.\n"),
> -			tag, blocksize);
> +			tag, *blocksize);
>  		goto out;
>  	}
>  
> @@ -221,8 +254,8 @@ construct_records_per_block(
>  		goto out;
>  	}
>  
> -	records_per_block[0] = blocksize / record_size;
> -	records_per_block[1] = blocksize / (key_size + ptr_size);
> +	records_per_block[0] = *blocksize / record_size;
> +	records_per_block[1] = *blocksize / (key_size + ptr_size);
>  	ret = 0;
>  out:
>  	free(toktag);
> @@ -238,12 +271,12 @@ report(
>  	char			*tag,
>  	unsigned int		report_what,
>  	unsigned long long	nr_records,
> -	unsigned int		blocksize)
> +	int			blocksize)
>  {
>  	unsigned int		records_per_block[2];
>  	int			ret;
>  
> -	ret = construct_records_per_block(tag, blocksize, records_per_block);
> +	ret = construct_records_per_block(tag, &blocksize, records_per_block);
>  	if (ret)
>  		return;
>  
> @@ -302,7 +335,7 @@ btheight_f(
>  	int		argc,
>  	char		**argv)
>  {
> -	long long	blocksize = mp->m_sb.sb_blocksize;
> +	long long	blocksize = -1;
>  	uint64_t	nr_records = 0;
>  	int		report_what = REPORT_DEFAULT;
>  	int		i, c;
> @@ -355,7 +388,7 @@ _("The largest block size this command will consider is %u bytes.\n"),
>  		return 0;
>  	}
>  
> -	if (blocksize < 128) {
> +	if (blocksize >= 0 && blocksize < 128) {
>  		fprintf(stderr,
>  _("The smallest block size this command will consider is 128 bytes.\n"));
>  		return 0;
> 

-- 
Carlos

Re: [PATCH 1/2] xfs_db: btheight should check geometry more carefully

[Darrick J. Wong]

On Thu, Sep 26, 2019 at 11:11:48AM +0200, Carlos Maiolino wrote:
> On Wed, Sep 25, 2019 at 02:40:53PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > The btheight command needs to check user-supplied geometry more
> > carefully so that we don't hit floating point exceptions.
> > 
> > Coverity-id: 1453661, 1453659
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  
> 
> Patch looks good, but.
> 
> 
> > +	if (record_size > blocksize) {
> > +		fprintf(stderr,
> > +			_("%s: record size must be less than %u bytes.\n"),
> 
> 	Couldn't this message maybe be better saying "less than current blocksize"?
> 	Saying it is less than X bytes sounds kind of meaningless, requiring a
> 	trip to the code to understand what exactly 'bytes' mean here.
> 
> 	Maybe something like:
> 
> 	_("%s: record size must be less than current block size (%u).\n"),

I think I'll change that to 'selected' from 'current' since the caller
can change the block size with -b, but otherwise I agree.

--D

> 
> Same for the next two.
> 
> > +			tag, blocksize);
> > +		goto out;
> > +	}
> > +
> > +	if (key_size > blocksize) {
> > +		fprintf(stderr,
> > +			_("%s: key size must be less than %u bytes.\n"),
> > +			tag, blocksize);
> > +		goto out;
> > +	}
> > +
> > +	if (ptr_size > blocksize) {
> > +		fprintf(stderr,
> > +			_("%s: pointer size must be less than %u bytes.\n"),
> > +			tag, blocksize);
> > +		goto out;
> > +	}
> > +
> >  	p = strtok(NULL, ":");
> >  	if (p) {
> >  		fprintf(stderr,
> > @@ -211,13 +244,24 @@ report(
> >  	int			ret;
> >  
> >  	ret = construct_records_per_block(tag, blocksize, records_per_block);
> > -	if (ret) {
> > -		printf(_("%s: Unable to determine records per block.\n"),
> > -				tag);
> > +	if (ret)
> >  		return;
> > -	}
> >  
> >  	if (report_what & REPORT_MAX) {
> > +		if (records_per_block[0] < 2) {
> > +			fprintf(stderr,
> > +_("%s: cannot calculate best case scenario due to leaf geometry underflow.\n"),
> > +				tag);
> > +			return;
> > +		}
> > +
> > +		if (records_per_block[1] < 4) {
> > +			fprintf(stderr,
> > +_("%s: cannot calculate best case scenario due to node geometry underflow.\n"),
> > +				tag);
> > +			return;
> > +		}
> > +
> >  		printf(
> >  _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
> >  				tag, blocksize, records_per_block[0],
> > @@ -230,6 +274,20 @@ _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
> >  		records_per_block[0] /= 2;
> >  		records_per_block[1] /= 2;
> >  
> > +		if (records_per_block[0] < 1) {
> > +			fprintf(stderr,
> > +_("%s: cannot calculate worst case scenario due to leaf geometry underflow.\n"),
> > +				tag);
> > +			return;
> > +		}
> > +
> > +		if (records_per_block[1] < 2) {
> > +			fprintf(stderr,
> > +_("%s: cannot calculate worst case scenario due to node geometry underflow.\n"),
> > +				tag);
> > +			return;
> > +		}
> > +
> >  		printf(
> >  _("%s: worst case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
> >  				tag, blocksize, records_per_block[0],
> > @@ -284,8 +342,26 @@ btheight_f(
> >  		}
> >  	}
> >  
> > -	if (argc == optind || blocksize <= 0 || blocksize > INT_MAX ||
> > -	    nr_records == 0) {
> > +	if (nr_records == 0) {
> > +		fprintf(stderr,
> > +_("Number of records must be greater than zero.\n"));
> > +		return 0;
> > +	}
> > +
> > +	if (blocksize > INT_MAX) {
> > +		fprintf(stderr,
> > +_("The largest block size this command will consider is %u bytes.\n"),
> > +			INT_MAX);
> > +		return 0;
> > +	}
> > +
> > +	if (blocksize < 128) {
> > +		fprintf(stderr,
> > +_("The smallest block size this command will consider is 128 bytes.\n"));
> > +		return 0;
> > +	}
> > +
> > +	if (argc == optind) {
> >  		btheight_help();
> >  		return 0;
> >  	}
> > 
> 
> -- 
> Carlos
> 

Re: [PATCH 2/2] xfs_db: calculate iext tree geometry in btheight command

[Darrick J. Wong]

On Thu, Sep 26, 2019 at 11:20:42AM +0200, Carlos Maiolino wrote:
> On Wed, Sep 25, 2019 at 02:40:59PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > (Ab)use the btheight command to calculate the geometry of the incore
> > extent tree.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  db/btheight.c |   87 +++++++++++++++++++++++++++++++++++++++------------------
> >  1 file changed, 60 insertions(+), 27 deletions(-)
> > 
> > 
> > diff --git a/db/btheight.c b/db/btheight.c
> > index e2c9759f..be604ebc 100644
> > --- a/db/btheight.c
> > +++ b/db/btheight.c
> > @@ -22,18 +22,37 @@ static int rmap_maxrecs(struct xfs_mount *mp, int blocklen, int leaf)
> >  	return libxfs_rmapbt_maxrecs(blocklen, leaf);
> >  }
> >  
> > +static int iext_maxrecs(struct xfs_mount *mp, int blocklen, int leaf)
> > +{
> > +	blocklen -= 2 * sizeof(void *);
> > +
> > +	return blocklen / sizeof(struct xfs_bmbt_rec);
> > +}
> > +
> > +static int disk_blocksize(struct xfs_mount *mp)
> 
> This naming looks confusing to me, disk_blocksize sounds to me like
> 'sector size', maybe fs_blocksize() or get_fs_blocksize()?

Fixed.

--D

> > +{
> > +	return mp->m_sb.sb_blocksize;
> > +}
> > +
> 
> Otherwise looks good.
> 
> 
> > +static int iext_blocksize(struct xfs_mount *mp)
> > +{
> > +	return 256;
> > +}
> > +
> >  struct btmap {
> >  	const char	*tag;
> >  	int		(*maxrecs)(struct xfs_mount *mp, int blocklen,
> >  				   int leaf);
> > +	int		(*default_blocksize)(struct xfs_mount *mp);
> >  } maps[] = {
> > -	{"bnobt", libxfs_allocbt_maxrecs},
> > -	{"cntbt", libxfs_allocbt_maxrecs},
> > -	{"inobt", libxfs_inobt_maxrecs},
> > -	{"finobt", libxfs_inobt_maxrecs},
> > -	{"bmapbt", libxfs_bmbt_maxrecs},
> > -	{"refcountbt", refc_maxrecs},
> > -	{"rmapbt", rmap_maxrecs},
> > +	{"bnobt", libxfs_allocbt_maxrecs, disk_blocksize},
> > +	{"cntbt", libxfs_allocbt_maxrecs, disk_blocksize},
> > +	{"inobt", libxfs_inobt_maxrecs, disk_blocksize},
> > +	{"finobt", libxfs_inobt_maxrecs, disk_blocksize},
> > +	{"bmapbt", libxfs_bmbt_maxrecs, disk_blocksize},
> > +	{"refcountbt", refc_maxrecs, disk_blocksize},
> > +	{"rmapbt", rmap_maxrecs, disk_blocksize},
> > +	{"iext", iext_maxrecs, iext_blocksize},
> >  };
> >  
> >  static void
> > @@ -108,7 +127,7 @@ calc_height(
> >  static int
> >  construct_records_per_block(
> >  	char		*tag,
> > -	int		blocksize,
> > +	int		*blocksize,
> >  	unsigned int	*records_per_block)
> >  {
> >  	char		*toktag;
> > @@ -119,8 +138,10 @@ construct_records_per_block(
> >  
> >  	for (i = 0, m = maps; i < ARRAY_SIZE(maps); i++, m++) {
> >  		if (!strcmp(m->tag, tag)) {
> > -			records_per_block[0] = m->maxrecs(mp, blocksize, 1);
> > -			records_per_block[1] = m->maxrecs(mp, blocksize, 0);
> > +			if (*blocksize < 0)
> > +				*blocksize = m->default_blocksize(mp);
> > +			records_per_block[0] = m->maxrecs(mp, *blocksize, 1);
> > +			records_per_block[1] = m->maxrecs(mp, *blocksize, 0);
> >  			return 0;
> >  		}
> >  	}
> > @@ -178,38 +199,50 @@ construct_records_per_block(
> >  		fprintf(stderr, _("%s: header type not found.\n"), tag);
> >  		goto out;
> >  	}
> > -	if (!strcmp(p, "short"))
> > +	if (!strcmp(p, "short")) {
> > +		if (*blocksize < 0)
> > +			*blocksize = mp->m_sb.sb_blocksize;
> >  		blocksize -= XFS_BTREE_SBLOCK_LEN;
> > -	else if (!strcmp(p, "shortcrc"))
> > +	} else if (!strcmp(p, "shortcrc")) {
> > +		if (*blocksize < 0)
> > +			*blocksize = mp->m_sb.sb_blocksize;
> >  		blocksize -= XFS_BTREE_SBLOCK_CRC_LEN;
> > -	else if (!strcmp(p, "long"))
> > +	} else if (!strcmp(p, "long")) {
> > +		if (*blocksize < 0)
> > +			*blocksize = mp->m_sb.sb_blocksize;
> >  		blocksize -= XFS_BTREE_LBLOCK_LEN;
> > -	else if (!strcmp(p, "longcrc"))
> > +	} else if (!strcmp(p, "longcrc")) {
> > +		if (*blocksize < 0)
> > +			*blocksize = mp->m_sb.sb_blocksize;
> >  		blocksize -= XFS_BTREE_LBLOCK_CRC_LEN;
> > -	else {
> > +	} else if (!strcmp(p, "iext")) {
> > +		if (*blocksize < 0)
> > +			*blocksize = 256;
> > +		blocksize -= 2 * sizeof(void *);
> > +	} else {
> >  		fprintf(stderr, _("%s: unrecognized btree header type."),
> >  				p);
> >  		goto out;
> >  	}
> >  
> > -	if (record_size > blocksize) {
> > +	if (record_size > *blocksize) {
> >  		fprintf(stderr,
> >  			_("%s: record size must be less than %u bytes.\n"),
> > -			tag, blocksize);
> > +			tag, *blocksize);
> >  		goto out;
> >  	}
> >  
> > -	if (key_size > blocksize) {
> > +	if (key_size > *blocksize) {
> >  		fprintf(stderr,
> >  			_("%s: key size must be less than %u bytes.\n"),
> > -			tag, blocksize);
> > +			tag, *blocksize);
> >  		goto out;
> >  	}
> >  
> > -	if (ptr_size > blocksize) {
> > +	if (ptr_size > *blocksize) {
> >  		fprintf(stderr,
> >  			_("%s: pointer size must be less than %u bytes.\n"),
> > -			tag, blocksize);
> > +			tag, *blocksize);
> >  		goto out;
> >  	}
> >  
> > @@ -221,8 +254,8 @@ construct_records_per_block(
> >  		goto out;
> >  	}
> >  
> > -	records_per_block[0] = blocksize / record_size;
> > -	records_per_block[1] = blocksize / (key_size + ptr_size);
> > +	records_per_block[0] = *blocksize / record_size;
> > +	records_per_block[1] = *blocksize / (key_size + ptr_size);
> >  	ret = 0;
> >  out:
> >  	free(toktag);
> > @@ -238,12 +271,12 @@ report(
> >  	char			*tag,
> >  	unsigned int		report_what,
> >  	unsigned long long	nr_records,
> > -	unsigned int		blocksize)
> > +	int			blocksize)
> >  {
> >  	unsigned int		records_per_block[2];
> >  	int			ret;
> >  
> > -	ret = construct_records_per_block(tag, blocksize, records_per_block);
> > +	ret = construct_records_per_block(tag, &blocksize, records_per_block);
> >  	if (ret)
> >  		return;
> >  
> > @@ -302,7 +335,7 @@ btheight_f(
> >  	int		argc,
> >  	char		**argv)
> >  {
> > -	long long	blocksize = mp->m_sb.sb_blocksize;
> > +	long long	blocksize = -1;
> >  	uint64_t	nr_records = 0;
> >  	int		report_what = REPORT_DEFAULT;
> >  	int		i, c;
> > @@ -355,7 +388,7 @@ _("The largest block size this command will consider is %u bytes.\n"),
> >  		return 0;
> >  	}
> >  
> > -	if (blocksize < 128) {
> > +	if (blocksize >= 0 && blocksize < 128) {
> >  		fprintf(stderr,
> >  _("The smallest block size this command will consider is 128 bytes.\n"));
> >  		return 0;
> > 
> 
> -- 
> Carlos
> 

[PATCH v2 1/2] xfs_db: btheight should check geometry more carefully

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The btheight command needs to check user-supplied geometry more
carefully so that we don't hit floating point exceptions.

Coverity-id: 1453661, 1453659
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/btheight.c |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 82 insertions(+), 6 deletions(-)

diff --git a/db/btheight.c b/db/btheight.c
index 289e5d84..8aa17c89 100644
--- a/db/btheight.c
+++ b/db/btheight.c
@@ -138,6 +138,10 @@ construct_records_per_block(
 		perror(p);
 		goto out;
 	}
+	if (record_size == 0) {
+		fprintf(stderr, _("%s: record size cannot be zero.\n"), tag);
+		goto out;
+	}
 
 	p = strtok(NULL, ":");
 	if (!p) {
@@ -149,6 +153,10 @@ construct_records_per_block(
 		perror(p);
 		goto out;
 	}
+	if (key_size == 0) {
+		fprintf(stderr, _("%s: key size cannot be zero.\n"), tag);
+		goto out;
+	}
 
 	p = strtok(NULL, ":");
 	if (!p) {
@@ -160,6 +168,10 @@ construct_records_per_block(
 		perror(p);
 		goto out;
 	}
+	if (ptr_size == 0) {
+		fprintf(stderr, _("%s: pointer size cannot be zero.\n"), tag);
+		goto out;
+	}
 
 	p = strtok(NULL, ":");
 	if (!p) {
@@ -180,6 +192,27 @@ construct_records_per_block(
 		goto out;
 	}
 
+	if (record_size > blocksize) {
+		fprintf(stderr,
+_("%s: record size must be less than selected block size (%u bytes).\n"),
+			tag, blocksize);
+		goto out;
+	}
+
+	if (key_size > blocksize) {
+		fprintf(stderr,
+_("%s: key size must be less than selected block size (%u bytes).\n"),
+			tag, blocksize);
+		goto out;
+	}
+
+	if (ptr_size > blocksize) {
+		fprintf(stderr,
+_("%s: pointer size must be less than selected block size (%u bytes).\n"),
+			tag, blocksize);
+		goto out;
+	}
+
 	p = strtok(NULL, ":");
 	if (p) {
 		fprintf(stderr,
@@ -211,13 +244,24 @@ report(
 	int			ret;
 
 	ret = construct_records_per_block(tag, blocksize, records_per_block);
-	if (ret) {
-		printf(_("%s: Unable to determine records per block.\n"),
-				tag);
+	if (ret)
 		return;
-	}
 
 	if (report_what & REPORT_MAX) {
+		if (records_per_block[0] < 2) {
+			fprintf(stderr,
+_("%s: cannot calculate best case scenario due to leaf geometry underflow.\n"),
+				tag);
+			return;
+		}
+
+		if (records_per_block[1] < 4) {
+			fprintf(stderr,
+_("%s: cannot calculate best case scenario due to node geometry underflow.\n"),
+				tag);
+			return;
+		}
+
 		printf(
 _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
 				tag, blocksize, records_per_block[0],
@@ -230,6 +274,20 @@ _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
 		records_per_block[0] /= 2;
 		records_per_block[1] /= 2;
 
+		if (records_per_block[0] < 1) {
+			fprintf(stderr,
+_("%s: cannot calculate worst case scenario due to leaf geometry underflow.\n"),
+				tag);
+			return;
+		}
+
+		if (records_per_block[1] < 2) {
+			fprintf(stderr,
+_("%s: cannot calculate worst case scenario due to node geometry underflow.\n"),
+				tag);
+			return;
+		}
+
 		printf(
 _("%s: worst case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
 				tag, blocksize, records_per_block[0],
@@ -284,8 +342,26 @@ btheight_f(
 		}
 	}
 
-	if (argc == optind || blocksize <= 0 || blocksize > INT_MAX ||
-	    nr_records == 0) {
+	if (nr_records == 0) {
+		fprintf(stderr,
+_("Number of records must be greater than zero.\n"));
+		return 0;
+	}
+
+	if (blocksize > INT_MAX) {
+		fprintf(stderr,
+_("The largest block size this command will consider is %u bytes.\n"),
+			INT_MAX);
+		return 0;
+	}
+
+	if (blocksize < 128) {
+		fprintf(stderr,
+_("The smallest block size this command will consider is 128 bytes.\n"));
+		return 0;
+	}
+
+	if (argc == optind) {
 		btheight_help();
 		return 0;
 	}

[PATCH v2 2/2] xfs_db: calculate iext tree geometry in btheight command

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

(Ab)use the btheight command to calculate the geometry of the incore
extent tree.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/btheight.c |   87 +++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 60 insertions(+), 27 deletions(-)

diff --git a/db/btheight.c b/db/btheight.c
index 8aa17c89..3406ac77 100644
--- a/db/btheight.c
+++ b/db/btheight.c
@@ -22,18 +22,37 @@ static int rmap_maxrecs(struct xfs_mount *mp, int blocklen, int leaf)
 	return libxfs_rmapbt_maxrecs(blocklen, leaf);
 }
 
+static int iext_maxrecs(struct xfs_mount *mp, int blocklen, int leaf)
+{
+	blocklen -= 2 * sizeof(void *);
+
+	return blocklen / sizeof(struct xfs_bmbt_rec);
+}
+
+static int fs_blocksize(struct xfs_mount *mp)
+{
+	return mp->m_sb.sb_blocksize;
+}
+
+static int iext_blocksize(struct xfs_mount *mp)
+{
+	return 256;
+}
+
 struct btmap {
 	const char	*tag;
 	int		(*maxrecs)(struct xfs_mount *mp, int blocklen,
 				   int leaf);
+	int		(*default_blocksize)(struct xfs_mount *mp);
 } maps[] = {
-	{"bnobt", libxfs_allocbt_maxrecs},
-	{"cntbt", libxfs_allocbt_maxrecs},
-	{"inobt", libxfs_inobt_maxrecs},
-	{"finobt", libxfs_inobt_maxrecs},
-	{"bmapbt", libxfs_bmbt_maxrecs},
-	{"refcountbt", refc_maxrecs},
-	{"rmapbt", rmap_maxrecs},
+	{"bnobt", libxfs_allocbt_maxrecs, fs_blocksize},
+	{"cntbt", libxfs_allocbt_maxrecs, fs_blocksize},
+	{"inobt", libxfs_inobt_maxrecs, fs_blocksize},
+	{"finobt", libxfs_inobt_maxrecs, fs_blocksize},
+	{"bmapbt", libxfs_bmbt_maxrecs, fs_blocksize},
+	{"refcountbt", refc_maxrecs, fs_blocksize},
+	{"rmapbt", rmap_maxrecs, fs_blocksize},
+	{"iext", iext_maxrecs, iext_blocksize},
 };
 
 static void
@@ -108,7 +127,7 @@ calc_height(
 static int
 construct_records_per_block(
 	char		*tag,
-	int		blocksize,
+	int		*blocksize,
 	unsigned int	*records_per_block)
 {
 	char		*toktag;
@@ -119,8 +138,10 @@ construct_records_per_block(
 
 	for (i = 0, m = maps; i < ARRAY_SIZE(maps); i++, m++) {
 		if (!strcmp(m->tag, tag)) {
-			records_per_block[0] = m->maxrecs(mp, blocksize, 1);
-			records_per_block[1] = m->maxrecs(mp, blocksize, 0);
+			if (*blocksize < 0)
+				*blocksize = m->default_blocksize(mp);
+			records_per_block[0] = m->maxrecs(mp, *blocksize, 1);
+			records_per_block[1] = m->maxrecs(mp, *blocksize, 0);
 			return 0;
 		}
 	}
@@ -178,38 +199,50 @@ construct_records_per_block(
 		fprintf(stderr, _("%s: header type not found.\n"), tag);
 		goto out;
 	}
-	if (!strcmp(p, "short"))
+	if (!strcmp(p, "short")) {
+		if (*blocksize < 0)
+			*blocksize = mp->m_sb.sb_blocksize;
 		blocksize -= XFS_BTREE_SBLOCK_LEN;
-	else if (!strcmp(p, "shortcrc"))
+	} else if (!strcmp(p, "shortcrc")) {
+		if (*blocksize < 0)
+			*blocksize = mp->m_sb.sb_blocksize;
 		blocksize -= XFS_BTREE_SBLOCK_CRC_LEN;
-	else if (!strcmp(p, "long"))
+	} else if (!strcmp(p, "long")) {
+		if (*blocksize < 0)
+			*blocksize = mp->m_sb.sb_blocksize;
 		blocksize -= XFS_BTREE_LBLOCK_LEN;
-	else if (!strcmp(p, "longcrc"))
+	} else if (!strcmp(p, "longcrc")) {
+		if (*blocksize < 0)
+			*blocksize = mp->m_sb.sb_blocksize;
 		blocksize -= XFS_BTREE_LBLOCK_CRC_LEN;
-	else {
+	} else if (!strcmp(p, "iext")) {
+		if (*blocksize < 0)
+			*blocksize = 256;
+		blocksize -= 2 * sizeof(void *);
+	} else {
 		fprintf(stderr, _("%s: unrecognized btree header type."),
 				p);
 		goto out;
 	}
 
-	if (record_size > blocksize) {
+	if (record_size > *blocksize) {
 		fprintf(stderr,
 _("%s: record size must be less than selected block size (%u bytes).\n"),
-			tag, blocksize);
+			tag, *blocksize);
 		goto out;
 	}
 
-	if (key_size > blocksize) {
+	if (key_size > *blocksize) {
 		fprintf(stderr,
 _("%s: key size must be less than selected block size (%u bytes).\n"),
-			tag, blocksize);
+			tag, *blocksize);
 		goto out;
 	}
 
-	if (ptr_size > blocksize) {
+	if (ptr_size > *blocksize) {
 		fprintf(stderr,
 _("%s: pointer size must be less than selected block size (%u bytes).\n"),
-			tag, blocksize);
+			tag, *blocksize);
 		goto out;
 	}
 
@@ -221,8 +254,8 @@ _("%s: pointer size must be less than selected block size (%u bytes).\n"),
 		goto out;
 	}
 
-	records_per_block[0] = blocksize / record_size;
-	records_per_block[1] = blocksize / (key_size + ptr_size);
+	records_per_block[0] = *blocksize / record_size;
+	records_per_block[1] = *blocksize / (key_size + ptr_size);
 	ret = 0;
 out:
 	free(toktag);
@@ -238,12 +271,12 @@ report(
 	char			*tag,
 	unsigned int		report_what,
 	unsigned long long	nr_records,
-	unsigned int		blocksize)
+	int			blocksize)
 {
 	unsigned int		records_per_block[2];
 	int			ret;
 
-	ret = construct_records_per_block(tag, blocksize, records_per_block);
+	ret = construct_records_per_block(tag, &blocksize, records_per_block);
 	if (ret)
 		return;
 
@@ -302,7 +335,7 @@ btheight_f(
 	int		argc,
 	char		**argv)
 {
-	long long	blocksize = mp->m_sb.sb_blocksize;
+	long long	blocksize = -1;
 	uint64_t	nr_records = 0;
 	int		report_what = REPORT_DEFAULT;
 	int		i, c;
@@ -355,7 +388,7 @@ _("The largest block size this command will consider is %u bytes.\n"),
 		return 0;
 	}
 
-	if (blocksize < 128) {
+	if (blocksize >= 0 && blocksize < 128) {
 		fprintf(stderr,
 _("The smallest block size this command will consider is 128 bytes.\n"));
 		return 0;

Re: [PATCH 2/2] xfs_db: calculate iext tree geometry in btheight command

[Dave Chinner]

On Wed, Sep 25, 2019 at 02:40:59PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> (Ab)use the btheight command to calculate the geometry of the incore
> extent tree.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  db/btheight.c |   87 +++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 60 insertions(+), 27 deletions(-)
> 
> 
> diff --git a/db/btheight.c b/db/btheight.c
> index e2c9759f..be604ebc 100644
> --- a/db/btheight.c
> +++ b/db/btheight.c
> @@ -22,18 +22,37 @@ static int rmap_maxrecs(struct xfs_mount *mp, int blocklen, int leaf)
>  	return libxfs_rmapbt_maxrecs(blocklen, leaf);
>  }
>  
> +static int iext_maxrecs(struct xfs_mount *mp, int blocklen, int leaf)
> +{
> +	blocklen -= 2 * sizeof(void *);
> +
> +	return blocklen / sizeof(struct xfs_bmbt_rec);
> +}

This isn't correct for the iext nodes. They hold 16 key/ptr pairs,
not 15.

I suspect you should be lifting the iext btree format definitions
like this one:

enum {                                                                           
        NODE_SIZE       = 256,                                                   
        KEYS_PER_NODE   = NODE_SIZE / (sizeof(uint64_t) + sizeof(void *)),       
        RECS_PER_LEAF   = (NODE_SIZE - (2 * sizeof(struct xfs_iext_leaf *))) /   
                                sizeof(struct xfs_iext_rec),                     
};                                                                               

from libxfs/xfs_iext_tree.c to a libxfs header file and then using
KEYS_PER_NODE and RECS_PER_LEAF here. See the patch below, lifted
from a varaint of my range locking prototypes...

However, these are not on-disk values and so are subject to change,
hence it may be that a warning might be needed when xfs_db is used
to calculate the height of this tree.

> +static int disk_blocksize(struct xfs_mount *mp)
> +{
> +	return mp->m_sb.sb_blocksize;
> +}
> +
> +static int iext_blocksize(struct xfs_mount *mp)
> +{
> +	return 256;
> +}

NODE_SIZE....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs_db: calculate iext tree geometry in btheight command

[Darrick J. Wong]

On Fri, Sep 27, 2019 at 07:41:02AM +1000, Dave Chinner wrote:
> On Wed, Sep 25, 2019 at 02:40:59PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > (Ab)use the btheight command to calculate the geometry of the incore
> > extent tree.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  db/btheight.c |   87 +++++++++++++++++++++++++++++++++++++++------------------
> >  1 file changed, 60 insertions(+), 27 deletions(-)
> > 
> > 
> > diff --git a/db/btheight.c b/db/btheight.c
> > index e2c9759f..be604ebc 100644
> > --- a/db/btheight.c
> > +++ b/db/btheight.c
> > @@ -22,18 +22,37 @@ static int rmap_maxrecs(struct xfs_mount *mp, int blocklen, int leaf)
> >  	return libxfs_rmapbt_maxrecs(blocklen, leaf);
> >  }
> >  
> > +static int iext_maxrecs(struct xfs_mount *mp, int blocklen, int leaf)
> > +{
> > +	blocklen -= 2 * sizeof(void *);
> > +
> > +	return blocklen / sizeof(struct xfs_bmbt_rec);
> > +}
> 
> This isn't correct for the iext nodes. They hold 16 key/ptr pairs,
> not 15.
> 
> I suspect you should be lifting the iext btree format definitions
> like this one:
> 
> enum {                                                                           
>         NODE_SIZE       = 256,                                                   
>         KEYS_PER_NODE   = NODE_SIZE / (sizeof(uint64_t) + sizeof(void *)),       
>         RECS_PER_LEAF   = (NODE_SIZE - (2 * sizeof(struct xfs_iext_leaf *))) /   
>                                 sizeof(struct xfs_iext_rec),                     
> };                                                                               
> 
> from libxfs/xfs_iext_tree.c to a libxfs header file and then using
> KEYS_PER_NODE and RECS_PER_LEAF here. See the patch below, lifted
> from a varaint of my range locking prototypes...
> 
> However, these are not on-disk values and so are subject to change,
> hence it may be that a warning might be needed when xfs_db is used
> to calculate the height of this tree.

Er... I don't mind lifting the iext values, but I don't see a patch?

--D

> > +static int disk_blocksize(struct xfs_mount *mp)
> > +{
> > +	return mp->m_sb.sb_blocksize;
> > +}
> > +
> > +static int iext_blocksize(struct xfs_mount *mp)
> > +{
> > +	return 256;
> > +}
> 
> NODE_SIZE....
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 1/2] xfs_db: btheight should check geometry more carefully

[Carlos Maiolino]

On Thu, Sep 26, 2019 at 10:38:43AM -0700, Darrick J. Wong wrote:
> On Thu, Sep 26, 2019 at 11:11:48AM +0200, Carlos Maiolino wrote:
> > On Wed, Sep 25, 2019 at 02:40:53PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > The btheight command needs to check user-supplied geometry more
> > > carefully so that we don't hit floating point exceptions.
> > > 
> > > Coverity-id: 1453661, 1453659
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  
> > 
> > Patch looks good, but.
> > 
> > 
> > > +	if (record_size > blocksize) {
> > > +		fprintf(stderr,
> > > +			_("%s: record size must be less than %u bytes.\n"),
> > 
> > 	Couldn't this message maybe be better saying "less than current blocksize"?
> > 	Saying it is less than X bytes sounds kind of meaningless, requiring a
> > 	trip to the code to understand what exactly 'bytes' mean here.
> > 
> > 	Maybe something like:
> > 
> > 	_("%s: record size must be less than current block size (%u).\n"),
> 
> I think I'll change that to 'selected' from 'current' since the caller
> can change the block size with -b, but otherwise I agree.

Sounds good o me too, thanks

> 
> --D
> 
> > 
> > Same for the next two.
> > 
> > > +			tag, blocksize);
> > > +		goto out;
> > > +	}
> > > +
> > > +	if (key_size > blocksize) {
> > > +		fprintf(stderr,
> > > +			_("%s: key size must be less than %u bytes.\n"),
> > > +			tag, blocksize);
> > > +		goto out;
> > > +	}
> > > +
> > > +	if (ptr_size > blocksize) {
> > > +		fprintf(stderr,
> > > +			_("%s: pointer size must be less than %u bytes.\n"),
> > > +			tag, blocksize);
> > > +		goto out;
> > > +	}
> > > +
> > >  	p = strtok(NULL, ":");
> > >  	if (p) {
> > >  		fprintf(stderr,
> > > @@ -211,13 +244,24 @@ report(
> > >  	int			ret;
> > >  
> > >  	ret = construct_records_per_block(tag, blocksize, records_per_block);
> > > -	if (ret) {
> > > -		printf(_("%s: Unable to determine records per block.\n"),
> > > -				tag);
> > > +	if (ret)
> > >  		return;
> > > -	}
> > >  
> > >  	if (report_what & REPORT_MAX) {
> > > +		if (records_per_block[0] < 2) {
> > > +			fprintf(stderr,
> > > +_("%s: cannot calculate best case scenario due to leaf geometry underflow.\n"),
> > > +				tag);
> > > +			return;
> > > +		}
> > > +
> > > +		if (records_per_block[1] < 4) {
> > > +			fprintf(stderr,
> > > +_("%s: cannot calculate best case scenario due to node geometry underflow.\n"),
> > > +				tag);
> > > +			return;
> > > +		}
> > > +
> > >  		printf(
> > >  _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
> > >  				tag, blocksize, records_per_block[0],
> > > @@ -230,6 +274,20 @@ _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
> > >  		records_per_block[0] /= 2;
> > >  		records_per_block[1] /= 2;
> > >  
> > > +		if (records_per_block[0] < 1) {
> > > +			fprintf(stderr,
> > > +_("%s: cannot calculate worst case scenario due to leaf geometry underflow.\n"),
> > > +				tag);
> > > +			return;
> > > +		}
> > > +
> > > +		if (records_per_block[1] < 2) {
> > > +			fprintf(stderr,
> > > +_("%s: cannot calculate worst case scenario due to node geometry underflow.\n"),
> > > +				tag);
> > > +			return;
> > > +		}
> > > +
> > >  		printf(
> > >  _("%s: worst case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
> > >  				tag, blocksize, records_per_block[0],
> > > @@ -284,8 +342,26 @@ btheight_f(
> > >  		}
> > >  	}
> > >  
> > > -	if (argc == optind || blocksize <= 0 || blocksize > INT_MAX ||
> > > -	    nr_records == 0) {
> > > +	if (nr_records == 0) {
> > > +		fprintf(stderr,
> > > +_("Number of records must be greater than zero.\n"));
> > > +		return 0;
> > > +	}
> > > +
> > > +	if (blocksize > INT_MAX) {
> > > +		fprintf(stderr,
> > > +_("The largest block size this command will consider is %u bytes.\n"),
> > > +			INT_MAX);
> > > +		return 0;
> > > +	}
> > > +
> > > +	if (blocksize < 128) {
> > > +		fprintf(stderr,
> > > +_("The smallest block size this command will consider is 128 bytes.\n"));
> > > +		return 0;
> > > +	}
> > > +
> > > +	if (argc == optind) {
> > >  		btheight_help();
> > >  		return 0;
> > >  	}
> > > 
> > 
> > -- 
> > Carlos
> > 

-- 
Carlos

Re: [PATCH 2/2] xfs_db: calculate iext tree geometry in btheight command

[Dave Chinner]

On Thu, Sep 26, 2019 at 07:58:57PM -0700, Darrick J. Wong wrote:
> On Fri, Sep 27, 2019 at 07:41:02AM +1000, Dave Chinner wrote:
> > On Wed, Sep 25, 2019 at 02:40:59PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > (Ab)use the btheight command to calculate the geometry of the incore
> > > extent tree.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  db/btheight.c |   87 +++++++++++++++++++++++++++++++++++++++------------------
> > >  1 file changed, 60 insertions(+), 27 deletions(-)
> > > 
> > > 
> > > diff --git a/db/btheight.c b/db/btheight.c
> > > index e2c9759f..be604ebc 100644
> > > --- a/db/btheight.c
> > > +++ b/db/btheight.c
> > > @@ -22,18 +22,37 @@ static int rmap_maxrecs(struct xfs_mount *mp, int blocklen, int leaf)
> > >  	return libxfs_rmapbt_maxrecs(blocklen, leaf);
> > >  }
> > >  
> > > +static int iext_maxrecs(struct xfs_mount *mp, int blocklen, int leaf)
> > > +{
> > > +	blocklen -= 2 * sizeof(void *);
> > > +
> > > +	return blocklen / sizeof(struct xfs_bmbt_rec);
> > > +}
> > 
> > This isn't correct for the iext nodes. They hold 16 key/ptr pairs,
> > not 15.
> > 
> > I suspect you should be lifting the iext btree format definitions
> > like this one:
> > 
> > enum {                                                                           
> >         NODE_SIZE       = 256,                                                   
> >         KEYS_PER_NODE   = NODE_SIZE / (sizeof(uint64_t) + sizeof(void *)),       
> >         RECS_PER_LEAF   = (NODE_SIZE - (2 * sizeof(struct xfs_iext_leaf *))) /   
> >                                 sizeof(struct xfs_iext_rec),                     
> > };                                                                               
> > 
> > from libxfs/xfs_iext_tree.c to a libxfs header file and then using
> > KEYS_PER_NODE and RECS_PER_LEAF here. See the patch below, lifted
> > from a varaint of my range locking prototypes...
> > 
> > However, these are not on-disk values and so are subject to change,
> > hence it may be that a warning might be needed when xfs_db is used
> > to calculate the height of this tree.
> 
> Er... I don't mind lifting the iext values, but I don't see a patch?

Ah, sorry, I realised it wouldn't apply at all - there's several
precursor patches that move stuff about (reworking the xfs_ifork
structure) and it didn't lift the enum (but could have) and so I
didn't attach it.  Of course, I then forgot to edit email... I'll
attach it anyway...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com


xfs: move iext tree interfaces to separate header file

From: Dave Chinner <dchinner@redhat.com>

The iext tree is sufficiently separated from the inode fork now
that it really needs it's own header file. This will make it easier
to add new functionality that uses the tree without needing to be
intermingled with the existing inode header files.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_bmap.h       |  1 +
 fs/xfs/libxfs/xfs_iext_tree.c  | 64 ++++++++++++++-------------------------
 fs/xfs/libxfs/xfs_iext_tree.h  | 69 ++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_inode_fork.h | 22 --------------
 fs/xfs/libxfs/xfs_types.h      | 16 ----------
 fs/xfs/xfs_inode.h             |  1 +
 fs/xfs/xfs_trace.h             |  1 +
 7 files changed, 94 insertions(+), 80 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index 302a6079a038..3989ecca4b78 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -13,6 +13,7 @@ struct xfs_inode;
 struct xfs_mount;
 struct xfs_trans;
 struct xfs_btree_cur;
+struct xfs_iext_cursor;
 
 extern kmem_zone_t	*xfs_bmap_free_item_zone;
 
diff --git a/fs/xfs/libxfs/xfs_iext_tree.c b/fs/xfs/libxfs/xfs_iext_tree.c
index eeddb0a1e194..5b60b92b50ef 100644
--- a/fs/xfs/libxfs/xfs_iext_tree.c
+++ b/fs/xfs/libxfs/xfs_iext_tree.c
@@ -17,48 +17,6 @@
 #include "xfs_bmap.h"
 #include "xfs_trace.h"
 
-/*
- * Extent tree offset/length record layout:
- *
- * +-------+----------------------------+
- * | 00:53 | all 54 bits of startoff	|
- * | 54:63 | User defined		|
- * +-------+----------------------------+
- * | 00:20 | all 21 bits of length      |
- * | 21:63 | User defined		|
- * +-------+----------------------------+
- *
- * This is derived from the on-disk packing for BMBT records, so the inode
- * extent tree can be efficiently dumped into the in-core extent tree. All users
- * of this tree must use this same offset/length layout as it is used for
- * lookups into the tree. Users can use whatever units they want for offset/len
- * as long as they fit within the sizes noted above.
- */
-#define XFS_IEXT_STARTOFF_MASK		xfs_mask64lo(BMBT_STARTOFF_BITLEN)
-#define XFS_IEXT_LENGTH_MASK		xfs_mask64lo(BMBT_BLOCKCOUNT_BITLEN)
-
-/*
- * Given that the length can't be a zero, only an empty hi value indicates an
- * unused record.
- */
-static bool xfs_iext_rec_is_empty(struct xfs_iext_rec *rec)
-{
-	return rec->hi == 0;
-}
-
-static inline void xfs_iext_rec_clear(struct xfs_iext_rec *rec)
-{
-	rec->lo = 0;
-	rec->hi = 0;
-}
-
-enum {
-	NODE_SIZE	= 256,
-	KEYS_PER_NODE	= NODE_SIZE / (sizeof(uint64_t) + sizeof(void *)),
-	RECS_PER_LEAF	= (NODE_SIZE - (2 * sizeof(struct xfs_iext_leaf *))) /
-				sizeof(struct xfs_iext_rec),
-};
-
 /*
  * In-core extent btree block layout:
  *
@@ -80,6 +38,13 @@ enum {
  * Inner:	| key 1 | key 2 | key 3 | key N | ptr 1 | ptr 2 | ptr3 | ptr N |
  *		+-------+-------+-------+-------+-------+-------+------+-------+
  */
+enum {
+	NODE_SIZE	= 256,
+	KEYS_PER_NODE	= NODE_SIZE / (sizeof(uint64_t) + sizeof(void *)),
+	RECS_PER_LEAF	= (NODE_SIZE - (2 * sizeof(struct xfs_iext_leaf *))) /
+				sizeof(struct xfs_iext_rec),
+};
+
 struct xfs_iext_node {
 	uint64_t		keys[KEYS_PER_NODE];
 #define XFS_IEXT_KEY_INVALID	(1ULL << 63)
@@ -92,6 +57,21 @@ struct xfs_iext_leaf {
 	struct xfs_iext_leaf	*next;
 };
 
+/*
+ * Given that the length can't be a zero, only an empty hi value indicates an
+ * unused record.
+ */
+static bool xfs_iext_rec_is_empty(struct xfs_iext_rec *rec)
+{
+	return rec->hi == 0;
+}
+
+static inline void xfs_iext_rec_clear(struct xfs_iext_rec *rec)
+{
+	rec->lo = 0;
+	rec->hi = 0;
+}
+
 static inline int xfs_iext_max_recs(struct xfs_iext_tree *tree)
 {
 	if (tree->height == 1)
diff --git a/fs/xfs/libxfs/xfs_iext_tree.h b/fs/xfs/libxfs/xfs_iext_tree.h
new file mode 100644
index 000000000000..c1756be83f09
--- /dev/null
+++ b/fs/xfs/libxfs/xfs_iext_tree.h
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2017 Christoph Hellwig.
+ * Copyright (c) 2019 Red Hat, Inc.
+ * All Rights Reserved.
+ */
+
+#ifndef __XFS_IEXT_TREE_H__
+#define	__XFS_IEXT_TREE_H__
+
+struct xfs_iext_leaf;
+
+/*
+ * Extent tree offset/length record layout:
+ *
+ * +-------+----------------------------+
+ * | 00:53 | all 54 bits of startoff	|
+ * | 54:63 | User defined		|
+ * +-------+----------------------------+
+ * | 00:20 | all 21 bits of length      |
+ * | 21:63 | User defined		|
+ * +-------+----------------------------+
+ *
+ * This is derived from the on-disk packing for BMBT records, so the inode
+ * extent tree can be efficiently dumped into the in-core extent tree. All users
+ * of this tree must use this same offset/length layout as it is used for
+ * lookups into the tree. Users can use whatever units they want for offset/len
+ * as long as they fit within the sizes noted above.
+ */
+#define XFS_IEXT_STARTOFF_MASK		xfs_mask64lo(BMBT_STARTOFF_BITLEN)
+#define XFS_IEXT_LENGTH_MASK		xfs_mask64lo(BMBT_BLOCKCOUNT_BITLEN)
+
+struct xfs_iext_rec {
+	uint64_t			lo;
+	uint64_t			hi;
+};
+
+struct xfs_iext_tree {
+	void	*root;		/* tree root block */
+	int	height;		/* tree height */
+	int	root_recs;	/* root block size when height = 1 */
+};
+
+
+struct xfs_iext_cursor {
+	struct xfs_iext_leaf	*leaf;
+	int			pos;
+};
+
+struct xfs_iext_rec *xfs_iext_cur_rec(struct xfs_iext_cursor *cur);
+void	xfs_iext_first_rec(struct xfs_iext_tree *tree,
+				struct xfs_iext_cursor *cur);
+void	xfs_iext_last_rec(struct xfs_iext_tree *tree,
+				struct xfs_iext_cursor *cur);
+void	xfs_iext_next_rec(struct xfs_iext_tree *tree,
+				struct xfs_iext_cursor *cur);
+void	xfs_iext_prev_rec(struct xfs_iext_tree *tree,
+				struct xfs_iext_cursor *cur);
+
+typedef void (*irec_fmt_f)(struct xfs_iext_rec *rec, void *irec);
+void	xfs_iext_insert_rec(struct xfs_iext_tree *tree,
+				struct xfs_iext_cursor *cur, uint64_t offset,
+				irec_fmt_f format_f, void *irec);
+void	xfs_iext_remove_rec(struct xfs_iext_tree *tree,
+				struct xfs_iext_cursor *cur);
+bool	xfs_iext_lookup_rec(struct xfs_iext_tree *tree,
+				struct xfs_iext_cursor *cur, uint64_t offset);
+
+#endif	/* __XFS_IEXT_TREE_H__ */
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 0e81a9234b99..c1910c116e48 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -127,28 +127,6 @@ void		xfs_iext_update_extent(struct xfs_inode *ip, int state,
 			struct xfs_iext_cursor *cur,
 			struct xfs_bmbt_irec *gotp);
 
-void	xfs_iext_first_rec(struct xfs_iext_tree *tree,
-				struct xfs_iext_cursor *cur);
-void	xfs_iext_last_rec(struct xfs_iext_tree *tree,
-				struct xfs_iext_cursor *cur);
-void	xfs_iext_next_rec(struct xfs_iext_tree *tree,
-				struct xfs_iext_cursor *cur);
-void	xfs_iext_prev_rec(struct xfs_iext_tree *tree,
-				struct xfs_iext_cursor *cur);
-
-struct xfs_iext_rec *xfs_iext_cur_rec(struct xfs_iext_cursor *cur);
-
-struct xfs_iext_rec;
-typedef void (*irec_fmt_f)(struct xfs_iext_rec *rec, void *irec);
-
-void	xfs_iext_insert_rec(struct xfs_iext_tree *tree,
-				struct xfs_iext_cursor *cur, uint64_t offset,
-				irec_fmt_f format_f, void *irec);
-void	xfs_iext_remove_rec(struct xfs_iext_tree *tree,
-				struct xfs_iext_cursor *cur);
-bool	xfs_iext_lookup_rec(struct xfs_iext_tree *tree,
-				struct xfs_iext_cursor *cur, uint64_t offset);
-
 static inline void
 xfs_iext_first(struct xfs_ifork *ifp, struct xfs_iext_cursor *cur)
 {
diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
index c0d6d8ab63fe..dac3b0a35d08 100644
--- a/fs/xfs/libxfs/xfs_types.h
+++ b/fs/xfs/libxfs/xfs_types.h
@@ -152,22 +152,6 @@ typedef uint32_t	xfs_dqid_t;
 #define	XFS_NBWORD	(1 << XFS_NBWORDLOG)
 #define	XFS_WORDMASK	((1 << XFS_WORDLOG) - 1)
 
-struct xfs_iext_tree {
-	void	*root;		/* tree root block */
-	int	height;		/* tree height */
-	int	root_recs;	/* root block size when height = 1 */
-};
-
-struct xfs_iext_cursor {
-	struct xfs_iext_leaf	*leaf;
-	int			pos;
-};
-
-struct xfs_iext_rec {
-	uint64_t		lo;
-	uint64_t		hi;
-};
-
 typedef enum {
 	XFS_EXT_NORM, XFS_EXT_UNWRITTEN,
 } xfs_exntst_t;
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index e62074a5257c..967c73e8292b 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -6,6 +6,7 @@
 #ifndef	__XFS_INODE_H__
 #define	__XFS_INODE_H__
 
+#include "xfs_iext_tree.h"
 #include "xfs_inode_buf.h"
 #include "xfs_inode_fork.h"
 
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 47fb07d86efd..3a03e289fbe0 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -30,6 +30,7 @@ struct xfs_btree_cur;
 struct xfs_refcount_irec;
 struct xfs_fsmap;
 struct xfs_rmap_irec;
+struct xfs_iext_cursor;
 
 DECLARE_EVENT_CLASS(xfs_attr_list_class,
 	TP_PROTO(struct xfs_attr_list_context *ctx),

Re: [PATCH 1/2] xfs_db: btheight should check geometry more carefully

[Christoph Hellwig]

Looks good,

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

Re: [PATCH 2/2] xfs_db: calculate iext tree geometry in btheight command

[Christoph Hellwig]

On Fri, Sep 27, 2019 at 07:41:02AM +1000, Dave Chinner wrote:
> > +static int iext_maxrecs(struct xfs_mount *mp, int blocklen, int leaf)
> > +{
> > +	blocklen -= 2 * sizeof(void *);
> > +
> > +	return blocklen / sizeof(struct xfs_bmbt_rec);
> > +}
> 
> This isn't correct for the iext nodes. They hold 16 key/ptr pairs,
> not 15.
> 
> I suspect you should be lifting the iext btree format definitions
> like this one:

Is the command supposed to deal with the on-disk or in-memory nodes?
The ones your quote are the in-memory btrees, but the file seems
(the way I read it, the documentation seems to be lacking) with the
on-disk btrees.

Re: [PATCH 2/2] xfs_db: calculate iext tree geometry in btheight command

[Darrick J. Wong]

On Mon, Sep 30, 2019 at 12:58:54AM -0700, Christoph Hellwig wrote:
> On Fri, Sep 27, 2019 at 07:41:02AM +1000, Dave Chinner wrote:
> > > +static int iext_maxrecs(struct xfs_mount *mp, int blocklen, int leaf)
> > > +{
> > > +	blocklen -= 2 * sizeof(void *);
> > > +
> > > +	return blocklen / sizeof(struct xfs_bmbt_rec);
> > > +}
> > 
> > This isn't correct for the iext nodes. They hold 16 key/ptr pairs,
> > not 15.
> > 
> > I suspect you should be lifting the iext btree format definitions
> > like this one:
> 
> Is the command supposed to deal with the on-disk or in-memory nodes?
> The ones your quote are the in-memory btrees, but the file seems
> (the way I read it, the documentation seems to be lacking) with the
> on-disk btrees.

It started as a command for calculating ondisk btree geometry but then
we were discussing the iext tree geometry on irc so I extended it to
handle that too.

(Ugh, where /did/ the documentation go...)

--D

Re: [PATCH 2/2] xfs_db: calculate iext tree geometry in btheight command

[Christoph Hellwig]

On Mon, Sep 30, 2019 at 09:11:52AM -0700, Darrick J. Wong wrote:
> > Is the command supposed to deal with the on-disk or in-memory nodes?
> > The ones your quote are the in-memory btrees, but the file seems
> > (the way I read it, the documentation seems to be lacking) with the
> > on-disk btrees.
> 
> It started as a command for calculating ondisk btree geometry but then
> we were discussing the iext tree geometry on irc so I extended it to
> handle that too.

I think mixing the on-disk trees an an in-memory structure in the same
command is a really bad idea.  In fact I'm not sure what use the
calculation for the iext tree is.  It is an implementation detail that
can (and did) change from release to release, unlike the other trees
that not going to change without an on-disk format change.

Re: [PATCH v2 1/2] xfs_db: btheight should check geometry more carefully

[Carlos Maiolino]

On Thu, Sep 26, 2019 at 12:09:11PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The btheight command needs to check user-supplied geometry more
> carefully so that we don't hit floating point exceptions.
> 
> Coverity-id: 1453661, 1453659
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good to me

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> ---
>  db/btheight.c |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 82 insertions(+), 6 deletions(-)
> 
> diff --git a/db/btheight.c b/db/btheight.c
> index 289e5d84..8aa17c89 100644
> --- a/db/btheight.c
> +++ b/db/btheight.c
> @@ -138,6 +138,10 @@ construct_records_per_block(
>  		perror(p);
>  		goto out;
>  	}
> +	if (record_size == 0) {
> +		fprintf(stderr, _("%s: record size cannot be zero.\n"), tag);
> +		goto out;
> +	}
>  
>  	p = strtok(NULL, ":");
>  	if (!p) {
> @@ -149,6 +153,10 @@ construct_records_per_block(
>  		perror(p);
>  		goto out;
>  	}
> +	if (key_size == 0) {
> +		fprintf(stderr, _("%s: key size cannot be zero.\n"), tag);
> +		goto out;
> +	}
>  
>  	p = strtok(NULL, ":");
>  	if (!p) {
> @@ -160,6 +168,10 @@ construct_records_per_block(
>  		perror(p);
>  		goto out;
>  	}
> +	if (ptr_size == 0) {
> +		fprintf(stderr, _("%s: pointer size cannot be zero.\n"), tag);
> +		goto out;
> +	}
>  
>  	p = strtok(NULL, ":");
>  	if (!p) {
> @@ -180,6 +192,27 @@ construct_records_per_block(
>  		goto out;
>  	}
>  
> +	if (record_size > blocksize) {
> +		fprintf(stderr,
> +_("%s: record size must be less than selected block size (%u bytes).\n"),
> +			tag, blocksize);
> +		goto out;
> +	}
> +
> +	if (key_size > blocksize) {
> +		fprintf(stderr,
> +_("%s: key size must be less than selected block size (%u bytes).\n"),
> +			tag, blocksize);
> +		goto out;
> +	}
> +
> +	if (ptr_size > blocksize) {
> +		fprintf(stderr,
> +_("%s: pointer size must be less than selected block size (%u bytes).\n"),
> +			tag, blocksize);
> +		goto out;
> +	}
> +
>  	p = strtok(NULL, ":");
>  	if (p) {
>  		fprintf(stderr,
> @@ -211,13 +244,24 @@ report(
>  	int			ret;
>  
>  	ret = construct_records_per_block(tag, blocksize, records_per_block);
> -	if (ret) {
> -		printf(_("%s: Unable to determine records per block.\n"),
> -				tag);
> +	if (ret)
>  		return;
> -	}
>  
>  	if (report_what & REPORT_MAX) {
> +		if (records_per_block[0] < 2) {
> +			fprintf(stderr,
> +_("%s: cannot calculate best case scenario due to leaf geometry underflow.\n"),
> +				tag);
> +			return;
> +		}
> +
> +		if (records_per_block[1] < 4) {
> +			fprintf(stderr,
> +_("%s: cannot calculate best case scenario due to node geometry underflow.\n"),
> +				tag);
> +			return;
> +		}
> +
>  		printf(
>  _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
>  				tag, blocksize, records_per_block[0],
> @@ -230,6 +274,20 @@ _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
>  		records_per_block[0] /= 2;
>  		records_per_block[1] /= 2;
>  
> +		if (records_per_block[0] < 1) {
> +			fprintf(stderr,
> +_("%s: cannot calculate worst case scenario due to leaf geometry underflow.\n"),
> +				tag);
> +			return;
> +		}
> +
> +		if (records_per_block[1] < 2) {
> +			fprintf(stderr,
> +_("%s: cannot calculate worst case scenario due to node geometry underflow.\n"),
> +				tag);
> +			return;
> +		}
> +
>  		printf(
>  _("%s: worst case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
>  				tag, blocksize, records_per_block[0],
> @@ -284,8 +342,26 @@ btheight_f(
>  		}
>  	}
>  
> -	if (argc == optind || blocksize <= 0 || blocksize > INT_MAX ||
> -	    nr_records == 0) {
> +	if (nr_records == 0) {
> +		fprintf(stderr,
> +_("Number of records must be greater than zero.\n"));
> +		return 0;
> +	}
> +
> +	if (blocksize > INT_MAX) {
> +		fprintf(stderr,
> +_("The largest block size this command will consider is %u bytes.\n"),
> +			INT_MAX);
> +		return 0;
> +	}
> +
> +	if (blocksize < 128) {
> +		fprintf(stderr,
> +_("The smallest block size this command will consider is 128 bytes.\n"));
> +		return 0;
> +	}
> +
> +	if (argc == optind) {
>  		btheight_help();
>  		return 0;
>  	}

-- 
Carlos