[PATCH V2 1/2] xfs: Pass xattr name and value length explicitly to xfs_attr_leaf_newentsize

[PATCH V2 1/2] 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. The next 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>
---
Changelog:
V1 -> V2:
1. Use convenience variables to reduce indentation of code.

 fs/xfs/libxfs/xfs_attr.c      |  3 ++-
 fs/xfs/libxfs/xfs_attr_leaf.c | 41 ++++++++++++++++++++++++-----------
 fs/xfs/libxfs/xfs_attr_leaf.h |  3 ++-
 3 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 0d7fcc983b3d..1eae1db74f6c 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -199,7 +199,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(mp, 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 08d4b10ae2d5..7cd57e5844d8 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -1338,7 +1338,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->dp->i_mount, args->namelen,
+					args->valuelen, NULL);
 
 	/*
 	 * Search through freemap for first-fit on new name length.
@@ -1411,6 +1412,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;
 
@@ -1440,11 +1442,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(mp, 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(mp, args->namelen, args->valuelen,
+					&tmp);
+	ichdr->freemap[mapindex].size -= entsize;
 
 	entry->nameidx = cpu_to_be16(ichdr->freemap[mapindex].base +
 				     ichdr->freemap[mapindex].size);
@@ -1831,6 +1836,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;
@@ -1840,14 +1847,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(state->mp, args->namelen,
+					args->valuelen, NULL);
+	half += ichdr1->usedbytes + ichdr2->usedbytes + entsize;
 	half /= 2;
 	lastdelta = state->args->geo->blksize;
 	entry = xfs_attr3_leaf_entryp(leaf1);
@@ -1858,8 +1867,11 @@ 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(state->mp,
+							args->namelen,
+							args->valuelen,
+							NULL);
+			tmp = totallen + sizeof(*entry) + entsize;
 			if (XFS_ATTR_ABS(half - tmp) > lastdelta)
 				break;
 			lastdelta = XFS_ATTR_ABS(half - tmp);
@@ -1894,8 +1906,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(state->mp, args->namelen,
+						args->valuelen, NULL);
+		totallen -= sizeof(*entry) + entsize;
 	}
 
 	*countarg = count;
@@ -2687,20 +2700,22 @@ xfs_attr_leaf_entsize(xfs_attr_leafblock_t *leaf, int index)
  */
 int
 xfs_attr_leaf_newentsize(
-	struct xfs_da_args	*args,
+	struct xfs_mount	*mp,
+	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(mp->m_attr_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 f4a188e28b7b..0ce1f9301157 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.h
+++ b/fs/xfs/libxfs/xfs_attr_leaf.h
@@ -106,7 +106,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_mount *mp, 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

[PATCH V2 2/2] 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 be count, block number, rmapbt 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. Hence log reservation
calculation for xattr insert operation gives incorrect value.

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

To fix these issues, this commit refactors xfs_attr_calc_size() to calculate,
1. The number of dabtree blocks that need to be logged.
2. The number of remote blocks that need to allocated.
3. The number of dabtree blocks that need to allocated.

xfs_attr_set() uses this information to compute
1. Number of blocks that needs to allocated during the transaction.
2. Number of bytes that needs 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 dabtree blocks to be
logged which it uses to figure out the total number of blocks to be logged.

Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
---
Changelog:
V1 -> V2:
1. xfs_attr_calc_size() computes
   - Number of blocks required to log dabtree blocks.
   - Number of remote blocks.
   - Total dabtree blocks to be allocated.
2. Add new function xfs_calc_attr_blocks() to compute the total number of
   blocks to be allocated during xattr insert operation.
3. Add new function xfs_calc_attr_res() to compute the log space to be
   reserved during xattr insert operation.

 fs/xfs/libxfs/xfs_attr.c       | 108 +++++++++++++++++++++------------
 fs/xfs/libxfs/xfs_attr.h       |   3 +
 fs/xfs/libxfs/xfs_log_rlimit.c |  17 +++---
 fs/xfs/libxfs/xfs_trans_resv.c |  56 +++++++++--------
 fs/xfs/libxfs/xfs_trans_resv.h |   2 +
 5 files changed, 113 insertions(+), 73 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 1eae1db74f6c..363b4c47b134 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -183,43 +183,6 @@ xfs_attr_get(
 	return 0;
 }
 
-/*
- * Calculate how many blocks we need for the new attribute,
- */
-STATIC int
-xfs_attr_calc_size(
-	struct xfs_da_args	*args,
-	int			*local)
-{
-	struct xfs_mount	*mp = args->dp->i_mount;
-	int			size;
-	int			nblks;
-
-	/*
-	 * Determine space new attribute will use, and if it would be
-	 * "local" or "remote" (note: local != inline).
-	 */
-	size = xfs_attr_leaf_newentsize(mp, args->namelen, args->valuelen,
-					local);
-	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
-	if (*local) {
-		if (size > (args->geo->blksize / 2)) {
-			/* Double split possible */
-			nblks *= 2;
-		}
-	} 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);
-	}
-
-	return nblks;
-}
-
 STATIC int
 xfs_attr_try_sf_addname(
 	struct xfs_inode	*dp,
@@ -248,6 +211,64 @@ xfs_attr_try_sf_addname(
 	return error ? error : error2;
 }
 
+STATIC uint
+xfs_calc_attr_blocks(
+	struct xfs_mount	*mp,
+	unsigned int		total_dablks,
+	unsigned int		rmt_blks)
+{
+	unsigned int bmbt_blks;
+
+	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
+					XFS_ATTR_FORK);
+	return total_dablks + rmt_blks + bmbt_blks;
+}
+
+/*
+ * Calculate how many blocks we need for the new attribute,
+ */
+void
+xfs_attr_calc_size(
+	struct xfs_mount	*mp,
+	int			namelen,
+	int			valuelen,
+	int			*local,
+	unsigned int		*log_dablks,
+	unsigned int		*rmt_blks,
+	unsigned int		*total_dablks)
+{
+	unsigned int		blksize;
+	int			size;
+
+	blksize = mp->m_dir_geo->blksize;
+	*log_dablks = 0;
+	*rmt_blks = 0;
+	*total_dablks = 0;
+
+	/*
+	 * Determine space new attribute will use, and if it would be
+	 * "local" or "remote" (note: local != inline).
+	 */
+	size = xfs_attr_leaf_newentsize(mp, namelen, valuelen, local);
+
+	*total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
+	*log_dablks = 2 * *total_dablks;
+
+	if (*local) {
+		if (size > (blksize / 2)) {
+			/* Double split possible */
+			*log_dablks += *total_dablks;
+			*total_dablks *= 2;
+		}
+	} else {
+		/*
+		 * Out of line attribute, cannot double split, but
+		 * make room for the attribute value itself.
+		 */
+		*rmt_blks = xfs_attr3_rmt_blocks(mp, valuelen);
+	}
+}
+
 /*
  * Set the attribute specified in @args.
  */
@@ -346,6 +367,9 @@ xfs_attr_set(
 	struct xfs_mount	*mp = dp->i_mount;
 	struct xfs_da_args	args;
 	struct xfs_trans_res	tres;
+	unsigned int		log_dablks;
+	unsigned int		rmt_blks;
+	unsigned int		total_dablks;
 	int			rsvd = (flags & ATTR_ROOT) != 0;
 	int			error, local;
 
@@ -361,7 +385,11 @@ xfs_attr_set(
 	args.value = value;
 	args.valuelen = valuelen;
 	args.op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
-	args.total = xfs_attr_calc_size(&args, &local);
+
+	xfs_attr_calc_size(mp, args.namelen, args.valuelen, &local,
+			&log_dablks, &rmt_blks, &total_dablks);
+
+	args.total = xfs_calc_attr_blocks(mp, total_dablks, rmt_blks);
 
 	error = xfs_qm_dqattach(dp);
 	if (error)
@@ -380,8 +408,8 @@ 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, log_dablks, rmt_blks,
+					total_dablks);
 	tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
 	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
 
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 94badfa1743e..a1c77618802b 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -154,5 +154,8 @@ int xfs_attr_remove_args(struct xfs_da_args *args);
 int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
 		  int flags, struct attrlist_cursor_kern *cursor);
 bool xfs_attr_namecheck(const void *name, size_t length);
+void xfs_attr_calc_size(struct xfs_mount *mp, int namelen, int valuelen,
+			int *local, unsigned int *log_dablks,
+			unsigned int *rmt_blks, unsigned int *total_dablks);
 
 #endif	/* __XFS_ATTR_H__ */
diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c b/fs/xfs/libxfs/xfs_log_rlimit.c
index 7f55eb3f3653..33b805411f72 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"
@@ -23,17 +24,19 @@ STATIC int
 xfs_log_calc_max_attrsetm_res(
 	struct xfs_mount	*mp)
 {
-	int			size;
-	int			nblks;
+	int		size;
+	int		local;
+	unsigned int	total_dablks;
+	unsigned int	rmt_blks;
+	unsigned int	log_dablks;
 
 	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, size, 0, &local, &log_dablks, &rmt_blks,
+			&total_dablks);
+	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, log_dablks, rmt_blks, total_dablks);
 }
 
 /*
diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index 824073a839ac..8a0fea655358 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -701,12 +701,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 +712,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 +810,32 @@ xfs_calc_sb_reservation(
 	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
 }
 
+uint
+xfs_calc_attr_res(
+	struct xfs_mount	*mp,
+	unsigned int		log_dablks,
+	unsigned int		rmt_blks,
+	unsigned int		total_dablks)
+{
+	unsigned int		da_blksize;
+	unsigned int		fs_blksize;
+	unsigned int		bmbt_blks;
+	unsigned int		space_blks;
+
+	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
+					XFS_ATTR_FORK);
+	space_blks = xfs_allocfree_log_count(mp,
+					total_dablks + rmt_blks + bmbt_blks);
+
+	da_blksize = mp->m_attr_geo->blksize;
+	fs_blksize = mp->m_sb.sb_blocksize;
+
+	return M_RES(mp)->tr_attrsetm.tr_logres +
+		xfs_calc_buf_res(log_dablks, da_blksize) +
+		xfs_calc_buf_res(bmbt_blks, fs_blksize) +
+		xfs_calc_buf_res(space_blks, fs_blksize);
+}
+
 void
 xfs_trans_resv_calc(
 	struct xfs_mount	*mp,
@@ -942,7 +946,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 7241ab28cf84..48ceba72fb12 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.h
+++ b/fs/xfs/libxfs/xfs_trans_resv.h
@@ -91,6 +91,8 @@ 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, unsigned int log_dablks,
+		unsigned int rmt_blks, unsigned int total_dablks);
 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 V2 2/2] xfs: Fix log reservation calculation for xattr insert operation

[Chandan Rajendra]

On Friday, January 17, 2020 6:16 AM Darrick J. Wong wrote: 
> On Wed, Jan 15, 2020 at 06:24:21PM +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 be count, block number, rmapbt 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. Hence log reservation
> > calculation for xattr insert operation gives incorrect value.
> > 
> > Apart from the above, xfs_log_calc_max_attrsetm_res() passes a byte count as
> > an argument to XFS_NEXTENTADD_SPACE_RES() instead of block count.
> > 
> > To fix these issues, this commit refactors xfs_attr_calc_size() to calculate,
> > 1. The number of dabtree blocks that need to be logged.
> > 2. The number of remote blocks that need to allocated.
> > 3. The number of dabtree blocks that need to allocated.
> > 
> > xfs_attr_set() uses this information to compute
> > 1. Number of blocks that needs to allocated during the transaction.
> > 2. Number of bytes that needs 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 dabtree blocks to be
> > logged which it uses to figure out the total number of blocks to be logged.
> > 
> > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > ---
> > Changelog:
> > V1 -> V2:
> > 1. xfs_attr_calc_size() computes
> >    - Number of blocks required to log dabtree blocks.
> >    - Number of remote blocks.
> >    - Total dabtree blocks to be allocated.
> > 2. Add new function xfs_calc_attr_blocks() to compute the total number of
> >    blocks to be allocated during xattr insert operation.
> > 3. Add new function xfs_calc_attr_res() to compute the log space to be
> >    reserved during xattr insert operation.
> > 
> >  fs/xfs/libxfs/xfs_attr.c       | 108 +++++++++++++++++++++------------
> >  fs/xfs/libxfs/xfs_attr.h       |   3 +
> >  fs/xfs/libxfs/xfs_log_rlimit.c |  17 +++---
> >  fs/xfs/libxfs/xfs_trans_resv.c |  56 +++++++++--------
> >  fs/xfs/libxfs/xfs_trans_resv.h |   2 +
> >  5 files changed, 113 insertions(+), 73 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 1eae1db74f6c..363b4c47b134 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -183,43 +183,6 @@ xfs_attr_get(
> >  	return 0;
> >  }
> >  
> > -/*
> > - * Calculate how many blocks we need for the new attribute,
> > - */
> > -STATIC int
> > -xfs_attr_calc_size(
> > -	struct xfs_da_args	*args,
> > -	int			*local)
> > -{
> > -	struct xfs_mount	*mp = args->dp->i_mount;
> > -	int			size;
> > -	int			nblks;
> > -
> > -	/*
> > -	 * Determine space new attribute will use, and if it would be
> > -	 * "local" or "remote" (note: local != inline).
> > -	 */
> > -	size = xfs_attr_leaf_newentsize(mp, args->namelen, args->valuelen,
> > -					local);
> > -	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
> > -	if (*local) {
> > -		if (size > (args->geo->blksize / 2)) {
> > -			/* Double split possible */
> > -			nblks *= 2;
> > -		}
> > -	} 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);
> > -	}
> > -
> > -	return nblks;
> > -}
> > -
> >  STATIC int
> >  xfs_attr_try_sf_addname(
> >  	struct xfs_inode	*dp,
> > @@ -248,6 +211,64 @@ xfs_attr_try_sf_addname(
> >  	return error ? error : error2;
> >  }
> >  
> > +STATIC uint
> > +xfs_calc_attr_blocks(
> > +	struct xfs_mount	*mp,
> > +	unsigned int		total_dablks,
> > +	unsigned int		rmt_blks)
> > +{
> > +	unsigned int bmbt_blks;
> > +
> > +	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> > +					XFS_ATTR_FORK);
> > +	return total_dablks + rmt_blks + bmbt_blks;
> 
> I think this calculation could be added to xfs_attr_calc_size and passed
> back to the caller as another outparam.
> 
> At this point we have five different block counts I'm wondering if we
> should create a struct and pass it around...
> 
> > +}
> > +
> > +/*
> > + * Calculate how many blocks we need for the new attribute,
> > + */
> > +void
> > +xfs_attr_calc_size(
> > +	struct xfs_mount	*mp,
> > +	int			namelen,
> > +	int			valuelen,
> > +	int			*local,
> > +	unsigned int		*log_dablks,
> > +	unsigned int		*rmt_blks,
> > +	unsigned int		*total_dablks)
> 
> ...something like this?  It'll be much easier to remember what each of
> those parameters actually do with a full sentence comment:
> 
> 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;
> 
> 	/* Blocks we might need to map into the attribute fork. */
> 	unsigned int		total_dablks;
> 
> 	/* Blocks we might need to allocate. */
> 	unsigned int		alloc_blks;
> 
> 	/* Blocks we might need to create all the new attr fork mappings. */
> 	unsigned int		bmbt_blks;
> };
> 
> > +{
> > +	unsigned int		blksize;
> > +	int			size;
> > +
> > +	blksize = mp->m_dir_geo->blksize;
> > +	*log_dablks = 0;
> > +	*rmt_blks = 0;
> > +	*total_dablks = 0;
> 
> No need to zero out variables that we're going to set two lines later.
> 
> > +
> > +	/*
> > +	 * Determine space new attribute will use, and if it would be
> > +	 * "local" or "remote" (note: local != inline).
> > +	 */
> > +	size = xfs_attr_leaf_newentsize(mp, namelen, valuelen, local);
> > +
> > +	*total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> > +	*log_dablks = 2 * *total_dablks;
> > +
> > +	if (*local) {
> > +		if (size > (blksize / 2)) {
> > +			/* Double split possible */
> > +			*log_dablks += *total_dablks;
> > +			*total_dablks *= 2;
> > +		}
> > +	} else {
> > +		/*
> > +		 * Out of line attribute, cannot double split, but
> > +		 * make room for the attribute value itself.
> > +		 */
> > +		*rmt_blks = xfs_attr3_rmt_blocks(mp, valuelen);
> > +	}
> > +}
> > +
> >  /*
> >   * Set the attribute specified in @args.
> >   */
> > @@ -346,6 +367,9 @@ xfs_attr_set(
> >  	struct xfs_mount	*mp = dp->i_mount;
> >  	struct xfs_da_args	args;
> >  	struct xfs_trans_res	tres;
> > +	unsigned int		log_dablks;
> > +	unsigned int		rmt_blks;
> > +	unsigned int		total_dablks;
> >  	int			rsvd = (flags & ATTR_ROOT) != 0;
> >  	int			error, local;
> >  
> > @@ -361,7 +385,11 @@ xfs_attr_set(
> >  	args.value = value;
> >  	args.valuelen = valuelen;
> >  	args.op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
> > -	args.total = xfs_attr_calc_size(&args, &local);
> > +
> > +	xfs_attr_calc_size(mp, args.namelen, args.valuelen, &local,
> > +			&log_dablks, &rmt_blks, &total_dablks);
> > +
> > +	args.total = xfs_calc_attr_blocks(mp, total_dablks, rmt_blks);
> >  
> >  	error = xfs_qm_dqattach(dp);
> >  	if (error)
> > @@ -380,8 +408,8 @@ 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, log_dablks, rmt_blks,
> > +					total_dablks);
> >  	tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
> >  	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> >  
> > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > index 94badfa1743e..a1c77618802b 100644
> > --- a/fs/xfs/libxfs/xfs_attr.h
> > +++ b/fs/xfs/libxfs/xfs_attr.h
> > @@ -154,5 +154,8 @@ int xfs_attr_remove_args(struct xfs_da_args *args);
> >  int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
> >  		  int flags, struct attrlist_cursor_kern *cursor);
> >  bool xfs_attr_namecheck(const void *name, size_t length);
> > +void xfs_attr_calc_size(struct xfs_mount *mp, int namelen, int valuelen,
> > +			int *local, unsigned int *log_dablks,
> > +			unsigned int *rmt_blks, unsigned int *total_dablks);
> >  
> >  #endif	/* __XFS_ATTR_H__ */
> > diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c b/fs/xfs/libxfs/xfs_log_rlimit.c
> > index 7f55eb3f3653..33b805411f72 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"
> > @@ -23,17 +24,19 @@ STATIC int
> >  xfs_log_calc_max_attrsetm_res(
> >  	struct xfs_mount	*mp)
> >  {
> > -	int			size;
> > -	int			nblks;
> > +	int		size;
> > +	int		local;
> > +	unsigned int	total_dablks;
> > +	unsigned int	rmt_blks;
> > +	unsigned int	log_dablks;
> >  
> >  	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, size, 0, &local, &log_dablks, &rmt_blks,
> > +			&total_dablks);
> > +	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, log_dablks, rmt_blks, total_dablks);
> >  }
> >  
> >  /*
> > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> > index 824073a839ac..8a0fea655358 100644
> > --- a/fs/xfs/libxfs/xfs_trans_resv.c
> > +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> > @@ -701,12 +701,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 +712,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);
> 
> What effect does changing these reservation calculations have on the
> computed minimum log size?
> 
> >  }
> >  
> >  /*
> > @@ -832,6 +810,32 @@ xfs_calc_sb_reservation(
> >  	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
> >  }
> >  
> > +uint
> > +xfs_calc_attr_res(
> > +	struct xfs_mount	*mp,
> > +	unsigned int		log_dablks,
> > +	unsigned int		rmt_blks,
> > +	unsigned int		total_dablks)
> > +{
> > +	unsigned int		da_blksize;
> > +	unsigned int		fs_blksize;
> > +	unsigned int		bmbt_blks;
> > +	unsigned int		space_blks;
> > +
> > +	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> > +					XFS_ATTR_FORK);
> 
> Pass in the resv structure above and you won't need to calculate this
> again.
> 
> > +	space_blks = xfs_allocfree_log_count(mp,
> > +					total_dablks + rmt_blks + bmbt_blks);
> 
> Only two levels of indent needed here:
> 
> 	space_blks = xfs_allocfree_log_count(mp,
> 			total_dablks + rmt_blks + bmbt_blks);
> 
> > +
> > +	da_blksize = mp->m_attr_geo->blksize;
> > +	fs_blksize = mp->m_sb.sb_blocksize;
> 
> You could probably pass these to xfs_calc_buf_res directly.
> 
> I'll give this a spin and see how it does.
> 
> --D
> 
> > +
> > +	return M_RES(mp)->tr_attrsetm.tr_logres +
> > +		xfs_calc_buf_res(log_dablks, da_blksize) +
> > +		xfs_calc_buf_res(bmbt_blks, fs_blksize) +
> > +		xfs_calc_buf_res(space_blks, fs_blksize);
> > +}
> > +
> >  void
> >  xfs_trans_resv_calc(
> >  	struct xfs_mount	*mp,
> > @@ -942,7 +946,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 7241ab28cf84..48ceba72fb12 100644
> > --- a/fs/xfs/libxfs/xfs_trans_resv.h
> > +++ b/fs/xfs/libxfs/xfs_trans_resv.h
> > @@ -91,6 +91,8 @@ 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, unsigned int log_dablks,
> > +		unsigned int rmt_blks, unsigned int total_dablks);
> >  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);
> >  
> 

Hi Darrick,

I agree to the changes you have suggested. I will apply them and post the next
version soon.

I will also figure out the effect of this patch on minimum log size.

-- 
chandan

Re: [PATCH V2 2/2] xfs: Fix log reservation calculation for xattr insert operation

[Chandan Rajendra]

On Friday, January 17, 2020 10:16 AM Chandan Rajendra wrote: 
> On Friday, January 17, 2020 6:16 AM Darrick J. Wong wrote: 
> > On Wed, Jan 15, 2020 at 06:24:21PM +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 be count, block number, rmapbt 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. Hence log reservation
> > > calculation for xattr insert operation gives incorrect value.
> > > 
> > > Apart from the above, xfs_log_calc_max_attrsetm_res() passes a byte count as
> > > an argument to XFS_NEXTENTADD_SPACE_RES() instead of block count.
> > > 
> > > To fix these issues, this commit refactors xfs_attr_calc_size() to calculate,
> > > 1. The number of dabtree blocks that need to be logged.
> > > 2. The number of remote blocks that need to allocated.
> > > 3. The number of dabtree blocks that need to allocated.
> > > 
> > > xfs_attr_set() uses this information to compute
> > > 1. Number of blocks that needs to allocated during the transaction.
> > > 2. Number of bytes that needs 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 dabtree blocks to be
> > > logged which it uses to figure out the total number of blocks to be logged.
> > > 
> > > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > > ---
> > > Changelog:
> > > V1 -> V2:
> > > 1. xfs_attr_calc_size() computes
> > >    - Number of blocks required to log dabtree blocks.
> > >    - Number of remote blocks.
> > >    - Total dabtree blocks to be allocated.
> > > 2. Add new function xfs_calc_attr_blocks() to compute the total number of
> > >    blocks to be allocated during xattr insert operation.
> > > 3. Add new function xfs_calc_attr_res() to compute the log space to be
> > >    reserved during xattr insert operation.
> > > 
> > >  fs/xfs/libxfs/xfs_attr.c       | 108 +++++++++++++++++++++------------
> > >  fs/xfs/libxfs/xfs_attr.h       |   3 +
> > >  fs/xfs/libxfs/xfs_log_rlimit.c |  17 +++---
> > >  fs/xfs/libxfs/xfs_trans_resv.c |  56 +++++++++--------
> > >  fs/xfs/libxfs/xfs_trans_resv.h |   2 +
> > >  5 files changed, 113 insertions(+), 73 deletions(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > index 1eae1db74f6c..363b4c47b134 100644
> > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > @@ -183,43 +183,6 @@ xfs_attr_get(
> > >  	return 0;
> > >  }
> > >  
> > > -/*
> > > - * Calculate how many blocks we need for the new attribute,
> > > - */
> > > -STATIC int
> > > -xfs_attr_calc_size(
> > > -	struct xfs_da_args	*args,
> > > -	int			*local)
> > > -{
> > > -	struct xfs_mount	*mp = args->dp->i_mount;
> > > -	int			size;
> > > -	int			nblks;
> > > -
> > > -	/*
> > > -	 * Determine space new attribute will use, and if it would be
> > > -	 * "local" or "remote" (note: local != inline).
> > > -	 */
> > > -	size = xfs_attr_leaf_newentsize(mp, args->namelen, args->valuelen,
> > > -					local);
> > > -	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
> > > -	if (*local) {
> > > -		if (size > (args->geo->blksize / 2)) {
> > > -			/* Double split possible */
> > > -			nblks *= 2;
> > > -		}
> > > -	} 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);
> > > -	}
> > > -
> > > -	return nblks;
> > > -}
> > > -
> > >  STATIC int
> > >  xfs_attr_try_sf_addname(
> > >  	struct xfs_inode	*dp,
> > > @@ -248,6 +211,64 @@ xfs_attr_try_sf_addname(
> > >  	return error ? error : error2;
> > >  }
> > >  
> > > +STATIC uint
> > > +xfs_calc_attr_blocks(
> > > +	struct xfs_mount	*mp,
> > > +	unsigned int		total_dablks,
> > > +	unsigned int		rmt_blks)
> > > +{
> > > +	unsigned int bmbt_blks;
> > > +
> > > +	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> > > +					XFS_ATTR_FORK);
> > > +	return total_dablks + rmt_blks + bmbt_blks;
> > 
> > I think this calculation could be added to xfs_attr_calc_size and passed
> > back to the caller as another outparam.
> > 
> > At this point we have five different block counts I'm wondering if we
> > should create a struct and pass it around...
> > 
> > > +}
> > > +
> > > +/*
> > > + * Calculate how many blocks we need for the new attribute,
> > > + */
> > > +void
> > > +xfs_attr_calc_size(
> > > +	struct xfs_mount	*mp,
> > > +	int			namelen,
> > > +	int			valuelen,
> > > +	int			*local,
> > > +	unsigned int		*log_dablks,
> > > +	unsigned int		*rmt_blks,
> > > +	unsigned int		*total_dablks)
> > 
> > ...something like this?  It'll be much easier to remember what each of
> > those parameters actually do with a full sentence comment:
> > 
> > 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;
> > 
> > 	/* Blocks we might need to map into the attribute fork. */
> > 	unsigned int		total_dablks;
> > 
> > 	/* Blocks we might need to allocate. */
> > 	unsigned int		alloc_blks;
> > 
> > 	/* Blocks we might need to create all the new attr fork mappings. */
> > 	unsigned int		bmbt_blks;
> > };
> > 
> > > +{
> > > +	unsigned int		blksize;
> > > +	int			size;
> > > +
> > > +	blksize = mp->m_dir_geo->blksize;
> > > +	*log_dablks = 0;
> > > +	*rmt_blks = 0;
> > > +	*total_dablks = 0;
> > 
> > No need to zero out variables that we're going to set two lines later.
> > 
> > > +
> > > +	/*
> > > +	 * Determine space new attribute will use, and if it would be
> > > +	 * "local" or "remote" (note: local != inline).
> > > +	 */
> > > +	size = xfs_attr_leaf_newentsize(mp, namelen, valuelen, local);
> > > +
> > > +	*total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> > > +	*log_dablks = 2 * *total_dablks;
> > > +
> > > +	if (*local) {
> > > +		if (size > (blksize / 2)) {
> > > +			/* Double split possible */
> > > +			*log_dablks += *total_dablks;
> > > +			*total_dablks *= 2;
> > > +		}
> > > +	} else {
> > > +		/*
> > > +		 * Out of line attribute, cannot double split, but
> > > +		 * make room for the attribute value itself.
> > > +		 */
> > > +		*rmt_blks = xfs_attr3_rmt_blocks(mp, valuelen);
> > > +	}
> > > +}
> > > +
> > >  /*
> > >   * Set the attribute specified in @args.
> > >   */
> > > @@ -346,6 +367,9 @@ xfs_attr_set(
> > >  	struct xfs_mount	*mp = dp->i_mount;
> > >  	struct xfs_da_args	args;
> > >  	struct xfs_trans_res	tres;
> > > +	unsigned int		log_dablks;
> > > +	unsigned int		rmt_blks;
> > > +	unsigned int		total_dablks;
> > >  	int			rsvd = (flags & ATTR_ROOT) != 0;
> > >  	int			error, local;
> > >  
> > > @@ -361,7 +385,11 @@ xfs_attr_set(
> > >  	args.value = value;
> > >  	args.valuelen = valuelen;
> > >  	args.op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
> > > -	args.total = xfs_attr_calc_size(&args, &local);
> > > +
> > > +	xfs_attr_calc_size(mp, args.namelen, args.valuelen, &local,
> > > +			&log_dablks, &rmt_blks, &total_dablks);
> > > +
> > > +	args.total = xfs_calc_attr_blocks(mp, total_dablks, rmt_blks);
> > >  
> > >  	error = xfs_qm_dqattach(dp);
> > >  	if (error)
> > > @@ -380,8 +408,8 @@ 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, log_dablks, rmt_blks,
> > > +					total_dablks);
> > >  	tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
> > >  	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> > >  
> > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > > index 94badfa1743e..a1c77618802b 100644
> > > --- a/fs/xfs/libxfs/xfs_attr.h
> > > +++ b/fs/xfs/libxfs/xfs_attr.h
> > > @@ -154,5 +154,8 @@ int xfs_attr_remove_args(struct xfs_da_args *args);
> > >  int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
> > >  		  int flags, struct attrlist_cursor_kern *cursor);
> > >  bool xfs_attr_namecheck(const void *name, size_t length);
> > > +void xfs_attr_calc_size(struct xfs_mount *mp, int namelen, int valuelen,
> > > +			int *local, unsigned int *log_dablks,
> > > +			unsigned int *rmt_blks, unsigned int *total_dablks);
> > >  
> > >  #endif	/* __XFS_ATTR_H__ */
> > > diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c b/fs/xfs/libxfs/xfs_log_rlimit.c
> > > index 7f55eb3f3653..33b805411f72 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"
> > > @@ -23,17 +24,19 @@ STATIC int
> > >  xfs_log_calc_max_attrsetm_res(
> > >  	struct xfs_mount	*mp)
> > >  {
> > > -	int			size;
> > > -	int			nblks;
> > > +	int		size;
> > > +	int		local;
> > > +	unsigned int	total_dablks;
> > > +	unsigned int	rmt_blks;
> > > +	unsigned int	log_dablks;
> > >  
> > >  	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, size, 0, &local, &log_dablks, &rmt_blks,
> > > +			&total_dablks);
> > > +	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, log_dablks, rmt_blks, total_dablks);
> > >  }
> > >  
> > >  /*
> > > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> > > index 824073a839ac..8a0fea655358 100644
> > > --- a/fs/xfs/libxfs/xfs_trans_resv.c
> > > +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> > > @@ -701,12 +701,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 +712,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);
> > 
> > What effect does changing these reservation calculations have on the
> > computed minimum log size?
> > 
> > >  }
> > >  
> > >  /*
> > > @@ -832,6 +810,32 @@ xfs_calc_sb_reservation(
> > >  	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
> > >  }
> > >  
> > > +uint
> > > +xfs_calc_attr_res(
> > > +	struct xfs_mount	*mp,
> > > +	unsigned int		log_dablks,
> > > +	unsigned int		rmt_blks,
> > > +	unsigned int		total_dablks)
> > > +{
> > > +	unsigned int		da_blksize;
> > > +	unsigned int		fs_blksize;
> > > +	unsigned int		bmbt_blks;
> > > +	unsigned int		space_blks;
> > > +
> > > +	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> > > +					XFS_ATTR_FORK);
> > 
> > Pass in the resv structure above and you won't need to calculate this
> > again.
> > 
> > > +	space_blks = xfs_allocfree_log_count(mp,
> > > +					total_dablks + rmt_blks + bmbt_blks);
> > 
> > Only two levels of indent needed here:
> > 
> > 	space_blks = xfs_allocfree_log_count(mp,
> > 			total_dablks + rmt_blks + bmbt_blks);
> > 
> > > +
> > > +	da_blksize = mp->m_attr_geo->blksize;
> > > +	fs_blksize = mp->m_sb.sb_blocksize;
> > 
> > You could probably pass these to xfs_calc_buf_res directly.
> > 
> > I'll give this a spin and see how it does.
> > 
> > --D
> > 
> > > +
> > > +	return M_RES(mp)->tr_attrsetm.tr_logres +
> > > +		xfs_calc_buf_res(log_dablks, da_blksize) +
> > > +		xfs_calc_buf_res(bmbt_blks, fs_blksize) +
> > > +		xfs_calc_buf_res(space_blks, fs_blksize);
> > > +}
> > > +
> > >  void
> > >  xfs_trans_resv_calc(
> > >  	struct xfs_mount	*mp,
> > > @@ -942,7 +946,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 7241ab28cf84..48ceba72fb12 100644
> > > --- a/fs/xfs/libxfs/xfs_trans_resv.h
> > > +++ b/fs/xfs/libxfs/xfs_trans_resv.h
> > > @@ -91,6 +91,8 @@ 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, unsigned int log_dablks,
> > > +		unsigned int rmt_blks, unsigned int total_dablks);
> > >  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);
> > >  
> > 
> 
> Hi Darrick,
> 
> I agree to the changes you have suggested. I will apply them and post the next
> version soon.
> 
> I will also figure out the effect of this patch on minimum log size.
> 

Darrick, The log space reservation numbers mentioned in response to one of the 
of earlier mails were not correct. I had misinterpreted tr_logres to be in units
of blocks when doing calculations.

Here are the correct numbers,

Without patch
-------------
xattr log reservation space = 555768

With patch
----------
xattr log reservation space = 834936

The changes in xattr log space reservation numbers do not have any effect on
minimum log size calculation. This is because log reservation for truncate
operation is much larger; i.e

tr_logres = 259968, tr_logcount = 8, tr_logflags = 4

... which would be 259968 * 8 = 2079744 bytes

Since 2079744 > 834936, the rest of the calculation for minimum log
reservation does not change.

-- 
chandan

Re: [PATCH V2 2/2] xfs: Fix log reservation calculation for xattr insert operation

[Darrick J. Wong]

On Fri, Jan 17, 2020 at 08:17:55PM +0530, Chandan Rajendra wrote:
> On Friday, January 17, 2020 10:16 AM Chandan Rajendra wrote: 
> > On Friday, January 17, 2020 6:16 AM Darrick J. Wong wrote: 
> > > On Wed, Jan 15, 2020 at 06:24:21PM +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 be count, block number, rmapbt 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. Hence log reservation
> > > > calculation for xattr insert operation gives incorrect value.
> > > > 
> > > > Apart from the above, xfs_log_calc_max_attrsetm_res() passes a byte count as
> > > > an argument to XFS_NEXTENTADD_SPACE_RES() instead of block count.
> > > > 
> > > > To fix these issues, this commit refactors xfs_attr_calc_size() to calculate,
> > > > 1. The number of dabtree blocks that need to be logged.
> > > > 2. The number of remote blocks that need to allocated.
> > > > 3. The number of dabtree blocks that need to allocated.
> > > > 
> > > > xfs_attr_set() uses this information to compute
> > > > 1. Number of blocks that needs to allocated during the transaction.
> > > > 2. Number of bytes that needs 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 dabtree blocks to be
> > > > logged which it uses to figure out the total number of blocks to be logged.
> > > > 
> > > > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > > > ---
> > > > Changelog:
> > > > V1 -> V2:
> > > > 1. xfs_attr_calc_size() computes
> > > >    - Number of blocks required to log dabtree blocks.
> > > >    - Number of remote blocks.
> > > >    - Total dabtree blocks to be allocated.
> > > > 2. Add new function xfs_calc_attr_blocks() to compute the total number of
> > > >    blocks to be allocated during xattr insert operation.
> > > > 3. Add new function xfs_calc_attr_res() to compute the log space to be
> > > >    reserved during xattr insert operation.
> > > > 
> > > >  fs/xfs/libxfs/xfs_attr.c       | 108 +++++++++++++++++++++------------
> > > >  fs/xfs/libxfs/xfs_attr.h       |   3 +
> > > >  fs/xfs/libxfs/xfs_log_rlimit.c |  17 +++---
> > > >  fs/xfs/libxfs/xfs_trans_resv.c |  56 +++++++++--------
> > > >  fs/xfs/libxfs/xfs_trans_resv.h |   2 +
> > > >  5 files changed, 113 insertions(+), 73 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > > index 1eae1db74f6c..363b4c47b134 100644
> > > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > > @@ -183,43 +183,6 @@ xfs_attr_get(
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -/*
> > > > - * Calculate how many blocks we need for the new attribute,
> > > > - */
> > > > -STATIC int
> > > > -xfs_attr_calc_size(
> > > > -	struct xfs_da_args	*args,
> > > > -	int			*local)
> > > > -{
> > > > -	struct xfs_mount	*mp = args->dp->i_mount;
> > > > -	int			size;
> > > > -	int			nblks;
> > > > -
> > > > -	/*
> > > > -	 * Determine space new attribute will use, and if it would be
> > > > -	 * "local" or "remote" (note: local != inline).
> > > > -	 */
> > > > -	size = xfs_attr_leaf_newentsize(mp, args->namelen, args->valuelen,
> > > > -					local);
> > > > -	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
> > > > -	if (*local) {
> > > > -		if (size > (args->geo->blksize / 2)) {
> > > > -			/* Double split possible */
> > > > -			nblks *= 2;
> > > > -		}
> > > > -	} 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);
> > > > -	}
> > > > -
> > > > -	return nblks;
> > > > -}
> > > > -
> > > >  STATIC int
> > > >  xfs_attr_try_sf_addname(
> > > >  	struct xfs_inode	*dp,
> > > > @@ -248,6 +211,64 @@ xfs_attr_try_sf_addname(
> > > >  	return error ? error : error2;
> > > >  }
> > > >  
> > > > +STATIC uint
> > > > +xfs_calc_attr_blocks(
> > > > +	struct xfs_mount	*mp,
> > > > +	unsigned int		total_dablks,
> > > > +	unsigned int		rmt_blks)
> > > > +{
> > > > +	unsigned int bmbt_blks;
> > > > +
> > > > +	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> > > > +					XFS_ATTR_FORK);
> > > > +	return total_dablks + rmt_blks + bmbt_blks;
> > > 
> > > I think this calculation could be added to xfs_attr_calc_size and passed
> > > back to the caller as another outparam.
> > > 
> > > At this point we have five different block counts I'm wondering if we
> > > should create a struct and pass it around...
> > > 
> > > > +}
> > > > +
> > > > +/*
> > > > + * Calculate how many blocks we need for the new attribute,
> > > > + */
> > > > +void
> > > > +xfs_attr_calc_size(
> > > > +	struct xfs_mount	*mp,
> > > > +	int			namelen,
> > > > +	int			valuelen,
> > > > +	int			*local,
> > > > +	unsigned int		*log_dablks,
> > > > +	unsigned int		*rmt_blks,
> > > > +	unsigned int		*total_dablks)
> > > 
> > > ...something like this?  It'll be much easier to remember what each of
> > > those parameters actually do with a full sentence comment:
> > > 
> > > 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;
> > > 
> > > 	/* Blocks we might need to map into the attribute fork. */
> > > 	unsigned int		total_dablks;
> > > 
> > > 	/* Blocks we might need to allocate. */
> > > 	unsigned int		alloc_blks;
> > > 
> > > 	/* Blocks we might need to create all the new attr fork mappings. */
> > > 	unsigned int		bmbt_blks;
> > > };
> > > 
> > > > +{
> > > > +	unsigned int		blksize;
> > > > +	int			size;
> > > > +
> > > > +	blksize = mp->m_dir_geo->blksize;
> > > > +	*log_dablks = 0;
> > > > +	*rmt_blks = 0;
> > > > +	*total_dablks = 0;
> > > 
> > > No need to zero out variables that we're going to set two lines later.
> > > 
> > > > +
> > > > +	/*
> > > > +	 * Determine space new attribute will use, and if it would be
> > > > +	 * "local" or "remote" (note: local != inline).
> > > > +	 */
> > > > +	size = xfs_attr_leaf_newentsize(mp, namelen, valuelen, local);
> > > > +
> > > > +	*total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> > > > +	*log_dablks = 2 * *total_dablks;
> > > > +
> > > > +	if (*local) {
> > > > +		if (size > (blksize / 2)) {
> > > > +			/* Double split possible */
> > > > +			*log_dablks += *total_dablks;
> > > > +			*total_dablks *= 2;
> > > > +		}
> > > > +	} else {
> > > > +		/*
> > > > +		 * Out of line attribute, cannot double split, but
> > > > +		 * make room for the attribute value itself.
> > > > +		 */
> > > > +		*rmt_blks = xfs_attr3_rmt_blocks(mp, valuelen);
> > > > +	}
> > > > +}
> > > > +
> > > >  /*
> > > >   * Set the attribute specified in @args.
> > > >   */
> > > > @@ -346,6 +367,9 @@ xfs_attr_set(
> > > >  	struct xfs_mount	*mp = dp->i_mount;
> > > >  	struct xfs_da_args	args;
> > > >  	struct xfs_trans_res	tres;
> > > > +	unsigned int		log_dablks;
> > > > +	unsigned int		rmt_blks;
> > > > +	unsigned int		total_dablks;
> > > >  	int			rsvd = (flags & ATTR_ROOT) != 0;
> > > >  	int			error, local;
> > > >  
> > > > @@ -361,7 +385,11 @@ xfs_attr_set(
> > > >  	args.value = value;
> > > >  	args.valuelen = valuelen;
> > > >  	args.op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
> > > > -	args.total = xfs_attr_calc_size(&args, &local);
> > > > +
> > > > +	xfs_attr_calc_size(mp, args.namelen, args.valuelen, &local,
> > > > +			&log_dablks, &rmt_blks, &total_dablks);
> > > > +
> > > > +	args.total = xfs_calc_attr_blocks(mp, total_dablks, rmt_blks);
> > > >  
> > > >  	error = xfs_qm_dqattach(dp);
> > > >  	if (error)
> > > > @@ -380,8 +408,8 @@ 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, log_dablks, rmt_blks,
> > > > +					total_dablks);
> > > >  	tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
> > > >  	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> > > >  
> > > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > > > index 94badfa1743e..a1c77618802b 100644
> > > > --- a/fs/xfs/libxfs/xfs_attr.h
> > > > +++ b/fs/xfs/libxfs/xfs_attr.h
> > > > @@ -154,5 +154,8 @@ int xfs_attr_remove_args(struct xfs_da_args *args);
> > > >  int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
> > > >  		  int flags, struct attrlist_cursor_kern *cursor);
> > > >  bool xfs_attr_namecheck(const void *name, size_t length);
> > > > +void xfs_attr_calc_size(struct xfs_mount *mp, int namelen, int valuelen,
> > > > +			int *local, unsigned int *log_dablks,
> > > > +			unsigned int *rmt_blks, unsigned int *total_dablks);
> > > >  
> > > >  #endif	/* __XFS_ATTR_H__ */
> > > > diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c b/fs/xfs/libxfs/xfs_log_rlimit.c
> > > > index 7f55eb3f3653..33b805411f72 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"
> > > > @@ -23,17 +24,19 @@ STATIC int
> > > >  xfs_log_calc_max_attrsetm_res(
> > > >  	struct xfs_mount	*mp)
> > > >  {
> > > > -	int			size;
> > > > -	int			nblks;
> > > > +	int		size;
> > > > +	int		local;
> > > > +	unsigned int	total_dablks;
> > > > +	unsigned int	rmt_blks;
> > > > +	unsigned int	log_dablks;
> > > >  
> > > >  	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, size, 0, &local, &log_dablks, &rmt_blks,
> > > > +			&total_dablks);
> > > > +	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, log_dablks, rmt_blks, total_dablks);
> > > >  }
> > > >  
> > > >  /*
> > > > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> > > > index 824073a839ac..8a0fea655358 100644
> > > > --- a/fs/xfs/libxfs/xfs_trans_resv.c
> > > > +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> > > > @@ -701,12 +701,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 +712,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);
> > > 
> > > What effect does changing these reservation calculations have on the
> > > computed minimum log size?
> > > 
> > > >  }
> > > >  
> > > >  /*
> > > > @@ -832,6 +810,32 @@ xfs_calc_sb_reservation(
> > > >  	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
> > > >  }
> > > >  
> > > > +uint
> > > > +xfs_calc_attr_res(
> > > > +	struct xfs_mount	*mp,
> > > > +	unsigned int		log_dablks,
> > > > +	unsigned int		rmt_blks,
> > > > +	unsigned int		total_dablks)
> > > > +{
> > > > +	unsigned int		da_blksize;
> > > > +	unsigned int		fs_blksize;
> > > > +	unsigned int		bmbt_blks;
> > > > +	unsigned int		space_blks;
> > > > +
> > > > +	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> > > > +					XFS_ATTR_FORK);
> > > 
> > > Pass in the resv structure above and you won't need to calculate this
> > > again.
> > > 
> > > > +	space_blks = xfs_allocfree_log_count(mp,
> > > > +					total_dablks + rmt_blks + bmbt_blks);
> > > 
> > > Only two levels of indent needed here:
> > > 
> > > 	space_blks = xfs_allocfree_log_count(mp,
> > > 			total_dablks + rmt_blks + bmbt_blks);
> > > 
> > > > +
> > > > +	da_blksize = mp->m_attr_geo->blksize;
> > > > +	fs_blksize = mp->m_sb.sb_blocksize;
> > > 
> > > You could probably pass these to xfs_calc_buf_res directly.
> > > 
> > > I'll give this a spin and see how it does.
> > > 
> > > --D
> > > 
> > > > +
> > > > +	return M_RES(mp)->tr_attrsetm.tr_logres +
> > > > +		xfs_calc_buf_res(log_dablks, da_blksize) +
> > > > +		xfs_calc_buf_res(bmbt_blks, fs_blksize) +
> > > > +		xfs_calc_buf_res(space_blks, fs_blksize);
> > > > +}
> > > > +
> > > >  void
> > > >  xfs_trans_resv_calc(
> > > >  	struct xfs_mount	*mp,
> > > > @@ -942,7 +946,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 7241ab28cf84..48ceba72fb12 100644
> > > > --- a/fs/xfs/libxfs/xfs_trans_resv.h
> > > > +++ b/fs/xfs/libxfs/xfs_trans_resv.h
> > > > @@ -91,6 +91,8 @@ 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, unsigned int log_dablks,
> > > > +		unsigned int rmt_blks, unsigned int total_dablks);
> > > >  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);
> > > >  
> > > 
> > 
> > Hi Darrick,
> > 
> > I agree to the changes you have suggested. I will apply them and post the next
> > version soon.
> > 
> > I will also figure out the effect of this patch on minimum log size.
> > 
> 
> Darrick, The log space reservation numbers mentioned in response to one of the 
> of earlier mails were not correct. I had misinterpreted tr_logres to be in units
> of blocks when doing calculations.
> 
> Here are the correct numbers,
> 
> Without patch
> -------------
> xattr log reservation space = 555768
> 
> With patch
> ----------
> xattr log reservation space = 834936
> 
> The changes in xattr log space reservation numbers do not have any effect on
> minimum log size calculation. This is because log reservation for truncate
> operation is much larger; i.e
> 
> tr_logres = 259968, tr_logcount = 8, tr_logflags = 4
> 
> ... which would be 259968 * 8 = 2079744 bytes
> 
> Since 2079744 > 834936, the rest of the calculation for minimum log
> reservation does not change.

How about for V4 filesystems and V5 filesystems that don't have rmap or
reflink enabled?  The problem with increasing space reservation
requirements increasing is that old filesystems stop mounting on new
kernels...

(Just FYI, trace_xfs_trans_resv_calc is your friend here for the kernel
side; and the xfs_db logres command in userspace.)

The patch seemed ok on an overnight fstests run....

--D

> -- 
> chandan
> 
> 
> 

Re: [PATCH V2 2/2] xfs: Fix log reservation calculation for xattr insert operation

[Darrick J. Wong]

On Fri, Jan 17, 2020 at 08:20:35AM -0800, Darrick J. Wong wrote:
> On Fri, Jan 17, 2020 at 08:17:55PM +0530, Chandan Rajendra wrote:
> > On Friday, January 17, 2020 10:16 AM Chandan Rajendra wrote: 
> > > On Friday, January 17, 2020 6:16 AM Darrick J. Wong wrote: 
> > > > On Wed, Jan 15, 2020 at 06:24:21PM +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 be count, block number, rmapbt 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. Hence log reservation
> > > > > calculation for xattr insert operation gives incorrect value.
> > > > > 
> > > > > Apart from the above, xfs_log_calc_max_attrsetm_res() passes a byte count as
> > > > > an argument to XFS_NEXTENTADD_SPACE_RES() instead of block count.
> > > > > 
> > > > > To fix these issues, this commit refactors xfs_attr_calc_size() to calculate,
> > > > > 1. The number of dabtree blocks that need to be logged.
> > > > > 2. The number of remote blocks that need to allocated.
> > > > > 3. The number of dabtree blocks that need to allocated.
> > > > > 
> > > > > xfs_attr_set() uses this information to compute
> > > > > 1. Number of blocks that needs to allocated during the transaction.
> > > > > 2. Number of bytes that needs 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 dabtree blocks to be
> > > > > logged which it uses to figure out the total number of blocks to be logged.
> > > > > 
> > > > > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > > > > ---
> > > > > Changelog:
> > > > > V1 -> V2:
> > > > > 1. xfs_attr_calc_size() computes
> > > > >    - Number of blocks required to log dabtree blocks.
> > > > >    - Number of remote blocks.
> > > > >    - Total dabtree blocks to be allocated.
> > > > > 2. Add new function xfs_calc_attr_blocks() to compute the total number of
> > > > >    blocks to be allocated during xattr insert operation.
> > > > > 3. Add new function xfs_calc_attr_res() to compute the log space to be
> > > > >    reserved during xattr insert operation.
> > > > > 
> > > > >  fs/xfs/libxfs/xfs_attr.c       | 108 +++++++++++++++++++++------------
> > > > >  fs/xfs/libxfs/xfs_attr.h       |   3 +
> > > > >  fs/xfs/libxfs/xfs_log_rlimit.c |  17 +++---
> > > > >  fs/xfs/libxfs/xfs_trans_resv.c |  56 +++++++++--------
> > > > >  fs/xfs/libxfs/xfs_trans_resv.h |   2 +
> > > > >  5 files changed, 113 insertions(+), 73 deletions(-)
> > > > > 
> > > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > > > index 1eae1db74f6c..363b4c47b134 100644
> > > > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > > > @@ -183,43 +183,6 @@ xfs_attr_get(
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > -/*
> > > > > - * Calculate how many blocks we need for the new attribute,
> > > > > - */
> > > > > -STATIC int
> > > > > -xfs_attr_calc_size(
> > > > > -	struct xfs_da_args	*args,
> > > > > -	int			*local)
> > > > > -{
> > > > > -	struct xfs_mount	*mp = args->dp->i_mount;
> > > > > -	int			size;
> > > > > -	int			nblks;
> > > > > -
> > > > > -	/*
> > > > > -	 * Determine space new attribute will use, and if it would be
> > > > > -	 * "local" or "remote" (note: local != inline).
> > > > > -	 */
> > > > > -	size = xfs_attr_leaf_newentsize(mp, args->namelen, args->valuelen,
> > > > > -					local);
> > > > > -	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
> > > > > -	if (*local) {
> > > > > -		if (size > (args->geo->blksize / 2)) {
> > > > > -			/* Double split possible */
> > > > > -			nblks *= 2;
> > > > > -		}
> > > > > -	} 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);
> > > > > -	}
> > > > > -
> > > > > -	return nblks;
> > > > > -}
> > > > > -
> > > > >  STATIC int
> > > > >  xfs_attr_try_sf_addname(
> > > > >  	struct xfs_inode	*dp,
> > > > > @@ -248,6 +211,64 @@ xfs_attr_try_sf_addname(
> > > > >  	return error ? error : error2;
> > > > >  }
> > > > >  
> > > > > +STATIC uint
> > > > > +xfs_calc_attr_blocks(
> > > > > +	struct xfs_mount	*mp,
> > > > > +	unsigned int		total_dablks,
> > > > > +	unsigned int		rmt_blks)
> > > > > +{
> > > > > +	unsigned int bmbt_blks;
> > > > > +
> > > > > +	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> > > > > +					XFS_ATTR_FORK);
> > > > > +	return total_dablks + rmt_blks + bmbt_blks;
> > > > 
> > > > I think this calculation could be added to xfs_attr_calc_size and passed
> > > > back to the caller as another outparam.
> > > > 
> > > > At this point we have five different block counts I'm wondering if we
> > > > should create a struct and pass it around...
> > > > 
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Calculate how many blocks we need for the new attribute,
> > > > > + */
> > > > > +void
> > > > > +xfs_attr_calc_size(
> > > > > +	struct xfs_mount	*mp,
> > > > > +	int			namelen,
> > > > > +	int			valuelen,
> > > > > +	int			*local,
> > > > > +	unsigned int		*log_dablks,
> > > > > +	unsigned int		*rmt_blks,
> > > > > +	unsigned int		*total_dablks)
> > > > 
> > > > ...something like this?  It'll be much easier to remember what each of
> > > > those parameters actually do with a full sentence comment:
> > > > 
> > > > 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;
> > > > 
> > > > 	/* Blocks we might need to map into the attribute fork. */
> > > > 	unsigned int		total_dablks;
> > > > 
> > > > 	/* Blocks we might need to allocate. */
> > > > 	unsigned int		alloc_blks;
> > > > 
> > > > 	/* Blocks we might need to create all the new attr fork mappings. */
> > > > 	unsigned int		bmbt_blks;
> > > > };
> > > > 
> > > > > +{
> > > > > +	unsigned int		blksize;
> > > > > +	int			size;
> > > > > +
> > > > > +	blksize = mp->m_dir_geo->blksize;
> > > > > +	*log_dablks = 0;
> > > > > +	*rmt_blks = 0;
> > > > > +	*total_dablks = 0;
> > > > 
> > > > No need to zero out variables that we're going to set two lines later.
> > > > 
> > > > > +
> > > > > +	/*
> > > > > +	 * Determine space new attribute will use, and if it would be
> > > > > +	 * "local" or "remote" (note: local != inline).
> > > > > +	 */
> > > > > +	size = xfs_attr_leaf_newentsize(mp, namelen, valuelen, local);
> > > > > +
> > > > > +	*total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> > > > > +	*log_dablks = 2 * *total_dablks;
> > > > > +
> > > > > +	if (*local) {
> > > > > +		if (size > (blksize / 2)) {
> > > > > +			/* Double split possible */
> > > > > +			*log_dablks += *total_dablks;
> > > > > +			*total_dablks *= 2;
> > > > > +		}
> > > > > +	} else {
> > > > > +		/*
> > > > > +		 * Out of line attribute, cannot double split, but
> > > > > +		 * make room for the attribute value itself.
> > > > > +		 */
> > > > > +		*rmt_blks = xfs_attr3_rmt_blocks(mp, valuelen);
> > > > > +	}
> > > > > +}
> > > > > +
> > > > >  /*
> > > > >   * Set the attribute specified in @args.
> > > > >   */
> > > > > @@ -346,6 +367,9 @@ xfs_attr_set(
> > > > >  	struct xfs_mount	*mp = dp->i_mount;
> > > > >  	struct xfs_da_args	args;
> > > > >  	struct xfs_trans_res	tres;
> > > > > +	unsigned int		log_dablks;
> > > > > +	unsigned int		rmt_blks;
> > > > > +	unsigned int		total_dablks;
> > > > >  	int			rsvd = (flags & ATTR_ROOT) != 0;
> > > > >  	int			error, local;
> > > > >  
> > > > > @@ -361,7 +385,11 @@ xfs_attr_set(
> > > > >  	args.value = value;
> > > > >  	args.valuelen = valuelen;
> > > > >  	args.op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
> > > > > -	args.total = xfs_attr_calc_size(&args, &local);
> > > > > +
> > > > > +	xfs_attr_calc_size(mp, args.namelen, args.valuelen, &local,
> > > > > +			&log_dablks, &rmt_blks, &total_dablks);
> > > > > +
> > > > > +	args.total = xfs_calc_attr_blocks(mp, total_dablks, rmt_blks);
> > > > >  
> > > > >  	error = xfs_qm_dqattach(dp);
> > > > >  	if (error)
> > > > > @@ -380,8 +408,8 @@ 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, log_dablks, rmt_blks,
> > > > > +					total_dablks);
> > > > >  	tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
> > > > >  	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> > > > >  
> > > > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > > > > index 94badfa1743e..a1c77618802b 100644
> > > > > --- a/fs/xfs/libxfs/xfs_attr.h
> > > > > +++ b/fs/xfs/libxfs/xfs_attr.h
> > > > > @@ -154,5 +154,8 @@ int xfs_attr_remove_args(struct xfs_da_args *args);
> > > > >  int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
> > > > >  		  int flags, struct attrlist_cursor_kern *cursor);
> > > > >  bool xfs_attr_namecheck(const void *name, size_t length);
> > > > > +void xfs_attr_calc_size(struct xfs_mount *mp, int namelen, int valuelen,
> > > > > +			int *local, unsigned int *log_dablks,
> > > > > +			unsigned int *rmt_blks, unsigned int *total_dablks);
> > > > >  
> > > > >  #endif	/* __XFS_ATTR_H__ */
> > > > > diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c b/fs/xfs/libxfs/xfs_log_rlimit.c
> > > > > index 7f55eb3f3653..33b805411f72 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"
> > > > > @@ -23,17 +24,19 @@ STATIC int
> > > > >  xfs_log_calc_max_attrsetm_res(
> > > > >  	struct xfs_mount	*mp)
> > > > >  {
> > > > > -	int			size;
> > > > > -	int			nblks;
> > > > > +	int		size;
> > > > > +	int		local;
> > > > > +	unsigned int	total_dablks;
> > > > > +	unsigned int	rmt_blks;
> > > > > +	unsigned int	log_dablks;
> > > > >  
> > > > >  	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, size, 0, &local, &log_dablks, &rmt_blks,
> > > > > +			&total_dablks);
> > > > > +	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, log_dablks, rmt_blks, total_dablks);
> > > > >  }
> > > > >  
> > > > >  /*
> > > > > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> > > > > index 824073a839ac..8a0fea655358 100644
> > > > > --- a/fs/xfs/libxfs/xfs_trans_resv.c
> > > > > +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> > > > > @@ -701,12 +701,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 +712,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);
> > > > 
> > > > What effect does changing these reservation calculations have on the
> > > > computed minimum log size?
> > > > 
> > > > >  }
> > > > >  
> > > > >  /*
> > > > > @@ -832,6 +810,32 @@ xfs_calc_sb_reservation(
> > > > >  	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
> > > > >  }
> > > > >  
> > > > > +uint
> > > > > +xfs_calc_attr_res(
> > > > > +	struct xfs_mount	*mp,
> > > > > +	unsigned int		log_dablks,
> > > > > +	unsigned int		rmt_blks,
> > > > > +	unsigned int		total_dablks)
> > > > > +{
> > > > > +	unsigned int		da_blksize;
> > > > > +	unsigned int		fs_blksize;
> > > > > +	unsigned int		bmbt_blks;
> > > > > +	unsigned int		space_blks;
> > > > > +
> > > > > +	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> > > > > +					XFS_ATTR_FORK);
> > > > 
> > > > Pass in the resv structure above and you won't need to calculate this
> > > > again.
> > > > 
> > > > > +	space_blks = xfs_allocfree_log_count(mp,
> > > > > +					total_dablks + rmt_blks + bmbt_blks);
> > > > 
> > > > Only two levels of indent needed here:
> > > > 
> > > > 	space_blks = xfs_allocfree_log_count(mp,
> > > > 			total_dablks + rmt_blks + bmbt_blks);
> > > > 
> > > > > +
> > > > > +	da_blksize = mp->m_attr_geo->blksize;
> > > > > +	fs_blksize = mp->m_sb.sb_blocksize;
> > > > 
> > > > You could probably pass these to xfs_calc_buf_res directly.
> > > > 
> > > > I'll give this a spin and see how it does.
> > > > 
> > > > --D
> > > > 
> > > > > +
> > > > > +	return M_RES(mp)->tr_attrsetm.tr_logres +
> > > > > +		xfs_calc_buf_res(log_dablks, da_blksize) +
> > > > > +		xfs_calc_buf_res(bmbt_blks, fs_blksize) +
> > > > > +		xfs_calc_buf_res(space_blks, fs_blksize);
> > > > > +}
> > > > > +
> > > > >  void
> > > > >  xfs_trans_resv_calc(
> > > > >  	struct xfs_mount	*mp,
> > > > > @@ -942,7 +946,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 7241ab28cf84..48ceba72fb12 100644
> > > > > --- a/fs/xfs/libxfs/xfs_trans_resv.h
> > > > > +++ b/fs/xfs/libxfs/xfs_trans_resv.h
> > > > > @@ -91,6 +91,8 @@ 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, unsigned int log_dablks,
> > > > > +		unsigned int rmt_blks, unsigned int total_dablks);
> > > > >  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);
> > > > >  
> > > > 
> > > 
> > > Hi Darrick,
> > > 
> > > I agree to the changes you have suggested. I will apply them and post the next
> > > version soon.
> > > 
> > > I will also figure out the effect of this patch on minimum log size.
> > > 
> > 
> > Darrick, The log space reservation numbers mentioned in response to one of the 
> > of earlier mails were not correct. I had misinterpreted tr_logres to be in units
> > of blocks when doing calculations.
> > 
> > Here are the correct numbers,
> > 
> > Without patch
> > -------------
> > xattr log reservation space = 555768
> > 
> > With patch
> > ----------
> > xattr log reservation space = 834936
> > 
> > The changes in xattr log space reservation numbers do not have any effect on
> > minimum log size calculation. This is because log reservation for truncate
> > operation is much larger; i.e
> > 
> > tr_logres = 259968, tr_logcount = 8, tr_logflags = 4
> > 
> > ... which would be 259968 * 8 = 2079744 bytes
> > 
> > Since 2079744 > 834936, the rest of the calculation for minimum log
> > reservation does not change.
> 
> How about for V4 filesystems and V5 filesystems that don't have rmap or
> reflink enabled?  The problem with increasing space reservation
> requirements increasing is that old filesystems stop mounting on new
> kernels...
> 
> (Just FYI, trace_xfs_trans_resv_calc is your friend here for the kernel
> side; and the xfs_db logres command in userspace.)
> 
> The patch seemed ok on an overnight fstests run....

...until I kicked off a 1k blocksize run, which hung in xfs/021. :(

FSTYP         -- xfs (debug)
PLATFORM      -- Linux/x86_64 magnolia-mtr00 5.5.0-rc4-djw #rc4 SMP PREEMPT Thu Jan 2 13:59:44 PST 2020
MKFS_OPTIONS  -- -f -m reflink=1,rmapbt=1,metadir=0,bigtime=0,inobtcount=0, -i sparse=1, /dev/sdf
MOUNT_OPTIONS -- -o usrquota,grpquota,prjquota, /dev/sdf /opt

[  101.582889] run fstests xfs/021 at 2020-01-17 20:11:22
[  102.981053] XFS (sdf): Mounting V5 Filesystem
[  102.997152] XFS (sdf): Ending clean mount
[  103.000368] XFS (sdf): Quotacheck needed: Please wait.
[  103.027380] XFS (sdf): Quotacheck: Done.
[  103.029857] xfs filesystem being mounted at /opt supports timestamps until 2038 (0x7fffffff)
[  103.283136] XFS: Assertion failed: BTOBB(need_bytes) < log->l_logBBsize, file: fs/xfs/xfs_log.c, line: 1606
[  103.285135] ------------[ cut here ]------------
[  103.286351] WARNING: CPU: 3 PID: 3620 at fs/xfs/xfs_message.c:112 assfail+0x30/0x50 [xfs]
[  103.288067] Modules linked in: xfs libcrc32c ip6t_REJECT nf_reject_ipv6 ipt_REJECT nf_reject_ipv4 xt_tcpudp ip_set_hash_ip ip_set_hash_net xt_set ip_set_hash_mac ip_set nfnetlink bfq ip6table_filter ip6_tables iptable_filter sch_fq_codel nfsd auth_rpcgss oid_registry ip_tables x_tables nfsv4 af_packet
[  103.293068] CPU: 3 PID: 3620 Comm: attr Not tainted 5.5.0-rc4-djw #rc4
[  103.294340] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-1ubuntu1 04/01/2014
[  103.295966] RIP: 0010:assfail+0x30/0x50 [xfs]
[  103.296773] Code: 41 89 c8 48 89 d1 48 89 f2 48 c7 c6 20 66 44 a0 e8 c5 f8 ff ff 0f b6 1d 5a 22 11 00 80 fb 01 0f 87 bb 21 06 00 83 e3 01 75 04 <0f> 0b 5b c3 0f 0b 48 c7 c7 40 75 4d a0 e8 ad 04 00 e1 0f 1f 40 00
[  103.300009] RSP: 0018:ffffc900032a3aa0 EFLAGS: 00010246
[  103.300941] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[  103.302370] RDX: 00000000ffffffc0 RSI: 000000000000000a RDI: ffffffffa04357a4
[  103.303627] RBP: ffff8880796db800 R08: 0000000000000000 R09: 0000000000000000
[  103.305007] R10: 000000000000000a R11: f000000000000000 R12: 0000000000000020
[  103.306342] R13: ffff88807f92ed28 R14: 00000000003517f8 R15: 0000000000000003
[  103.307711] FS:  00007ff338bc5740(0000) GS:ffff88807e200000(0000) knlGS:0000000000000000
[  103.309615] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  103.310874] CR2: 00007ff33846e530 CR3: 000000006f08d002 CR4: 00000000001606a0
[  103.312112] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  103.313443] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  103.314766] Call Trace:
[  103.315334]  xlog_grant_push_ail+0xda/0xe0 [xfs]
[  103.316304]  xfs_log_reserve+0x127/0x4f0 [xfs]
[  103.317221]  xfs_trans_reserve+0x1ae/0x2c0 [xfs]
[  103.318318]  xfs_trans_alloc+0xca/0x200 [xfs]
[  103.319391]  xfs_attr_set+0x1ff/0x430 [xfs]
[  103.320460]  xfs_xattr_set+0x67/0xb0 [xfs]
[  103.321261]  __vfs_setxattr+0x66/0x80
[  103.321961]  __vfs_setxattr_noperm+0x54/0xf0
[  103.322814]  vfs_setxattr+0x81/0xa0
[  103.323480]  setxattr+0x13b/0x1c0
[  103.324194]  ? mnt_want_write+0x20/0x50
[  103.325054]  ? rcu_read_lock_any_held+0x83/0xb0
[  103.326124]  ? __sb_start_write+0x185/0x280
[  103.327123]  ? preempt_count_add+0x4d/0xa0
[  103.328036]  path_setxattr+0xbe/0xe0
[  103.328692]  __x64_sys_lsetxattr+0x24/0x30
[  103.329405]  do_syscall_64+0x50/0x1a0
[  103.330171]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[  103.331056] RIP: 0033:0x7ff3384d0839
[  103.331728] Code: 00 f3 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 1f f6 2c 00 f7 d8 64 89 01 48
[  103.335916] RSP: 002b:00007fff8fe9dab8 EFLAGS: 00000206 ORIG_RAX: 00000000000000bd
[  103.337223] RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007ff3384d0839
[  103.338407] RDX: 0000565357fa62b0 RSI: 00007fff8fe9daf0 RDI: 00007fff8fe9f29e
[  103.339654] RBP: 0000000000000000 R08: 0000000000000000 R09: 00007fff8fe9daf0
[  103.340900] R10: 000000000000ffff R11: 0000000000000206 R12: 0000000000000000
[  103.342210] R13: 00007fff8fe9daf0 R14: 00007fff8fe9f29b R15: 0000000000000000
[  103.343486] irq event stamp: 3026
[  103.344130] hardirqs last  enabled at (3025): [<ffffffff810d6f28>] console_unlock+0x428/0x580
[  103.347571] hardirqs last disabled at (3026): [<ffffffff81001d50>] trace_hardirqs_off_thunk+0x1a/0x1c
[  103.349236] softirqs last  enabled at (2744): [<ffffffff81a003af>] __do_softirq+0x3af/0x4a4
[  103.350835] softirqs last disabled at (2737): [<ffffffff810655ac>] irq_exit+0xbc/0xe0
[  103.352277] ---[ end trace faf0d211155beb6f ]---

(this repeated a bunch of times before it hanged)

--D

> --D
> 
> > -- 
> > chandan
> > 
> > 
> > 

Re: [PATCH V2 2/2] xfs: Fix log reservation calculation for xattr insert operation

[Chandan Rajendra]

On Saturday, January 18, 2020 10:15 AM Darrick J. Wong wrote: 
> On Fri, Jan 17, 2020 at 08:20:35AM -0800, Darrick J. Wong wrote:
> > On Fri, Jan 17, 2020 at 08:17:55PM +0530, Chandan Rajendra wrote:
> > > On Friday, January 17, 2020 10:16 AM Chandan Rajendra wrote: 
> > > > On Friday, January 17, 2020 6:16 AM Darrick J. Wong wrote: 
> > > > > On Wed, Jan 15, 2020 at 06:24:21PM +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 be count, block number, rmapbt 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. Hence log reservation
> > > > > > calculation for xattr insert operation gives incorrect value.
> > > > > > 
> > > > > > Apart from the above, xfs_log_calc_max_attrsetm_res() passes a byte count as
> > > > > > an argument to XFS_NEXTENTADD_SPACE_RES() instead of block count.
> > > > > > 
> > > > > > To fix these issues, this commit refactors xfs_attr_calc_size() to calculate,
> > > > > > 1. The number of dabtree blocks that need to be logged.
> > > > > > 2. The number of remote blocks that need to allocated.
> > > > > > 3. The number of dabtree blocks that need to allocated.
> > > > > > 
> > > > > > xfs_attr_set() uses this information to compute
> > > > > > 1. Number of blocks that needs to allocated during the transaction.
> > > > > > 2. Number of bytes that needs 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 dabtree blocks to be
> > > > > > logged which it uses to figure out the total number of blocks to be logged.
> > > > > > 
> > > > > > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > > > > > ---
> > > > > > Changelog:
> > > > > > V1 -> V2:
> > > > > > 1. xfs_attr_calc_size() computes
> > > > > >    - Number of blocks required to log dabtree blocks.
> > > > > >    - Number of remote blocks.
> > > > > >    - Total dabtree blocks to be allocated.
> > > > > > 2. Add new function xfs_calc_attr_blocks() to compute the total number of
> > > > > >    blocks to be allocated during xattr insert operation.
> > > > > > 3. Add new function xfs_calc_attr_res() to compute the log space to be
> > > > > >    reserved during xattr insert operation.
> > > > > > 
> > > > > >  fs/xfs/libxfs/xfs_attr.c       | 108 +++++++++++++++++++++------------
> > > > > >  fs/xfs/libxfs/xfs_attr.h       |   3 +
> > > > > >  fs/xfs/libxfs/xfs_log_rlimit.c |  17 +++---
> > > > > >  fs/xfs/libxfs/xfs_trans_resv.c |  56 +++++++++--------
> > > > > >  fs/xfs/libxfs/xfs_trans_resv.h |   2 +
> > > > > >  5 files changed, 113 insertions(+), 73 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > > > > index 1eae1db74f6c..363b4c47b134 100644
> > > > > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > > > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > > > > @@ -183,43 +183,6 @@ xfs_attr_get(
> > > > > >  	return 0;
> > > > > >  }
> > > > > >  
> > > > > > -/*
> > > > > > - * Calculate how many blocks we need for the new attribute,
> > > > > > - */
> > > > > > -STATIC int
> > > > > > -xfs_attr_calc_size(
> > > > > > -	struct xfs_da_args	*args,
> > > > > > -	int			*local)
> > > > > > -{
> > > > > > -	struct xfs_mount	*mp = args->dp->i_mount;
> > > > > > -	int			size;
> > > > > > -	int			nblks;
> > > > > > -
> > > > > > -	/*
> > > > > > -	 * Determine space new attribute will use, and if it would be
> > > > > > -	 * "local" or "remote" (note: local != inline).
> > > > > > -	 */
> > > > > > -	size = xfs_attr_leaf_newentsize(mp, args->namelen, args->valuelen,
> > > > > > -					local);
> > > > > > -	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
> > > > > > -	if (*local) {
> > > > > > -		if (size > (args->geo->blksize / 2)) {
> > > > > > -			/* Double split possible */
> > > > > > -			nblks *= 2;
> > > > > > -		}
> > > > > > -	} 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);
> > > > > > -	}
> > > > > > -
> > > > > > -	return nblks;
> > > > > > -}
> > > > > > -
> > > > > >  STATIC int
> > > > > >  xfs_attr_try_sf_addname(
> > > > > >  	struct xfs_inode	*dp,
> > > > > > @@ -248,6 +211,64 @@ xfs_attr_try_sf_addname(
> > > > > >  	return error ? error : error2;
> > > > > >  }
> > > > > >  
> > > > > > +STATIC uint
> > > > > > +xfs_calc_attr_blocks(
> > > > > > +	struct xfs_mount	*mp,
> > > > > > +	unsigned int		total_dablks,
> > > > > > +	unsigned int		rmt_blks)
> > > > > > +{
> > > > > > +	unsigned int bmbt_blks;
> > > > > > +
> > > > > > +	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> > > > > > +					XFS_ATTR_FORK);
> > > > > > +	return total_dablks + rmt_blks + bmbt_blks;
> > > > > 
> > > > > I think this calculation could be added to xfs_attr_calc_size and passed
> > > > > back to the caller as another outparam.
> > > > > 
> > > > > At this point we have five different block counts I'm wondering if we
> > > > > should create a struct and pass it around...
> > > > > 
> > > > > > +}
> > > > > > +
> > > > > > +/*
> > > > > > + * Calculate how many blocks we need for the new attribute,
> > > > > > + */
> > > > > > +void
> > > > > > +xfs_attr_calc_size(
> > > > > > +	struct xfs_mount	*mp,
> > > > > > +	int			namelen,
> > > > > > +	int			valuelen,
> > > > > > +	int			*local,
> > > > > > +	unsigned int		*log_dablks,
> > > > > > +	unsigned int		*rmt_blks,
> > > > > > +	unsigned int		*total_dablks)
> > > > > 
> > > > > ...something like this?  It'll be much easier to remember what each of
> > > > > those parameters actually do with a full sentence comment:
> > > > > 
> > > > > 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;
> > > > > 
> > > > > 	/* Blocks we might need to map into the attribute fork. */
> > > > > 	unsigned int		total_dablks;
> > > > > 
> > > > > 	/* Blocks we might need to allocate. */
> > > > > 	unsigned int		alloc_blks;
> > > > > 
> > > > > 	/* Blocks we might need to create all the new attr fork mappings. */
> > > > > 	unsigned int		bmbt_blks;
> > > > > };
> > > > > 
> > > > > > +{
> > > > > > +	unsigned int		blksize;
> > > > > > +	int			size;
> > > > > > +
> > > > > > +	blksize = mp->m_dir_geo->blksize;
> > > > > > +	*log_dablks = 0;
> > > > > > +	*rmt_blks = 0;
> > > > > > +	*total_dablks = 0;
> > > > > 
> > > > > No need to zero out variables that we're going to set two lines later.
> > > > > 
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Determine space new attribute will use, and if it would be
> > > > > > +	 * "local" or "remote" (note: local != inline).
> > > > > > +	 */
> > > > > > +	size = xfs_attr_leaf_newentsize(mp, namelen, valuelen, local);
> > > > > > +
> > > > > > +	*total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> > > > > > +	*log_dablks = 2 * *total_dablks;
> > > > > > +
> > > > > > +	if (*local) {
> > > > > > +		if (size > (blksize / 2)) {
> > > > > > +			/* Double split possible */
> > > > > > +			*log_dablks += *total_dablks;
> > > > > > +			*total_dablks *= 2;
> > > > > > +		}
> > > > > > +	} else {
> > > > > > +		/*
> > > > > > +		 * Out of line attribute, cannot double split, but
> > > > > > +		 * make room for the attribute value itself.
> > > > > > +		 */
> > > > > > +		*rmt_blks = xfs_attr3_rmt_blocks(mp, valuelen);
> > > > > > +	}
> > > > > > +}
> > > > > > +
> > > > > >  /*
> > > > > >   * Set the attribute specified in @args.
> > > > > >   */
> > > > > > @@ -346,6 +367,9 @@ xfs_attr_set(
> > > > > >  	struct xfs_mount	*mp = dp->i_mount;
> > > > > >  	struct xfs_da_args	args;
> > > > > >  	struct xfs_trans_res	tres;
> > > > > > +	unsigned int		log_dablks;
> > > > > > +	unsigned int		rmt_blks;
> > > > > > +	unsigned int		total_dablks;
> > > > > >  	int			rsvd = (flags & ATTR_ROOT) != 0;
> > > > > >  	int			error, local;
> > > > > >  
> > > > > > @@ -361,7 +385,11 @@ xfs_attr_set(
> > > > > >  	args.value = value;
> > > > > >  	args.valuelen = valuelen;
> > > > > >  	args.op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
> > > > > > -	args.total = xfs_attr_calc_size(&args, &local);
> > > > > > +
> > > > > > +	xfs_attr_calc_size(mp, args.namelen, args.valuelen, &local,
> > > > > > +			&log_dablks, &rmt_blks, &total_dablks);
> > > > > > +
> > > > > > +	args.total = xfs_calc_attr_blocks(mp, total_dablks, rmt_blks);
> > > > > >  
> > > > > >  	error = xfs_qm_dqattach(dp);
> > > > > >  	if (error)
> > > > > > @@ -380,8 +408,8 @@ 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, log_dablks, rmt_blks,
> > > > > > +					total_dablks);
> > > > > >  	tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
> > > > > >  	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> > > > > >  
> > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > > > > > index 94badfa1743e..a1c77618802b 100644
> > > > > > --- a/fs/xfs/libxfs/xfs_attr.h
> > > > > > +++ b/fs/xfs/libxfs/xfs_attr.h
> > > > > > @@ -154,5 +154,8 @@ int xfs_attr_remove_args(struct xfs_da_args *args);
> > > > > >  int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
> > > > > >  		  int flags, struct attrlist_cursor_kern *cursor);
> > > > > >  bool xfs_attr_namecheck(const void *name, size_t length);
> > > > > > +void xfs_attr_calc_size(struct xfs_mount *mp, int namelen, int valuelen,
> > > > > > +			int *local, unsigned int *log_dablks,
> > > > > > +			unsigned int *rmt_blks, unsigned int *total_dablks);
> > > > > >  
> > > > > >  #endif	/* __XFS_ATTR_H__ */
> > > > > > diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c b/fs/xfs/libxfs/xfs_log_rlimit.c
> > > > > > index 7f55eb3f3653..33b805411f72 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"
> > > > > > @@ -23,17 +24,19 @@ STATIC int
> > > > > >  xfs_log_calc_max_attrsetm_res(
> > > > > >  	struct xfs_mount	*mp)
> > > > > >  {
> > > > > > -	int			size;
> > > > > > -	int			nblks;
> > > > > > +	int		size;
> > > > > > +	int		local;
> > > > > > +	unsigned int	total_dablks;
> > > > > > +	unsigned int	rmt_blks;
> > > > > > +	unsigned int	log_dablks;
> > > > > >  
> > > > > >  	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, size, 0, &local, &log_dablks, &rmt_blks,
> > > > > > +			&total_dablks);
> > > > > > +	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, log_dablks, rmt_blks, total_dablks);
> > > > > >  }
> > > > > >  
> > > > > >  /*
> > > > > > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> > > > > > index 824073a839ac..8a0fea655358 100644
> > > > > > --- a/fs/xfs/libxfs/xfs_trans_resv.c
> > > > > > +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> > > > > > @@ -701,12 +701,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 +712,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);
> > > > > 
> > > > > What effect does changing these reservation calculations have on the
> > > > > computed minimum log size?
> > > > > 
> > > > > >  }
> > > > > >  
> > > > > >  /*
> > > > > > @@ -832,6 +810,32 @@ xfs_calc_sb_reservation(
> > > > > >  	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
> > > > > >  }
> > > > > >  
> > > > > > +uint
> > > > > > +xfs_calc_attr_res(
> > > > > > +	struct xfs_mount	*mp,
> > > > > > +	unsigned int		log_dablks,
> > > > > > +	unsigned int		rmt_blks,
> > > > > > +	unsigned int		total_dablks)
> > > > > > +{
> > > > > > +	unsigned int		da_blksize;
> > > > > > +	unsigned int		fs_blksize;
> > > > > > +	unsigned int		bmbt_blks;
> > > > > > +	unsigned int		space_blks;
> > > > > > +
> > > > > > +	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> > > > > > +					XFS_ATTR_FORK);
> > > > > 
> > > > > Pass in the resv structure above and you won't need to calculate this
> > > > > again.
> > > > > 
> > > > > > +	space_blks = xfs_allocfree_log_count(mp,
> > > > > > +					total_dablks + rmt_blks + bmbt_blks);
> > > > > 
> > > > > Only two levels of indent needed here:
> > > > > 
> > > > > 	space_blks = xfs_allocfree_log_count(mp,
> > > > > 			total_dablks + rmt_blks + bmbt_blks);
> > > > > 
> > > > > > +
> > > > > > +	da_blksize = mp->m_attr_geo->blksize;
> > > > > > +	fs_blksize = mp->m_sb.sb_blocksize;
> > > > > 
> > > > > You could probably pass these to xfs_calc_buf_res directly.
> > > > > 
> > > > > I'll give this a spin and see how it does.
> > > > > 
> > > > > --D
> > > > > 
> > > > > > +
> > > > > > +	return M_RES(mp)->tr_attrsetm.tr_logres +
> > > > > > +		xfs_calc_buf_res(log_dablks, da_blksize) +
> > > > > > +		xfs_calc_buf_res(bmbt_blks, fs_blksize) +
> > > > > > +		xfs_calc_buf_res(space_blks, fs_blksize);
> > > > > > +}
> > > > > > +
> > > > > >  void
> > > > > >  xfs_trans_resv_calc(
> > > > > >  	struct xfs_mount	*mp,
> > > > > > @@ -942,7 +946,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 7241ab28cf84..48ceba72fb12 100644
> > > > > > --- a/fs/xfs/libxfs/xfs_trans_resv.h
> > > > > > +++ b/fs/xfs/libxfs/xfs_trans_resv.h
> > > > > > @@ -91,6 +91,8 @@ 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, unsigned int log_dablks,
> > > > > > +		unsigned int rmt_blks, unsigned int total_dablks);
> > > > > >  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);
> > > > > >  
> > > > > 
> > > > 
> > > > Hi Darrick,
> > > > 
> > > > I agree to the changes you have suggested. I will apply them and post the next
> > > > version soon.
> > > > 
> > > > I will also figure out the effect of this patch on minimum log size.
> > > > 
> > > 
> > > Darrick, The log space reservation numbers mentioned in response to one of the 
> > > of earlier mails were not correct. I had misinterpreted tr_logres to be in units
> > > of blocks when doing calculations.
> > > 
> > > Here are the correct numbers,
> > > 
> > > Without patch
> > > -------------
> > > xattr log reservation space = 555768
> > > 
> > > With patch
> > > ----------
> > > xattr log reservation space = 834936
> > > 
> > > The changes in xattr log space reservation numbers do not have any effect on
> > > minimum log size calculation. This is because log reservation for truncate
> > > operation is much larger; i.e
> > > 
> > > tr_logres = 259968, tr_logcount = 8, tr_logflags = 4
> > > 
> > > ... which would be 259968 * 8 = 2079744 bytes
> > > 
> > > Since 2079744 > 834936, the rest of the calculation for minimum log
> > > reservation does not change.
> > 
> > How about for V4 filesystems and V5 filesystems that don't have rmap or
> > reflink enabled?  The problem with increasing space reservation
> > requirements increasing is that old filesystems stop mounting on new
> > kernels...
> > 
> > (Just FYI, trace_xfs_trans_resv_calc is your friend here for the kernel
> > side; and the xfs_db logres command in userspace.)

For V4 filesystem, both userspace and kernel had the largest log reservation
for the "rename" operation i.e.

306528 * 2 (logcount) = 613056 bytes.

> > 
> > The patch seemed ok on an overnight fstests run....
> 
> ...until I kicked off a 1k blocksize run, which hung in xfs/021. :(


The command which causes the assert to be triggered is,

touch /mnt/testfile-0; (POSIXLY_CORRECT=yes dd if=/dev/zero bs=65535 count=1) | attr -s a1 /mnt/testfile-0 >/dev/null

Because of the large size of the xattr value, the value will be saved as a
remote attribute.

In the above patch we have,

tres.tr_logres = xfs_calc_attr_res(mp, log_dablks, rmt_blks,
				total_dablks);

And inside xfs_calc_attr_res(), we have,

space_blks = xfs_allocfree_log_count(mp, total_dablks + rmt_blks + bmbt_blks);

i.e. we are considering the value of rmt_blks when calculating log space
reservation for the free space tree blocks. In the existing code, we don't
have any calculation done for reserving log space for free space tree blocks.

With ~64k as the value length of the xattr and 4k as the block size, we get,
Remote blks = 17
Total dablks = 5
Bmbt blks = 2
free space tree blks = 768

With the 1k block size we will have,
Remote blks = 68
Total dablks = 5
Bmbt blks = 6
free space tree blks = 3002

If we do not consider "remote blocks" when performing log space reservation
calculation for free space trees, we will have,

For 1k block size,
12 (i.e. 5 (total_dablks) + 7 (bmbt_blks)) * 2 (nr_alloc_trees) * 4 (m_ag_maxlevels) = 96 blocks

For 4k block size,
10 (i.e. 5 (total_dablks) + 5 (bmbt_blks)) * 2 (nr_alloc_trees) * 3 (m_ag_maxlevels) = 60 blocks

Since we log only in units of 128 byte chunks and not the complete metadata
block, the above log reservation should suffice.

I removed "remote blocks" from log space reservation calculation and executed
fstests in the following configurations,
- 512 on v4
- 1k
- 1k on v4
- 4k
- 4k on v4

... and I didn't find any regression.

-- 
chandan