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

[PATCH V3 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>
---
 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 0d7fcc983b3da..1eae1db74f6cd 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 08d4b10ae2d53..7cd57e5844d80 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 f4a188e28b7b6..0ce1f9301157e 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 V3 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 an incorrect value.

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

To fix these issues, this commit 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 be allocated.
3. The number of dabtree blocks that need to be allocated.
4. The number of bmbt blocks that need to be allocated.
5. The total number of blocks that need to be allocated.

xfs_attr_set() uses this information to compute 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 blocks to be logged which it uses
to figure out the total number of bytes to be logged.

Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
---
Changelog:
V1 -> V2:
1. Use convenience variables to reduce indentation of code.

V2 -> V3:
1. Introduce 'struct xfs_attr_set_resv' to be used an as out parameter
   holding xattr reservation values.
2. Calculate number of bmbt blocks and total allocation blocks within
   xfs_attr_calc_size().

 fs/xfs/libxfs/xfs_attr.c       | 93 +++++++++++++++++++---------------
 fs/xfs/libxfs/xfs_attr.h       | 20 +++++++-
 fs/xfs/libxfs/xfs_log_rlimit.c | 14 ++---
 fs/xfs/libxfs/xfs_trans_resv.c | 52 +++++++++----------
 fs/xfs/libxfs/xfs_trans_resv.h |  2 +
 5 files changed, 107 insertions(+), 74 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 1eae1db74f6cd..1f3b001a1092e 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,53 @@ xfs_attr_try_sf_addname(
 	return error ? error : error2;
 }
 
+/*
+ * Calculate how many blocks we need for the new attribute,
+ */
+void
+xfs_attr_calc_size(
+	struct xfs_mount		*mp,
+	struct xfs_attr_set_resv	*resv,
+	int				namelen,
+	int				valuelen,
+	int				*local)
+{
+	unsigned int		blksize;
+	int			size;
+
+	blksize = mp->m_dir_geo->blksize;
+
+	/*
+	 * 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);
+
+	resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
+	resv->log_dablks = 2 * resv->total_dablks;
+
+	if (*local) {
+		if (size > (blksize / 2)) {
+			/* Double split possible */
+			resv->log_dablks += resv->total_dablks;
+			resv->total_dablks *= 2;
+		}
+	} else {
+		/*
+		 * Out of line attribute, cannot double split, but
+		 * make room for the attribute value itself.
+		 */
+		resv->rmt_blks = xfs_attr3_rmt_blocks(mp, valuelen);
+	}
+
+	resv->bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp,
+					resv->total_dablks + resv->rmt_blks,
+					XFS_ATTR_FORK);
+
+	resv->alloc_blks = resv->total_dablks + resv->rmt_blks +
+		resv->bmbt_blks;
+}
+
 /*
  * Set the attribute specified in @args.
  */
@@ -344,6 +354,7 @@ xfs_attr_set(
 	int			flags)
 {
 	struct xfs_mount	*mp = dp->i_mount;
+	struct xfs_attr_set_resv resv = { 0 };
 	struct xfs_da_args	args;
 	struct xfs_trans_res	tres;
 	int			rsvd = (flags & ATTR_ROOT) != 0;
@@ -361,7 +372,10 @@ 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, &resv, args.namelen, args.valuelen, &local);
+
+	args.total = resv.alloc_blks;
 
 	error = xfs_qm_dqattach(dp);
 	if (error)
@@ -380,8 +394,7 @@ xfs_attr_set(
 			return error;
 	}
 
-	tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
-			 M_RES(mp)->tr_attrsetrt.tr_logres * args.total;
+	tres.tr_logres = xfs_calc_attr_res(mp, &resv);
 	tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
 	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
 
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 94badfa1743e3..0b42faf7d6a1f 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -131,6 +131,22 @@ typedef struct xfs_attr_list_context {
 	int				index;		/* index into output buffer */
 } xfs_attr_list_context_t;
 
+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;
+
+	/* Number of blocks to allocate for the da btree. */
+	unsigned int		total_dablks;
+
+	/* Blocks we might need to create all the new attr fork mappings. */
+	unsigned int		bmbt_blks;
+
+	/* Total number of blocks we might have to allocate. */
+	unsigned int		alloc_blks;
+};
 
 /*========================================================================
  * Function prototypes for the kernel.
@@ -154,5 +170,7 @@ 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,
+			struct xfs_attr_set_resv *resv,
+			int namelen, int valuelen, int *local);
 #endif	/* __XFS_ATTR_H__ */
diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c b/fs/xfs/libxfs/xfs_log_rlimit.c
index 7f55eb3f36536..26566c25c7e2c 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,16 @@ STATIC int
 xfs_log_calc_max_attrsetm_res(
 	struct xfs_mount	*mp)
 {
-	int			size;
-	int			nblks;
+	struct xfs_attr_set_resv resv = { 0 };
+	int		size;
+	int		local;
 
 	size = xfs_attr_leaf_entsize_local_max(mp->m_attr_geo->blksize) -
 	       MAXNAMELEN - 1;
-	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
-	nblks += XFS_B_TO_FSB(mp, size);
-	nblks += XFS_NEXTENTADD_SPACE_RES(mp, size, XFS_ATTR_FORK);
+	xfs_attr_calc_size(mp, &resv, size, 0, &local);
+	ASSERT(local == 1);
 
-	return  M_RES(mp)->tr_attrsetm.tr_logres +
-		M_RES(mp)->tr_attrsetrt.tr_logres * nblks;
+	return xfs_calc_attr_res(mp, &resv);
 }
 
 /*
diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index 824073a839acb..867f1954c49bc 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -19,6 +19,7 @@
 #include "xfs_trans.h"
 #include "xfs_qm.h"
 #include "xfs_trans_space.h"
+#include "xfs_attr.h"
 
 #define _ALLOC	true
 #define _FREE	false
@@ -701,12 +702,10 @@ xfs_calc_attrinval_reservation(
  * Setting an attribute at mount time.
  *	the inode getting the attribute
  *	the superblock for allocations
- *	the agfs extents are allocated from
- *	the attribute btree * max depth
- *	the inode allocation btree
+ *	the agf extents are allocated from
  * Since attribute transaction space is dependent on the size of the attribute,
  * the calculation is done partially at mount time and partially at runtime(see
- * below).
+ * xfs_attr_calc_size()).
  */
 STATIC uint
 xfs_calc_attrsetm_reservation(
@@ -714,27 +713,7 @@ xfs_calc_attrsetm_reservation(
 {
 	return XFS_DQUOT_LOGRES(mp) +
 		xfs_calc_inode_res(mp, 1) +
-		xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
-		xfs_calc_buf_res(XFS_DA_NODE_MAXDEPTH, XFS_FSB_TO_B(mp, 1));
-}
-
-/*
- * Setting an attribute at runtime, transaction space unit per block.
- * 	the superblock for allocations: sector size
- *	the inode bmap btree could join or split: max depth * block size
- * Since the runtime attribute transaction space is dependent on the total
- * blocks needed for the 1st bmap, here we calculate out the space unit for
- * one block so that the caller could figure out the total space according
- * to the attibute extent length in blocks by:
- *	ext * M_RES(mp)->tr_attrsetrt.tr_logres
- */
-STATIC uint
-xfs_calc_attrsetrt_reservation(
-	struct xfs_mount	*mp)
-{
-	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
-		xfs_calc_buf_res(XFS_BM_MAXLEVELS(mp, XFS_ATTR_FORK),
-				 XFS_FSB_TO_B(mp, 1));
+		xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
 }
 
 /*
@@ -832,6 +811,27 @@ xfs_calc_sb_reservation(
 	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
 }
 
+uint
+xfs_calc_attr_res(
+	struct xfs_mount		*mp,
+	struct xfs_attr_set_resv	*resv)
+{
+	unsigned int		space_blks;
+	unsigned int		attr_res;
+
+	space_blks = xfs_allocfree_log_count(mp,
+			resv->total_dablks + resv->bmbt_blks);
+
+	attr_res = M_RES(mp)->tr_attrsetm.tr_logres +
+		xfs_calc_buf_res(resv->log_dablks,
+				mp->m_attr_geo->blksize) +
+		xfs_calc_buf_res(resv->bmbt_blks,
+				mp->m_sb.sb_blocksize) +
+		xfs_calc_buf_res(space_blks, mp->m_sb.sb_blocksize);
+
+	return attr_res;
+}
+
 void
 xfs_trans_resv_calc(
 	struct xfs_mount	*mp,
@@ -942,7 +942,7 @@ xfs_trans_resv_calc(
 	resp->tr_ichange.tr_logres = xfs_calc_ichange_reservation(mp);
 	resp->tr_fsyncts.tr_logres = xfs_calc_swrite_reservation(mp);
 	resp->tr_writeid.tr_logres = xfs_calc_writeid_reservation(mp);
-	resp->tr_attrsetrt.tr_logres = xfs_calc_attrsetrt_reservation(mp);
+	resp->tr_attrsetrt.tr_logres = 0;
 	resp->tr_clearagi.tr_logres = xfs_calc_clear_agi_bucket_reservation(mp);
 	resp->tr_growrtzero.tr_logres = xfs_calc_growrtzero_reservation(mp);
 	resp->tr_growrtfree.tr_logres = xfs_calc_growrtfree_reservation(mp);
diff --git a/fs/xfs/libxfs/xfs_trans_resv.h b/fs/xfs/libxfs/xfs_trans_resv.h
index 7241ab28cf84f..3a6a0bf21e9b1 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.h
+++ b/fs/xfs/libxfs/xfs_trans_resv.h
@@ -7,6 +7,7 @@
 #define	__XFS_TRANS_RESV_H__
 
 struct xfs_mount;
+struct xfs_attr_set_resv;
 
 /*
  * structure for maintaining pre-calculated transaction reservations.
@@ -91,6 +92,7 @@ struct xfs_trans_resv {
 #define	XFS_ATTRSET_LOG_COUNT		3
 #define	XFS_ATTRRM_LOG_COUNT		3
 
+uint xfs_calc_attr_res(struct xfs_mount *mp, struct xfs_attr_set_resv *resv);
 void xfs_trans_resv_calc(struct xfs_mount *mp, struct xfs_trans_resv *resp);
 uint xfs_allocfree_log_count(struct xfs_mount *mp, uint num_ops);
 
-- 
2.19.1

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

[Brian Foster]

On Wed, Jan 29, 2020 at 10:29:39AM +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 an incorrect value.
> 
> Apart from the above, xfs_log_calc_max_attrsetm_res() passes byte count as
> an argument to XFS_NEXTENTADD_SPACE_RES() instead of block count.
> 
> To fix these issues, this commit 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 be allocated.
> 3. The number of dabtree blocks that need to be allocated.
> 4. The number of bmbt blocks that need to be allocated.
> 5. The total number of blocks that need to be allocated.
> 
> xfs_attr_set() uses this information to compute 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 blocks to be logged which it uses
> to figure out the total number of bytes to be logged.
> 
> Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> ---
> Changelog:
> V1 -> V2:
> 1. Use convenience variables to reduce indentation of code.
> 
> V2 -> V3:
> 1. Introduce 'struct xfs_attr_set_resv' to be used an as out parameter
>    holding xattr reservation values.
> 2. Calculate number of bmbt blocks and total allocation blocks within
>    xfs_attr_calc_size().
> 
>  fs/xfs/libxfs/xfs_attr.c       | 93 +++++++++++++++++++---------------
>  fs/xfs/libxfs/xfs_attr.h       | 20 +++++++-
>  fs/xfs/libxfs/xfs_log_rlimit.c | 14 ++---
>  fs/xfs/libxfs/xfs_trans_resv.c | 52 +++++++++----------
>  fs/xfs/libxfs/xfs_trans_resv.h |  2 +
>  5 files changed, 107 insertions(+), 74 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 1eae1db74f6cd..1f3b001a1092e 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
...
> @@ -248,6 +211,53 @@ xfs_attr_try_sf_addname(
>  	return error ? error : error2;
>  }
>  
> +/*
> + * Calculate how many blocks we need for the new attribute,
> + */
> +void
> +xfs_attr_calc_size(
> +	struct xfs_mount		*mp,
> +	struct xfs_attr_set_resv	*resv,
> +	int				namelen,
> +	int				valuelen,
> +	int				*local)
> +{
> +	unsigned int		blksize;
> +	int			size;
> +
> +	blksize = mp->m_dir_geo->blksize;
> +
> +	/*
> +	 * 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);
> +
> +	resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> +	resv->log_dablks = 2 * resv->total_dablks;
> +

It looks like this changes the setxattr transaction reservation
calculation at the same time as refactoring how the reservation is
calculated, which makes it hard to even identify what is changing. Can
you split the general refactoring and calculation changes into
independent patches? E.g., refactor the existing calculation first and
then subsequently fix up the calculation..?

Brian

> +	if (*local) {
> +		if (size > (blksize / 2)) {
> +			/* Double split possible */
> +			resv->log_dablks += resv->total_dablks;
> +			resv->total_dablks *= 2;
> +		}
> +	} else {
> +		/*
> +		 * Out of line attribute, cannot double split, but
> +		 * make room for the attribute value itself.
> +		 */
> +		resv->rmt_blks = xfs_attr3_rmt_blocks(mp, valuelen);
> +	}
> +
> +	resv->bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp,
> +					resv->total_dablks + resv->rmt_blks,
> +					XFS_ATTR_FORK);
> +
> +	resv->alloc_blks = resv->total_dablks + resv->rmt_blks +
> +		resv->bmbt_blks;
> +}
> +
>  /*
>   * Set the attribute specified in @args.
>   */
> @@ -344,6 +354,7 @@ xfs_attr_set(
>  	int			flags)
>  {
>  	struct xfs_mount	*mp = dp->i_mount;
> +	struct xfs_attr_set_resv resv = { 0 };
>  	struct xfs_da_args	args;
>  	struct xfs_trans_res	tres;
>  	int			rsvd = (flags & ATTR_ROOT) != 0;
> @@ -361,7 +372,10 @@ 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, &resv, args.namelen, args.valuelen, &local);
> +
> +	args.total = resv.alloc_blks;
>  
>  	error = xfs_qm_dqattach(dp);
>  	if (error)
> @@ -380,8 +394,7 @@ xfs_attr_set(
>  			return error;
>  	}
>  
> -	tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
> -			 M_RES(mp)->tr_attrsetrt.tr_logres * args.total;
> +	tres.tr_logres = xfs_calc_attr_res(mp, &resv);
>  	tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
>  	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
>  
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index 94badfa1743e3..0b42faf7d6a1f 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -131,6 +131,22 @@ typedef struct xfs_attr_list_context {
>  	int				index;		/* index into output buffer */
>  } xfs_attr_list_context_t;
>  
> +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;
> +
> +	/* Number of blocks to allocate for the da btree. */
> +	unsigned int		total_dablks;
> +
> +	/* Blocks we might need to create all the new attr fork mappings. */
> +	unsigned int		bmbt_blks;
> +
> +	/* Total number of blocks we might have to allocate. */
> +	unsigned int		alloc_blks;
> +};
>  
>  /*========================================================================
>   * Function prototypes for the kernel.
> @@ -154,5 +170,7 @@ 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,
> +			struct xfs_attr_set_resv *resv,
> +			int namelen, int valuelen, int *local);
>  #endif	/* __XFS_ATTR_H__ */
> diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c b/fs/xfs/libxfs/xfs_log_rlimit.c
> index 7f55eb3f36536..26566c25c7e2c 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,16 @@ STATIC int
>  xfs_log_calc_max_attrsetm_res(
>  	struct xfs_mount	*mp)
>  {
> -	int			size;
> -	int			nblks;
> +	struct xfs_attr_set_resv resv = { 0 };
> +	int		size;
> +	int		local;
>  
>  	size = xfs_attr_leaf_entsize_local_max(mp->m_attr_geo->blksize) -
>  	       MAXNAMELEN - 1;
> -	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
> -	nblks += XFS_B_TO_FSB(mp, size);
> -	nblks += XFS_NEXTENTADD_SPACE_RES(mp, size, XFS_ATTR_FORK);
> +	xfs_attr_calc_size(mp, &resv, size, 0, &local);
> +	ASSERT(local == 1);
>  
> -	return  M_RES(mp)->tr_attrsetm.tr_logres +
> -		M_RES(mp)->tr_attrsetrt.tr_logres * nblks;
> +	return xfs_calc_attr_res(mp, &resv);
>  }
>  
>  /*
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 824073a839acb..867f1954c49bc 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -19,6 +19,7 @@
>  #include "xfs_trans.h"
>  #include "xfs_qm.h"
>  #include "xfs_trans_space.h"
> +#include "xfs_attr.h"
>  
>  #define _ALLOC	true
>  #define _FREE	false
> @@ -701,12 +702,10 @@ xfs_calc_attrinval_reservation(
>   * Setting an attribute at mount time.
>   *	the inode getting the attribute
>   *	the superblock for allocations
> - *	the agfs extents are allocated from
> - *	the attribute btree * max depth
> - *	the inode allocation btree
> + *	the agf extents are allocated from
>   * Since attribute transaction space is dependent on the size of the attribute,
>   * the calculation is done partially at mount time and partially at runtime(see
> - * below).
> + * xfs_attr_calc_size()).
>   */
>  STATIC uint
>  xfs_calc_attrsetm_reservation(
> @@ -714,27 +713,7 @@ xfs_calc_attrsetm_reservation(
>  {
>  	return XFS_DQUOT_LOGRES(mp) +
>  		xfs_calc_inode_res(mp, 1) +
> -		xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> -		xfs_calc_buf_res(XFS_DA_NODE_MAXDEPTH, XFS_FSB_TO_B(mp, 1));
> -}
> -
> -/*
> - * Setting an attribute at runtime, transaction space unit per block.
> - * 	the superblock for allocations: sector size
> - *	the inode bmap btree could join or split: max depth * block size
> - * Since the runtime attribute transaction space is dependent on the total
> - * blocks needed for the 1st bmap, here we calculate out the space unit for
> - * one block so that the caller could figure out the total space according
> - * to the attibute extent length in blocks by:
> - *	ext * M_RES(mp)->tr_attrsetrt.tr_logres
> - */
> -STATIC uint
> -xfs_calc_attrsetrt_reservation(
> -	struct xfs_mount	*mp)
> -{
> -	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> -		xfs_calc_buf_res(XFS_BM_MAXLEVELS(mp, XFS_ATTR_FORK),
> -				 XFS_FSB_TO_B(mp, 1));
> +		xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
>  }
>  
>  /*
> @@ -832,6 +811,27 @@ xfs_calc_sb_reservation(
>  	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
>  }
>  
> +uint
> +xfs_calc_attr_res(
> +	struct xfs_mount		*mp,
> +	struct xfs_attr_set_resv	*resv)
> +{
> +	unsigned int		space_blks;
> +	unsigned int		attr_res;
> +
> +	space_blks = xfs_allocfree_log_count(mp,
> +			resv->total_dablks + resv->bmbt_blks);
> +
> +	attr_res = M_RES(mp)->tr_attrsetm.tr_logres +
> +		xfs_calc_buf_res(resv->log_dablks,
> +				mp->m_attr_geo->blksize) +
> +		xfs_calc_buf_res(resv->bmbt_blks,
> +				mp->m_sb.sb_blocksize) +
> +		xfs_calc_buf_res(space_blks, mp->m_sb.sb_blocksize);
> +
> +	return attr_res;
> +}
> +
>  void
>  xfs_trans_resv_calc(
>  	struct xfs_mount	*mp,
> @@ -942,7 +942,7 @@ xfs_trans_resv_calc(
>  	resp->tr_ichange.tr_logres = xfs_calc_ichange_reservation(mp);
>  	resp->tr_fsyncts.tr_logres = xfs_calc_swrite_reservation(mp);
>  	resp->tr_writeid.tr_logres = xfs_calc_writeid_reservation(mp);
> -	resp->tr_attrsetrt.tr_logres = xfs_calc_attrsetrt_reservation(mp);
> +	resp->tr_attrsetrt.tr_logres = 0;
>  	resp->tr_clearagi.tr_logres = xfs_calc_clear_agi_bucket_reservation(mp);
>  	resp->tr_growrtzero.tr_logres = xfs_calc_growrtzero_reservation(mp);
>  	resp->tr_growrtfree.tr_logres = xfs_calc_growrtfree_reservation(mp);
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.h b/fs/xfs/libxfs/xfs_trans_resv.h
> index 7241ab28cf84f..3a6a0bf21e9b1 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.h
> +++ b/fs/xfs/libxfs/xfs_trans_resv.h
> @@ -7,6 +7,7 @@
>  #define	__XFS_TRANS_RESV_H__
>  
>  struct xfs_mount;
> +struct xfs_attr_set_resv;
>  
>  /*
>   * structure for maintaining pre-calculated transaction reservations.
> @@ -91,6 +92,7 @@ struct xfs_trans_resv {
>  #define	XFS_ATTRSET_LOG_COUNT		3
>  #define	XFS_ATTRRM_LOG_COUNT		3
>  
> +uint xfs_calc_attr_res(struct xfs_mount *mp, struct xfs_attr_set_resv *resv);
>  void xfs_trans_resv_calc(struct xfs_mount *mp, struct xfs_trans_resv *resp);
>  uint xfs_allocfree_log_count(struct xfs_mount *mp, uint num_ops);
>  
> -- 
> 2.19.1
> 

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

[Chandan Rajendra]

On Wednesday, February 12, 2020 8:43 PM Brian Foster wrote: 
> On Wed, Jan 29, 2020 at 10:29:39AM +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 an incorrect value.
> > 
> > Apart from the above, xfs_log_calc_max_attrsetm_res() passes byte count as
> > an argument to XFS_NEXTENTADD_SPACE_RES() instead of block count.
> > 
> > To fix these issues, this commit 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 be allocated.
> > 3. The number of dabtree blocks that need to be allocated.
> > 4. The number of bmbt blocks that need to be allocated.
> > 5. The total number of blocks that need to be allocated.
> > 
> > xfs_attr_set() uses this information to compute 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 blocks to be logged which it uses
> > to figure out the total number of bytes to be logged.
> > 
> > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > ---
> > Changelog:
> > V1 -> V2:
> > 1. Use convenience variables to reduce indentation of code.
> > 
> > V2 -> V3:
> > 1. Introduce 'struct xfs_attr_set_resv' to be used an as out parameter
> >    holding xattr reservation values.
> > 2. Calculate number of bmbt blocks and total allocation blocks within
> >    xfs_attr_calc_size().
> > 
> >  fs/xfs/libxfs/xfs_attr.c       | 93 +++++++++++++++++++---------------
> >  fs/xfs/libxfs/xfs_attr.h       | 20 +++++++-
> >  fs/xfs/libxfs/xfs_log_rlimit.c | 14 ++---
> >  fs/xfs/libxfs/xfs_trans_resv.c | 52 +++++++++----------
> >  fs/xfs/libxfs/xfs_trans_resv.h |  2 +
> >  5 files changed, 107 insertions(+), 74 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 1eae1db74f6cd..1f3b001a1092e 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> ...
> > @@ -248,6 +211,53 @@ xfs_attr_try_sf_addname(
> >  	return error ? error : error2;
> >  }
> >  
> > +/*
> > + * Calculate how many blocks we need for the new attribute,
> > + */
> > +void
> > +xfs_attr_calc_size(
> > +	struct xfs_mount		*mp,
> > +	struct xfs_attr_set_resv	*resv,
> > +	int				namelen,
> > +	int				valuelen,
> > +	int				*local)
> > +{
> > +	unsigned int		blksize;
> > +	int			size;
> > +
> > +	blksize = mp->m_dir_geo->blksize;
> > +
> > +	/*
> > +	 * 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);
> > +
> > +	resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> > +	resv->log_dablks = 2 * resv->total_dablks;
> > +
> 
> It looks like this changes the setxattr transaction reservation
> calculation at the same time as refactoring how the reservation is
> calculated, which makes it hard to even identify what is changing. Can
> you split the general refactoring and calculation changes into
> independent patches? E.g., refactor the existing calculation first and
> then subsequently fix up the calculation..?

Sure.

I will also rebase my patches on top of Christoph's "Clean attr
interface" patchset and post the next version.

-- 
chandan

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

[Darrick J. Wong]

On Wed, Jan 29, 2020 at 10:29:39AM +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 an incorrect value.
> 
> Apart from the above, xfs_log_calc_max_attrsetm_res() passes byte count as
> an argument to XFS_NEXTENTADD_SPACE_RES() instead of block count.
> 
> To fix these issues, this commit 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 be allocated.
> 3. The number of dabtree blocks that need to be allocated.
> 4. The number of bmbt blocks that need to be allocated.
> 5. The total number of blocks that need to be allocated.
> 
> xfs_attr_set() uses this information to compute 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 blocks to be logged which it uses
> to figure out the total number of bytes to be logged.
> 
> Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> ---
> Changelog:
> V1 -> V2:
> 1. Use convenience variables to reduce indentation of code.
> 
> V2 -> V3:
> 1. Introduce 'struct xfs_attr_set_resv' to be used an as out parameter
>    holding xattr reservation values.
> 2. Calculate number of bmbt blocks and total allocation blocks within
>    xfs_attr_calc_size().
> 
>  fs/xfs/libxfs/xfs_attr.c       | 93 +++++++++++++++++++---------------
>  fs/xfs/libxfs/xfs_attr.h       | 20 +++++++-
>  fs/xfs/libxfs/xfs_log_rlimit.c | 14 ++---
>  fs/xfs/libxfs/xfs_trans_resv.c | 52 +++++++++----------
>  fs/xfs/libxfs/xfs_trans_resv.h |  2 +
>  5 files changed, 107 insertions(+), 74 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 1eae1db74f6cd..1f3b001a1092e 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,53 @@ xfs_attr_try_sf_addname(
>  	return error ? error : error2;
>  }
>  
> +/*
> + * Calculate how many blocks we need for the new attribute,
> + */
> +void
> +xfs_attr_calc_size(
> +	struct xfs_mount		*mp,
> +	struct xfs_attr_set_resv	*resv,
> +	int				namelen,
> +	int				valuelen,
> +	int				*local)
> +{
> +	unsigned int		blksize;
> +	int			size;
> +
> +	blksize = mp->m_dir_geo->blksize;

This could be streamlined a bit:

	unsigned int			blksize = mp->m_attr_geo->blksize;
	int				size;

and indented to match the argument list.

Also please note that I changed m_dir_geo to m_attr_geo; this is the
attribute fork, not a directory.

> +	/*
> +	 * 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);
> +
> +	resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> +	resv->log_dablks = 2 * resv->total_dablks;
> +
> +	if (*local) {
> +		if (size > (blksize / 2)) {
> +			/* Double split possible */
> +			resv->log_dablks += resv->total_dablks;
> +			resv->total_dablks *= 2;
> +		}

I think this code block should set rmt_blks = 0 so that this function
always returns a fully initialized resv structure, and then you can skip
the "= { 0 };" stuff below.

> +	} else {
> +		/*
> +		 * Out of line attribute, cannot double split, but
> +		 * make room for the attribute value itself.
> +		 */
> +		resv->rmt_blks = xfs_attr3_rmt_blocks(mp, valuelen);
> +	}
> +
> +	resv->bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp,
> +					resv->total_dablks + resv->rmt_blks,
> +					XFS_ATTR_FORK);
> +
> +	resv->alloc_blks = resv->total_dablks + resv->rmt_blks +
> +		resv->bmbt_blks;

Please fix the nth-line indentation to be consistent with (most of) the
rest of xfs here:

	resv->bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp,
			resv->total_dablks + resv->rmt_blks,
			XFS_ATTR_FORK);

	resv->alloc_blks = resv->total_dablks + resv->rmt_blks +
			resv->bmbt_blks;


> +}
> +
>  /*
>   * Set the attribute specified in @args.
>   */
> @@ -344,6 +354,7 @@ xfs_attr_set(
>  	int			flags)
>  {
>  	struct xfs_mount	*mp = dp->i_mount;
> +	struct xfs_attr_set_resv resv = { 0 };
>  	struct xfs_da_args	args;
>  	struct xfs_trans_res	tres;
>  	int			rsvd = (flags & ATTR_ROOT) != 0;
> @@ -361,7 +372,10 @@ 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, &resv, args.namelen, args.valuelen, &local);
> +
> +	args.total = resv.alloc_blks;
>  
>  	error = xfs_qm_dqattach(dp);
>  	if (error)
> @@ -380,8 +394,7 @@ xfs_attr_set(
>  			return error;
>  	}
>  
> -	tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
> -			 M_RES(mp)->tr_attrsetrt.tr_logres * args.total;
> +	tres.tr_logres = xfs_calc_attr_res(mp, &resv);
>  	tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
>  	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
>  
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index 94badfa1743e3..0b42faf7d6a1f 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -131,6 +131,22 @@ typedef struct xfs_attr_list_context {
>  	int				index;		/* index into output buffer */
>  } xfs_attr_list_context_t;
>  
> +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;
> +
> +	/* Number of blocks to allocate for the da btree. */

This comment ought to read "Number of filesystem blocks..." so that
people (er... me) won't mistakenly think that total_dablks is in units
of da blocks.

Granted that might just be me overcomplicating things since da blocks ==
fs blocks for every attr tree ever.

> +	unsigned int		total_dablks;
> +
> +	/* Blocks we might need to create all the new attr fork mappings. */
> +	unsigned int		bmbt_blks;
> +
> +	/* Total number of blocks we might have to allocate. */
> +	unsigned int		alloc_blks;
> +};
>  
>  /*========================================================================
>   * Function prototypes for the kernel.
> @@ -154,5 +170,7 @@ 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,
> +			struct xfs_attr_set_resv *resv,
> +			int namelen, int valuelen, int *local);
>  #endif	/* __XFS_ATTR_H__ */
> diff --git a/fs/xfs/libxfs/xfs_log_rlimit.c b/fs/xfs/libxfs/xfs_log_rlimit.c
> index 7f55eb3f36536..26566c25c7e2c 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,16 @@ STATIC int
>  xfs_log_calc_max_attrsetm_res(
>  	struct xfs_mount	*mp)
>  {
> -	int			size;
> -	int			nblks;
> +	struct xfs_attr_set_resv resv = { 0 };
> +	int		size;
> +	int		local;
>  
>  	size = xfs_attr_leaf_entsize_local_max(mp->m_attr_geo->blksize) -
>  	       MAXNAMELEN - 1;
> -	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
> -	nblks += XFS_B_TO_FSB(mp, size);
> -	nblks += XFS_NEXTENTADD_SPACE_RES(mp, size, XFS_ATTR_FORK);
> +	xfs_attr_calc_size(mp, &resv, size, 0, &local);
> +	ASSERT(local == 1);
>  
> -	return  M_RES(mp)->tr_attrsetm.tr_logres +
> -		M_RES(mp)->tr_attrsetrt.tr_logres * nblks;
> +	return xfs_calc_attr_res(mp, &resv);
>  }
>  
>  /*
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 824073a839acb..867f1954c49bc 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -19,6 +19,7 @@
>  #include "xfs_trans.h"
>  #include "xfs_qm.h"
>  #include "xfs_trans_space.h"
> +#include "xfs_attr.h"
>  
>  #define _ALLOC	true
>  #define _FREE	false
> @@ -701,12 +702,10 @@ xfs_calc_attrinval_reservation(
>   * Setting an attribute at mount time.
>   *	the inode getting the attribute
>   *	the superblock for allocations
> - *	the agfs extents are allocated from
> - *	the attribute btree * max depth
> - *	the inode allocation btree
> + *	the agf extents are allocated from
>   * Since attribute transaction space is dependent on the size of the attribute,
>   * the calculation is done partially at mount time and partially at runtime(see
> - * below).
> + * xfs_attr_calc_size()).
>   */
>  STATIC uint
>  xfs_calc_attrsetm_reservation(
> @@ -714,27 +713,7 @@ xfs_calc_attrsetm_reservation(
>  {
>  	return XFS_DQUOT_LOGRES(mp) +
>  		xfs_calc_inode_res(mp, 1) +
> -		xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> -		xfs_calc_buf_res(XFS_DA_NODE_MAXDEPTH, XFS_FSB_TO_B(mp, 1));
> -}
> -
> -/*
> - * Setting an attribute at runtime, transaction space unit per block.
> - * 	the superblock for allocations: sector size
> - *	the inode bmap btree could join or split: max depth * block size
> - * Since the runtime attribute transaction space is dependent on the total
> - * blocks needed for the 1st bmap, here we calculate out the space unit for
> - * one block so that the caller could figure out the total space according
> - * to the attibute extent length in blocks by:
> - *	ext * M_RES(mp)->tr_attrsetrt.tr_logres
> - */
> -STATIC uint
> -xfs_calc_attrsetrt_reservation(
> -	struct xfs_mount	*mp)
> -{
> -	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> -		xfs_calc_buf_res(XFS_BM_MAXLEVELS(mp, XFS_ATTR_FORK),
> -				 XFS_FSB_TO_B(mp, 1));
> +		xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
>  }
>  
>  /*
> @@ -832,6 +811,27 @@ xfs_calc_sb_reservation(
>  	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
>  }
>  
> +uint
> +xfs_calc_attr_res(
> +	struct xfs_mount		*mp,
> +	struct xfs_attr_set_resv	*resv)
> +{
> +	unsigned int		space_blks;
> +	unsigned int		attr_res;

Same complaint from above about the names not lining up here...

> +
> +	space_blks = xfs_allocfree_log_count(mp,
> +			resv->total_dablks + resv->bmbt_blks);
> +
> +	attr_res = M_RES(mp)->tr_attrsetm.tr_logres +
> +		xfs_calc_buf_res(resv->log_dablks,
> +				mp->m_attr_geo->blksize) +
> +		xfs_calc_buf_res(resv->bmbt_blks,
> +				mp->m_sb.sb_blocksize) +
> +		xfs_calc_buf_res(space_blks, mp->m_sb.sb_blocksize);

Each of the xfs_calc_buf_res() calls will fit on a single line, right?

--D

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

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

[Brian Foster]

On Wed, Jan 29, 2020 at 10:29:38AM +0530, Chandan Rajendra wrote:
> This commit changes xfs_attr_leaf_newentsize() to explicitly accept name and
> value length instead of a pointer to struct xfs_da_args. 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>
> ---
>  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_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 08d4b10ae2d53..7cd57e5844d80 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
...
> @@ -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,

Any reason not to just pass the geo here rather than the mount?
Otherwise looks fine to me.

Brian

> +	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 f4a188e28b7b6..0ce1f9301157e 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
> 

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

[Chandan Rajendra]

On Wednesday, February 12, 2020 8:41 PM Brian Foster wrote: 
> On Wed, Jan 29, 2020 at 10:29:38AM +0530, Chandan Rajendra wrote:
> > This commit changes xfs_attr_leaf_newentsize() to explicitly accept name and
> > value length instead of a pointer to struct xfs_da_args. 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>
> > ---
> >  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_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> > index 08d4b10ae2d53..7cd57e5844d80 100644
> > --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> ...
> > @@ -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,
> 
> Any reason not to just pass the geo here rather than the mount?
> Otherwise looks fine to me.

Sorry, I noticed this message now.

You are right. I will change the first argument to be attr geometry pointer.

-- 
chandan

[PATCH V3 1/2] xfsprogs: 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>
---
 libxfs/xfs_attr.c      |  3 ++-
 libxfs/xfs_attr_leaf.c | 42 ++++++++++++++++++++++++++++--------------
 libxfs/xfs_attr_leaf.h |  3 ++-
 3 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/libxfs/xfs_attr.c b/libxfs/xfs_attr.c
index ada1b5f4..2a0050f4 100644
--- a/libxfs/xfs_attr.c
+++ b/libxfs/xfs_attr.c
@@ -198,7 +198,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/libxfs/xfs_attr_leaf.c b/libxfs/xfs_attr_leaf.c
index 0249a0a9..d161216d 100644
--- a/libxfs/xfs_attr_leaf.c
+++ b/libxfs/xfs_attr_leaf.c
@@ -1272,8 +1272,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.
 	 * (may need to figure in size of entry struct too)
@@ -1345,6 +1345,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;
 
@@ -1374,11 +1375,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);
@@ -1763,6 +1767,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;
@@ -1772,14 +1778,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);
@@ -1790,8 +1798,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);
@@ -1826,8 +1837,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;
@@ -2613,20 +2625,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/libxfs/xfs_attr_leaf.h b/libxfs/xfs_attr_leaf.h
index 7b74e18b..7334d43a 100644
--- a/libxfs/xfs_attr_leaf.h
+++ b/libxfs/xfs_attr_leaf.h
@@ -83,7 +83,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, xfs_daddr_t mappedbno,
 			struct xfs_buf **bpp);
-- 
2.19.1

[PATCH V3 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 an incorrect value.

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

To fix these issues, this commit 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 be allocated.
3. The number of dabtree blocks that need to be allocated.
4. The number of bmbt blocks that need to be allocated.
5. The total number of blocks that need to be allocated.

xfs_attr_set() uses this information to compute 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 blocks to be logged which it uses
to figure out the total number of bytes to be logged.

Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
---
Changelog:
V1 -> V2:
1. Use convenience variables to reduce indentation of code.

V2 -> V3:
1. Introduce 'struct xfs_attr_set_resv' to be used an as out parameter
   holding xattr reservation values.
2. Calculate number of bmbt blocks and total allocation blocks within
   xfs_attr_calc_size().
   
 libxfs/xfs_attr.c       | 93 +++++++++++++++++++++++------------------
 libxfs/xfs_attr.h       | 20 ++++++++-
 libxfs/xfs_log_rlimit.c | 14 +++----
 libxfs/xfs_trans_resv.c | 52 +++++++++++------------
 libxfs/xfs_trans_resv.h |  2 +
 5 files changed, 107 insertions(+), 74 deletions(-)

diff --git a/libxfs/xfs_attr.c b/libxfs/xfs_attr.c
index 2a0050f4..60e1ac2e 100644
--- a/libxfs/xfs_attr.c
+++ b/libxfs/xfs_attr.c
@@ -182,43 +182,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,
@@ -247,6 +210,53 @@ xfs_attr_try_sf_addname(
 	return error ? error : error2;
 }
 
+/*
+ * Calculate how many blocks we need for the new attribute,
+ */
+void
+xfs_attr_calc_size(
+	struct xfs_mount		*mp,
+	struct xfs_attr_set_resv	*resv,
+	int				namelen,
+	int				valuelen,
+	int				*local)
+{
+	unsigned int		blksize;
+	int			size;
+
+	blksize = mp->m_dir_geo->blksize;
+
+	/*
+	 * 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);
+
+	resv->total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
+	resv->log_dablks = 2 * resv->total_dablks;
+
+	if (*local) {
+		if (size > (blksize / 2)) {
+			/* Double split possible */
+			resv->log_dablks += resv->total_dablks;
+			resv->total_dablks *= 2;
+		}
+	} else {
+		/*
+		 * Out of line attribute, cannot double split, but
+		 * make room for the attribute value itself.
+		 */
+		resv->rmt_blks = xfs_attr3_rmt_blocks(mp, valuelen);
+	}
+
+	resv->bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp,
+					resv->total_dablks + resv->rmt_blks,
+					XFS_ATTR_FORK);
+
+	resv->alloc_blks = resv->total_dablks + resv->rmt_blks +
+		resv->bmbt_blks;
+}
+
 /*
  * Set the attribute specified in @args.
  */
@@ -343,6 +353,7 @@ xfs_attr_set(
 	int			flags)
 {
 	struct xfs_mount	*mp = dp->i_mount;
+	struct xfs_attr_set_resv resv = { 0 };
 	struct xfs_da_args	args;
 	struct xfs_trans_res	tres;
 	int			rsvd = (flags & ATTR_ROOT) != 0;
@@ -360,7 +371,10 @@ 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, &resv, args.namelen, args.valuelen, &local);
+
+	args.total = resv.alloc_blks;
 
 	error = xfs_qm_dqattach(dp);
 	if (error)
@@ -379,8 +393,7 @@ xfs_attr_set(
 			return error;
 	}
 
-	tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
-			 M_RES(mp)->tr_attrsetrt.tr_logres * args.total;
+	tres.tr_logres = xfs_calc_attr_res(mp, &resv);
 	tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
 	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
 
diff --git a/libxfs/xfs_attr.h b/libxfs/xfs_attr.h
index 94badfa1..0b42faf7 100644
--- a/libxfs/xfs_attr.h
+++ b/libxfs/xfs_attr.h
@@ -131,6 +131,22 @@ typedef struct xfs_attr_list_context {
 	int				index;		/* index into output buffer */
 } xfs_attr_list_context_t;
 
+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;
+
+	/* Number of blocks to allocate for the da btree. */
+	unsigned int		total_dablks;
+
+	/* Blocks we might need to create all the new attr fork mappings. */
+	unsigned int		bmbt_blks;
+
+	/* Total number of blocks we might have to allocate. */
+	unsigned int		alloc_blks;
+};
 
 /*========================================================================
  * Function prototypes for the kernel.
@@ -154,5 +170,7 @@ 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,
+			struct xfs_attr_set_resv *resv,
+			int namelen, int valuelen, int *local);
 #endif	/* __XFS_ATTR_H__ */
diff --git a/libxfs/xfs_log_rlimit.c b/libxfs/xfs_log_rlimit.c
index c8398b7d..81188ea0 100644
--- a/libxfs/xfs_log_rlimit.c
+++ b/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,16 @@ STATIC int
 xfs_log_calc_max_attrsetm_res(
 	struct xfs_mount	*mp)
 {
-	int			size;
-	int			nblks;
+	struct xfs_attr_set_resv resv = { 0 };
+	int		size;
+	int		local;
 
 	size = xfs_attr_leaf_entsize_local_max(mp->m_attr_geo->blksize) -
 	       MAXNAMELEN - 1;
-	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
-	nblks += XFS_B_TO_FSB(mp, size);
-	nblks += XFS_NEXTENTADD_SPACE_RES(mp, size, XFS_ATTR_FORK);
+	xfs_attr_calc_size(mp, &resv, size, 0, &local);
+	ASSERT(local == 1);
 
-	return  M_RES(mp)->tr_attrsetm.tr_logres +
-		M_RES(mp)->tr_attrsetrt.tr_logres * nblks;
+	return xfs_calc_attr_res(mp, &resv);
 }
 
 /*
diff --git a/libxfs/xfs_trans_resv.c b/libxfs/xfs_trans_resv.c
index 270e92a3..925691fe 100644
--- a/libxfs/xfs_trans_resv.c
+++ b/libxfs/xfs_trans_resv.c
@@ -17,6 +17,7 @@
 #include "xfs_trans.h"
 #include "xfs_trans_space.h"
 #include "xfs_quota_defs.h"
+#include "xfs_attr.h"
 
 #define _ALLOC	true
 #define _FREE	false
@@ -641,12 +642,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(
@@ -654,27 +653,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);
 }
 
 /*
@@ -772,6 +751,27 @@ xfs_calc_sb_reservation(
 	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
 }
 
+uint
+xfs_calc_attr_res(
+	struct xfs_mount		*mp,
+	struct xfs_attr_set_resv	*resv)
+{
+	unsigned int		space_blks;
+	unsigned int		attr_res;
+
+	space_blks = xfs_allocfree_log_count(mp,
+			resv->total_dablks + resv->bmbt_blks);
+
+	attr_res = M_RES(mp)->tr_attrsetm.tr_logres +
+		xfs_calc_buf_res(resv->log_dablks,
+				mp->m_attr_geo->blksize) +
+		xfs_calc_buf_res(resv->bmbt_blks,
+				mp->m_sb.sb_blocksize) +
+		xfs_calc_buf_res(space_blks, mp->m_sb.sb_blocksize);
+
+	return attr_res;
+}
+
 void
 xfs_trans_resv_calc(
 	struct xfs_mount	*mp,
@@ -882,7 +882,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/libxfs/xfs_trans_resv.h b/libxfs/xfs_trans_resv.h
index 7241ab28..3a6a0bf2 100644
--- a/libxfs/xfs_trans_resv.h
+++ b/libxfs/xfs_trans_resv.h
@@ -7,6 +7,7 @@
 #define	__XFS_TRANS_RESV_H__
 
 struct xfs_mount;
+struct xfs_attr_set_resv;
 
 /*
  * structure for maintaining pre-calculated transaction reservations.
@@ -91,6 +92,7 @@ struct xfs_trans_resv {
 #define	XFS_ATTRSET_LOG_COUNT		3
 #define	XFS_ATTRRM_LOG_COUNT		3
 
+uint xfs_calc_attr_res(struct xfs_mount *mp, struct xfs_attr_set_resv *resv);
 void xfs_trans_resv_calc(struct xfs_mount *mp, struct xfs_trans_resv *resp);
 uint xfs_allocfree_log_count(struct xfs_mount *mp, uint num_ops);
 
-- 
2.19.1