[3/7] xfs: streamline xfs_attr3_leaf_inactive

Commit Message

Darrick J. Wong Jan. 15, 2020, 5:03 p.m. UTC

From: Darrick J. Wong <[e-mail]>

Now that we know we don't have to take a transaction to stale the incore
buffers for a remote value, get rid of the unnecessary memory allocation
in the leaf walker and call the rmt_stale function directly.  Flatten
the loop while we're at it.

Signed-off-by: Darrick J. Wong <[e-mail]>
---
 fs/xfs/libxfs/xfs_attr_leaf.h |    9 ----
 fs/xfs/xfs_attr_inactive.c    |   83 ++++++++++-------------------------------
 2 files changed, 21 insertions(+), 71 deletions(-)

Comments

Christoph Hellwig Jan. 15, 2020, 7:14 p.m. UTC | #1

> -	xfs_dablk_t		blkno,
> -	int			blkcnt)
> +	xfs_dablk_t		tblkno,
> +	int			tblkcnt)

Nit: I would keep the names without the t, presuming it means temporary.
In fact the hunks touching this function seems to be unrelated to the
rest of the patch.

> +	struct xfs_mount	*mp = bp->b_mount;
> +	struct xfs_attr_leafblock *leaf;
>  	struct xfs_attr_leaf_entry *entry;
>  	struct xfs_attr_leaf_name_remote *name_rmt;
> -	struct xfs_attr_inactive_list *list;
> -	struct xfs_attr_inactive_list *lp;
>  	int			error;
> -	int			count;
> -	int			size;
> -	int			tmp;
>  	int			i;
> -	struct xfs_mount	*mp = bp->b_mount;
>  
>  	leaf = bp->b_addr;

Maybe move this to the declaration line when you touch that area anyway?

>  	entry = xfs_attr3_leaf_entryp(leaf);
>  	for (i = 0; i < ichdr.count; entry++, i++) {
> +		int		blkcnt;
>  
> +		if (!be16_to_cpu(entry->nameidx) ||

While we touch this: the be16_to_cpu is superflous, given that the value
is used in boolean context.

Otherwise this looks good to me.

Patch

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.h b/fs/xfs/libxfs/xfs_attr_leaf.h
index f4a188e28b7b..73615b1dd1a8 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.h
+++ b/fs/xfs/libxfs/xfs_attr_leaf.h
@@ -39,15 +39,6 @@  struct xfs_attr3_icleaf_hdr {
 	} freemap[XFS_ATTR_LEAF_MAPSIZE];
 };
 
-/*
- * Used to keep a list of "remote value" extents when unlinking an inode.
- */
-typedef struct xfs_attr_inactive_list {
-	xfs_dablk_t	valueblk;	/* block number of value bytes */
-	int		valuelen;	/* number of bytes in value */
-} xfs_attr_inactive_list_t;
-
-
 /*========================================================================
  * Function prototypes for the kernel.
  *========================================================================*/
diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
index edb079087a0c..27cb6bf614c5 100644
--- a/fs/xfs/xfs_attr_inactive.c
+++ b/fs/xfs/xfs_attr_inactive.c
@@ -33,12 +33,10 @@ 
 STATIC int
 xfs_attr3_rmt_stale(
 	struct xfs_inode	*dp,
-	xfs_dablk_t		blkno,
-	int			blkcnt)
+	xfs_dablk_t		tblkno,
+	int			tblkcnt)
 {
 	struct xfs_bmbt_irec	map;
-	xfs_dablk_t		tblkno;
-	int			tblkcnt;
 	int			nmap;
 	int			error;
 
@@ -46,8 +44,6 @@  xfs_attr3_rmt_stale(
 	 * Roll through the "value", invalidating the attribute value's
 	 * blocks.
 	 */
-	tblkno = blkno;
-	tblkcnt = blkcnt;
 	while (tblkcnt > 0) {
 		/*
 		 * Try to remember where we decided to put the value.
@@ -88,80 +84,43 @@  xfs_attr3_leaf_inactive(
 	struct xfs_inode	*dp,
 	struct xfs_buf		*bp)
 {
-	struct xfs_attr_leafblock *leaf;
 	struct xfs_attr3_icleaf_hdr ichdr;
+	struct xfs_mount	*mp = bp->b_mount;
+	struct xfs_attr_leafblock *leaf;
 	struct xfs_attr_leaf_entry *entry;
 	struct xfs_attr_leaf_name_remote *name_rmt;
-	struct xfs_attr_inactive_list *list;
-	struct xfs_attr_inactive_list *lp;
 	int			error;
-	int			count;
-	int			size;
-	int			tmp;
 	int			i;
-	struct xfs_mount	*mp = bp->b_mount;
 
 	leaf = bp->b_addr;
 	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
 
 	/*
-	 * Count the number of "remote" value extents.
+	 * Find the remote value extents for this leaf and invalidate their
+	 * incore buffers.
 	 */
-	count = 0;
 	entry = xfs_attr3_leaf_entryp(leaf);
 	for (i = 0; i < ichdr.count; entry++, i++) {
-		if (be16_to_cpu(entry->nameidx) &&
-		    ((entry->flags & XFS_ATTR_LOCAL) == 0)) {
-			name_rmt = xfs_attr3_leaf_name_remote(leaf, i);
-			if (name_rmt->valueblk)
-				count++;
-		}
-	}
+		int		blkcnt;
 
-	/*
-	 * If there are no "remote" values, we're done.
-	 */
-	if (count == 0) {
-		xfs_trans_brelse(*trans, bp);
-		return 0;
-	}
+		if (!be16_to_cpu(entry->nameidx) ||
+		    (entry->flags & XFS_ATTR_LOCAL))
+			continue;
 
-	/*
-	 * Allocate storage for a list of all the "remote" value extents.
-	 */
-	size = count * sizeof(xfs_attr_inactive_list_t);
-	list = kmem_alloc(size, 0);
+		name_rmt = xfs_attr3_leaf_name_remote(leaf, i);
+		if (!name_rmt->valueblk)
+			continue;
 
-	/*
-	 * Identify each of the "remote" value extents.
-	 */
-	lp = list;
-	entry = xfs_attr3_leaf_entryp(leaf);
-	for (i = 0; i < ichdr.count; entry++, i++) {
-		if (be16_to_cpu(entry->nameidx) &&
-		    ((entry->flags & XFS_ATTR_LOCAL) == 0)) {
-			name_rmt = xfs_attr3_leaf_name_remote(leaf, i);
-			if (name_rmt->valueblk) {
-				lp->valueblk = be32_to_cpu(name_rmt->valueblk);
-				lp->valuelen = xfs_attr3_rmt_blocks(dp->i_mount,
-						    be32_to_cpu(name_rmt->valuelen));
-				lp++;
-			}
-		}
-	}
-	xfs_trans_brelse(*trans, bp);	/* unlock for trans. in freextent() */
-
-	/*
-	 * Invalidate each of the "remote" value extents.
-	 */
-	error = 0;
-	for (lp = list, i = 0; i < count; i++, lp++) {
-		tmp = xfs_attr3_rmt_stale(dp, lp->valueblk, lp->valuelen);
-		if (error == 0)
-			error = tmp;	/* save only the 1st errno */
+		blkcnt = xfs_attr3_rmt_blocks(dp->i_mount,
+				be32_to_cpu(name_rmt->valuelen));
+		error = xfs_attr3_rmt_stale(dp,
+				be32_to_cpu(name_rmt->valueblk), blkcnt);
+		if (error)
+			goto err;
 	}
 
-	kmem_free(list);
+	xfs_trans_brelse(*trans, bp);
+err:
 	return error;
 }