[PATCH V4 RESEND 0/7] Fix log reservation calculation for xattr insert operation

[PATCH V4 RESEND 0/7] Fix log reservation calculation for xattr insert operation

[Chandan Rajendra]

Log space reservation for xattr insert operation can be divided into two
parts,
1. Mount time
   - Inode
   - Superblock for accounting space allocations
   - AGF for accounting space used by count, block number, rmap and refcnt
     btrees.

2. The remaining log space can only be calculated at run time because,
   - A local xattr can be large enough to cause a double split of the dabtree.
   - The value of the xattr can be large enough to be stored in remote
     blocks. The contents of the remote blocks are not logged.

   The log space reservation could be,
   - 2 * XFS_DA_NODE_MAXDEPTH number of blocks. Additional XFS_DA_NODE_MAXDEPTH
     number of blocks are required if xattr is large enough to cause another
     split of the dabtree path from root to leaf block.
   - BMBT blocks for storing (2 * XFS_DA_NODE_MAXDEPTH) record
     entries. Additional XFS_DA_NODE_MAXDEPTH number of blocks are required in
     case of a double split of the dabtree path from root to leaf blocks.
   - Space for logging blocks of count, block number, rmap and refcnt btrees.

At present mount time log reservation includes block count required for a
single split of the dabtree. The dabtree block count is also taken into
account by xfs_attr_calc_size().

Also, AGF log space reservation isn't accounted for.

Due to the reasons mentioned above, log reservation calculation for xattr
insert operation gives an incorrect value.

Apart from the above, xfs_log_calc_max_attrsetm_res() passes byte count as
an argument to XFS_NEXTENTADD_SPACE_RES() instead of block count.

This patchset aims to fix this log space reservation calcuation bug.

Patches 1-2 and 4-6 refactor the existing code around
xfs_attr_calc_size(). Patches 3 and 7 change the logic to fix log
reservation calculation.

The patchset can also be obtained from
https://github.com/chandanr/linux/tree/xfs-fix-attr-resv-calc-v4.

Changelog:
V1 -> V2:
1. Use convenience variables to reduce indentation of code.

V2 -> V3:
1. Introduce 'struct xfs_attr_set_resv' to collect various block size
   reservations when inserting an xattr.
2. Add xfs_calc_attr_res() to calculate the total log reservation to
   required when inserting an xattr.

V3 -> V4:
1. Rebase the patchset on top of Christoph's "Clean attr interface"
   patchset.
2. Split the patchset into
   - Patches which refactor the existing calculation in
     xfs_attr_calc_size().
   - One patch which fixes the calculation inside
     xfs_attr_calc_size().
3. Fix indentation issues.
4. Pass attribute geometry pointer to xfs_attr_leaf_newentsize()
   instead of a pointer to 'struct xfs_mount'.

Chandan Rajendra (7):
  xfs: Pass xattr name and value length explicitly to
    xfs_attr_leaf_newentsize
  xfs: xfs_attr_calc_size: Use local variables to track individual space
    components
  xfs: xfs_attr_calc_size: Calculate Bmbt blks only once
  xfs: Introduce struct xfs_attr_set_resv
  xfs: xfs_attr_calc_size: Explicitly pass mp, namelen and valuelen args
  xfs: Make xfs_attr_calc_size() non-static
  xfs: Fix log reservation calculation for xattr insert operation

 fs/xfs/libxfs/xfs_attr.c       | 107 ++++++++++++++++++---------------
 fs/xfs/libxfs/xfs_attr.h       |  18 ++++++
 fs/xfs/libxfs/xfs_attr_leaf.c  |  39 ++++++++----
 fs/xfs/libxfs/xfs_attr_leaf.h  |   3 +-
 fs/xfs/libxfs/xfs_log_rlimit.c |  16 ++---
 fs/xfs/libxfs/xfs_trans_resv.c |  50 ++++++++-------
 fs/xfs/libxfs/xfs_trans_resv.h |   2 +
 7 files changed, 140 insertions(+), 95 deletions(-)

-- 
2.19.1

[PATCH V4 RESEND 1/7] xfs: Pass xattr name and value length explicitly to xfs_attr_leaf_newentsize

[Chandan Rajendra]

This commit changes xfs_attr_leaf_newentsize() to explicitly accept name and
value length instead of a pointer to struct xfs_da_args. A future commit will
need to invoke xfs_attr_leaf_newentsize() from functions that do not have
a struct xfs_da_args to pass in.

Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_attr.c      |  3 ++-
 fs/xfs/libxfs/xfs_attr_leaf.c | 39 +++++++++++++++++++++++------------
 fs/xfs/libxfs/xfs_attr_leaf.h |  3 ++-
 3 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 23e0d8ce39f8c..1875210cc8e40 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -149,7 +149,8 @@ xfs_attr_calc_size(
 	 * Determine space new attribute will use, and if it would be
 	 * "local" or "remote" (note: local != inline).
 	 */
-	size = xfs_attr_leaf_newentsize(args, local);
+	size = xfs_attr_leaf_newentsize(args->geo, args->namelen,
+			args->valuelen, local);
 	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
 	if (*local) {
 		if (size > (args->geo->blksize / 2)) {
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index fae322105457a..65a3bf40c4f9d 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -1330,7 +1330,8 @@ xfs_attr3_leaf_add(
 	leaf = bp->b_addr;
 	xfs_attr3_leaf_hdr_from_disk(args->geo, &ichdr, leaf);
 	ASSERT(args->index >= 0 && args->index <= ichdr.count);
-	entsize = xfs_attr_leaf_newentsize(args, NULL);
+	entsize = xfs_attr_leaf_newentsize(args->geo, args->namelen,
+			args->valuelen, NULL);
 
 	/*
 	 * Search through freemap for first-fit on new name length.
@@ -1403,6 +1404,7 @@ xfs_attr3_leaf_add_work(
 	struct xfs_attr_leaf_name_local *name_loc;
 	struct xfs_attr_leaf_name_remote *name_rmt;
 	struct xfs_mount	*mp;
+	int			entsize;
 	int			tmp;
 	int			i;
 
@@ -1432,11 +1434,14 @@ xfs_attr3_leaf_add_work(
 	ASSERT(ichdr->freemap[mapindex].base < args->geo->blksize);
 	ASSERT((ichdr->freemap[mapindex].base & 0x3) == 0);
 	ASSERT(ichdr->freemap[mapindex].size >=
-		xfs_attr_leaf_newentsize(args, NULL));
+		xfs_attr_leaf_newentsize(args->geo, args->namelen,
+				args->valuelen, NULL));
 	ASSERT(ichdr->freemap[mapindex].size < args->geo->blksize);
 	ASSERT((ichdr->freemap[mapindex].size & 0x3) == 0);
 
-	ichdr->freemap[mapindex].size -= xfs_attr_leaf_newentsize(args, &tmp);
+	entsize = xfs_attr_leaf_newentsize(args->geo, args->namelen,
+			args->valuelen, &tmp);
+	ichdr->freemap[mapindex].size -= entsize;
 
 	entry->nameidx = cpu_to_be16(ichdr->freemap[mapindex].base +
 				     ichdr->freemap[mapindex].size);
@@ -1824,6 +1829,8 @@ xfs_attr3_leaf_figure_balance(
 	struct xfs_attr_leafblock	*leaf1 = blk1->bp->b_addr;
 	struct xfs_attr_leafblock	*leaf2 = blk2->bp->b_addr;
 	struct xfs_attr_leaf_entry	*entry;
+	struct xfs_da_args		*args;
+	int				entsize;
 	int				count;
 	int				max;
 	int				index;
@@ -1833,14 +1840,16 @@ xfs_attr3_leaf_figure_balance(
 	int				foundit = 0;
 	int				tmp;
 
+	args = state->args;
 	/*
 	 * Examine entries until we reduce the absolute difference in
 	 * byte usage between the two blocks to a minimum.
 	 */
 	max = ichdr1->count + ichdr2->count;
 	half = (max + 1) * sizeof(*entry);
-	half += ichdr1->usedbytes + ichdr2->usedbytes +
-			xfs_attr_leaf_newentsize(state->args, NULL);
+	entsize = xfs_attr_leaf_newentsize(args->geo, args->namelen,
+			args->valuelen, NULL);
+	half += ichdr1->usedbytes + ichdr2->usedbytes + entsize;
 	half /= 2;
 	lastdelta = state->args->geo->blksize;
 	entry = xfs_attr3_leaf_entryp(leaf1);
@@ -1851,8 +1860,9 @@ xfs_attr3_leaf_figure_balance(
 		 * The new entry is in the first block, account for it.
 		 */
 		if (count == blk1->index) {
-			tmp = totallen + sizeof(*entry) +
-				xfs_attr_leaf_newentsize(state->args, NULL);
+			entsize = xfs_attr_leaf_newentsize(args->geo,
+					args->namelen, args->valuelen, NULL);
+			tmp = totallen + sizeof(*entry) + entsize;
 			if (XFS_ATTR_ABS(half - tmp) > lastdelta)
 				break;
 			lastdelta = XFS_ATTR_ABS(half - tmp);
@@ -1887,8 +1897,9 @@ xfs_attr3_leaf_figure_balance(
 	 */
 	totallen -= count * sizeof(*entry);
 	if (foundit) {
-		totallen -= sizeof(*entry) +
-				xfs_attr_leaf_newentsize(state->args, NULL);
+		entsize = xfs_attr_leaf_newentsize(args->geo, args->namelen,
+				args->valuelen, NULL);
+		totallen -= sizeof(*entry) + entsize;
 	}
 
 	*countarg = count;
@@ -2664,20 +2675,22 @@ xfs_attr_leaf_entsize(xfs_attr_leafblock_t *leaf, int index)
  */
 int
 xfs_attr_leaf_newentsize(
-	struct xfs_da_args	*args,
+	struct xfs_da_geometry	*geo,
+	int			namelen,
+	int			valuelen,
 	int			*local)
 {
 	int			size;
 
-	size = xfs_attr_leaf_entsize_local(args->namelen, args->valuelen);
-	if (size < xfs_attr_leaf_entsize_local_max(args->geo->blksize)) {
+	size = xfs_attr_leaf_entsize_local(namelen, valuelen);
+	if (size < xfs_attr_leaf_entsize_local_max(geo->blksize)) {
 		if (local)
 			*local = 1;
 		return size;
 	}
 	if (local)
 		*local = 0;
-	return xfs_attr_leaf_entsize_remote(args->namelen);
+	return xfs_attr_leaf_entsize_remote(namelen);
 }
 
 
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.h b/fs/xfs/libxfs/xfs_attr_leaf.h
index 6dd2d937a42a3..7bc5dd6c4d66a 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.h
+++ b/fs/xfs/libxfs/xfs_attr_leaf.h
@@ -96,7 +96,8 @@ void	xfs_attr3_leaf_unbalance(struct xfs_da_state *state,
 xfs_dahash_t	xfs_attr_leaf_lasthash(struct xfs_buf *bp, int *count);
 int	xfs_attr_leaf_order(struct xfs_buf *leaf1_bp,
 				   struct xfs_buf *leaf2_bp);
-int	xfs_attr_leaf_newentsize(struct xfs_da_args *args, int *local);
+int	xfs_attr_leaf_newentsize(struct xfs_da_geometry	*geo, int namelen,
+			int valuelen, int *local);
 int	xfs_attr3_leaf_read(struct xfs_trans *tp, struct xfs_inode *dp,
 			xfs_dablk_t bno, struct xfs_buf **bpp);
 void	xfs_attr3_leaf_hdr_from_disk(struct xfs_da_geometry *geo,
-- 
2.19.1

Re: [PATCH V4 RESEND 1/7] xfs: Pass xattr name and value length explicitly to xfs_attr_leaf_newentsize

[Brian Foster]

On Mon, Feb 24, 2020 at 09:30:38AM +0530, Chandan Rajendra wrote:
> This commit changes xfs_attr_leaf_newentsize() to explicitly accept name and
> value length instead of a pointer to struct xfs_da_args. A future commit will
> need to invoke xfs_attr_leaf_newentsize() from functions that do not have
> a struct xfs_da_args to pass in.
> 
> Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> ---

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

>  fs/xfs/libxfs/xfs_attr.c      |  3 ++-
>  fs/xfs/libxfs/xfs_attr_leaf.c | 39 +++++++++++++++++++++++------------
>  fs/xfs/libxfs/xfs_attr_leaf.h |  3 ++-
>  3 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 23e0d8ce39f8c..1875210cc8e40 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -149,7 +149,8 @@ xfs_attr_calc_size(
>  	 * Determine space new attribute will use, and if it would be
>  	 * "local" or "remote" (note: local != inline).
>  	 */
> -	size = xfs_attr_leaf_newentsize(args, local);
> +	size = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> +			args->valuelen, local);
>  	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
>  	if (*local) {
>  		if (size > (args->geo->blksize / 2)) {
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index fae322105457a..65a3bf40c4f9d 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -1330,7 +1330,8 @@ xfs_attr3_leaf_add(
>  	leaf = bp->b_addr;
>  	xfs_attr3_leaf_hdr_from_disk(args->geo, &ichdr, leaf);
>  	ASSERT(args->index >= 0 && args->index <= ichdr.count);
> -	entsize = xfs_attr_leaf_newentsize(args, NULL);
> +	entsize = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> +			args->valuelen, NULL);
>  
>  	/*
>  	 * Search through freemap for first-fit on new name length.
> @@ -1403,6 +1404,7 @@ xfs_attr3_leaf_add_work(
>  	struct xfs_attr_leaf_name_local *name_loc;
>  	struct xfs_attr_leaf_name_remote *name_rmt;
>  	struct xfs_mount	*mp;
> +	int			entsize;
>  	int			tmp;
>  	int			i;
>  
> @@ -1432,11 +1434,14 @@ xfs_attr3_leaf_add_work(
>  	ASSERT(ichdr->freemap[mapindex].base < args->geo->blksize);
>  	ASSERT((ichdr->freemap[mapindex].base & 0x3) == 0);
>  	ASSERT(ichdr->freemap[mapindex].size >=
> -		xfs_attr_leaf_newentsize(args, NULL));
> +		xfs_attr_leaf_newentsize(args->geo, args->namelen,
> +				args->valuelen, NULL));
>  	ASSERT(ichdr->freemap[mapindex].size < args->geo->blksize);
>  	ASSERT((ichdr->freemap[mapindex].size & 0x3) == 0);
>  
> -	ichdr->freemap[mapindex].size -= xfs_attr_leaf_newentsize(args, &tmp);
> +	entsize = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> +			args->valuelen, &tmp);
> +	ichdr->freemap[mapindex].size -= entsize;
>  
>  	entry->nameidx = cpu_to_be16(ichdr->freemap[mapindex].base +
>  				     ichdr->freemap[mapindex].size);
> @@ -1824,6 +1829,8 @@ xfs_attr3_leaf_figure_balance(
>  	struct xfs_attr_leafblock	*leaf1 = blk1->bp->b_addr;
>  	struct xfs_attr_leafblock	*leaf2 = blk2->bp->b_addr;
>  	struct xfs_attr_leaf_entry	*entry;
> +	struct xfs_da_args		*args;
> +	int				entsize;
>  	int				count;
>  	int				max;
>  	int				index;
> @@ -1833,14 +1840,16 @@ xfs_attr3_leaf_figure_balance(
>  	int				foundit = 0;
>  	int				tmp;
>  
> +	args = state->args;
>  	/*
>  	 * Examine entries until we reduce the absolute difference in
>  	 * byte usage between the two blocks to a minimum.
>  	 */
>  	max = ichdr1->count + ichdr2->count;
>  	half = (max + 1) * sizeof(*entry);
> -	half += ichdr1->usedbytes + ichdr2->usedbytes +
> -			xfs_attr_leaf_newentsize(state->args, NULL);
> +	entsize = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> +			args->valuelen, NULL);
> +	half += ichdr1->usedbytes + ichdr2->usedbytes + entsize;
>  	half /= 2;
>  	lastdelta = state->args->geo->blksize;
>  	entry = xfs_attr3_leaf_entryp(leaf1);
> @@ -1851,8 +1860,9 @@ xfs_attr3_leaf_figure_balance(
>  		 * The new entry is in the first block, account for it.
>  		 */
>  		if (count == blk1->index) {
> -			tmp = totallen + sizeof(*entry) +
> -				xfs_attr_leaf_newentsize(state->args, NULL);
> +			entsize = xfs_attr_leaf_newentsize(args->geo,
> +					args->namelen, args->valuelen, NULL);
> +			tmp = totallen + sizeof(*entry) + entsize;
>  			if (XFS_ATTR_ABS(half - tmp) > lastdelta)
>  				break;
>  			lastdelta = XFS_ATTR_ABS(half - tmp);
> @@ -1887,8 +1897,9 @@ xfs_attr3_leaf_figure_balance(
>  	 */
>  	totallen -= count * sizeof(*entry);
>  	if (foundit) {
> -		totallen -= sizeof(*entry) +
> -				xfs_attr_leaf_newentsize(state->args, NULL);
> +		entsize = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> +				args->valuelen, NULL);
> +		totallen -= sizeof(*entry) + entsize;
>  	}
>  
>  	*countarg = count;
> @@ -2664,20 +2675,22 @@ xfs_attr_leaf_entsize(xfs_attr_leafblock_t *leaf, int index)
>   */
>  int
>  xfs_attr_leaf_newentsize(
> -	struct xfs_da_args	*args,
> +	struct xfs_da_geometry	*geo,
> +	int			namelen,
> +	int			valuelen,
>  	int			*local)
>  {
>  	int			size;
>  
> -	size = xfs_attr_leaf_entsize_local(args->namelen, args->valuelen);
> -	if (size < xfs_attr_leaf_entsize_local_max(args->geo->blksize)) {
> +	size = xfs_attr_leaf_entsize_local(namelen, valuelen);
> +	if (size < xfs_attr_leaf_entsize_local_max(geo->blksize)) {
>  		if (local)
>  			*local = 1;
>  		return size;
>  	}
>  	if (local)
>  		*local = 0;
> -	return xfs_attr_leaf_entsize_remote(args->namelen);
> +	return xfs_attr_leaf_entsize_remote(namelen);
>  }
>  
>  
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.h b/fs/xfs/libxfs/xfs_attr_leaf.h
> index 6dd2d937a42a3..7bc5dd6c4d66a 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.h
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.h
> @@ -96,7 +96,8 @@ void	xfs_attr3_leaf_unbalance(struct xfs_da_state *state,
>  xfs_dahash_t	xfs_attr_leaf_lasthash(struct xfs_buf *bp, int *count);
>  int	xfs_attr_leaf_order(struct xfs_buf *leaf1_bp,
>  				   struct xfs_buf *leaf2_bp);
> -int	xfs_attr_leaf_newentsize(struct xfs_da_args *args, int *local);
> +int	xfs_attr_leaf_newentsize(struct xfs_da_geometry	*geo, int namelen,
> +			int valuelen, int *local);
>  int	xfs_attr3_leaf_read(struct xfs_trans *tp, struct xfs_inode *dp,
>  			xfs_dablk_t bno, struct xfs_buf **bpp);
>  void	xfs_attr3_leaf_hdr_from_disk(struct xfs_da_geometry *geo,
> -- 
> 2.19.1
> 

Re: [PATCH V4 RESEND 1/7] xfs: Pass xattr name and value length explicitly to xfs_attr_leaf_newentsize

[Christoph Hellwig]

> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index fae322105457a..65a3bf40c4f9d 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -1403,6 +1404,7 @@ xfs_attr3_leaf_add_work(
>  	struct xfs_attr_leaf_name_local *name_loc;
>  	struct xfs_attr_leaf_name_remote *name_rmt;
>  	struct xfs_mount	*mp;
> +	int			entsize;
>  	int			tmp;
>  	int			i;
>  
> @@ -1432,11 +1434,14 @@ xfs_attr3_leaf_add_work(
>  	ASSERT(ichdr->freemap[mapindex].base < args->geo->blksize);
>  	ASSERT((ichdr->freemap[mapindex].base & 0x3) == 0);
>  	ASSERT(ichdr->freemap[mapindex].size >=
> -		xfs_attr_leaf_newentsize(args, NULL));
> +		xfs_attr_leaf_newentsize(args->geo, args->namelen,
> +				args->valuelen, NULL));
>  	ASSERT(ichdr->freemap[mapindex].size < args->geo->blksize);
>  	ASSERT((ichdr->freemap[mapindex].size & 0x3) == 0);
>  
> -	ichdr->freemap[mapindex].size -= xfs_attr_leaf_newentsize(args, &tmp);
> +	entsize = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> +			args->valuelen, &tmp);
> +	ichdr->freemap[mapindex].size -= entsize;

As-is this entsize variable is a little pointless.  Please move the
assignment to it up and reuse it in the assert.

> @@ -1824,6 +1829,8 @@ xfs_attr3_leaf_figure_balance(
>  	struct xfs_attr_leafblock	*leaf1 = blk1->bp->b_addr;
>  	struct xfs_attr_leafblock	*leaf2 = blk2->bp->b_addr;
>  	struct xfs_attr_leaf_entry	*entry;
> +	struct xfs_da_args		*args;
> +	int				entsize;
>  	int				count;
>  	int				max;
>  	int				index;
> @@ -1833,14 +1840,16 @@ xfs_attr3_leaf_figure_balance(
>  	int				foundit = 0;
>  	int				tmp;
>  
> +	args = state->args;

Please assign the value to the variable at the time of declaration.

>  	/*
>  	 * Examine entries until we reduce the absolute difference in
>  	 * byte usage between the two blocks to a minimum.
>  	 */
>  	max = ichdr1->count + ichdr2->count;
>  	half = (max + 1) * sizeof(*entry);
> -	half += ichdr1->usedbytes + ichdr2->usedbytes +
> -			xfs_attr_leaf_newentsize(state->args, NULL);
> +	entsize = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> +			args->valuelen, NULL);
> +	half += ichdr1->usedbytes + ichdr2->usedbytes + entsize;
>  	half /= 2;
>  	lastdelta = state->args->geo->blksize;
>  	entry = xfs_attr3_leaf_entryp(leaf1);
> @@ -1851,8 +1860,9 @@ xfs_attr3_leaf_figure_balance(
>  		 * The new entry is in the first block, account for it.
>  		 */
>  		if (count == blk1->index) {
> -			tmp = totallen + sizeof(*entry) +
> -				xfs_attr_leaf_newentsize(state->args, NULL);
> +			entsize = xfs_attr_leaf_newentsize(args->geo,
> +					args->namelen, args->valuelen, NULL);
> +			tmp = totallen + sizeof(*entry) + entsize;
>  			if (XFS_ATTR_ABS(half - tmp) > lastdelta)
>  				break;
>  			lastdelta = XFS_ATTR_ABS(half - tmp);
> @@ -1887,8 +1897,9 @@ xfs_attr3_leaf_figure_balance(
>  	 */
>  	totallen -= count * sizeof(*entry);
>  	if (foundit) {
> -		totallen -= sizeof(*entry) +
> -				xfs_attr_leaf_newentsize(state->args, NULL);
> +		entsize = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> +				args->valuelen, NULL);
> +		totallen -= sizeof(*entry) + entsize;

AFAICS there is no need to assign the same value to entsize again and
again in this function.  It should be enough to assign to it once and
then reuse the value.

Re: [PATCH V4 RESEND 1/7] xfs: Pass xattr name and value length explicitly to xfs_attr_leaf_newentsize

[Chandan Rajendra]

On Wednesday, February 26, 2020 10:28 PM Christoph Hellwig wrote: 
> > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> > index fae322105457a..65a3bf40c4f9d 100644
> > --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> > @@ -1403,6 +1404,7 @@ xfs_attr3_leaf_add_work(
> >  	struct xfs_attr_leaf_name_local *name_loc;
> >  	struct xfs_attr_leaf_name_remote *name_rmt;
> >  	struct xfs_mount	*mp;
> > +	int			entsize;
> >  	int			tmp;
> >  	int			i;
> >  
> > @@ -1432,11 +1434,14 @@ xfs_attr3_leaf_add_work(
> >  	ASSERT(ichdr->freemap[mapindex].base < args->geo->blksize);
> >  	ASSERT((ichdr->freemap[mapindex].base & 0x3) == 0);
> >  	ASSERT(ichdr->freemap[mapindex].size >=
> > -		xfs_attr_leaf_newentsize(args, NULL));
> > +		xfs_attr_leaf_newentsize(args->geo, args->namelen,
> > +				args->valuelen, NULL));
> >  	ASSERT(ichdr->freemap[mapindex].size < args->geo->blksize);
> >  	ASSERT((ichdr->freemap[mapindex].size & 0x3) == 0);
> >  
> > -	ichdr->freemap[mapindex].size -= xfs_attr_leaf_newentsize(args, &tmp);
> > +	entsize = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> > +			args->valuelen, &tmp);
> > +	ichdr->freemap[mapindex].size -= entsize;
> 
> As-is this entsize variable is a little pointless.  Please move the
> assignment to it up and reuse it in the assert.
> 
> > @@ -1824,6 +1829,8 @@ xfs_attr3_leaf_figure_balance(
> >  	struct xfs_attr_leafblock	*leaf1 = blk1->bp->b_addr;
> >  	struct xfs_attr_leafblock	*leaf2 = blk2->bp->b_addr;
> >  	struct xfs_attr_leaf_entry	*entry;
> > +	struct xfs_da_args		*args;
> > +	int				entsize;
> >  	int				count;
> >  	int				max;
> >  	int				index;
> > @@ -1833,14 +1840,16 @@ xfs_attr3_leaf_figure_balance(
> >  	int				foundit = 0;
> >  	int				tmp;
> >  
> > +	args = state->args;
> 
> Please assign the value to the variable at the time of declaration.
> 
> >  	/*
> >  	 * Examine entries until we reduce the absolute difference in
> >  	 * byte usage between the two blocks to a minimum.
> >  	 */
> >  	max = ichdr1->count + ichdr2->count;
> >  	half = (max + 1) * sizeof(*entry);
> > -	half += ichdr1->usedbytes + ichdr2->usedbytes +
> > -			xfs_attr_leaf_newentsize(state->args, NULL);
> > +	entsize = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> > +			args->valuelen, NULL);
> > +	half += ichdr1->usedbytes + ichdr2->usedbytes + entsize;
> >  	half /= 2;
> >  	lastdelta = state->args->geo->blksize;
> >  	entry = xfs_attr3_leaf_entryp(leaf1);
> > @@ -1851,8 +1860,9 @@ xfs_attr3_leaf_figure_balance(
> >  		 * The new entry is in the first block, account for it.
> >  		 */
> >  		if (count == blk1->index) {
> > -			tmp = totallen + sizeof(*entry) +
> > -				xfs_attr_leaf_newentsize(state->args, NULL);
> > +			entsize = xfs_attr_leaf_newentsize(args->geo,
> > +					args->namelen, args->valuelen, NULL);
> > +			tmp = totallen + sizeof(*entry) + entsize;
> >  			if (XFS_ATTR_ABS(half - tmp) > lastdelta)
> >  				break;
> >  			lastdelta = XFS_ATTR_ABS(half - tmp);
> > @@ -1887,8 +1897,9 @@ xfs_attr3_leaf_figure_balance(
> >  	 */
> >  	totallen -= count * sizeof(*entry);
> >  	if (foundit) {
> > -		totallen -= sizeof(*entry) +
> > -				xfs_attr_leaf_newentsize(state->args, NULL);
> > +		entsize = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> > +				args->valuelen, NULL);
> > +		totallen -= sizeof(*entry) + entsize;
> 
> AFAICS there is no need to assign the same value to entsize again and
> again in this function.  It should be enough to assign to it once and
> then reuse the value.
> 

Sorry about those redundant function calls. I will include the changes
suggested in the next version of the patchset.

-- 
chandan

[PATCH V4 RESEND 2/7] xfs: xfs_attr_calc_size: Use local variables to track individual space components

[Chandan Rajendra]

The size calculated by xfs_attr_calc_size() is a sum of three components,
1. Number of dabtree blocks
2. Number of Bmbt blocks
3. Number of remote blocks

This commit introduces new local variables to track these numbers explicitly.

Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_attr.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 1875210cc8e40..942ba552e0bdd 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -142,8 +142,10 @@ xfs_attr_calc_size(
 	int			*local)
 {
 	struct xfs_mount	*mp = args->dp->i_mount;
+	unsigned int		total_dablks;
+	unsigned int		bmbt_blks;
+	unsigned int		rmt_blks;
 	int			size;
-	int			nblks;
 
 	/*
 	 * Determine space new attribute will use, and if it would be
@@ -151,23 +153,26 @@ xfs_attr_calc_size(
 	 */
 	size = xfs_attr_leaf_newentsize(args->geo, args->namelen,
 			args->valuelen, local);
-	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
+	total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
+	bmbt_blks = XFS_DAENTER_BMAPS(mp, XFS_ATTR_FORK);
 	if (*local) {
 		if (size > (args->geo->blksize / 2)) {
 			/* Double split possible */
-			nblks *= 2;
+			total_dablks *= 2;
+			bmbt_blks *= 2;
 		}
+		rmt_blks = 0;
 	} else {
 		/*
 		 * Out of line attribute, cannot double split, but
 		 * make room for the attribute value itself.
 		 */
-		uint	dblocks = xfs_attr3_rmt_blocks(mp, args->valuelen);
-		nblks += dblocks;
-		nblks += XFS_NEXTENTADD_SPACE_RES(mp, dblocks, XFS_ATTR_FORK);
+		rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
+		bmbt_blks += XFS_NEXTENTADD_SPACE_RES(mp, rmt_blks,
+				XFS_ATTR_FORK);
 	}
 
-	return nblks;
+	return total_dablks + rmt_blks + bmbt_blks;
 }
 
 STATIC int
-- 
2.19.1

Re: [PATCH V4 RESEND 2/7] xfs: xfs_attr_calc_size: Use local variables to track individual space components

[Brian Foster]

On Mon, Feb 24, 2020 at 09:30:39AM +0530, Chandan Rajendra wrote:
> The size calculated by xfs_attr_calc_size() is a sum of three components,
> 1. Number of dabtree blocks
> 2. Number of Bmbt blocks
> 3. Number of remote blocks
> 
> This commit introduces new local variables to track these numbers explicitly.
> 
> Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 1875210cc8e40..942ba552e0bdd 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -142,8 +142,10 @@ xfs_attr_calc_size(
>  	int			*local)
>  {
>  	struct xfs_mount	*mp = args->dp->i_mount;
> +	unsigned int		total_dablks;
> +	unsigned int		bmbt_blks;
> +	unsigned int		rmt_blks;
>  	int			size;
> -	int			nblks;
>  
>  	/*
>  	 * Determine space new attribute will use, and if it would be
> @@ -151,23 +153,26 @@ xfs_attr_calc_size(
>  	 */
>  	size = xfs_attr_leaf_newentsize(args->geo, args->namelen,
>  			args->valuelen, local);
> -	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
> +	total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> +	bmbt_blks = XFS_DAENTER_BMAPS(mp, XFS_ATTR_FORK);
>  	if (*local) {
>  		if (size > (args->geo->blksize / 2)) {
>  			/* Double split possible */
> -			nblks *= 2;
> +			total_dablks *= 2;
> +			bmbt_blks *= 2;
>  		}
> +		rmt_blks = 0;

I'd just initialize this one to zero above. Otherwise looks fine:

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

>  	} else {
>  		/*
>  		 * Out of line attribute, cannot double split, but
>  		 * make room for the attribute value itself.
>  		 */
> -		uint	dblocks = xfs_attr3_rmt_blocks(mp, args->valuelen);
> -		nblks += dblocks;
> -		nblks += XFS_NEXTENTADD_SPACE_RES(mp, dblocks, XFS_ATTR_FORK);
> +		rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
> +		bmbt_blks += XFS_NEXTENTADD_SPACE_RES(mp, rmt_blks,
> +				XFS_ATTR_FORK);
>  	}
>  
> -	return nblks;
> +	return total_dablks + rmt_blks + bmbt_blks;
>  }
>  
>  STATIC int
> -- 
> 2.19.1
> 

Re: [PATCH V4 RESEND 2/7] xfs: xfs_attr_calc_size: Use local variables to track individual space components

[Chandan Rajendra]

On Tuesday, February 25, 2020 9:41 PM Brian Foster wrote: 
> On Mon, Feb 24, 2020 at 09:30:39AM +0530, Chandan Rajendra wrote:
> > The size calculated by xfs_attr_calc_size() is a sum of three components,
> > 1. Number of dabtree blocks
> > 2. Number of Bmbt blocks
> > 3. Number of remote blocks
> > 
> > This commit introduces new local variables to track these numbers explicitly.
> > 
> > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > ---
> >  fs/xfs/libxfs/xfs_attr.c | 19 ++++++++++++-------
> >  1 file changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 1875210cc8e40..942ba552e0bdd 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -142,8 +142,10 @@ xfs_attr_calc_size(
> >  	int			*local)
> >  {
> >  	struct xfs_mount	*mp = args->dp->i_mount;
> > +	unsigned int		total_dablks;
> > +	unsigned int		bmbt_blks;
> > +	unsigned int		rmt_blks;
> >  	int			size;
> > -	int			nblks;
> >  
> >  	/*
> >  	 * Determine space new attribute will use, and if it would be
> > @@ -151,23 +153,26 @@ xfs_attr_calc_size(
> >  	 */
> >  	size = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> >  			args->valuelen, local);
> > -	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
> > +	total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> > +	bmbt_blks = XFS_DAENTER_BMAPS(mp, XFS_ATTR_FORK);
> >  	if (*local) {
> >  		if (size > (args->geo->blksize / 2)) {
> >  			/* Double split possible */
> > -			nblks *= 2;
> > +			total_dablks *= 2;
> > +			bmbt_blks *= 2;
> >  		}
> > +		rmt_blks = 0;
> 
> I'd just initialize this one to zero above. Otherwise looks fine:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Thanks for the review. I will fix this up in the next iteration of the
patchset.

-- 
chandan

[PATCH V4 RESEND 3/7] xfs: xfs_attr_calc_size: Calculate Bmbt blks only once

[Chandan Rajendra]

The number of Bmbt blocks that is required can be calculated only once by
passing the sum of total number of dabtree blocks and remote blocks to
XFS_NEXTENTADD_SPACE_RES() macro.

Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_attr.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 942ba552e0bdd..a708b142f69b6 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -154,12 +154,10 @@ xfs_attr_calc_size(
 	size = xfs_attr_leaf_newentsize(args->geo, args->namelen,
 			args->valuelen, local);
 	total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
-	bmbt_blks = XFS_DAENTER_BMAPS(mp, XFS_ATTR_FORK);
 	if (*local) {
 		if (size > (args->geo->blksize / 2)) {
 			/* Double split possible */
 			total_dablks *= 2;
-			bmbt_blks *= 2;
 		}
 		rmt_blks = 0;
 	} else {
@@ -168,10 +166,11 @@ xfs_attr_calc_size(
 		 * make room for the attribute value itself.
 		 */
 		rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
-		bmbt_blks += XFS_NEXTENTADD_SPACE_RES(mp, rmt_blks,
-				XFS_ATTR_FORK);
 	}
 
+	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
+			XFS_ATTR_FORK);
+
 	return total_dablks + rmt_blks + bmbt_blks;
 }
 
-- 
2.19.1

Re: [PATCH V4 RESEND 3/7] xfs: xfs_attr_calc_size: Calculate Bmbt blks only once

[Brian Foster]

On Mon, Feb 24, 2020 at 09:30:40AM +0530, Chandan Rajendra wrote:
> The number of Bmbt blocks that is required can be calculated only once by
> passing the sum of total number of dabtree blocks and remote blocks to
> XFS_NEXTENTADD_SPACE_RES() macro.
> 
> Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> ---

According to the cover letter this is fixing a reservation calculation
issue, though the commit log kind of gives the impression it's a
refactor. Can you elaborate on what this fixes in the commit log?

Brian

>  fs/xfs/libxfs/xfs_attr.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 942ba552e0bdd..a708b142f69b6 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -154,12 +154,10 @@ xfs_attr_calc_size(
>  	size = xfs_attr_leaf_newentsize(args->geo, args->namelen,
>  			args->valuelen, local);
>  	total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> -	bmbt_blks = XFS_DAENTER_BMAPS(mp, XFS_ATTR_FORK);
>  	if (*local) {
>  		if (size > (args->geo->blksize / 2)) {
>  			/* Double split possible */
>  			total_dablks *= 2;
> -			bmbt_blks *= 2;
>  		}
>  		rmt_blks = 0;
>  	} else {
> @@ -168,10 +166,11 @@ xfs_attr_calc_size(
>  		 * make room for the attribute value itself.
>  		 */
>  		rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
> -		bmbt_blks += XFS_NEXTENTADD_SPACE_RES(mp, rmt_blks,
> -				XFS_ATTR_FORK);
>  	}
>  
> +	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> +			XFS_ATTR_FORK);
> +
>  	return total_dablks + rmt_blks + bmbt_blks;
>  }
>  
> -- 
> 2.19.1
> 

Re: [PATCH V4 RESEND 3/7] xfs: xfs_attr_calc_size: Calculate Bmbt blks only once

[Chandan Rajendra]

On Tuesday, February 25, 2020 9:41 PM Brian Foster wrote: 
> On Mon, Feb 24, 2020 at 09:30:40AM +0530, Chandan Rajendra wrote:
> > The number of Bmbt blocks that is required can be calculated only once by
> > passing the sum of total number of dabtree blocks and remote blocks to
> > XFS_NEXTENTADD_SPACE_RES() macro.
> > 
> > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > ---
> 
> According to the cover letter this is fixing a reservation calculation
> issue, though the commit log kind of gives the impression it's a
> refactor. Can you elaborate on what this fixes in the commit log?
> 

XFS_NEXTENTADD_SPACE_RES() first figures out the number of Bmbt leaf blocks
needed for mapping the 'block count' passed to it as the second argument.
When calculating the number of leaf blocks, it accommodates the 'block count'
argument in groups of XFS_MAX_CONTIG_EXTENTS_PER_BLOCK(mp). For each such
group it decides that a bmbt leaf block is required. For each of the leaf
blocks that needs to be allocated, it assumes that there will be a split of
the bmbt tree from root to leaf. Hence it multiplies the number of leaf blocks
with the maximum height of the tree.

With two individual calls to XFS_NEXTENTADD_SPACE_RES() (one is indirectly
through the call to XFS_DAENTER_BMAPS() => XFS_DAENTER_BMAP1B() and the other
is for remote attr blocks) we miss out on the opportunity to group the bmbt
leaf blocks and hence overcompensate on the bmbt blocks calculation.

Please let me know if my understanding is incorrect.

> 
> >  fs/xfs/libxfs/xfs_attr.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 942ba552e0bdd..a708b142f69b6 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -154,12 +154,10 @@ xfs_attr_calc_size(
> >  	size = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> >  			args->valuelen, local);
> >  	total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> > -	bmbt_blks = XFS_DAENTER_BMAPS(mp, XFS_ATTR_FORK);
> >  	if (*local) {
> >  		if (size > (args->geo->blksize / 2)) {
> >  			/* Double split possible */
> >  			total_dablks *= 2;
> > -			bmbt_blks *= 2;
> >  		}
> >  		rmt_blks = 0;
> >  	} else {
> > @@ -168,10 +166,11 @@ xfs_attr_calc_size(
> >  		 * make room for the attribute value itself.
> >  		 */
> >  		rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
> > -		bmbt_blks += XFS_NEXTENTADD_SPACE_RES(mp, rmt_blks,
> > -				XFS_ATTR_FORK);
> >  	}
> >  
> > +	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> > +			XFS_ATTR_FORK);
> > +
> >  	return total_dablks + rmt_blks + bmbt_blks;
> >  }
> >  
> 
> 

-- 
chandan

Re: [PATCH V4 RESEND 3/7] xfs: xfs_attr_calc_size: Calculate Bmbt blks only once

[Brian Foster]

On Wed, Feb 26, 2020 at 08:33:12PM +0530, Chandan Rajendra wrote:
> On Tuesday, February 25, 2020 9:41 PM Brian Foster wrote: 
> > On Mon, Feb 24, 2020 at 09:30:40AM +0530, Chandan Rajendra wrote:
> > > The number of Bmbt blocks that is required can be calculated only once by
> > > passing the sum of total number of dabtree blocks and remote blocks to
> > > XFS_NEXTENTADD_SPACE_RES() macro.
> > > 
> > > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > > ---
> > 
> > According to the cover letter this is fixing a reservation calculation
> > issue, though the commit log kind of gives the impression it's a
> > refactor. Can you elaborate on what this fixes in the commit log?
> > 
> 
> XFS_NEXTENTADD_SPACE_RES() first figures out the number of Bmbt leaf blocks
> needed for mapping the 'block count' passed to it as the second argument.
> When calculating the number of leaf blocks, it accommodates the 'block count'
> argument in groups of XFS_MAX_CONTIG_EXTENTS_PER_BLOCK(mp). For each such
> group it decides that a bmbt leaf block is required. For each of the leaf
> blocks that needs to be allocated, it assumes that there will be a split of
> the bmbt tree from root to leaf. Hence it multiplies the number of leaf blocks
> with the maximum height of the tree.
> 
> With two individual calls to XFS_NEXTENTADD_SPACE_RES() (one is indirectly
> through the call to XFS_DAENTER_BMAPS() => XFS_DAENTER_BMAP1B() and the other
> is for remote attr blocks) we miss out on the opportunity to group the bmbt
> leaf blocks and hence overcompensate on the bmbt blocks calculation.
> 
> Please let me know if my understanding is incorrect.
> 

Ok, thanks. I think I follow the intent. This patch is actually intended
to reduce block reservation by simplifying this calculation, right?

I'm not hugely familiar with the dabtree code, but is it possible the
existing reservations are written this way because each dabtree
extension along with a remote block allocation are independent
xfs_bmapi_write() calls? IOW, perhaps we cannot assume these can all
land in the same bmbt blocks across the xattr operation? ISTM that might
explain that XFS_DAENTER_BMAPS() calculates the reservation for a single
attr block and multiplies it by the max depth, but I could easily be
misunderstanding something.

What is the motivation for this patch btw? Have you observed a problem
or excessive reservation sizes, or is this by code inspection?

Brian

> > 
> > >  fs/xfs/libxfs/xfs_attr.c | 7 +++----
> > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > index 942ba552e0bdd..a708b142f69b6 100644
> > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > @@ -154,12 +154,10 @@ xfs_attr_calc_size(
> > >  	size = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> > >  			args->valuelen, local);
> > >  	total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> > > -	bmbt_blks = XFS_DAENTER_BMAPS(mp, XFS_ATTR_FORK);
> > >  	if (*local) {
> > >  		if (size > (args->geo->blksize / 2)) {
> > >  			/* Double split possible */
> > >  			total_dablks *= 2;
> > > -			bmbt_blks *= 2;
> > >  		}
> > >  		rmt_blks = 0;
> > >  	} else {
> > > @@ -168,10 +166,11 @@ xfs_attr_calc_size(
> > >  		 * make room for the attribute value itself.
> > >  		 */
> > >  		rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
> > > -		bmbt_blks += XFS_NEXTENTADD_SPACE_RES(mp, rmt_blks,
> > > -				XFS_ATTR_FORK);
> > >  	}
> > >  
> > > +	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> > > +			XFS_ATTR_FORK);
> > > +
> > >  	return total_dablks + rmt_blks + bmbt_blks;
> > >  }
> > >  
> > 
> > 
> 
> -- 
> chandan
> 
> 
> 

Re: [PATCH V4 RESEND 3/7] xfs: xfs_attr_calc_size: Calculate Bmbt blks only once

[Chandan Rajendra]

On Wednesday, February 26, 2020 10:12 PM Brian Foster wrote: 
> On Wed, Feb 26, 2020 at 08:33:12PM +0530, Chandan Rajendra wrote:
> > On Tuesday, February 25, 2020 9:41 PM Brian Foster wrote: 
> > > On Mon, Feb 24, 2020 at 09:30:40AM +0530, Chandan Rajendra wrote:
> > > > The number of Bmbt blocks that is required can be calculated only once by
> > > > passing the sum of total number of dabtree blocks and remote blocks to
> > > > XFS_NEXTENTADD_SPACE_RES() macro.
> > > > 
> > > > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > > > ---
> > > 
> > > According to the cover letter this is fixing a reservation calculation
> > > issue, though the commit log kind of gives the impression it's a
> > > refactor. Can you elaborate on what this fixes in the commit log?
> > > 
> > 
> > XFS_NEXTENTADD_SPACE_RES() first figures out the number of Bmbt leaf blocks
> > needed for mapping the 'block count' passed to it as the second argument.
> > When calculating the number of leaf blocks, it accommodates the 'block count'
> > argument in groups of XFS_MAX_CONTIG_EXTENTS_PER_BLOCK(mp). For each such
> > group it decides that a bmbt leaf block is required. For each of the leaf
> > blocks that needs to be allocated, it assumes that there will be a split of
> > the bmbt tree from root to leaf. Hence it multiplies the number of leaf blocks
> > with the maximum height of the tree.
> > 
> > With two individual calls to XFS_NEXTENTADD_SPACE_RES() (one is indirectly
> > through the call to XFS_DAENTER_BMAPS() => XFS_DAENTER_BMAP1B() and the other
> > is for remote attr blocks) we miss out on the opportunity to group the bmbt
> > leaf blocks and hence overcompensate on the bmbt blocks calculation.
> > 
> > Please let me know if my understanding is incorrect.
> > 
> 
> Ok, thanks. I think I follow the intent. This patch is actually intended
> to reduce block reservation by simplifying this calculation, right?

I noticed xfs/132 test failing when I had changed the code to have 32-bit
xattr extent counter. The corresponding mount failure was due to log size
checks failing in xfs_log_mount(). The difference in value returned by
xfs_log_calc_minimum_size() => xfs_log_get_max_trans_res() =>
xfs_log_calc_max_attrsetm_res() was pretty large.

Upon code inspection I found the inconsistencies in
xfs_log_calc_max_attrsetm_res() which are described in the cover letter and as
part of the commit message of the last patch.

After a quick chat with Dave on irc, we figured that the best approach was to
convert xfs_attr_calc_size() into a helper function so that the size
calculations for an xattr set operation is placed in a single function. These
values can then be used by other functions like xfs_attr_set() and
xfs_log_calc_max_attrsetm_res().

Along the way, I found that the mount time reservation was incorrectly done as
well. For E.g. dabtree splits getting accounted as part of mount time
reservation was wrong. Due to these reasons and others listed in the cover
letter I ended up changing the mount time and run time reservation
calculations.

Hence, The reduced reservation sizes are actually a side effect of fixing the
inconsistencies.

> 
> I'm not hugely familiar with the dabtree code, but is it possible the
> existing reservations are written this way because each dabtree
> extension along with a remote block allocation are independent
> xfs_bmapi_write() calls? IOW, perhaps we cannot assume these can all
> land in the same bmbt blocks across the xattr operation? ISTM that might
> explain that XFS_DAENTER_BMAPS() calculates the reservation for a single
> attr block and multiplies it by the max depth, but I could easily be
> misunderstanding something.

I think you are right. I will keep the bmbt calculations separate for dabtree
and remote blocks and add them up at the end of the function.

> 
> What is the motivation for this patch btw? Have you observed a problem
> or excessive reservation sizes, or is this by code inspection?
> 
> Brian
> 
> > > 
> > > >  fs/xfs/libxfs/xfs_attr.c | 7 +++----
> > > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > > index 942ba552e0bdd..a708b142f69b6 100644
> > > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > > @@ -154,12 +154,10 @@ xfs_attr_calc_size(
> > > >  	size = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> > > >  			args->valuelen, local);
> > > >  	total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> > > > -	bmbt_blks = XFS_DAENTER_BMAPS(mp, XFS_ATTR_FORK);
> > > >  	if (*local) {
> > > >  		if (size > (args->geo->blksize / 2)) {
> > > >  			/* Double split possible */
> > > >  			total_dablks *= 2;
> > > > -			bmbt_blks *= 2;
> > > >  		}
> > > >  		rmt_blks = 0;
> > > >  	} else {
> > > > @@ -168,10 +166,11 @@ xfs_attr_calc_size(
> > > >  		 * make room for the attribute value itself.
> > > >  		 */
> > > >  		rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
> > > > -		bmbt_blks += XFS_NEXTENTADD_SPACE_RES(mp, rmt_blks,
> > > > -				XFS_ATTR_FORK);
> > > >  	}
> > > >  
> > > > +	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> > > > +			XFS_ATTR_FORK);
> > > > +
> > > >  	return total_dablks + rmt_blks + bmbt_blks;
> > > >  }
> > > >  
> > > 
> > > 
> > 
> 
> 

-- 
chandan

Re: [PATCH V4 RESEND 3/7] xfs: xfs_attr_calc_size: Calculate Bmbt blks only once

[Brian Foster]

On Thu, Feb 27, 2020 at 02:29:16PM +0530, Chandan Rajendra wrote:
> On Wednesday, February 26, 2020 10:12 PM Brian Foster wrote: 
> > On Wed, Feb 26, 2020 at 08:33:12PM +0530, Chandan Rajendra wrote:
> > > On Tuesday, February 25, 2020 9:41 PM Brian Foster wrote: 
> > > > On Mon, Feb 24, 2020 at 09:30:40AM +0530, Chandan Rajendra wrote:
> > > > > The number of Bmbt blocks that is required can be calculated only once by
> > > > > passing the sum of total number of dabtree blocks and remote blocks to
> > > > > XFS_NEXTENTADD_SPACE_RES() macro.
> > > > > 
> > > > > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > > > > ---
> > > > 
> > > > According to the cover letter this is fixing a reservation calculation
> > > > issue, though the commit log kind of gives the impression it's a
> > > > refactor. Can you elaborate on what this fixes in the commit log?
> > > > 
> > > 
> > > XFS_NEXTENTADD_SPACE_RES() first figures out the number of Bmbt leaf blocks
> > > needed for mapping the 'block count' passed to it as the second argument.
> > > When calculating the number of leaf blocks, it accommodates the 'block count'
> > > argument in groups of XFS_MAX_CONTIG_EXTENTS_PER_BLOCK(mp). For each such
> > > group it decides that a bmbt leaf block is required. For each of the leaf
> > > blocks that needs to be allocated, it assumes that there will be a split of
> > > the bmbt tree from root to leaf. Hence it multiplies the number of leaf blocks
> > > with the maximum height of the tree.
> > > 
> > > With two individual calls to XFS_NEXTENTADD_SPACE_RES() (one is indirectly
> > > through the call to XFS_DAENTER_BMAPS() => XFS_DAENTER_BMAP1B() and the other
> > > is for remote attr blocks) we miss out on the opportunity to group the bmbt
> > > leaf blocks and hence overcompensate on the bmbt blocks calculation.
> > > 
> > > Please let me know if my understanding is incorrect.
> > > 
> > 
> > Ok, thanks. I think I follow the intent. This patch is actually intended
> > to reduce block reservation by simplifying this calculation, right?
> 
> I noticed xfs/132 test failing when I had changed the code to have 32-bit
> xattr extent counter. The corresponding mount failure was due to log size
> checks failing in xfs_log_mount(). The difference in value returned by
> xfs_log_calc_minimum_size() => xfs_log_get_max_trans_res() =>
> xfs_log_calc_max_attrsetm_res() was pretty large.
> 
> Upon code inspection I found the inconsistencies in
> xfs_log_calc_max_attrsetm_res() which are described in the cover letter and as
> part of the commit message of the last patch.
> 

Ok, so the fixes to xfs_log_calc_max_attrsetm_res() are what actually
fixed the test failure? If so, that strikes me as a good independent fix
candidate (re: refactoring) because the commit log for that one should
probably describe the observable problem and the fix separate from other
issues.

> After a quick chat with Dave on irc, we figured that the best approach was to
> convert xfs_attr_calc_size() into a helper function so that the size
> calculations for an xattr set operation is placed in a single function. These
> values can then be used by other functions like xfs_attr_set() and
> xfs_log_calc_max_attrsetm_res().
> 
> Along the way, I found that the mount time reservation was incorrectly done as
> well. For E.g. dabtree splits getting accounted as part of mount time
> reservation was wrong. Due to these reasons and others listed in the cover
> letter I ended up changing the mount time and run time reservation
> calculations.
> 
> Hence, The reduced reservation sizes are actually a side effect of fixing the
> inconsistencies.
> 

Ok, so most of the rest sounds like bogosity discovered by code
inspection. That's not that surprising given that transaction
reservations are worst case values and thus it seems we sometimes get
away with bogus calculations just so long as the reservations are large
enough. :)

As it is, the final result of this patchset looks nice to me, it's just
a matter of getting there more incrementally to facilitate reviewing the
changes being made. FWIW, if we do end up with a final "fix the broken
xattr res calculation" patch at the end of the series, I think it would
be helpful to have a very deliberate commit log that contains something
like the following:

'The xattr reservation currently consists of:

	- superblock
	- dabtree * maxdepth
	- ...

This calculation is bogus because it double accounts X as part of Y and
Z, doesn't account for AGF, etc. etc. ...

The xattr reservation needs to account the following:

	- superblock
	- agf
	- dabtree * maxdepth
	- rmtblocks
	- ...

... '

> > 
> > I'm not hugely familiar with the dabtree code, but is it possible the
> > existing reservations are written this way because each dabtree
> > extension along with a remote block allocation are independent
> > xfs_bmapi_write() calls? IOW, perhaps we cannot assume these can all
> > land in the same bmbt blocks across the xattr operation? ISTM that might
> > explain that XFS_DAENTER_BMAPS() calculates the reservation for a single
> > attr block and multiplies it by the max depth, but I could easily be
> > misunderstanding something.
> 
> I think you are right. I will keep the bmbt calculations separate for dabtree
> and remote blocks and add them up at the end of the function.
> 

Ok. I think it's probably safer to preserve historical behavior in that
regard, unless somebody can confirm otherwise in the meantime.

Brian

> > 
> > What is the motivation for this patch btw? Have you observed a problem
> > or excessive reservation sizes, or is this by code inspection?
> > 
> > Brian
> > 
> > > > 
> > > > >  fs/xfs/libxfs/xfs_attr.c | 7 +++----
> > > > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > > > index 942ba552e0bdd..a708b142f69b6 100644
> > > > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > > > @@ -154,12 +154,10 @@ xfs_attr_calc_size(
> > > > >  	size = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> > > > >  			args->valuelen, local);
> > > > >  	total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> > > > > -	bmbt_blks = XFS_DAENTER_BMAPS(mp, XFS_ATTR_FORK);
> > > > >  	if (*local) {
> > > > >  		if (size > (args->geo->blksize / 2)) {
> > > > >  			/* Double split possible */
> > > > >  			total_dablks *= 2;
> > > > > -			bmbt_blks *= 2;
> > > > >  		}
> > > > >  		rmt_blks = 0;
> > > > >  	} else {
> > > > > @@ -168,10 +166,11 @@ xfs_attr_calc_size(
> > > > >  		 * make room for the attribute value itself.
> > > > >  		 */
> > > > >  		rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
> > > > > -		bmbt_blks += XFS_NEXTENTADD_SPACE_RES(mp, rmt_blks,
> > > > > -				XFS_ATTR_FORK);
> > > > >  	}
> > > > >  
> > > > > +	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> > > > > +			XFS_ATTR_FORK);
> > > > > +
> > > > >  	return total_dablks + rmt_blks + bmbt_blks;
> > > > >  }
> > > > >  
> > > > 
> > > > 
> > > 
> > 
> > 
> 
> -- 
> chandan
> 
> 
> 

Re: [PATCH V4 RESEND 3/7] xfs: xfs_attr_calc_size: Calculate Bmbt blks only once

[Chandan Rajendra]

On Thursday, February 27, 2020 5:23 PM Brian Foster wrote: 
> On Thu, Feb 27, 2020 at 02:29:16PM +0530, Chandan Rajendra wrote:
> > On Wednesday, February 26, 2020 10:12 PM Brian Foster wrote: 
> > > On Wed, Feb 26, 2020 at 08:33:12PM +0530, Chandan Rajendra wrote:
> > > > On Tuesday, February 25, 2020 9:41 PM Brian Foster wrote: 
> > > > > On Mon, Feb 24, 2020 at 09:30:40AM +0530, Chandan Rajendra wrote:
> > > > > > The number of Bmbt blocks that is required can be calculated only once by
> > > > > > passing the sum of total number of dabtree blocks and remote blocks to
> > > > > > XFS_NEXTENTADD_SPACE_RES() macro.
> > > > > > 
> > > > > > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > > > > > ---
> > > > > 
> > > > > According to the cover letter this is fixing a reservation calculation
> > > > > issue, though the commit log kind of gives the impression it's a
> > > > > refactor. Can you elaborate on what this fixes in the commit log?
> > > > > 
> > > > 
> > > > XFS_NEXTENTADD_SPACE_RES() first figures out the number of Bmbt leaf blocks
> > > > needed for mapping the 'block count' passed to it as the second argument.
> > > > When calculating the number of leaf blocks, it accommodates the 'block count'
> > > > argument in groups of XFS_MAX_CONTIG_EXTENTS_PER_BLOCK(mp). For each such
> > > > group it decides that a bmbt leaf block is required. For each of the leaf
> > > > blocks that needs to be allocated, it assumes that there will be a split of
> > > > the bmbt tree from root to leaf. Hence it multiplies the number of leaf blocks
> > > > with the maximum height of the tree.
> > > > 
> > > > With two individual calls to XFS_NEXTENTADD_SPACE_RES() (one is indirectly
> > > > through the call to XFS_DAENTER_BMAPS() => XFS_DAENTER_BMAP1B() and the other
> > > > is for remote attr blocks) we miss out on the opportunity to group the bmbt
> > > > leaf blocks and hence overcompensate on the bmbt blocks calculation.
> > > > 
> > > > Please let me know if my understanding is incorrect.
> > > > 
> > > 
> > > Ok, thanks. I think I follow the intent. This patch is actually intended
> > > to reduce block reservation by simplifying this calculation, right?
> > 
> > I noticed xfs/132 test failing when I had changed the code to have 32-bit
> > xattr extent counter. The corresponding mount failure was due to log size
> > checks failing in xfs_log_mount(). The difference in value returned by
> > xfs_log_calc_minimum_size() => xfs_log_get_max_trans_res() =>
> > xfs_log_calc_max_attrsetm_res() was pretty large.
> > 
> > Upon code inspection I found the inconsistencies in
> > xfs_log_calc_max_attrsetm_res() which are described in the cover letter and as
> > part of the commit message of the last patch.
> > 
> 
> Ok, so the fixes to xfs_log_calc_max_attrsetm_res() are what actually
> fixed the test failure? If so, that strikes me as a good independent fix
> candidate (re: refactoring) because the commit log for that one should
> probably describe the observable problem and the fix separate from other
> issues.
> 
> > After a quick chat with Dave on irc, we figured that the best approach was to
> > convert xfs_attr_calc_size() into a helper function so that the size
> > calculations for an xattr set operation is placed in a single function. These
> > values can then be used by other functions like xfs_attr_set() and
> > xfs_log_calc_max_attrsetm_res().
> > 
> > Along the way, I found that the mount time reservation was incorrectly done as
> > well. For E.g. dabtree splits getting accounted as part of mount time
> > reservation was wrong. Due to these reasons and others listed in the cover
> > letter I ended up changing the mount time and run time reservation
> > calculations.
> > 
> > Hence, The reduced reservation sizes are actually a side effect of fixing the
> > inconsistencies.
> > 
> 
> Ok, so most of the rest sounds like bogosity discovered by code
> inspection. That's not that surprising given that transaction
> reservations are worst case values and thus it seems we sometimes get
> away with bogus calculations just so long as the reservations are large
> enough. :)
> 
> As it is, the final result of this patchset looks nice to me, it's just
> a matter of getting there more incrementally to facilitate reviewing the
> changes being made. FWIW, if we do end up with a final "fix the broken
> xattr res calculation" patch at the end of the series, I think it would
> be helpful to have a very deliberate commit log that contains something
> like the following:
> 
> 'The xattr reservation currently consists of:
> 
> 	- superblock
> 	- dabtree * maxdepth
> 	- ...
> 
> This calculation is bogus because it double accounts X as part of Y and
> Z, doesn't account for AGF, etc. etc. ...
> 
> The xattr reservation needs to account the following:
> 
> 	- superblock
> 	- agf
> 	- dabtree * maxdepth
> 	- rmtblocks
> 	- ...
> 
> ... '
>

I agree with both the comments. I will try to get the patchset
going in the direction suggested above.

Thanks for taking time to review the patchset.

-- 
chandan

[PATCH V4 RESEND 4/7] xfs: Introduce struct xfs_attr_set_resv

[Chandan Rajendra]

The intermediate numbers calculated by xfs_attr_calc_size() will be needed by
a future commit to correctly calculate log reservation for xattr set
operation. Towards this goal, this commit introduces 'struct
xfs_attr_set_resv' to collect,
1. Number of dabtree blocks.
2. Number of remote blocks.
3. Number of Bmbt blocks.
4. Total number of blocks we need to reserve.

This will be returned as an out argument by xfs_attr_calc_size().

Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_attr.c | 50 ++++++++++++++++++++++------------------
 fs/xfs/libxfs/xfs_attr.h | 13 +++++++++++
 2 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index a708b142f69b6..921acac71e5d9 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -136,16 +136,14 @@ xfs_attr_get(
 /*
  * Calculate how many blocks we need for the new attribute,
  */
-STATIC int
+STATIC void
 xfs_attr_calc_size(
-	struct xfs_da_args	*args,
-	int			*local)
+	struct xfs_da_args		*args,
+	struct xfs_attr_set_resv	*resv,
+	int				*local)
 {
-	struct xfs_mount	*mp = args->dp->i_mount;
-	unsigned int		total_dablks;
-	unsigned int		bmbt_blks;
-	unsigned int		rmt_blks;
-	int			size;
+	struct xfs_mount		*mp = args->dp->i_mount;
+	int				size;
 
 	/*
 	 * Determine space new attribute will use, and if it would be
@@ -153,25 +151,27 @@ xfs_attr_calc_size(
 	 */
 	size = xfs_attr_leaf_newentsize(args->geo, args->namelen,
 			args->valuelen, local);
-	total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
+	resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
 	if (*local) {
 		if (size > (args->geo->blksize / 2)) {
 			/* Double split possible */
-			total_dablks *= 2;
+			resv->total_dablks *= 2;
 		}
-		rmt_blks = 0;
+		resv->rmt_blks = 0;
 	} else {
 		/*
 		 * Out of line attribute, cannot double split, but
 		 * make room for the attribute value itself.
 		 */
-		rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
+		resv->rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
 	}
 
-	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
-			XFS_ATTR_FORK);
+	resv->bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp,
+				resv->total_dablks + resv->rmt_blks,
+				XFS_ATTR_FORK);
 
-	return total_dablks + rmt_blks + bmbt_blks;
+	resv->alloc_blks = resv->total_dablks + resv->rmt_blks +
+		resv->bmbt_blks;
 }
 
 STATIC int
@@ -295,14 +295,17 @@ xfs_attr_remove_args(
  */
 int
 xfs_attr_set(
-	struct xfs_da_args	*args)
+	struct xfs_da_args		*args)
 {
-	struct xfs_inode	*dp = args->dp;
-	struct xfs_mount	*mp = dp->i_mount;
-	struct xfs_trans_res	tres;
-	bool			rsvd = (args->attr_namespace & XFS_ATTR_ROOT);
-	int			error, local;
-	unsigned int		total;
+	struct xfs_inode		*dp = args->dp;
+	struct xfs_mount		*mp = dp->i_mount;
+	struct xfs_attr_set_resv	resv;
+	struct xfs_trans_res		tres;
+	bool				rsvd;
+	int				error, local;
+	unsigned int			total;
+
+	rsvd = (args->attr_namespace & XFS_ATTR_ROOT);
 
 	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
 		return -EIO;
@@ -326,7 +329,8 @@ xfs_attr_set(
 		XFS_STATS_INC(mp, xs_attr_set);
 
 		args->op_flags |= XFS_DA_OP_ADDNAME;
-		args->total = xfs_attr_calc_size(args, &local);
+		xfs_attr_calc_size(args, &resv, &local);
+		args->total = resv.alloc_blks;
 
 		/*
 		 * If the inode doesn't have an attribute fork, add one.
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 861c81f9bb918..dc08bdfbc9615 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -73,6 +73,19 @@ struct xfs_attr_list_context {
 	int			index;		/* index into output buffer */
 };
 
+struct xfs_attr_set_resv {
+	/* Number of unlogged blocks needed to store the remote attr value. */
+	unsigned int		rmt_blks;
+
+	/* Number of filesystem blocks to allocate for the da btree. */
+	unsigned int		total_dablks;
+
+	/* Blocks we might need to create all the new attr fork mappings. */
+	unsigned int		bmbt_blks;
+
+	/* Total number of blocks we might have to allocate. */
+	unsigned int		alloc_blks;
+};
 
 /*========================================================================
  * Function prototypes for the kernel.
-- 
2.19.1

Re: [PATCH V4 RESEND 4/7] xfs: Introduce struct xfs_attr_set_resv

[Brian Foster]

On Mon, Feb 24, 2020 at 09:30:41AM +0530, Chandan Rajendra wrote:
> The intermediate numbers calculated by xfs_attr_calc_size() will be needed by
> a future commit to correctly calculate log reservation for xattr set
> operation. Towards this goal, this commit introduces 'struct
> xfs_attr_set_resv' to collect,
> 1. Number of dabtree blocks.
> 2. Number of remote blocks.
> 3. Number of Bmbt blocks.
> 4. Total number of blocks we need to reserve.
> 
> This will be returned as an out argument by xfs_attr_calc_size().
> 
> Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 50 ++++++++++++++++++++++------------------
>  fs/xfs/libxfs/xfs_attr.h | 13 +++++++++++
>  2 files changed, 40 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index a708b142f69b6..921acac71e5d9 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -136,16 +136,14 @@ xfs_attr_get(
>  /*
>   * Calculate how many blocks we need for the new attribute,
>   */
> -STATIC int
> +STATIC void
>  xfs_attr_calc_size(
> -	struct xfs_da_args	*args,
> -	int			*local)
> +	struct xfs_da_args		*args,
> +	struct xfs_attr_set_resv	*resv,
> +	int				*local)
>  {
> -	struct xfs_mount	*mp = args->dp->i_mount;
> -	unsigned int		total_dablks;
> -	unsigned int		bmbt_blks;
> -	unsigned int		rmt_blks;
> -	int			size;
> +	struct xfs_mount		*mp = args->dp->i_mount;
> +	int				size;
>  
>  	/*
>  	 * Determine space new attribute will use, and if it would be
> @@ -153,25 +151,27 @@ xfs_attr_calc_size(
>  	 */
>  	size = xfs_attr_leaf_newentsize(args->geo, args->namelen,
>  			args->valuelen, local);
> -	total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> +	resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
>  	if (*local) {
>  		if (size > (args->geo->blksize / 2)) {
>  			/* Double split possible */
> -			total_dablks *= 2;
> +			resv->total_dablks *= 2;
>  		}
> -		rmt_blks = 0;
> +		resv->rmt_blks = 0;
>  	} else {
>  		/*
>  		 * Out of line attribute, cannot double split, but
>  		 * make room for the attribute value itself.
>  		 */
> -		rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
> +		resv->rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
>  	}
>  
> -	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> -			XFS_ATTR_FORK);
> +	resv->bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp,
> +				resv->total_dablks + resv->rmt_blks,
> +				XFS_ATTR_FORK);
>  
> -	return total_dablks + rmt_blks + bmbt_blks;
> +	resv->alloc_blks = resv->total_dablks + resv->rmt_blks +
> +		resv->bmbt_blks;

Do we really need a field to track the total of three other fields in
the same structure? I'd rather just let the caller add them up for
args.total if that's the only usage.

Brian

>  }
>  
>  STATIC int
> @@ -295,14 +295,17 @@ xfs_attr_remove_args(
>   */
>  int
>  xfs_attr_set(
> -	struct xfs_da_args	*args)
> +	struct xfs_da_args		*args)
>  {
> -	struct xfs_inode	*dp = args->dp;
> -	struct xfs_mount	*mp = dp->i_mount;
> -	struct xfs_trans_res	tres;
> -	bool			rsvd = (args->attr_namespace & XFS_ATTR_ROOT);
> -	int			error, local;
> -	unsigned int		total;
> +	struct xfs_inode		*dp = args->dp;
> +	struct xfs_mount		*mp = dp->i_mount;
> +	struct xfs_attr_set_resv	resv;
> +	struct xfs_trans_res		tres;
> +	bool				rsvd;
> +	int				error, local;
> +	unsigned int			total;
> +
> +	rsvd = (args->attr_namespace & XFS_ATTR_ROOT);
>  
>  	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
>  		return -EIO;
> @@ -326,7 +329,8 @@ xfs_attr_set(
>  		XFS_STATS_INC(mp, xs_attr_set);
>  
>  		args->op_flags |= XFS_DA_OP_ADDNAME;
> -		args->total = xfs_attr_calc_size(args, &local);
> +		xfs_attr_calc_size(args, &resv, &local);
> +		args->total = resv.alloc_blks;
>  
>  		/*
>  		 * If the inode doesn't have an attribute fork, add one.
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index 861c81f9bb918..dc08bdfbc9615 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -73,6 +73,19 @@ struct xfs_attr_list_context {
>  	int			index;		/* index into output buffer */
>  };
>  
> +struct xfs_attr_set_resv {
> +	/* Number of unlogged blocks needed to store the remote attr value. */
> +	unsigned int		rmt_blks;
> +
> +	/* Number of filesystem blocks to allocate for the da btree. */
> +	unsigned int		total_dablks;
> +
> +	/* Blocks we might need to create all the new attr fork mappings. */
> +	unsigned int		bmbt_blks;
> +
> +	/* Total number of blocks we might have to allocate. */
> +	unsigned int		alloc_blks;
> +};
>  
>  /*========================================================================
>   * Function prototypes for the kernel.
> -- 
> 2.19.1
> 

Re: [PATCH V4 RESEND 4/7] xfs: Introduce struct xfs_attr_set_resv

[Chandan Rajendra]

On Tuesday, February 25, 2020 9:57 PM Brian Foster wrote: 
> On Mon, Feb 24, 2020 at 09:30:41AM +0530, Chandan Rajendra wrote:
> > The intermediate numbers calculated by xfs_attr_calc_size() will be needed by
> > a future commit to correctly calculate log reservation for xattr set
> > operation. Towards this goal, this commit introduces 'struct
> > xfs_attr_set_resv' to collect,
> > 1. Number of dabtree blocks.
> > 2. Number of remote blocks.
> > 3. Number of Bmbt blocks.
> > 4. Total number of blocks we need to reserve.
> > 
> > This will be returned as an out argument by xfs_attr_calc_size().
> > 
> > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > ---
> >  fs/xfs/libxfs/xfs_attr.c | 50 ++++++++++++++++++++++------------------
> >  fs/xfs/libxfs/xfs_attr.h | 13 +++++++++++
> >  2 files changed, 40 insertions(+), 23 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index a708b142f69b6..921acac71e5d9 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -136,16 +136,14 @@ xfs_attr_get(
> >  /*
> >   * Calculate how many blocks we need for the new attribute,
> >   */
> > -STATIC int
> > +STATIC void
> >  xfs_attr_calc_size(
> > -	struct xfs_da_args	*args,
> > -	int			*local)
> > +	struct xfs_da_args		*args,
> > +	struct xfs_attr_set_resv	*resv,
> > +	int				*local)
> >  {
> > -	struct xfs_mount	*mp = args->dp->i_mount;
> > -	unsigned int		total_dablks;
> > -	unsigned int		bmbt_blks;
> > -	unsigned int		rmt_blks;
> > -	int			size;
> > +	struct xfs_mount		*mp = args->dp->i_mount;
> > +	int				size;
> >  
> >  	/*
> >  	 * Determine space new attribute will use, and if it would be
> > @@ -153,25 +151,27 @@ xfs_attr_calc_size(
> >  	 */
> >  	size = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> >  			args->valuelen, local);
> > -	total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> > +	resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> >  	if (*local) {
> >  		if (size > (args->geo->blksize / 2)) {
> >  			/* Double split possible */
> > -			total_dablks *= 2;
> > +			resv->total_dablks *= 2;
> >  		}
> > -		rmt_blks = 0;
> > +		resv->rmt_blks = 0;
> >  	} else {
> >  		/*
> >  		 * Out of line attribute, cannot double split, but
> >  		 * make room for the attribute value itself.
> >  		 */
> > -		rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
> > +		resv->rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
> >  	}
> >  
> > -	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> > -			XFS_ATTR_FORK);
> > +	resv->bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp,
> > +				resv->total_dablks + resv->rmt_blks,
> > +				XFS_ATTR_FORK);
> >  
> > -	return total_dablks + rmt_blks + bmbt_blks;
> > +	resv->alloc_blks = resv->total_dablks + resv->rmt_blks +
> > +		resv->bmbt_blks;
> 
> Do we really need a field to track the total of three other fields in
> the same structure? I'd rather just let the caller add them up for
> args.total if that's the only usage.
> 

You are right. The caller can derive total blocks from the other components. I
will fix this up in the next iteration of the patchset.

-- 
chandan

[PATCH V4 RESEND 5/7] xfs: xfs_attr_calc_size: Explicitly pass mp, namelen and valuelen args

[Chandan Rajendra]

In a future commit, xfs_attr_calc_size() will be invoked from a function that
does not have a 'struct xfs_da_args' handy. Hence this commit changes
xfs_attr_calc_size() to let invokers to explicitly pass 'struct xfs_mount *',
namelen and valuelen arguments.

Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_attr.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 921acac71e5d9..f781724bf85ce 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -138,22 +138,24 @@ xfs_attr_get(
  */
 STATIC void
 xfs_attr_calc_size(
-	struct xfs_da_args		*args,
+	struct xfs_mount		*mp,
 	struct xfs_attr_set_resv	*resv,
+	int				namelen,
+	int				valuelen,
 	int				*local)
 {
-	struct xfs_mount		*mp = args->dp->i_mount;
+	unsigned int			blksize = mp->m_attr_geo->blksize;
 	int				size;
 
 	/*
 	 * Determine space new attribute will use, and if it would be
 	 * "local" or "remote" (note: local != inline).
 	 */
-	size = xfs_attr_leaf_newentsize(args->geo, args->namelen,
-			args->valuelen, local);
+	size = xfs_attr_leaf_newentsize(mp->m_attr_geo, namelen, valuelen,
+			local);
 	resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
 	if (*local) {
-		if (size > (args->geo->blksize / 2)) {
+		if (size > (blksize / 2)) {
 			/* Double split possible */
 			resv->total_dablks *= 2;
 		}
@@ -163,7 +165,7 @@ xfs_attr_calc_size(
 		 * Out of line attribute, cannot double split, but
 		 * make room for the attribute value itself.
 		 */
-		resv->rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
+		resv->rmt_blks = xfs_attr3_rmt_blocks(mp, valuelen);
 	}
 
 	resv->bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp,
@@ -329,7 +331,8 @@ xfs_attr_set(
 		XFS_STATS_INC(mp, xs_attr_set);
 
 		args->op_flags |= XFS_DA_OP_ADDNAME;
-		xfs_attr_calc_size(args, &resv, &local);
+		xfs_attr_calc_size(mp, &resv, args->namelen, args->valuelen,
+				&local);
 		args->total = resv.alloc_blks;
 
 		/*
-- 
2.19.1

Re: [PATCH V4 RESEND 5/7] xfs: xfs_attr_calc_size: Explicitly pass mp, namelen and valuelen args

[Brian Foster]

On Mon, Feb 24, 2020 at 09:30:42AM +0530, Chandan Rajendra wrote:
> In a future commit, xfs_attr_calc_size() will be invoked from a function that
> does not have a 'struct xfs_da_args' handy. Hence this commit changes
> xfs_attr_calc_size() to let invokers to explicitly pass 'struct xfs_mount *',
> namelen and valuelen arguments.
> 
> Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> ---

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

>  fs/xfs/libxfs/xfs_attr.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 921acac71e5d9..f781724bf85ce 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -138,22 +138,24 @@ xfs_attr_get(
>   */
>  STATIC void
>  xfs_attr_calc_size(
> -	struct xfs_da_args		*args,
> +	struct xfs_mount		*mp,
>  	struct xfs_attr_set_resv	*resv,
> +	int				namelen,
> +	int				valuelen,
>  	int				*local)
>  {
> -	struct xfs_mount		*mp = args->dp->i_mount;
> +	unsigned int			blksize = mp->m_attr_geo->blksize;
>  	int				size;
>  
>  	/*
>  	 * Determine space new attribute will use, and if it would be
>  	 * "local" or "remote" (note: local != inline).
>  	 */
> -	size = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> -			args->valuelen, local);
> +	size = xfs_attr_leaf_newentsize(mp->m_attr_geo, namelen, valuelen,
> +			local);
>  	resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
>  	if (*local) {
> -		if (size > (args->geo->blksize / 2)) {
> +		if (size > (blksize / 2)) {
>  			/* Double split possible */
>  			resv->total_dablks *= 2;
>  		}
> @@ -163,7 +165,7 @@ xfs_attr_calc_size(
>  		 * Out of line attribute, cannot double split, but
>  		 * make room for the attribute value itself.
>  		 */
> -		resv->rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
> +		resv->rmt_blks = xfs_attr3_rmt_blocks(mp, valuelen);
>  	}
>  
>  	resv->bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp,
> @@ -329,7 +331,8 @@ xfs_attr_set(
>  		XFS_STATS_INC(mp, xs_attr_set);
>  
>  		args->op_flags |= XFS_DA_OP_ADDNAME;
> -		xfs_attr_calc_size(args, &resv, &local);
> +		xfs_attr_calc_size(mp, &resv, args->namelen, args->valuelen,
> +				&local);
>  		args->total = resv.alloc_blks;
>  
>  		/*
> -- 
> 2.19.1
> 

[PATCH V4 RESEND 6/7] xfs: Make xfs_attr_calc_size() non-static

[Chandan Rajendra]

A future commit will cause xfs_attr_calc_size() to be invoked from a function
defined in another file. Hence this commit makes xfs_attr_calc_size() as
non-static.

Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_attr.c | 58 ++++++++++++++++++++--------------------
 fs/xfs/libxfs/xfs_attr.h |  2 ++
 2 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index f781724bf85ce..1d62ce80d7949 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -133,10 +133,38 @@ xfs_attr_get(
 	return error;
 }
 
+STATIC int
+xfs_attr_try_sf_addname(
+	struct xfs_inode	*dp,
+	struct xfs_da_args	*args)
+{
+
+	struct xfs_mount	*mp = dp->i_mount;
+	int			error, error2;
+
+	error = xfs_attr_shortform_addname(args);
+	if (error == -ENOSPC)
+		return error;
+
+	/*
+	 * Commit the shortform mods, and we're done.
+	 * NOTE: this is also the error path (EEXIST, etc).
+	 */
+	if (!error && !(args->op_flags & XFS_DA_OP_NOTIME))
+		xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
+
+	if (mp->m_flags & XFS_MOUNT_WSYNC)
+		xfs_trans_set_sync(args->trans);
+
+	error2 = xfs_trans_commit(args->trans);
+	args->trans = NULL;
+	return error ? error : error2;
+}
+
 /*
  * Calculate how many blocks we need for the new attribute,
  */
-STATIC void
+void
 xfs_attr_calc_size(
 	struct xfs_mount		*mp,
 	struct xfs_attr_set_resv	*resv,
@@ -176,34 +204,6 @@ xfs_attr_calc_size(
 		resv->bmbt_blks;
 }
 
-STATIC int
-xfs_attr_try_sf_addname(
-	struct xfs_inode	*dp,
-	struct xfs_da_args	*args)
-{
-
-	struct xfs_mount	*mp = dp->i_mount;
-	int			error, error2;
-
-	error = xfs_attr_shortform_addname(args);
-	if (error == -ENOSPC)
-		return error;
-
-	/*
-	 * Commit the shortform mods, and we're done.
-	 * NOTE: this is also the error path (EEXIST, etc).
-	 */
-	if (!error && !(args->op_flags & XFS_DA_OP_NOTIME))
-		xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
-
-	if (mp->m_flags & XFS_MOUNT_WSYNC)
-		xfs_trans_set_sync(args->trans);
-
-	error2 = xfs_trans_commit(args->trans);
-	args->trans = NULL;
-	return error ? error : error2;
-}
-
 /*
  * Set the attribute specified in @args.
  */
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index dc08bdfbc9615..0e387230744c3 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -104,5 +104,7 @@ int xfs_attr_set(struct xfs_da_args *args);
 int xfs_attr_set_args(struct xfs_da_args *args);
 int xfs_attr_remove_args(struct xfs_da_args *args);
 bool xfs_attr_namecheck(const void *name, size_t length);
+void xfs_attr_calc_size(struct xfs_mount *mp, struct xfs_attr_set_resv *resv,
+		int namelen, int valuelen, int *local);
 
 #endif	/* __XFS_ATTR_H__ */
-- 
2.19.1

Re: [PATCH V4 RESEND 6/7] xfs: Make xfs_attr_calc_size() non-static

[Darrick J. Wong]

On Mon, Feb 24, 2020 at 09:30:43AM +0530, Chandan Rajendra wrote:
> A future commit will cause xfs_attr_calc_size() to be invoked from a function
> defined in another file. Hence this commit makes xfs_attr_calc_size() as
> non-static.
> 
> Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 58 ++++++++++++++++++++--------------------
>  fs/xfs/libxfs/xfs_attr.h |  2 ++
>  2 files changed, 31 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index f781724bf85ce..1d62ce80d7949 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -133,10 +133,38 @@ xfs_attr_get(
>  	return error;
>  }
>  
> +STATIC int
> +xfs_attr_try_sf_addname(

Why does this function move?

--D

> +	struct xfs_inode	*dp,
> +	struct xfs_da_args	*args)
> +{
> +
> +	struct xfs_mount	*mp = dp->i_mount;
> +	int			error, error2;
> +
> +	error = xfs_attr_shortform_addname(args);
> +	if (error == -ENOSPC)
> +		return error;
> +
> +	/*
> +	 * Commit the shortform mods, and we're done.
> +	 * NOTE: this is also the error path (EEXIST, etc).
> +	 */
> +	if (!error && !(args->op_flags & XFS_DA_OP_NOTIME))
> +		xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
> +
> +	if (mp->m_flags & XFS_MOUNT_WSYNC)
> +		xfs_trans_set_sync(args->trans);
> +
> +	error2 = xfs_trans_commit(args->trans);
> +	args->trans = NULL;
> +	return error ? error : error2;
> +}
> +
>  /*
>   * Calculate how many blocks we need for the new attribute,
>   */
> -STATIC void
> +void
>  xfs_attr_calc_size(
>  	struct xfs_mount		*mp,
>  	struct xfs_attr_set_resv	*resv,
> @@ -176,34 +204,6 @@ xfs_attr_calc_size(
>  		resv->bmbt_blks;
>  }
>  
> -STATIC int
> -xfs_attr_try_sf_addname(
> -	struct xfs_inode	*dp,
> -	struct xfs_da_args	*args)
> -{
> -
> -	struct xfs_mount	*mp = dp->i_mount;
> -	int			error, error2;
> -
> -	error = xfs_attr_shortform_addname(args);
> -	if (error == -ENOSPC)
> -		return error;
> -
> -	/*
> -	 * Commit the shortform mods, and we're done.
> -	 * NOTE: this is also the error path (EEXIST, etc).
> -	 */
> -	if (!error && !(args->op_flags & XFS_DA_OP_NOTIME))
> -		xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
> -
> -	if (mp->m_flags & XFS_MOUNT_WSYNC)
> -		xfs_trans_set_sync(args->trans);
> -
> -	error2 = xfs_trans_commit(args->trans);
> -	args->trans = NULL;
> -	return error ? error : error2;
> -}
> -
>  /*
>   * Set the attribute specified in @args.
>   */
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index dc08bdfbc9615..0e387230744c3 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -104,5 +104,7 @@ int xfs_attr_set(struct xfs_da_args *args);
>  int xfs_attr_set_args(struct xfs_da_args *args);
>  int xfs_attr_remove_args(struct xfs_da_args *args);
>  bool xfs_attr_namecheck(const void *name, size_t length);
> +void xfs_attr_calc_size(struct xfs_mount *mp, struct xfs_attr_set_resv *resv,
> +		int namelen, int valuelen, int *local);
>  
>  #endif	/* __XFS_ATTR_H__ */
> -- 
> 2.19.1
> 

[PATCH V4 RESEND 7/7] xfs: Fix log reservation calculation for xattr insert operation

[Chandan Rajendra]

Log space reservation for xattr insert operation can be divided into two
parts,
1. Mount time
   - Inode
   - Superblock for accounting space allocations
   - AGF for accounting space used by count, block number, rmap and refcnt
     btrees.

2. The remaining log space can only be calculated at run time because,
   - A local xattr can be large enough to cause a double split of the dabtree.
   - The value of the xattr can be large enough to be stored in remote
     blocks. The contents of the remote blocks are not logged.

   The log space reservation could be,
   - 2 * XFS_DA_NODE_MAXDEPTH number of blocks. Additional XFS_DA_NODE_MAXDEPTH
     number of blocks are required if xattr is large enough to cause another
     split of the dabtree path from root to leaf block.
   - BMBT blocks for storing (2 * XFS_DA_NODE_MAXDEPTH) record
     entries. Additional XFS_DA_NODE_MAXDEPTH number of blocks are required in
     case of a double split of the dabtree path from root to leaf blocks.
   - Space for logging blocks of count, block number, rmap and refcnt btrees.

Presently, mount time log reservation includes block count required for a
single split of the dabtree. The dabtree block count is also taken into
account by xfs_attr_calc_size().

Also, AGF log space reservation isn't accounted for.

Due to the reasons mentioned above, log reservation calculation for xattr
insert operation gives an incorrect value.

Apart from the above, xfs_log_calc_max_attrsetm_res() passes byte count as
an argument to XFS_NEXTENTADD_SPACE_RES() instead of block count.

To fix these issues, this commit changes xfs_attr_calc_size() to also
calculate the number of dabtree blocks that need to be logged.

xfs_attr_set() uses the following values computed by xfs_attr_calc_size()
1. The number of dabtree blocks that need to be logged.
2. The number of remote blocks that need to be allocated.
3. The number of dabtree blocks that need to be allocated.
4. The number of bmbt blocks that need to be allocated.
5. The total number of blocks that need to be allocated.

... to compute number of bytes that need to be reserved in the log.

This commit also modifies xfs_log_calc_max_attrsetm_res() to invoke
xfs_attr_calc_size() to obtain the number of blocks to be logged which it uses
to figure out the total number of bytes to be logged.

Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_attr.c       |  7 +++--
 fs/xfs/libxfs/xfs_attr.h       |  3 ++
 fs/xfs/libxfs/xfs_log_rlimit.c | 16 +++++------
 fs/xfs/libxfs/xfs_trans_resv.c | 50 ++++++++++++++++------------------
 fs/xfs/libxfs/xfs_trans_resv.h |  2 ++
 5 files changed, 41 insertions(+), 37 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 1d62ce80d7949..f056f8597ee03 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -182,9 +182,12 @@ xfs_attr_calc_size(
 	size = xfs_attr_leaf_newentsize(mp->m_attr_geo, namelen, valuelen,
 			local);
 	resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
+	resv->log_dablks = 2 * resv->total_dablks;
+
 	if (*local) {
 		if (size > (blksize / 2)) {
 			/* Double split possible */
+			resv->log_dablks += resv->total_dablks;
 			resv->total_dablks *= 2;
 		}
 		resv->rmt_blks = 0;
@@ -349,9 +352,7 @@ xfs_attr_set(
 				return error;
 		}
 
-		tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
-				 M_RES(mp)->tr_attrsetrt.tr_logres *
-					args->total;
+		tres.tr_logres = xfs_calc_attr_res(mp, &resv);
 		tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
 		tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
 		total = args->total;
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 0e387230744c3..83508148bbd12 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -74,6 +74,9 @@ struct xfs_attr_list_context {
 };
 
 struct xfs_attr_set_resv {
+	/* Number of blocks in the da btree that we might need to log. */
+	unsigned int		log_dablks;
+
 	/* Number of unlogged blocks needed to store the remote attr value. */
 	unsigned int		rmt_blks;
 
diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c b/fs/xfs/libxfs/xfs_log_rlimit.c
index 7f55eb3f36536..a132ffa7adf32 100644
--- a/fs/xfs/libxfs/xfs_log_rlimit.c
+++ b/fs/xfs/libxfs/xfs_log_rlimit.c
@@ -10,6 +10,7 @@
 #include "xfs_log_format.h"
 #include "xfs_trans_resv.h"
 #include "xfs_mount.h"
+#include "xfs_attr.h"
 #include "xfs_da_format.h"
 #include "xfs_trans_space.h"
 #include "xfs_da_btree.h"
@@ -21,19 +22,18 @@
  */
 STATIC int
 xfs_log_calc_max_attrsetm_res(
-	struct xfs_mount	*mp)
+	struct xfs_mount		*mp)
 {
-	int			size;
-	int			nblks;
+	struct xfs_attr_set_resv	resv;
+	int				size;
+	int				local;
 
 	size = xfs_attr_leaf_entsize_local_max(mp->m_attr_geo->blksize) -
 	       MAXNAMELEN - 1;
-	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
-	nblks += XFS_B_TO_FSB(mp, size);
-	nblks += XFS_NEXTENTADD_SPACE_RES(mp, size, XFS_ATTR_FORK);
+	xfs_attr_calc_size(mp, &resv, size, 0, &local);
+	ASSERT(local == 1);
 
-	return  M_RES(mp)->tr_attrsetm.tr_logres +
-		M_RES(mp)->tr_attrsetrt.tr_logres * nblks;
+	return xfs_calc_attr_res(mp, &resv);
 }
 
 /*
diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index 7a9c04920505a..c6b8cd56df2d7 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -19,6 +19,7 @@
 #include "xfs_trans.h"
 #include "xfs_qm.h"
 #include "xfs_trans_space.h"
+#include "xfs_attr.h"
 
 #define _ALLOC	true
 #define _FREE	false
@@ -701,12 +702,10 @@ xfs_calc_attrinval_reservation(
  * Setting an attribute at mount time.
  *	the inode getting the attribute
  *	the superblock for allocations
- *	the agfs extents are allocated from
- *	the attribute btree * max depth
- *	the inode allocation btree
+ *	the agf extents are allocated from
  * Since attribute transaction space is dependent on the size of the attribute,
  * the calculation is done partially at mount time and partially at runtime(see
- * below).
+ * xfs_attr_calc_size()).
  */
 STATIC uint
 xfs_calc_attrsetm_reservation(
@@ -714,27 +713,7 @@ xfs_calc_attrsetm_reservation(
 {
 	return XFS_DQUOT_LOGRES(mp) +
 		xfs_calc_inode_res(mp, 1) +
-		xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
-		xfs_calc_buf_res(XFS_DA_NODE_MAXDEPTH, XFS_FSB_TO_B(mp, 1));
-}
-
-/*
- * Setting an attribute at runtime, transaction space unit per block.
- * 	the superblock for allocations: sector size
- *	the inode bmap btree could join or split: max depth * block size
- * Since the runtime attribute transaction space is dependent on the total
- * blocks needed for the 1st bmap, here we calculate out the space unit for
- * one block so that the caller could figure out the total space according
- * to the attibute extent length in blocks by:
- *	ext * M_RES(mp)->tr_attrsetrt.tr_logres
- */
-STATIC uint
-xfs_calc_attrsetrt_reservation(
-	struct xfs_mount	*mp)
-{
-	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
-		xfs_calc_buf_res(XFS_BM_MAXLEVELS(mp, XFS_ATTR_FORK),
-				 XFS_FSB_TO_B(mp, 1));
+		xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
 }
 
 /*
@@ -832,6 +811,25 @@ xfs_calc_sb_reservation(
 	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
 }
 
+uint
+xfs_calc_attr_res(
+	struct xfs_mount		*mp,
+	struct xfs_attr_set_resv	*resv)
+{
+	unsigned int			space_blks;
+	unsigned int			attr_res;
+
+	space_blks = xfs_allocfree_log_count(mp,
+		resv->total_dablks + resv->bmbt_blks);
+
+	attr_res = M_RES(mp)->tr_attrsetm.tr_logres +
+		xfs_calc_buf_res(resv->log_dablks, mp->m_attr_geo->blksize) +
+		xfs_calc_buf_res(resv->bmbt_blks, mp->m_sb.sb_blocksize) +
+		xfs_calc_buf_res(space_blks, mp->m_sb.sb_blocksize);
+
+	return attr_res;
+}
+
 void
 xfs_trans_resv_calc(
 	struct xfs_mount	*mp,
@@ -942,7 +940,7 @@ xfs_trans_resv_calc(
 	resp->tr_ichange.tr_logres = xfs_calc_ichange_reservation(mp);
 	resp->tr_fsyncts.tr_logres = xfs_calc_swrite_reservation(mp);
 	resp->tr_writeid.tr_logres = xfs_calc_writeid_reservation(mp);
-	resp->tr_attrsetrt.tr_logres = xfs_calc_attrsetrt_reservation(mp);
+	resp->tr_attrsetrt.tr_logres = 0;
 	resp->tr_clearagi.tr_logres = xfs_calc_clear_agi_bucket_reservation(mp);
 	resp->tr_growrtzero.tr_logres = xfs_calc_growrtzero_reservation(mp);
 	resp->tr_growrtfree.tr_logres = xfs_calc_growrtfree_reservation(mp);
diff --git a/fs/xfs/libxfs/xfs_trans_resv.h b/fs/xfs/libxfs/xfs_trans_resv.h
index 7241ab28cf84f..3a6a0bf21e9b1 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.h
+++ b/fs/xfs/libxfs/xfs_trans_resv.h
@@ -7,6 +7,7 @@
 #define	__XFS_TRANS_RESV_H__
 
 struct xfs_mount;
+struct xfs_attr_set_resv;
 
 /*
  * structure for maintaining pre-calculated transaction reservations.
@@ -91,6 +92,7 @@ struct xfs_trans_resv {
 #define	XFS_ATTRSET_LOG_COUNT		3
 #define	XFS_ATTRRM_LOG_COUNT		3
 
+uint xfs_calc_attr_res(struct xfs_mount *mp, struct xfs_attr_set_resv *resv);
 void xfs_trans_resv_calc(struct xfs_mount *mp, struct xfs_trans_resv *resp);
 uint xfs_allocfree_log_count(struct xfs_mount *mp, uint num_ops);
 
-- 
2.19.1

Re: [PATCH V4 RESEND 7/7] xfs: Fix log reservation calculation for xattr insert operation

[Brian Foster]

On Mon, Feb 24, 2020 at 09:30:44AM +0530, Chandan Rajendra wrote:
> Log space reservation for xattr insert operation can be divided into two
> parts,
> 1. Mount time
>    - Inode
>    - Superblock for accounting space allocations
>    - AGF for accounting space used by count, block number, rmap and refcnt
>      btrees.
> 
> 2. The remaining log space can only be calculated at run time because,
>    - A local xattr can be large enough to cause a double split of the dabtree.
>    - The value of the xattr can be large enough to be stored in remote
>      blocks. The contents of the remote blocks are not logged.
> 
>    The log space reservation could be,
>    - 2 * XFS_DA_NODE_MAXDEPTH number of blocks. Additional XFS_DA_NODE_MAXDEPTH
>      number of blocks are required if xattr is large enough to cause another
>      split of the dabtree path from root to leaf block.
>    - BMBT blocks for storing (2 * XFS_DA_NODE_MAXDEPTH) record
>      entries. Additional XFS_DA_NODE_MAXDEPTH number of blocks are required in
>      case of a double split of the dabtree path from root to leaf blocks.
>    - Space for logging blocks of count, block number, rmap and refcnt btrees.
> 
> Presently, mount time log reservation includes block count required for a
> single split of the dabtree. The dabtree block count is also taken into
> account by xfs_attr_calc_size().
> 
> Also, AGF log space reservation isn't accounted for.
> 
> Due to the reasons mentioned above, log reservation calculation for xattr
> insert operation gives an incorrect value.
> 
> Apart from the above, xfs_log_calc_max_attrsetm_res() passes byte count as
> an argument to XFS_NEXTENTADD_SPACE_RES() instead of block count.
> 
> To fix these issues, this commit changes xfs_attr_calc_size() to also
> calculate the number of dabtree blocks that need to be logged.
> 
> xfs_attr_set() uses the following values computed by xfs_attr_calc_size()
> 1. The number of dabtree blocks that need to be logged.
> 2. The number of remote blocks that need to be allocated.
> 3. The number of dabtree blocks that need to be allocated.
> 4. The number of bmbt blocks that need to be allocated.
> 5. The total number of blocks that need to be allocated.
> 
> ... to compute number of bytes that need to be reserved in the log.
> 
> This commit also modifies xfs_log_calc_max_attrsetm_res() to invoke
> xfs_attr_calc_size() to obtain the number of blocks to be logged which it uses
> to figure out the total number of bytes to be logged.
> 
> Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c       |  7 +++--
>  fs/xfs/libxfs/xfs_attr.h       |  3 ++
>  fs/xfs/libxfs/xfs_log_rlimit.c | 16 +++++------
>  fs/xfs/libxfs/xfs_trans_resv.c | 50 ++++++++++++++++------------------
>  fs/xfs/libxfs/xfs_trans_resv.h |  2 ++
>  5 files changed, 41 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 1d62ce80d7949..f056f8597ee03 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -182,9 +182,12 @@ xfs_attr_calc_size(
>  	size = xfs_attr_leaf_newentsize(mp->m_attr_geo, namelen, valuelen,
>  			local);
>  	resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> +	resv->log_dablks = 2 * resv->total_dablks;
> +
>  	if (*local) {
>  		if (size > (blksize / 2)) {
>  			/* Double split possible */
> +			resv->log_dablks += resv->total_dablks;

I'm not quite clear on why log is double the total above, then we add an
increment of total here. Can you elaborate on this?

>  			resv->total_dablks *= 2;
>  		}
>  		resv->rmt_blks = 0;
> @@ -349,9 +352,7 @@ xfs_attr_set(
>  				return error;
>  		}
>  
> -		tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
> -				 M_RES(mp)->tr_attrsetrt.tr_logres *
> -					args->total;
> +		tres.tr_logres = xfs_calc_attr_res(mp, &resv);

So instead of using the runtime reservation calculation, we call this
new function that uses the block res structure and calculates log
reservation for us. Seems like a decent cleanup, but I'm still having
some trouble determining what is cleanup vs. what is fixing the res
calculation.

>  		tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
>  		tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
>  		total = args->total;
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index 0e387230744c3..83508148bbd12 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -74,6 +74,9 @@ struct xfs_attr_list_context {
>  };
>  
>  struct xfs_attr_set_resv {
> +	/* Number of blocks in the da btree that we might need to log. */
> +	unsigned int		log_dablks;
> +
>  	/* Number of unlogged blocks needed to store the remote attr value. */
>  	unsigned int		rmt_blks;
>  
> diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c b/fs/xfs/libxfs/xfs_log_rlimit.c
> index 7f55eb3f36536..a132ffa7adf32 100644
> --- a/fs/xfs/libxfs/xfs_log_rlimit.c
> +++ b/fs/xfs/libxfs/xfs_log_rlimit.c
> @@ -10,6 +10,7 @@
>  #include "xfs_log_format.h"
>  #include "xfs_trans_resv.h"
>  #include "xfs_mount.h"
> +#include "xfs_attr.h"
>  #include "xfs_da_format.h"
>  #include "xfs_trans_space.h"
>  #include "xfs_da_btree.h"
> @@ -21,19 +22,18 @@
>   */
>  STATIC int
>  xfs_log_calc_max_attrsetm_res(
> -	struct xfs_mount	*mp)
> +	struct xfs_mount		*mp)
>  {
> -	int			size;
> -	int			nblks;
> +	struct xfs_attr_set_resv	resv;
> +	int				size;
> +	int				local;
>  
>  	size = xfs_attr_leaf_entsize_local_max(mp->m_attr_geo->blksize) -
>  	       MAXNAMELEN - 1;
> -	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
> -	nblks += XFS_B_TO_FSB(mp, size);
> -	nblks += XFS_NEXTENTADD_SPACE_RES(mp, size, XFS_ATTR_FORK);
> +	xfs_attr_calc_size(mp, &resv, size, 0, &local);
> +	ASSERT(local == 1);
>  
> -	return  M_RES(mp)->tr_attrsetm.tr_logres +
> -		M_RES(mp)->tr_attrsetrt.tr_logres * nblks;
> +	return xfs_calc_attr_res(mp, &resv);
>  }
>  
>  /*
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 7a9c04920505a..c6b8cd56df2d7 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -19,6 +19,7 @@
>  #include "xfs_trans.h"
>  #include "xfs_qm.h"
>  #include "xfs_trans_space.h"
> +#include "xfs_attr.h"
>  
>  #define _ALLOC	true
>  #define _FREE	false
> @@ -701,12 +702,10 @@ xfs_calc_attrinval_reservation(
>   * Setting an attribute at mount time.
>   *	the inode getting the attribute
>   *	the superblock for allocations
> - *	the agfs extents are allocated from
> - *	the attribute btree * max depth
> - *	the inode allocation btree
> + *	the agf extents are allocated from
>   * Since attribute transaction space is dependent on the size of the attribute,
>   * the calculation is done partially at mount time and partially at runtime(see
> - * below).
> + * xfs_attr_calc_size()).
>   */
>  STATIC uint
>  xfs_calc_attrsetm_reservation(
> @@ -714,27 +713,7 @@ xfs_calc_attrsetm_reservation(
>  {
>  	return XFS_DQUOT_LOGRES(mp) +
>  		xfs_calc_inode_res(mp, 1) +
> -		xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> -		xfs_calc_buf_res(XFS_DA_NODE_MAXDEPTH, XFS_FSB_TO_B(mp, 1));
> -}
> -
> -/*
> - * Setting an attribute at runtime, transaction space unit per block.
> - * 	the superblock for allocations: sector size
> - *	the inode bmap btree could join or split: max depth * block size
> - * Since the runtime attribute transaction space is dependent on the total
> - * blocks needed for the 1st bmap, here we calculate out the space unit for
> - * one block so that the caller could figure out the total space according
> - * to the attibute extent length in blocks by:
> - *	ext * M_RES(mp)->tr_attrsetrt.tr_logres
> - */
> -STATIC uint
> -xfs_calc_attrsetrt_reservation(
> -	struct xfs_mount	*mp)
> -{
> -	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> -		xfs_calc_buf_res(XFS_BM_MAXLEVELS(mp, XFS_ATTR_FORK),
> -				 XFS_FSB_TO_B(mp, 1));
> +		xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
>  }
>  
>  /*
> @@ -832,6 +811,25 @@ xfs_calc_sb_reservation(
>  	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
>  }
>  
> +uint
> +xfs_calc_attr_res(
> +	struct xfs_mount		*mp,
> +	struct xfs_attr_set_resv	*resv)
> +{
> +	unsigned int			space_blks;
> +	unsigned int			attr_res;
> +
> +	space_blks = xfs_allocfree_log_count(mp,
> +		resv->total_dablks + resv->bmbt_blks);
> +
> +	attr_res = M_RES(mp)->tr_attrsetm.tr_logres +
> +		xfs_calc_buf_res(resv->log_dablks, mp->m_attr_geo->blksize) +
> +		xfs_calc_buf_res(resv->bmbt_blks, mp->m_sb.sb_blocksize) +
> +		xfs_calc_buf_res(space_blks, mp->m_sb.sb_blocksize);
> +
> +	return attr_res;
> +}

This function could use a header comment to explain what the reservation
covers.

> +
>  void
>  xfs_trans_resv_calc(
>  	struct xfs_mount	*mp,
> @@ -942,7 +940,7 @@ xfs_trans_resv_calc(
>  	resp->tr_ichange.tr_logres = xfs_calc_ichange_reservation(mp);
>  	resp->tr_fsyncts.tr_logres = xfs_calc_swrite_reservation(mp);
>  	resp->tr_writeid.tr_logres = xfs_calc_writeid_reservation(mp);
> -	resp->tr_attrsetrt.tr_logres = xfs_calc_attrsetrt_reservation(mp);
> +	resp->tr_attrsetrt.tr_logres = 0;

Should this go away if it's going to end up as zero?

Brian

>  	resp->tr_clearagi.tr_logres = xfs_calc_clear_agi_bucket_reservation(mp);
>  	resp->tr_growrtzero.tr_logres = xfs_calc_growrtzero_reservation(mp);
>  	resp->tr_growrtfree.tr_logres = xfs_calc_growrtfree_reservation(mp);
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.h b/fs/xfs/libxfs/xfs_trans_resv.h
> index 7241ab28cf84f..3a6a0bf21e9b1 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.h
> +++ b/fs/xfs/libxfs/xfs_trans_resv.h
> @@ -7,6 +7,7 @@
>  #define	__XFS_TRANS_RESV_H__
>  
>  struct xfs_mount;
> +struct xfs_attr_set_resv;
>  
>  /*
>   * structure for maintaining pre-calculated transaction reservations.
> @@ -91,6 +92,7 @@ struct xfs_trans_resv {
>  #define	XFS_ATTRSET_LOG_COUNT		3
>  #define	XFS_ATTRRM_LOG_COUNT		3
>  
> +uint xfs_calc_attr_res(struct xfs_mount *mp, struct xfs_attr_set_resv *resv);
>  void xfs_trans_resv_calc(struct xfs_mount *mp, struct xfs_trans_resv *resp);
>  uint xfs_allocfree_log_count(struct xfs_mount *mp, uint num_ops);
>  
> -- 
> 2.19.1
> 

Re: [PATCH V4 RESEND 7/7] xfs: Fix log reservation calculation for xattr insert operation

[Chandan Rajendra]

On Tuesday, February 25, 2020 10:49 PM Brian Foster wrote: 
> On Mon, Feb 24, 2020 at 09:30:44AM +0530, Chandan Rajendra wrote:
> > Log space reservation for xattr insert operation can be divided into two
> > parts,
> > 1. Mount time
> >    - Inode
> >    - Superblock for accounting space allocations
> >    - AGF for accounting space used by count, block number, rmap and refcnt
> >      btrees.
> > 
> > 2. The remaining log space can only be calculated at run time because,
> >    - A local xattr can be large enough to cause a double split of the dabtree.
> >    - The value of the xattr can be large enough to be stored in remote
> >      blocks. The contents of the remote blocks are not logged.
> > 
> >    The log space reservation could be,
> >    - 2 * XFS_DA_NODE_MAXDEPTH number of blocks. Additional XFS_DA_NODE_MAXDEPTH
> >      number of blocks are required if xattr is large enough to cause another
> >      split of the dabtree path from root to leaf block.
> >    - BMBT blocks for storing (2 * XFS_DA_NODE_MAXDEPTH) record
> >      entries. Additional XFS_DA_NODE_MAXDEPTH number of blocks are required in
> >      case of a double split of the dabtree path from root to leaf blocks.
> >    - Space for logging blocks of count, block number, rmap and refcnt btrees.
> > 
> > Presently, mount time log reservation includes block count required for a
> > single split of the dabtree. The dabtree block count is also taken into
> > account by xfs_attr_calc_size().
> > 
> > Also, AGF log space reservation isn't accounted for.
> > 
> > Due to the reasons mentioned above, log reservation calculation for xattr
> > insert operation gives an incorrect value.
> > 
> > Apart from the above, xfs_log_calc_max_attrsetm_res() passes byte count as
> > an argument to XFS_NEXTENTADD_SPACE_RES() instead of block count.
> > 
> > To fix these issues, this commit changes xfs_attr_calc_size() to also
> > calculate the number of dabtree blocks that need to be logged.
> > 
> > xfs_attr_set() uses the following values computed by xfs_attr_calc_size()
> > 1. The number of dabtree blocks that need to be logged.
> > 2. The number of remote blocks that need to be allocated.
> > 3. The number of dabtree blocks that need to be allocated.
> > 4. The number of bmbt blocks that need to be allocated.
> > 5. The total number of blocks that need to be allocated.
> > 
> > ... to compute number of bytes that need to be reserved in the log.
> > 
> > This commit also modifies xfs_log_calc_max_attrsetm_res() to invoke
> > xfs_attr_calc_size() to obtain the number of blocks to be logged which it uses
> > to figure out the total number of bytes to be logged.
> > 
> > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > ---
> >  fs/xfs/libxfs/xfs_attr.c       |  7 +++--
> >  fs/xfs/libxfs/xfs_attr.h       |  3 ++
> >  fs/xfs/libxfs/xfs_log_rlimit.c | 16 +++++------
> >  fs/xfs/libxfs/xfs_trans_resv.c | 50 ++++++++++++++++------------------
> >  fs/xfs/libxfs/xfs_trans_resv.h |  2 ++
> >  5 files changed, 41 insertions(+), 37 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 1d62ce80d7949..f056f8597ee03 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -182,9 +182,12 @@ xfs_attr_calc_size(
> >  	size = xfs_attr_leaf_newentsize(mp->m_attr_geo, namelen, valuelen,
> >  			local);
> >  	resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> > +	resv->log_dablks = 2 * resv->total_dablks;
> > +
> >  	if (*local) {
> >  		if (size > (blksize / 2)) {
> >  			/* Double split possible */
> > +			resv->log_dablks += resv->total_dablks;
> 
> I'm not quite clear on why log is double the total above, then we add an
> increment of total here. Can you elaborate on this?

- resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK); 

  This gives the number of dabtree blocks that we might need to allocate. If
  we do indeed allocate resv->total_dablks worth of blocks then it would mean
  that we would have split blocks from the root node to the leaf node of the
  dabtree. We would then need to log the following blocks,
  - Blocks belonging to the original path from root node to the leaf node.
  - The new set of blocks from the root node to the leaf node which resulted
    from splitting the corresponding blocks in the original path.
  Thus the number of blocks to be logged is 2 * resv->total_dablks.

- Double split
  If the size of the xattr is large enough to cause another split, then we
  would need yet another set of new blocks for representing the path from root
  to the leaf.  This is taken into consideration by 'resv->total_dablks *= 2'.
  These new blocks might also need to logged. So resv->log_dablks should be
  incremented by number of blocks b/w root node and the leaf. I now feel that
  the statement could be rewritten in the following manner, 'resv->log_dablks
  += XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK)' if that makes it easier to
  understand.

> 
> >  			resv->total_dablks *= 2;
> >  		}
> >  		resv->rmt_blks = 0;
> > @@ -349,9 +352,7 @@ xfs_attr_set(
> >  				return error;
> >  		}
> >  
> > -		tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
> > -				 M_RES(mp)->tr_attrsetrt.tr_logres *
> > -					args->total;
> > +		tres.tr_logres = xfs_calc_attr_res(mp, &resv);
> 
> So instead of using the runtime reservation calculation, we call this
> new function that uses the block res structure and calculates log
> reservation for us. Seems like a decent cleanup, but I'm still having
> some trouble determining what is cleanup vs. what is fixing the res
> calculation.

Log reservation calculation for xattr set operation in XFS is
incorrect because,

- xfs_attr_set() computes the total log space reservation as the
  sum of 
  1. Mount time log reservation (M_RES(mp)->tr_attrsetm.tr_logres)
     Here, XFS incorrectly includes the number of blocks from root
     node to the leaf of a dabtree. This is incorrect because the
     number of such paths (from root node to leaf) depends on the
     size of the xattr. If the xattr is local but large enough to
     cause a double split then we would need three times the
     number of blocks in a path from root node to leaf. Hence the
     number of dabtree blocks that need to be logged is something
     that cannot be calculated at mount time.

     Also, the mount log reservation calcuation does not account
     for log space required for logging the AGF.
     
  2. Run time log reservatation
     This is calculated by multiplying two components,
     - The first component consists of sum of the following
       - Superblock
       - The number of bytes for logging a single path from root
         to leaf of a bmbt tree.
     - The second component consists of,
       - The number of dabtree blocks
       - The number of bmbt blocks required for mapping new
         dabtree blocks.
     Here Bmbt blocks gets accounted twice. Also it does not make sense to
     multiply these two components.

The other bug is related to calcuation performed in
xfs_log_calc_max_attrsetm_res(),
1. The call to XFS_DAENTER_SPACE_RES() macro returns the number of
   blocks required for a single split of dabtree and the
   corresponding number of bmbt blocks required for mapping the new
   dabtree blocks.
2. The space occupied by the attribute value is treated as if it
   were a remote attribute and the space for it along with the
   corresponding bmbt blocks required is added to the number of
   blocks obtained in step 1.
3. This sum is then multiplied with the runtime reservation. Here
   again Bmbt blocks gets accounted twice, once from the 'run time
   reservation' and the other from the call to
   XFS_DAENTER_SPACE_RES().
4. The result from step 3 is added to 'mount time log
   reservation'. This means that the log space reservation is
   accounted twice for dabtree blocks.

To solve these problems, the fix consists of,
1. Mounting time log reservation now includes only
   - Inode
   - Superblock
   - AGF
   i.e. we don't consider the dabtree logging space requirement
   when calculating mount time log reservation.

2. Run time log reservation
   To calculate run time log reservation we change xfs_attr_calc_size()
   to return, 
   1. Number of Dabtree blocks required.
   2. Number of Dabtree blocks that might need to be logged.
   3. Number of remote blocks.
   4. Number of Bmbt blocks that might be required.

   The values returned by xfs_attr_calc_size() is used by 
   - xfs_attr_set()
     - To compute total number of blocks required for attr set operation.
     - To compute the total log reservation (via a call to
       xfs_calc_attr_res()).
       - This uses dabtree blocks and bmbt blocks as input to calculate
         log space reservation for free space trees.
       - The total log space reservation is then computed by
         adding up 
         - Mount time log reservation space.
	 - dabtree blocks that need to logged and 
	 - free space tree blocks required.
   - xfs_log_calc_max_attrsetm_res()
     - To compute the log reservation space for a maximum sized local xattr.
       It uses the helper function xfs_calc_attr_res() accomplish this.
	  
   i.e. xfs_attr_calc_size() is now a helper function which
   returns various sizes (dabtree blocks, dabtree blocks that
   need to be logged, bmbt blocks, and remote blocks) that could be used by
   other functions (xfs_attr_set() and xfs_log_calc_max_attrsetm_res()) to
   obtain various block reservation values associated with xattrs.

One of the cleanups included in this patch was the addition of
xfs_calc_attr_res() function so that the reservation code is limited to
xfs_trans_resv.c.

Please let me know if the above explaination does not answer your question
satisfactorily.

> 
> >  		tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
> >  		tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> >  		total = args->total;
> > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > index 0e387230744c3..83508148bbd12 100644
> > --- a/fs/xfs/libxfs/xfs_attr.h
> > +++ b/fs/xfs/libxfs/xfs_attr.h
> > @@ -74,6 +74,9 @@ struct xfs_attr_list_context {
> >  };
> >  
> >  struct xfs_attr_set_resv {
> > +	/* Number of blocks in the da btree that we might need to log. */
> > +	unsigned int		log_dablks;
> > +
> >  	/* Number of unlogged blocks needed to store the remote attr value. */
> >  	unsigned int		rmt_blks;
> >  
> > diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c b/fs/xfs/libxfs/xfs_log_rlimit.c
> > index 7f55eb3f36536..a132ffa7adf32 100644
> > --- a/fs/xfs/libxfs/xfs_log_rlimit.c
> > +++ b/fs/xfs/libxfs/xfs_log_rlimit.c
> > @@ -10,6 +10,7 @@
> >  #include "xfs_log_format.h"
> >  #include "xfs_trans_resv.h"
> >  #include "xfs_mount.h"
> > +#include "xfs_attr.h"
> >  #include "xfs_da_format.h"
> >  #include "xfs_trans_space.h"
> >  #include "xfs_da_btree.h"
> > @@ -21,19 +22,18 @@
> >   */
> >  STATIC int
> >  xfs_log_calc_max_attrsetm_res(
> > -	struct xfs_mount	*mp)
> > +	struct xfs_mount		*mp)
> >  {
> > -	int			size;
> > -	int			nblks;
> > +	struct xfs_attr_set_resv	resv;
> > +	int				size;
> > +	int				local;
> >  
> >  	size = xfs_attr_leaf_entsize_local_max(mp->m_attr_geo->blksize) -
> >  	       MAXNAMELEN - 1;
> > -	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
> > -	nblks += XFS_B_TO_FSB(mp, size);
> > -	nblks += XFS_NEXTENTADD_SPACE_RES(mp, size, XFS_ATTR_FORK);
> > +	xfs_attr_calc_size(mp, &resv, size, 0, &local);
> > +	ASSERT(local == 1);
> >  
> > -	return  M_RES(mp)->tr_attrsetm.tr_logres +
> > -		M_RES(mp)->tr_attrsetrt.tr_logres * nblks;
> > +	return xfs_calc_attr_res(mp, &resv);
> >  }
> >  
> >  /*
> > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> > index 7a9c04920505a..c6b8cd56df2d7 100644
> > --- a/fs/xfs/libxfs/xfs_trans_resv.c
> > +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> > @@ -19,6 +19,7 @@
> >  #include "xfs_trans.h"
> >  #include "xfs_qm.h"
> >  #include "xfs_trans_space.h"
> > +#include "xfs_attr.h"
> >  
> >  #define _ALLOC	true
> >  #define _FREE	false
> > @@ -701,12 +702,10 @@ xfs_calc_attrinval_reservation(
> >   * Setting an attribute at mount time.
> >   *	the inode getting the attribute
> >   *	the superblock for allocations
> > - *	the agfs extents are allocated from
> > - *	the attribute btree * max depth
> > - *	the inode allocation btree
> > + *	the agf extents are allocated from
> >   * Since attribute transaction space is dependent on the size of the attribute,
> >   * the calculation is done partially at mount time and partially at runtime(see
> > - * below).
> > + * xfs_attr_calc_size()).
> >   */
> >  STATIC uint
> >  xfs_calc_attrsetm_reservation(
> > @@ -714,27 +713,7 @@ xfs_calc_attrsetm_reservation(
> >  {
> >  	return XFS_DQUOT_LOGRES(mp) +
> >  		xfs_calc_inode_res(mp, 1) +
> > -		xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> > -		xfs_calc_buf_res(XFS_DA_NODE_MAXDEPTH, XFS_FSB_TO_B(mp, 1));
> > -}
> > -
> > -/*
> > - * Setting an attribute at runtime, transaction space unit per block.
> > - * 	the superblock for allocations: sector size
> > - *	the inode bmap btree could join or split: max depth * block size
> > - * Since the runtime attribute transaction space is dependent on the total
> > - * blocks needed for the 1st bmap, here we calculate out the space unit for
> > - * one block so that the caller could figure out the total space according
> > - * to the attibute extent length in blocks by:
> > - *	ext * M_RES(mp)->tr_attrsetrt.tr_logres
> > - */
> > -STATIC uint
> > -xfs_calc_attrsetrt_reservation(
> > -	struct xfs_mount	*mp)
> > -{
> > -	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> > -		xfs_calc_buf_res(XFS_BM_MAXLEVELS(mp, XFS_ATTR_FORK),
> > -				 XFS_FSB_TO_B(mp, 1));
> > +		xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
> >  }
> >  
> >  /*
> > @@ -832,6 +811,25 @@ xfs_calc_sb_reservation(
> >  	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
> >  }
> >  
> > +uint
> > +xfs_calc_attr_res(
> > +	struct xfs_mount		*mp,
> > +	struct xfs_attr_set_resv	*resv)
> > +{
> > +	unsigned int			space_blks;
> > +	unsigned int			attr_res;
> > +
> > +	space_blks = xfs_allocfree_log_count(mp,
> > +		resv->total_dablks + resv->bmbt_blks);
> > +
> > +	attr_res = M_RES(mp)->tr_attrsetm.tr_logres +
> > +		xfs_calc_buf_res(resv->log_dablks, mp->m_attr_geo->blksize) +
> > +		xfs_calc_buf_res(resv->bmbt_blks, mp->m_sb.sb_blocksize) +
> > +		xfs_calc_buf_res(space_blks, mp->m_sb.sb_blocksize);
> > +
> > +	return attr_res;
> > +}
> 
> This function could use a header comment to explain what the reservation
> covers.
>

Sure, I will do that.

> > +
> >  void
> >  xfs_trans_resv_calc(
> >  	struct xfs_mount	*mp,
> > @@ -942,7 +940,7 @@ xfs_trans_resv_calc(
> >  	resp->tr_ichange.tr_logres = xfs_calc_ichange_reservation(mp);
> >  	resp->tr_fsyncts.tr_logres = xfs_calc_swrite_reservation(mp);
> >  	resp->tr_writeid.tr_logres = xfs_calc_writeid_reservation(mp);
> > -	resp->tr_attrsetrt.tr_logres = xfs_calc_attrsetrt_reservation(mp);
> > +	resp->tr_attrsetrt.tr_logres = 0;
> 
> Should this go away if it's going to end up as zero?

Yes, you are right about this. I will remove this before posting the next
iteration of the patchset.

> >  	resp->tr_clearagi.tr_logres = xfs_calc_clear_agi_bucket_reservation(mp);
> >  	resp->tr_growrtzero.tr_logres = xfs_calc_growrtzero_reservation(mp);
> >  	resp->tr_growrtfree.tr_logres = xfs_calc_growrtfree_reservation(mp);
> > diff --git a/fs/xfs/libxfs/xfs_trans_resv.h b/fs/xfs/libxfs/xfs_trans_resv.h
> > index 7241ab28cf84f..3a6a0bf21e9b1 100644
> > --- a/fs/xfs/libxfs/xfs_trans_resv.h
> > +++ b/fs/xfs/libxfs/xfs_trans_resv.h
> > @@ -7,6 +7,7 @@
> >  #define	__XFS_TRANS_RESV_H__
> >  
> >  struct xfs_mount;
> > +struct xfs_attr_set_resv;
> >  
> >  /*
> >   * structure for maintaining pre-calculated transaction reservations.
> > @@ -91,6 +92,7 @@ struct xfs_trans_resv {
> >  #define	XFS_ATTRSET_LOG_COUNT		3
> >  #define	XFS_ATTRRM_LOG_COUNT		3
> >  
> > +uint xfs_calc_attr_res(struct xfs_mount *mp, struct xfs_attr_set_resv *resv);
> >  void xfs_trans_resv_calc(struct xfs_mount *mp, struct xfs_trans_resv *resp);
> >  uint xfs_allocfree_log_count(struct xfs_mount *mp, uint num_ops);
> >  
> 
> 

-- 
chandan

Re: [PATCH V4 RESEND 7/7] xfs: Fix log reservation calculation for xattr insert operation

[Brian Foster]

On Wed, Feb 26, 2020 at 04:51:13PM +0530, Chandan Rajendra wrote:
> On Tuesday, February 25, 2020 10:49 PM Brian Foster wrote: 
> > On Mon, Feb 24, 2020 at 09:30:44AM +0530, Chandan Rajendra wrote:
> > > Log space reservation for xattr insert operation can be divided into two
> > > parts,
> > > 1. Mount time
> > >    - Inode
> > >    - Superblock for accounting space allocations
> > >    - AGF for accounting space used by count, block number, rmap and refcnt
> > >      btrees.
> > > 
> > > 2. The remaining log space can only be calculated at run time because,
> > >    - A local xattr can be large enough to cause a double split of the dabtree.
> > >    - The value of the xattr can be large enough to be stored in remote
> > >      blocks. The contents of the remote blocks are not logged.
> > > 
> > >    The log space reservation could be,
> > >    - 2 * XFS_DA_NODE_MAXDEPTH number of blocks. Additional XFS_DA_NODE_MAXDEPTH
> > >      number of blocks are required if xattr is large enough to cause another
> > >      split of the dabtree path from root to leaf block.
> > >    - BMBT blocks for storing (2 * XFS_DA_NODE_MAXDEPTH) record
> > >      entries. Additional XFS_DA_NODE_MAXDEPTH number of blocks are required in
> > >      case of a double split of the dabtree path from root to leaf blocks.
> > >    - Space for logging blocks of count, block number, rmap and refcnt btrees.
> > > 
> > > Presently, mount time log reservation includes block count required for a
> > > single split of the dabtree. The dabtree block count is also taken into
> > > account by xfs_attr_calc_size().
> > > 
> > > Also, AGF log space reservation isn't accounted for.
> > > 
> > > Due to the reasons mentioned above, log reservation calculation for xattr
> > > insert operation gives an incorrect value.
> > > 
> > > Apart from the above, xfs_log_calc_max_attrsetm_res() passes byte count as
> > > an argument to XFS_NEXTENTADD_SPACE_RES() instead of block count.
> > > 
> > > To fix these issues, this commit changes xfs_attr_calc_size() to also
> > > calculate the number of dabtree blocks that need to be logged.
> > > 
> > > xfs_attr_set() uses the following values computed by xfs_attr_calc_size()
> > > 1. The number of dabtree blocks that need to be logged.
> > > 2. The number of remote blocks that need to be allocated.
> > > 3. The number of dabtree blocks that need to be allocated.
> > > 4. The number of bmbt blocks that need to be allocated.
> > > 5. The total number of blocks that need to be allocated.
> > > 
> > > ... to compute number of bytes that need to be reserved in the log.
> > > 
> > > This commit also modifies xfs_log_calc_max_attrsetm_res() to invoke
> > > xfs_attr_calc_size() to obtain the number of blocks to be logged which it uses
> > > to figure out the total number of bytes to be logged.
> > > 
> > > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > > ---
> > >  fs/xfs/libxfs/xfs_attr.c       |  7 +++--
> > >  fs/xfs/libxfs/xfs_attr.h       |  3 ++
> > >  fs/xfs/libxfs/xfs_log_rlimit.c | 16 +++++------
> > >  fs/xfs/libxfs/xfs_trans_resv.c | 50 ++++++++++++++++------------------
> > >  fs/xfs/libxfs/xfs_trans_resv.h |  2 ++
> > >  5 files changed, 41 insertions(+), 37 deletions(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > index 1d62ce80d7949..f056f8597ee03 100644
> > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > @@ -182,9 +182,12 @@ xfs_attr_calc_size(
> > >  	size = xfs_attr_leaf_newentsize(mp->m_attr_geo, namelen, valuelen,
> > >  			local);
> > >  	resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> > > +	resv->log_dablks = 2 * resv->total_dablks;
> > > +
> > >  	if (*local) {
> > >  		if (size > (blksize / 2)) {
> > >  			/* Double split possible */
> > > +			resv->log_dablks += resv->total_dablks;
> > 
> > I'm not quite clear on why log is double the total above, then we add an
> > increment of total here. Can you elaborate on this?
> 
> - resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK); 
> 
>   This gives the number of dabtree blocks that we might need to allocate. If
>   we do indeed allocate resv->total_dablks worth of blocks then it would mean
>   that we would have split blocks from the root node to the leaf node of the
>   dabtree. We would then need to log the following blocks,
>   - Blocks belonging to the original path from root node to the leaf node.
>   - The new set of blocks from the root node to the leaf node which resulted
>     from splitting the corresponding blocks in the original path.
>   Thus the number of blocks to be logged is 2 * resv->total_dablks.
> 

Got it.

> - Double split
>   If the size of the xattr is large enough to cause another split, then we
>   would need yet another set of new blocks for representing the path from root
>   to the leaf.  This is taken into consideration by 'resv->total_dablks *= 2'.
>   These new blocks might also need to logged. So resv->log_dablks should be
>   incremented by number of blocks b/w root node and the leaf. I now feel that
>   the statement could be rewritten in the following manner, 'resv->log_dablks
>   += XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK)' if that makes it easier to
>   understand.
> 

Yeah, it might be clearer overall to not intertwine the fields.

> > 
> > >  			resv->total_dablks *= 2;
> > >  		}
> > >  		resv->rmt_blks = 0;
> > > @@ -349,9 +352,7 @@ xfs_attr_set(
> > >  				return error;
> > >  		}
> > >  
> > > -		tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
> > > -				 M_RES(mp)->tr_attrsetrt.tr_logres *
> > > -					args->total;
> > > +		tres.tr_logres = xfs_calc_attr_res(mp, &resv);
> > 
> > So instead of using the runtime reservation calculation, we call this
> > new function that uses the block res structure and calculates log
> > reservation for us. Seems like a decent cleanup, but I'm still having
> > some trouble determining what is cleanup vs. what is fixing the res
> > calculation.
> 
> Log reservation calculation for xattr set operation in XFS is
> incorrect because,
> 
> - xfs_attr_set() computes the total log space reservation as the
>   sum of 
>   1. Mount time log reservation (M_RES(mp)->tr_attrsetm.tr_logres)
>      Here, XFS incorrectly includes the number of blocks from root
>      node to the leaf of a dabtree. This is incorrect because the
>      number of such paths (from root node to leaf) depends on the
>      size of the xattr. If the xattr is local but large enough to
>      cause a double split then we would need three times the
>      number of blocks in a path from root node to leaf. Hence the
>      number of dabtree blocks that need to be logged is something
>      that cannot be calculated at mount time.
> 

Ok, so the dabtree portion doesn't properly account for dabtree splits
and cannot do so as part of a mount time calculation.

>      Also, the mount log reservation calcuation does not account
>      for log space required for logging the AGF.
>  

Ok.
    
>   2. Run time log reservatation
>      This is calculated by multiplying two components,
>      - The first component consists of sum of the following
>        - Superblock
>        - The number of bytes for logging a single path from root
>          to leaf of a bmbt tree.
>      - The second component consists of,
>        - The number of dabtree blocks
>        - The number of bmbt blocks required for mapping new
>          dabtree blocks.
>      Here Bmbt blocks gets accounted twice. Also it does not make sense to
>      multiply these two components.
> 

Hm, Ok. I think I follow, though I'm still curious if this was a
reproducible problem or not.

> The other bug is related to calcuation performed in
> xfs_log_calc_max_attrsetm_res(),
> 1. The call to XFS_DAENTER_SPACE_RES() macro returns the number of
>    blocks required for a single split of dabtree and the
>    corresponding number of bmbt blocks required for mapping the new
>    dabtree blocks.
> 2. The space occupied by the attribute value is treated as if it
>    were a remote attribute and the space for it along with the
>    corresponding bmbt blocks required is added to the number of
>    blocks obtained in step 1.
> 3. This sum is then multiplied with the runtime reservation. Here
>    again Bmbt blocks gets accounted twice, once from the 'run time
>    reservation' and the other from the call to
>    XFS_DAENTER_SPACE_RES().
> 4. The result from step 3 is added to 'mount time log
>    reservation'. This means that the log space reservation is
>    accounted twice for dabtree blocks.
> 
> To solve these problems, the fix consists of,
> 1. Mounting time log reservation now includes only
>    - Inode
>    - Superblock
>    - AGF
>    i.e. we don't consider the dabtree logging space requirement
>    when calculating mount time log reservation.
> 
> 2. Run time log reservation
>    To calculate run time log reservation we change xfs_attr_calc_size()
>    to return, 
>    1. Number of Dabtree blocks required.
>    2. Number of Dabtree blocks that might need to be logged.
>    3. Number of remote blocks.
>    4. Number of Bmbt blocks that might be required.
> 

Can we further break down some of these changes into smaller patches
that either fix one issue or do some refactoring? For example, one patch
could move the existing rt reservation out of ->tr_attrsetrt (removing
it) and establish the new xfs_calc_attr_res() function without changing
the reservation. Another patch could move the dabtree component from the
mount time to the runtime calculation. It's not clear to me if the
individual components of the new rt calculation can be further broken
down from there without risk of transient regression, but it seems we
should at least be able to rework the reservation calculation in a
single patch that does so from first principles with the necessary
refactoring work to accommodate the proper mount/run time components
already done..

Brian

>    The values returned by xfs_attr_calc_size() is used by 
>    - xfs_attr_set()
>      - To compute total number of blocks required for attr set operation.
>      - To compute the total log reservation (via a call to
>        xfs_calc_attr_res()).
>        - This uses dabtree blocks and bmbt blocks as input to calculate
>          log space reservation for free space trees.
>        - The total log space reservation is then computed by
>          adding up 
>          - Mount time log reservation space.
> 	 - dabtree blocks that need to logged and 
> 	 - free space tree blocks required.
>    - xfs_log_calc_max_attrsetm_res()
>      - To compute the log reservation space for a maximum sized local xattr.
>        It uses the helper function xfs_calc_attr_res() accomplish this.
> 	  
>    i.e. xfs_attr_calc_size() is now a helper function which
>    returns various sizes (dabtree blocks, dabtree blocks that
>    need to be logged, bmbt blocks, and remote blocks) that could be used by
>    other functions (xfs_attr_set() and xfs_log_calc_max_attrsetm_res()) to
>    obtain various block reservation values associated with xattrs.
> 
> One of the cleanups included in this patch was the addition of
> xfs_calc_attr_res() function so that the reservation code is limited to
> xfs_trans_resv.c.
> 
> Please let me know if the above explaination does not answer your question
> satisfactorily.
> 
> > 
> > >  		tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
> > >  		tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> > >  		total = args->total;
> > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > > index 0e387230744c3..83508148bbd12 100644
> > > --- a/fs/xfs/libxfs/xfs_attr.h
> > > +++ b/fs/xfs/libxfs/xfs_attr.h
> > > @@ -74,6 +74,9 @@ struct xfs_attr_list_context {
> > >  };
> > >  
> > >  struct xfs_attr_set_resv {
> > > +	/* Number of blocks in the da btree that we might need to log. */
> > > +	unsigned int		log_dablks;
> > > +
> > >  	/* Number of unlogged blocks needed to store the remote attr value. */
> > >  	unsigned int		rmt_blks;
> > >  
> > > diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c b/fs/xfs/libxfs/xfs_log_rlimit.c
> > > index 7f55eb3f36536..a132ffa7adf32 100644
> > > --- a/fs/xfs/libxfs/xfs_log_rlimit.c
> > > +++ b/fs/xfs/libxfs/xfs_log_rlimit.c
> > > @@ -10,6 +10,7 @@
> > >  #include "xfs_log_format.h"
> > >  #include "xfs_trans_resv.h"
> > >  #include "xfs_mount.h"
> > > +#include "xfs_attr.h"
> > >  #include "xfs_da_format.h"
> > >  #include "xfs_trans_space.h"
> > >  #include "xfs_da_btree.h"
> > > @@ -21,19 +22,18 @@
> > >   */
> > >  STATIC int
> > >  xfs_log_calc_max_attrsetm_res(
> > > -	struct xfs_mount	*mp)
> > > +	struct xfs_mount		*mp)
> > >  {
> > > -	int			size;
> > > -	int			nblks;
> > > +	struct xfs_attr_set_resv	resv;
> > > +	int				size;
> > > +	int				local;
> > >  
> > >  	size = xfs_attr_leaf_entsize_local_max(mp->m_attr_geo->blksize) -
> > >  	       MAXNAMELEN - 1;
> > > -	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
> > > -	nblks += XFS_B_TO_FSB(mp, size);
> > > -	nblks += XFS_NEXTENTADD_SPACE_RES(mp, size, XFS_ATTR_FORK);
> > > +	xfs_attr_calc_size(mp, &resv, size, 0, &local);
> > > +	ASSERT(local == 1);
> > >  
> > > -	return  M_RES(mp)->tr_attrsetm.tr_logres +
> > > -		M_RES(mp)->tr_attrsetrt.tr_logres * nblks;
> > > +	return xfs_calc_attr_res(mp, &resv);
> > >  }
> > >  
> > >  /*
> > > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> > > index 7a9c04920505a..c6b8cd56df2d7 100644
> > > --- a/fs/xfs/libxfs/xfs_trans_resv.c
> > > +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> > > @@ -19,6 +19,7 @@
> > >  #include "xfs_trans.h"
> > >  #include "xfs_qm.h"
> > >  #include "xfs_trans_space.h"
> > > +#include "xfs_attr.h"
> > >  
> > >  #define _ALLOC	true
> > >  #define _FREE	false
> > > @@ -701,12 +702,10 @@ xfs_calc_attrinval_reservation(
> > >   * Setting an attribute at mount time.
> > >   *	the inode getting the attribute
> > >   *	the superblock for allocations
> > > - *	the agfs extents are allocated from
> > > - *	the attribute btree * max depth
> > > - *	the inode allocation btree
> > > + *	the agf extents are allocated from
> > >   * Since attribute transaction space is dependent on the size of the attribute,
> > >   * the calculation is done partially at mount time and partially at runtime(see
> > > - * below).
> > > + * xfs_attr_calc_size()).
> > >   */
> > >  STATIC uint
> > >  xfs_calc_attrsetm_reservation(
> > > @@ -714,27 +713,7 @@ xfs_calc_attrsetm_reservation(
> > >  {
> > >  	return XFS_DQUOT_LOGRES(mp) +
> > >  		xfs_calc_inode_res(mp, 1) +
> > > -		xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> > > -		xfs_calc_buf_res(XFS_DA_NODE_MAXDEPTH, XFS_FSB_TO_B(mp, 1));
> > > -}
> > > -
> > > -/*
> > > - * Setting an attribute at runtime, transaction space unit per block.
> > > - * 	the superblock for allocations: sector size
> > > - *	the inode bmap btree could join or split: max depth * block size
> > > - * Since the runtime attribute transaction space is dependent on the total
> > > - * blocks needed for the 1st bmap, here we calculate out the space unit for
> > > - * one block so that the caller could figure out the total space according
> > > - * to the attibute extent length in blocks by:
> > > - *	ext * M_RES(mp)->tr_attrsetrt.tr_logres
> > > - */
> > > -STATIC uint
> > > -xfs_calc_attrsetrt_reservation(
> > > -	struct xfs_mount	*mp)
> > > -{
> > > -	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> > > -		xfs_calc_buf_res(XFS_BM_MAXLEVELS(mp, XFS_ATTR_FORK),
> > > -				 XFS_FSB_TO_B(mp, 1));
> > > +		xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
> > >  }
> > >  
> > >  /*
> > > @@ -832,6 +811,25 @@ xfs_calc_sb_reservation(
> > >  	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
> > >  }
> > >  
> > > +uint
> > > +xfs_calc_attr_res(
> > > +	struct xfs_mount		*mp,
> > > +	struct xfs_attr_set_resv	*resv)
> > > +{
> > > +	unsigned int			space_blks;
> > > +	unsigned int			attr_res;
> > > +
> > > +	space_blks = xfs_allocfree_log_count(mp,
> > > +		resv->total_dablks + resv->bmbt_blks);
> > > +
> > > +	attr_res = M_RES(mp)->tr_attrsetm.tr_logres +
> > > +		xfs_calc_buf_res(resv->log_dablks, mp->m_attr_geo->blksize) +
> > > +		xfs_calc_buf_res(resv->bmbt_blks, mp->m_sb.sb_blocksize) +
> > > +		xfs_calc_buf_res(space_blks, mp->m_sb.sb_blocksize);
> > > +
> > > +	return attr_res;
> > > +}
> > 
> > This function could use a header comment to explain what the reservation
> > covers.
> >
> 
> Sure, I will do that.
> 
> > > +
> > >  void
> > >  xfs_trans_resv_calc(
> > >  	struct xfs_mount	*mp,
> > > @@ -942,7 +940,7 @@ xfs_trans_resv_calc(
> > >  	resp->tr_ichange.tr_logres = xfs_calc_ichange_reservation(mp);
> > >  	resp->tr_fsyncts.tr_logres = xfs_calc_swrite_reservation(mp);
> > >  	resp->tr_writeid.tr_logres = xfs_calc_writeid_reservation(mp);
> > > -	resp->tr_attrsetrt.tr_logres = xfs_calc_attrsetrt_reservation(mp);
> > > +	resp->tr_attrsetrt.tr_logres = 0;
> > 
> > Should this go away if it's going to end up as zero?
> 
> Yes, you are right about this. I will remove this before posting the next
> iteration of the patchset.
> 
> > >  	resp->tr_clearagi.tr_logres = xfs_calc_clear_agi_bucket_reservation(mp);
> > >  	resp->tr_growrtzero.tr_logres = xfs_calc_growrtzero_reservation(mp);
> > >  	resp->tr_growrtfree.tr_logres = xfs_calc_growrtfree_reservation(mp);
> > > diff --git a/fs/xfs/libxfs/xfs_trans_resv.h b/fs/xfs/libxfs/xfs_trans_resv.h
> > > index 7241ab28cf84f..3a6a0bf21e9b1 100644
> > > --- a/fs/xfs/libxfs/xfs_trans_resv.h
> > > +++ b/fs/xfs/libxfs/xfs_trans_resv.h
> > > @@ -7,6 +7,7 @@
> > >  #define	__XFS_TRANS_RESV_H__
> > >  
> > >  struct xfs_mount;
> > > +struct xfs_attr_set_resv;
> > >  
> > >  /*
> > >   * structure for maintaining pre-calculated transaction reservations.
> > > @@ -91,6 +92,7 @@ struct xfs_trans_resv {
> > >  #define	XFS_ATTRSET_LOG_COUNT		3
> > >  #define	XFS_ATTRRM_LOG_COUNT		3
> > >  
> > > +uint xfs_calc_attr_res(struct xfs_mount *mp, struct xfs_attr_set_resv *resv);
> > >  void xfs_trans_resv_calc(struct xfs_mount *mp, struct xfs_trans_resv *resp);
> > >  uint xfs_allocfree_log_count(struct xfs_mount *mp, uint num_ops);
> > >  
> > 
> > 
> 
> -- 
> chandan
> 
> 
> 

Re: [PATCH V4 RESEND 7/7] xfs: Fix log reservation calculation for xattr insert operation

[Chandan Rajendra]

On Thursday, February 27, 2020 12:20 AM Brian Foster wrote: 
> On Wed, Feb 26, 2020 at 04:51:13PM +0530, Chandan Rajendra wrote:
> > On Tuesday, February 25, 2020 10:49 PM Brian Foster wrote: 
> > > On Mon, Feb 24, 2020 at 09:30:44AM +0530, Chandan Rajendra wrote:
> > > > Log space reservation for xattr insert operation can be divided into two
> > > > parts,
> > > > 1. Mount time
> > > >    - Inode
> > > >    - Superblock for accounting space allocations
> > > >    - AGF for accounting space used by count, block number, rmap and refcnt
> > > >      btrees.
> > > > 
> > > > 2. The remaining log space can only be calculated at run time because,
> > > >    - A local xattr can be large enough to cause a double split of the dabtree.
> > > >    - The value of the xattr can be large enough to be stored in remote
> > > >      blocks. The contents of the remote blocks are not logged.
> > > > 
> > > >    The log space reservation could be,
> > > >    - 2 * XFS_DA_NODE_MAXDEPTH number of blocks. Additional XFS_DA_NODE_MAXDEPTH
> > > >      number of blocks are required if xattr is large enough to cause another
> > > >      split of the dabtree path from root to leaf block.
> > > >    - BMBT blocks for storing (2 * XFS_DA_NODE_MAXDEPTH) record
> > > >      entries. Additional XFS_DA_NODE_MAXDEPTH number of blocks are required in
> > > >      case of a double split of the dabtree path from root to leaf blocks.
> > > >    - Space for logging blocks of count, block number, rmap and refcnt btrees.
> > > > 
> > > > Presently, mount time log reservation includes block count required for a
> > > > single split of the dabtree. The dabtree block count is also taken into
> > > > account by xfs_attr_calc_size().
> > > > 
> > > > Also, AGF log space reservation isn't accounted for.
> > > > 
> > > > Due to the reasons mentioned above, log reservation calculation for xattr
> > > > insert operation gives an incorrect value.
> > > > 
> > > > Apart from the above, xfs_log_calc_max_attrsetm_res() passes byte count as
> > > > an argument to XFS_NEXTENTADD_SPACE_RES() instead of block count.
> > > > 
> > > > To fix these issues, this commit changes xfs_attr_calc_size() to also
> > > > calculate the number of dabtree blocks that need to be logged.
> > > > 
> > > > xfs_attr_set() uses the following values computed by xfs_attr_calc_size()
> > > > 1. The number of dabtree blocks that need to be logged.
> > > > 2. The number of remote blocks that need to be allocated.
> > > > 3. The number of dabtree blocks that need to be allocated.
> > > > 4. The number of bmbt blocks that need to be allocated.
> > > > 5. The total number of blocks that need to be allocated.
> > > > 
> > > > ... to compute number of bytes that need to be reserved in the log.
> > > > 
> > > > This commit also modifies xfs_log_calc_max_attrsetm_res() to invoke
> > > > xfs_attr_calc_size() to obtain the number of blocks to be logged which it uses
> > > > to figure out the total number of bytes to be logged.
> > > > 
> > > > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > > > ---
> > > >  fs/xfs/libxfs/xfs_attr.c       |  7 +++--
> > > >  fs/xfs/libxfs/xfs_attr.h       |  3 ++
> > > >  fs/xfs/libxfs/xfs_log_rlimit.c | 16 +++++------
> > > >  fs/xfs/libxfs/xfs_trans_resv.c | 50 ++++++++++++++++------------------
> > > >  fs/xfs/libxfs/xfs_trans_resv.h |  2 ++
> > > >  5 files changed, 41 insertions(+), 37 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > > index 1d62ce80d7949..f056f8597ee03 100644
> > > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > > @@ -182,9 +182,12 @@ xfs_attr_calc_size(
> > > >  	size = xfs_attr_leaf_newentsize(mp->m_attr_geo, namelen, valuelen,
> > > >  			local);
> > > >  	resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> > > > +	resv->log_dablks = 2 * resv->total_dablks;
> > > > +
> > > >  	if (*local) {
> > > >  		if (size > (blksize / 2)) {
> > > >  			/* Double split possible */
> > > > +			resv->log_dablks += resv->total_dablks;
> > > 
> > > I'm not quite clear on why log is double the total above, then we add an
> > > increment of total here. Can you elaborate on this?
> > 
> > - resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK); 
> > 
> >   This gives the number of dabtree blocks that we might need to allocate. If
> >   we do indeed allocate resv->total_dablks worth of blocks then it would mean
> >   that we would have split blocks from the root node to the leaf node of the
> >   dabtree. We would then need to log the following blocks,
> >   - Blocks belonging to the original path from root node to the leaf node.
> >   - The new set of blocks from the root node to the leaf node which resulted
> >     from splitting the corresponding blocks in the original path.
> >   Thus the number of blocks to be logged is 2 * resv->total_dablks.
> > 
> 
> Got it.
> 
> > - Double split
> >   If the size of the xattr is large enough to cause another split, then we
> >   would need yet another set of new blocks for representing the path from root
> >   to the leaf.  This is taken into consideration by 'resv->total_dablks *= 2'.
> >   These new blocks might also need to logged. So resv->log_dablks should be
> >   incremented by number of blocks b/w root node and the leaf. I now feel that
> >   the statement could be rewritten in the following manner, 'resv->log_dablks
> >   += XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK)' if that makes it easier to
> >   understand.
> > 
> 
> Yeah, it might be clearer overall to not intertwine the fields.
> 
> > > 
> > > >  			resv->total_dablks *= 2;
> > > >  		}
> > > >  		resv->rmt_blks = 0;
> > > > @@ -349,9 +352,7 @@ xfs_attr_set(
> > > >  				return error;
> > > >  		}
> > > >  
> > > > -		tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
> > > > -				 M_RES(mp)->tr_attrsetrt.tr_logres *
> > > > -					args->total;
> > > > +		tres.tr_logres = xfs_calc_attr_res(mp, &resv);
> > > 
> > > So instead of using the runtime reservation calculation, we call this
> > > new function that uses the block res structure and calculates log
> > > reservation for us. Seems like a decent cleanup, but I'm still having
> > > some trouble determining what is cleanup vs. what is fixing the res
> > > calculation.
> > 
> > Log reservation calculation for xattr set operation in XFS is
> > incorrect because,
> > 
> > - xfs_attr_set() computes the total log space reservation as the
> >   sum of 
> >   1. Mount time log reservation (M_RES(mp)->tr_attrsetm.tr_logres)
> >      Here, XFS incorrectly includes the number of blocks from root
> >      node to the leaf of a dabtree. This is incorrect because the
> >      number of such paths (from root node to leaf) depends on the
> >      size of the xattr. If the xattr is local but large enough to
> >      cause a double split then we would need three times the
> >      number of blocks in a path from root node to leaf. Hence the
> >      number of dabtree blocks that need to be logged is something
> >      that cannot be calculated at mount time.
> > 
> 
> Ok, so the dabtree portion doesn't properly account for dabtree splits
> and cannot do so as part of a mount time calculation.
> 
> >      Also, the mount log reservation calcuation does not account
> >      for log space required for logging the AGF.
> >  
> 
> Ok.
>     
> >   2. Run time log reservatation
> >      This is calculated by multiplying two components,
> >      - The first component consists of sum of the following
> >        - Superblock
> >        - The number of bytes for logging a single path from root
> >          to leaf of a bmbt tree.
> >      - The second component consists of,
> >        - The number of dabtree blocks
> >        - The number of bmbt blocks required for mapping new
> >          dabtree blocks.
> >      Here Bmbt blocks gets accounted twice. Also it does not make sense to
> >      multiply these two components.
> > 
> 
> Hm, Ok. I think I follow, though I'm still curious if this was a
> reproducible problem or not.
> 
> > The other bug is related to calcuation performed in
> > xfs_log_calc_max_attrsetm_res(),
> > 1. The call to XFS_DAENTER_SPACE_RES() macro returns the number of
> >    blocks required for a single split of dabtree and the
> >    corresponding number of bmbt blocks required for mapping the new
> >    dabtree blocks.
> > 2. The space occupied by the attribute value is treated as if it
> >    were a remote attribute and the space for it along with the
> >    corresponding bmbt blocks required is added to the number of
> >    blocks obtained in step 1.
> > 3. This sum is then multiplied with the runtime reservation. Here
> >    again Bmbt blocks gets accounted twice, once from the 'run time
> >    reservation' and the other from the call to
> >    XFS_DAENTER_SPACE_RES().
> > 4. The result from step 3 is added to 'mount time log
> >    reservation'. This means that the log space reservation is
> >    accounted twice for dabtree blocks.
> > 
> > To solve these problems, the fix consists of,
> > 1. Mounting time log reservation now includes only
> >    - Inode
> >    - Superblock
> >    - AGF
> >    i.e. we don't consider the dabtree logging space requirement
> >    when calculating mount time log reservation.
> > 
> > 2. Run time log reservation
> >    To calculate run time log reservation we change xfs_attr_calc_size()
> >    to return, 
> >    1. Number of Dabtree blocks required.
> >    2. Number of Dabtree blocks that might need to be logged.
> >    3. Number of remote blocks.
> >    4. Number of Bmbt blocks that might be required.
> > 
> 
> Can we further break down some of these changes into smaller patches
> that either fix one issue or do some refactoring? For example, one patch
> could move the existing rt reservation out of ->tr_attrsetrt (removing
> it) and establish the new xfs_calc_attr_res() function without changing
> the reservation. Another patch could move the dabtree component from the
> mount time to the runtime calculation. It's not clear to me if the
> individual components of the new rt calculation can be further broken
> down from there without risk of transient regression, but it seems we
> should at least be able to rework the reservation calculation in a
> single patch that does so from first principles with the necessary
> refactoring work to accommodate the proper mount/run time components
> already done..
> 

Sure, I will try to break it down into as many logical patches as possible.

-- 
chandan