xfs: merge xfs_bmap_free_item and xfs_extent_busy

Merge the two structures to track a freed extent into a single one, to simply tracking the flow in the extent free code and reduce the amount of required memory allocations.

Signed-off-by Christoph Hellwig <[email protected]>

fs/xfs/xfs_alloc.c +48 -28

index c1cf6a3..656991d 100644
@@ -2579,37 +2579,41 @@ error0:
 	return error;
 }
 
-/*
- * Free an extent.
- * Just break up the extent address and hand off to xfs_free_ag_extent
- * after fixing up the freelist.
- */
-int				/* error */
-xfs_free_extent(
-	xfs_trans_t	*tp,	/* transaction pointer */
-	xfs_fsblock_t	bno,	/* starting block number of extent */
-	xfs_extlen_t	len)	/* length of extent */
+struct xfs_freed_extent *
+xfs_freed_extent_alloc(
+	xfs_agnumber_t		agno,
+	xfs_agblock_t		bno,
+	xfs_extlen_t		len,
+	unsigned int		flags)
+{
+	struct xfs_freed_extent	*new;
+
+	new = kmem_zone_zalloc(xfs_freed_extent_zone, KM_SLEEP);
+	if (!new)
+		return NULL;
+
+	new->agno = agno;
+	new->bno = bno;
+	new->length = len;
+	INIT_LIST_HEAD(&new->list);
+	new->flags = flags;
+	return new;
+}
+
+int
+__xfs_free_extent(
+	struct xfs_trans	*tp,
+	struct xfs_freed_extent	*free)
 {
 	xfs_alloc_arg_t	args;
 	int		error;
 
-	ASSERT(len != 0);
+	ASSERT(free->length != 0);
 	memset(&args, 0, sizeof(xfs_alloc_arg_t));
 	args.tp = tp;
 	args.mp = tp->t_mountp;
-
-	/*
-	 * validate that the block number is legal - the enables us to detect
-	 * and handle a silent filesystem corruption rather than crashing.
-	 */
-	args.agno = XFS_FSB_TO_AGNO(args.mp, bno);
-	if (args.agno >= args.mp->m_sb.sb_agcount)
-		return EFSCORRUPTED;
-
-	args.agbno = XFS_FSB_TO_AGBNO(args.mp, bno);
-	if (args.agbno >= args.mp->m_sb.sb_agblocks)
-		return EFSCORRUPTED;
-
+	args.agno = free->agno;
+	args.agbno = free->bno;
 	args.pag = xfs_perag_get(args.mp, args.agno);
 	ASSERT(args.pag);
 

@@ -2618,16 +2622,45 @@ xfs_free_extent(
 		goto error0;
 
 	/* validate the extent size is legal now we have the agf locked */
-	if (args.agbno + len >
+	if (args.agbno + free->length >
 			be32_to_cpu(XFS_BUF_TO_AGF(args.agbp)->agf_length)) {
 		error = EFSCORRUPTED;
 		goto error0;
 	}
 
-	error = xfs_free_ag_extent(tp, args.agbp, args.agno, args.agbno, len, 0);
-	if (!error)
-		xfs_extent_busy_insert(tp, args.agno, args.agbno, len, 0);
+	error = xfs_free_ag_extent(tp, args.agbp, args.agno, args.agbno,
+				   free->length, 0);
 error0:
 	xfs_perag_put(args.pag);
 	return error;
 }
+
+int
+xfs_free_extent(
+	struct xfs_trans	*tp,
+	xfs_fsblock_t		bno,
+	xfs_extlen_t		len)
+{
+	struct xfs_mount	*mp = tp->t_mountp;
+	xfs_agnumber_t		agno = XFS_FSB_TO_AGNO(mp, bno);
+	xfs_agblock_t		agbno = XFS_FSB_TO_AGBNO(mp, bno);
+	struct xfs_freed_extent	*free;
+	int			error;
+
+	/*
+	 * validate that the block number is legal - the enables us to detect
+	 * and handle a silent filesystem corruption rather than crashing.
+	 */

fs/xfs/xfs_alloc.c +3 -0

+	}
+	return error;
+}

fs/xfs/xfs_alloc.h +0 -0

index feacb06..4aa7f8c 100644

fs/xfs/xfs_alloc.h +20 -0

+	xfs_extlen_t	length;
+	unsigned int	flags;
+#define XFS_EXTENT_DISCARDED	0x01	/* undergoing a discard op. */
+#define XFS_EXTENT_SKIP_DISCARD	0x02	/* do not discard */
+};
+
 /*
  * Defines for userdata
  */

@@ -210,6 +221,11 @@ xfs_free_extent(
 	xfs_fsblock_t	bno,	/* starting block number of extent */
 	xfs_extlen_t	len);	/* length of extent */
 
+int
+__xfs_free_extent(
+	struct xfs_trans	*tp,
+	struct xfs_freed_extent	*free);
+
 int					/* error */
 xfs_alloc_lookup_le(
 	struct xfs_btree_cur	*cur,	/* btree cursor */

@@ -231,4 +247,13 @@ xfs_alloc_get_rec(
 	xfs_extlen_t		*len,	/* output: length of extent */
 	int			*stat);	/* output: success/failure */
 
+struct xfs_freed_extent *
+xfs_freed_extent_alloc(
+	xfs_agnumber_t		agno,
+	xfs_agblock_t		bno,
+	xfs_extlen_t		len,
+	unsigned int		flags);
+
+extern kmem_zone_t	*xfs_freed_extent_zone;
+
 #endif	/* __XFS_ALLOC_H__ */

fs/xfs/xfs_alloc_btree.c +2 -0

index cc1eadc..ce3041a 100644
@@ -107,21 +107,24 @@ xfs_allocbt_free_block(
 	struct xfs_btree_cur	*cur,
 	struct xfs_buf		*bp)
 {
+	struct xfs_trans	*tp = cur->bc_tp;
 	struct xfs_buf		*agbp = cur->bc_private.a.agbp;
 	struct xfs_agf		*agf = XFS_BUF_TO_AGF(agbp);
+	struct xfs_freed_extent	*free;
 	xfs_agblock_t		bno;
 	int			error;

fs/xfs/xfs_alloc_btree.c +5 -1

+	free = xfs_freed_extent_alloc(be32_to_cpu(agf->agf_seqno), bno, 1,
+			      XFS_EXTENT_SKIP_DISCARD);
+	xfs_extent_busy_insert(tp, free);
+	list_add(&free->list, &tp->t_busy);
 
-	xfs_trans_binval(cur->bc_tp, bp);
+	xfs_trans_binval(tp, bp);
 	return 0;
 }
 

fs/xfs/xfs_bmap.c +1 -1

index 5b6092e..9c2c00c 100644
@@ -50,7 +50,7 @@
 #include "xfs_filestream.h"
 
 
-kmem_zone_t		*xfs_bmap_free_item_zone;
+kmem_zone_t		*xfs_freed_extent_zone;
 
 /*
  * Miscellaneous helper functions

fs/xfs/xfs_bmap.c +12 -50

@@ -602,58 +598,22 @@ xfs_bmap_add_free(
 	xfs_bmap_free_t		*flist,		/* list of extents */
 	xfs_mount_t		*mp)		/* mount point structure */
 {
-	xfs_bmap_free_item_t	*cur;		/* current (next) element */
-	xfs_bmap_free_item_t	*new;		/* new element */
-	xfs_bmap_free_item_t	*prev;		/* previous element */
-#ifdef DEBUG
-	xfs_agnumber_t		agno;
-	xfs_agblock_t		agbno;
+	xfs_agnumber_t		agno = XFS_FSB_TO_AGNO(mp, bno);
+	xfs_agblock_t		agbno = XFS_FSB_TO_AGBNO(mp, bno);
+	struct xfs_freed_extent	*new;
 
 	ASSERT(bno != NULLFSBLOCK);
 	ASSERT(len > 0);
 	ASSERT(len <= MAXEXTLEN);
 	ASSERT(!isnullstartblock(bno));
-	agno = XFS_FSB_TO_AGNO(mp, bno);
-	agbno = XFS_FSB_TO_AGBNO(mp, bno);
 	ASSERT(agno < mp->m_sb.sb_agcount);
 	ASSERT(agbno < mp->m_sb.sb_agblocks);
 	ASSERT(len < mp->m_sb.sb_agblocks);
 	ASSERT(agbno + len <= mp->m_sb.sb_agblocks);
-#endif
-	ASSERT(xfs_bmap_free_item_zone != NULL);
-	new = kmem_zone_alloc(xfs_bmap_free_item_zone, KM_SLEEP);
-	new->xbfi_startblock = bno;
-	new->xbfi_blockcount = (xfs_extlen_t)len;
-	for (prev = NULL, cur = flist->xbf_first;
-	     cur != NULL;
-	     prev = cur, cur = cur->xbfi_next) {
-		if (cur->xbfi_startblock >= bno)
-			break;
-	}
-	if (prev)
-		prev->xbfi_next = new;
-	else
-		flist->xbf_first = new;
-	new->xbfi_next = cur;
-	flist->xbf_count++;
-}
 
-/*
- * Remove the entry "free" from the free item list.  Prev points to the
- * previous entry, unless "free" is the head of the list.
- */
-void
-xfs_bmap_del_free(
-	xfs_bmap_free_t		*flist,	/* free item list header */
-	xfs_bmap_free_item_t	*prev,	/* previous item on list, if any */
-	xfs_bmap_free_item_t	*free)	/* list item to be freed */
-{
-	if (prev)
-		prev->xbfi_next = free->xbfi_next;
-	else
-		flist->xbf_first = free->xbfi_next;
-	flist->xbf_count--;
-	kmem_zone_free(xfs_bmap_free_item_zone, free);
+	new = xfs_freed_extent_alloc(agno, agbno, len, 0);
+	list_add_tail(&new->list, &flist->xbf_list);
+	flist->xbf_count++;
 }
 
 /*

@@ -663,16 +623,14 @@ void
 xfs_bmap_cancel(
 	xfs_bmap_free_t		*flist)	/* list of bmap_free_items */
 {
-	xfs_bmap_free_item_t	*free;	/* free list item */
-	xfs_bmap_free_item_t	*next;
+	struct xfs_freed_extent	*free, *n;
 
-	if (flist->xbf_count == 0)
-		return;
-	ASSERT(flist->xbf_first != NULL);
-	for (free = flist->xbf_first; free; free = next) {
-		next = free->xbfi_next;
-		xfs_bmap_del_free(flist, NULL, free);
+	list_for_each_entry_safe(free, n, &flist->xbf_list, list) {
+		list_del(&free->list);
+		flist->xbf_count--;
+		kmem_zone_free(xfs_freed_extent_zone, free);
 	}
+
 	ASSERT(flist->xbf_count == 0);
 }
 

fs/xfs/xfs_bmap.h +6 -18

index f84bd7a..73cedc4 100644
@@ -25,19 +25,6 @@ struct xfs_inode;
 struct xfs_mount;
 struct xfs_trans;
 
-extern kmem_zone_t	*xfs_bmap_free_item_zone;
-
-/*
- * List of extents to be free "later".
- * The list is kept sorted on xbf_startblock.
- */
-typedef struct xfs_bmap_free_item
-{
-	xfs_fsblock_t		xbfi_startblock;/* starting fs block number */
-	xfs_extlen_t		xbfi_blockcount;/* number of blocks in extent */
-	struct xfs_bmap_free_item *xbfi_next;	/* link to next entry */
-} xfs_bmap_free_item_t;
-
 /*
  * Header for free extent list.
  *

@@ -52,9 +39,8 @@ typedef struct xfs_bmap_free_item
  * transaction reservations have been made then this algorithm will eventually
  * find all the space it needs.
  */
-typedef	struct xfs_bmap_free
-{
-	xfs_bmap_free_item_t	*xbf_first;	/* list of to-be-free extents */
+typedef	struct xfs_bmap_free {
+	struct list_head	xbf_list;
 	int			xbf_count;	/* count of items on list */
 	int			xbf_low;	/* alloc in low mode */
 } xfs_bmap_free_t;

@@ -103,8 +89,10 @@ static inline int xfs_bmapi_aflag(int w)
 
 static inline void xfs_bmap_init(xfs_bmap_free_t *flp, xfs_fsblock_t *fbp)
 {
-	((flp)->xbf_first = NULL, (flp)->xbf_count = 0, \
-		(flp)->xbf_low = 0, *(fbp) = NULLFSBLOCK);
+	INIT_LIST_HEAD(&flp->xbf_list);
+	flp->xbf_count = 0;
+	flp->xbf_low = 0;
+	*fbp = NULLFSBLOCK;
 }
 
 /*

fs/xfs/xfs_bmap_util.c +3 -0

index 01f6a64..ade325f 100644
@@ -59,6 +59,22 @@ xfs_fsb_to_db(struct xfs_inode *ip, xfs_fsblock_t fsb)
 		 XFS_FSB_TO_DADDR((ip)->i_mount, (fsb)));
 }
 
+STATIC int
+xfs_freed_extent_cmp(
+	void			*priv,

fs/xfs/xfs_bmap_util.c +13 -7

+	return a->agno - b->agno;
+}
+
 /*
  * Routine to be called at transaction's end by xfs_bmapi, xfs_bunmapi
  * caller.  Frees all the extents that need freeing, which must be done

@@ -74,13 +90,12 @@ xfs_bmap_finish(
 	xfs_bmap_free_t		*flist,		/* i/o: list extents to free */
 	int			*committed)	/* xact committed or not */
 {
+	struct xfs_mount	*mp = (*tp)->t_mountp;
+	struct xfs_freed_extent	*free;
 	xfs_efd_log_item_t	*efd;		/* extent free data */
 	xfs_efi_log_item_t	*efi;		/* extent free intention */
 	int			error;		/* error return value */
-	xfs_bmap_free_item_t	*free;		/* free extent item */
 	struct xfs_trans_res	tres;		/* new log reservation */
-	xfs_mount_t		*mp;		/* filesystem mount structure */
-	xfs_bmap_free_item_t	*next;		/* next item on free list */
 	xfs_trans_t		*ntp;		/* new transaction pointer */
 
 	ASSERT((*tp)->t_flags & XFS_TRANS_PERM_LOG_RES);

@@ -90,9 +105,14 @@ xfs_bmap_finish(
 	}
 	ntp = *tp;
 	efi = xfs_trans_get_efi(ntp, flist->xbf_count);
-	for (free = flist->xbf_first; free; free = free->xbfi_next)
-		xfs_trans_log_efi_extent(ntp, efi, free->xbfi_startblock,
-			free->xbfi_blockcount);
+
+	list_sort(NULL, &flist->xbf_list, xfs_freed_extent_cmp);
+
+	list_for_each_entry(free, &flist->xbf_list, list) {
+		xfs_trans_log_efi_extent(ntp, efi,
+			XFS_AGB_TO_FSB(mp, free->agno, free->bno),
+			free->length);
+	}
 
 	tres.tr_logres = ntp->t_log_res;
 	tres.tr_logcount = ntp->t_log_count;

@@ -118,10 +138,10 @@ xfs_bmap_finish(
 	if (error)
 		return error;
 	efd = xfs_trans_get_efd(ntp, efi, flist->xbf_count);
-	for (free = flist->xbf_first; free != NULL; free = next) {

fs/xfs/xfs_bmap_util.c +7 -5

@@ -130,7 +150,6 @@ xfs_bmap_finish(
 			 * happens, since this transaction may not be
 			 * dirty yet.
 			 */
-			mp = ntp->t_mountp;
 			if (!XFS_FORCED_SHUTDOWN(mp))
 				xfs_force_shutdown(mp,
 						   (error == EFSCORRUPTED) ?

@@ -138,10 +157,13 @@ xfs_bmap_finish(
 						   SHUTDOWN_META_IO_ERROR);
 			return error;
 		}
-		xfs_trans_log_efd_extent(ntp, efd, free->xbfi_startblock,
-			free->xbfi_blockcount);
-		xfs_bmap_del_free(flist, NULL, free);
+
+		xfs_trans_log_efd_extent(ntp, efd,
+			XFS_AGB_TO_FSB(mp, free->agno, free->bno),
+			free->length);
 	}
+
+	list_splice_init(&flist->xbf_list, &ntp->t_busy);
 	return 0;
 }
 

@@ -826,7 +848,7 @@ xfs_bmap_punch_delalloc_range(
 		if (error)
 			break;
 
-		ASSERT(!flist.xbf_count && !flist.xbf_first);
+		ASSERT(!flist.xbf_count && list_empty(&flist.xbf_list));
 next_block:
 		start_fsb++;
 		remaining--;

fs/xfs/xfs_bmap_util.h +0 -4

index 935ed2b..ffb26ea 100644
@@ -21,7 +21,6 @@
 /* Kernel only BMAP related definitions and functions */
 
 struct xfs_bmbt_irec;
-struct xfs_bmap_free_item;
 struct xfs_ifork;
 struct xfs_inode;
 struct xfs_mount;

@@ -80,9 +79,6 @@ int	xfs_getbmap(struct xfs_inode *ip, struct getbmapx *bmv,
 		xfs_bmap_format_t formatter, void *arg);
 
 /* functions in xfs_bmap.c that are only needed by xfs_bmap_util.c */
-void	xfs_bmap_del_free(struct xfs_bmap_free *flist,
-			  struct xfs_bmap_free_item *prev,
-			  struct xfs_bmap_free_item *free);
 int	xfs_bmap_extsize_align(struct xfs_mount *mp, struct xfs_bmbt_irec *gotp,
 			       struct xfs_bmbt_irec *prevp, xfs_extlen_t extsz,
 			       int rt, int eof, int delay, int convert,

fs/xfs/xfs_discard.c +1 -1

index 4f11ef0..e3d0f18 100644
@@ -215,7 +215,7 @@ xfs_discard_extents(
 	struct xfs_mount	*mp,
 	struct list_head	*list)
 {
-	struct xfs_extent_busy	*busyp;
+	struct xfs_freed_extent	*busyp;
 	int			error = 0;
 
 	list_for_each_entry(busyp, list, list) {

fs/xfs/xfs_extent_busy.c +0 -0

index fd22f69..f4711ee 100644

fs/xfs/xfs_extent_busy.c +20 -40

-	struct xfs_extent_busy	*new;
-	struct xfs_extent_busy	*busyp;
+	struct xfs_freed_extent	*busyp;
 	struct xfs_perag	*pag;
 	struct rb_node		**rbp;
 	struct rb_node		*parent = NULL;
 
-	new = kmem_zalloc(sizeof(struct xfs_extent_busy), KM_MAYFAIL);
-	if (!new) {
-		/*
-		 * No Memory!  Since it is now not possible to track the free
-		 * block, make this a synchronous transaction to insure that
-		 * the block is not reused before this transaction commits.
-		 */
-		trace_xfs_extent_busy_enomem(tp->t_mountp, agno, bno, len);
-		xfs_trans_set_sync(tp);
-		return;
-	}
-
-	new->agno = agno;
-	new->bno = bno;
-	new->length = len;
-	INIT_LIST_HEAD(&new->list);
-	new->flags = flags;
-
 	/* trace before insert to be able to see failed inserts */
-	trace_xfs_extent_busy(tp->t_mountp, agno, bno, len);
+	trace_xfs_extent_busy(tp->t_mountp, new->agno, new->bno, new->length);
 
 	pag = xfs_perag_get(tp->t_mountp, new->agno);
 	spin_lock(&pag->pagb_lock);
 	rbp = &pag->pagb_tree.rb_node;
 	while (*rbp) {
 		parent = *rbp;
-		busyp = rb_entry(parent, struct xfs_extent_busy, rb_node);
+		busyp = rb_entry(parent, struct xfs_freed_extent, rb_node);
 
 		if (new->bno < busyp->bno) {
 			rbp = &(*rbp)->rb_left;
 			ASSERT(new->bno + new->length <= busyp->bno);
 		} else if (new->bno > busyp->bno) {
 			rbp = &(*rbp)->rb_right;
-			ASSERT(bno >= busyp->bno + busyp->length);
+			ASSERT(new->bno >= busyp->bno + busyp->length);
 		} else {
 			ASSERT(0);
 		}

@@ -88,7 +66,6 @@ xfs_extent_busy_insert(
 	rb_link_node(&new->rb_node, parent, rbp);
 	rb_insert_color(&new->rb_node, &pag->pagb_tree);
 
-	list_add(&new->list, &tp->t_busy);
 	spin_unlock(&pag->pagb_lock);
 	xfs_perag_put(pag);
 }

@@ -111,7 +88,7 @@ xfs_extent_busy_search(
 {
 	struct xfs_perag	*pag;
 	struct rb_node		*rbp;
-	struct xfs_extent_busy	*busyp;
+	struct xfs_freed_extent	*busyp;
 	int			match = 0;
 
 	pag = xfs_perag_get(mp, agno);

@@ -121,7 +98,7 @@ xfs_extent_busy_search(
 
 	/* find closest start bno overlap */
 	while (rbp) {
-		busyp = rb_entry(rbp, struct xfs_extent_busy, rb_node);
+		busyp = rb_entry(rbp, struct xfs_freed_extent, rb_node);
 		if (bno < busyp->bno) {
 			/* may overlap, but exact start block is lower */
 			if (bno + len > busyp->bno)

@@ -158,7 +135,7 @@ STATIC bool
 xfs_extent_busy_update_extent(
 	struct xfs_mount	*mp,
 	struct xfs_perag	*pag,
-	struct xfs_extent_busy	*busyp,
+	struct xfs_freed_extent	*busyp,
 	xfs_agblock_t		fbno,
 	xfs_extlen_t		flen,
 	bool			userdata) __releases(&pag->pagb_lock)

@@ -173,7 +150,7 @@ xfs_extent_busy_update_extent(
 	 * performing the discard a chance to mark the extent unbusy
 	 * and retry.
 	 */
-	if (busyp->flags & XFS_EXTENT_BUSY_DISCARDED) {
+	if (busyp->flags & XFS_EXTENT_DISCARDED) {
 		spin_unlock(&pag->pagb_lock);
 		delay(1);
 		spin_lock(&pag->pagb_lock);

@@ -320,8 +297,8 @@ xfs_extent_busy_reuse(
 restart:
 	rbp = pag->pagb_tree.rb_node;
 	while (rbp) {
-		struct xfs_extent_busy *busyp =
-			rb_entry(rbp, struct xfs_extent_busy, rb_node);
+		struct xfs_freed_extent *busyp =
+			rb_entry(rbp, struct xfs_freed_extent, rb_node);
 		xfs_agblock_t	bbno = busyp->bno;
 		xfs_agblock_t	bend = bbno + busyp->length;
 

@@ -367,8 +344,8 @@ restart:
 	flen = len;
 	rbp = args->pag->pagb_tree.rb_node;
 	while (rbp && flen >= args->minlen) {
-		struct xfs_extent_busy *busyp =
-			rb_entry(rbp, struct xfs_extent_busy, rb_node);
+		struct xfs_freed_extent *busyp =
+			rb_entry(rbp, struct xfs_freed_extent, rb_node);
 		xfs_agblock_t	fend = fbno + flen;
 		xfs_agblock_t	bbno = busyp->bno;
 		xfs_agblock_t	bend = bbno + busyp->length;

@@ -386,7 +363,7 @@ restart:
 		 * extent instead of trimming the allocation.
 		 */
 		if (!args->userdata &&
-		    !(busyp->flags & XFS_EXTENT_BUSY_DISCARDED)) {
+		    !(busyp->flags & XFS_EXTENT_DISCARDED)) {
 			if (!xfs_extent_busy_update_extent(args->mp, args->pag,
 							  busyp, fbno, flen,
 							  false))

@@ -540,7 +517,7 @@ STATIC void
 xfs_extent_busy_clear_one(
 	struct xfs_mount	*mp,
 	struct xfs_perag	*pag,
-	struct xfs_extent_busy	*busyp)
+	struct xfs_freed_extent	*busyp)
 {
 	if (busyp->length) {
 		trace_xfs_extent_busy_clear(mp, busyp->agno, busyp->bno,

@@ -549,7 +526,7 @@ xfs_extent_busy_clear_one(
 	}
 
 	list_del_init(&busyp->list);
-	kmem_free(busyp);
+	kmem_zone_free(xfs_freed_extent_zone, busyp);
 }
 
 /*

@@ -563,7 +540,7 @@ xfs_extent_busy_clear(
 	struct list_head	*list,
 	bool			do_discard)
 {
-	struct xfs_extent_busy	*busyp, *n;
+	struct xfs_freed_extent	*busyp, *n;
 	struct xfs_perag	*pag = NULL;
 	xfs_agnumber_t		agno = NULLAGNUMBER;
 

@@ -579,8 +556,8 @@ xfs_extent_busy_clear(
 		}
 
 		if (do_discard && busyp->length &&
-		    !(busyp->flags & XFS_EXTENT_BUSY_SKIP_DISCARD))
-			busyp->flags = XFS_EXTENT_BUSY_DISCARDED;
+		    !(busyp->flags & XFS_EXTENT_SKIP_DISCARD))
+			busyp->flags = XFS_EXTENT_DISCARDED;
 		else
 			xfs_extent_busy_clear_one(mp, pag, busyp);
 	}

@@ -600,6 +577,6 @@ xfs_extent_busy_ag_cmp(
 	struct list_head	*a,
 	struct list_head	*b)
 {
-	return container_of(a, struct xfs_extent_busy, list)->agno -
-		container_of(b, struct xfs_extent_busy, list)->agno;
+	return container_of(a, struct xfs_freed_extent, list)->agno -
+		container_of(b, struct xfs_freed_extent, list)->agno;
 }

fs/xfs/xfs_extent_busy.h +2 -20

index bfff284..ccc8a13 100644
@@ -20,31 +20,13 @@
 #ifndef __XFS_EXTENT_BUSY_H__
 #define	__XFS_EXTENT_BUSY_H__
 
+struct xfs_freed_extent;
 struct xfs_mount;
 struct xfs_trans;
 struct xfs_alloc_arg;
 
-/*
- * Busy block/extent entry.  Indexed by a rbtree in perag to mark blocks that
- * have been freed but whose transactions aren't committed to disk yet.
- *
- * Note that we use the transaction ID to record the transaction, not the
- * transaction structure itself. See xfs_extent_busy_insert() for details.
- */
-struct xfs_extent_busy {
-	struct rb_node	rb_node;	/* ag by-bno indexed search tree */
-	struct list_head list;		/* transaction busy extent list */
-	xfs_agnumber_t	agno;
-	xfs_agblock_t	bno;
-	xfs_extlen_t	length;
-	unsigned int	flags;
-#define XFS_EXTENT_BUSY_DISCARDED	0x01	/* undergoing a discard op. */
-#define XFS_EXTENT_BUSY_SKIP_DISCARD	0x02	/* do not discard */
-};
-
 void
-xfs_extent_busy_insert(struct xfs_trans *tp, xfs_agnumber_t agno,
-	xfs_agblock_t bno, xfs_extlen_t len, unsigned int flags);
+xfs_extent_busy_insert(struct xfs_trans	*tp, struct xfs_freed_extent *free);
 
 void
 xfs_extent_busy_clear(struct xfs_mount *mp, struct list_head *list,

fs/xfs/xfs_super.c +7 -7

index f317488..a674664 100644
@@ -1581,15 +1581,15 @@ xfs_init_zones(void)
 	if (!xfs_log_ticket_zone)
 		goto out_destroy_ioend_pool;
 
-	xfs_bmap_free_item_zone = kmem_zone_init(sizeof(xfs_bmap_free_item_t),
-						"xfs_bmap_free_item");
-	if (!xfs_bmap_free_item_zone)
+	xfs_freed_extent_zone = kmem_zone_init(sizeof(struct xfs_freed_extent),
+						"xfs_freed_extent");
+	if (!xfs_freed_extent_zone)
 		goto out_destroy_log_ticket_zone;
 
 	xfs_btree_cur_zone = kmem_zone_init(sizeof(xfs_btree_cur_t),
 						"xfs_btree_cur");
 	if (!xfs_btree_cur_zone)
-		goto out_destroy_bmap_free_item_zone;
+		goto out_destroy_freed_extent_zone;
 
 	xfs_da_state_zone = kmem_zone_init(sizeof(xfs_da_state_t),
 						"xfs_da_state");

@@ -1671,8 +1671,8 @@ xfs_init_zones(void)
 	kmem_zone_destroy(xfs_da_state_zone);
  out_destroy_btree_cur_zone:
 	kmem_zone_destroy(xfs_btree_cur_zone);
- out_destroy_bmap_free_item_zone:
-	kmem_zone_destroy(xfs_bmap_free_item_zone);
+ out_destroy_freed_extent_zone:
+	kmem_zone_destroy(xfs_freed_extent_zone);
  out_destroy_log_ticket_zone:
 	kmem_zone_destroy(xfs_log_ticket_zone);
  out_destroy_ioend_pool:

@@ -1702,7 +1702,7 @@ xfs_destroy_zones(void)
 	kmem_zone_destroy(xfs_ifork_zone);
 	kmem_zone_destroy(xfs_da_state_zone);
 	kmem_zone_destroy(xfs_btree_cur_zone);
-	kmem_zone_destroy(xfs_bmap_free_item_zone);
+	kmem_zone_destroy(xfs_freed_extent_zone);
 	kmem_zone_destroy(xfs_log_ticket_zone);
 	mempool_destroy(xfs_ioend_pool);
 	kmem_zone_destroy(xfs_ioend_zone);

fs/xfs/xfs_trace.h +0 -1

index a4ae41c..2dfe819 100644
@@ -1313,7 +1313,6 @@ DEFINE_EVENT(xfs_extent_busy_class, name, \
 		 xfs_agblock_t agbno, xfs_extlen_t len), \
 	TP_ARGS(mp, agno, agbno, len))
 DEFINE_BUSY_EVENT(xfs_extent_busy);
-DEFINE_BUSY_EVENT(xfs_extent_busy_enomem);
 DEFINE_BUSY_EVENT(xfs_extent_busy_force);
 DEFINE_BUSY_EVENT(xfs_extent_busy_reuse);
 DEFINE_BUSY_EVENT(xfs_extent_busy_clear);

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Review

Quoted 2 lines

(quoted diff) +2 -0

+		xfs_extent_busy_insert(tp, free);
+		list_add(&free->list, &tp->t_busy);

If I follow correctly, the list_add() is removed from xfs_extent_busy_insert() because we use the list field for the bmap flist as well as the t_busy list.

It appears we've lost an error check associated with allocation failure in xfs_freed_extent_alloc() (here and at other callers). The current code looks like it handles this by marking the transaction as synchronous. Have we avoided the need for this by using kmem_zone_alloc()? I guess it looks like the sleep param will cause it to continue to retry...

fs/xfs/xfs_alloc.h typedef struct xfs_alloc_arg { · line 129

Quoted 5 lines

fs/xfs/xfs_alloc.h +5 -0

+struct xfs_freed_extent {
+	struct rb_node	rb_node;	/* ag by-bno indexed search tree */
+	struct list_head list;		/* transaction busy extent list */
+	xfs_agnumber_t	agno;
+	xfs_agblock_t	bno;

agbno?

fs/xfs/xfs_alloc_btree.c xfs_allocbt_free_block( · line 122

Quoted 3 lines

fs/xfs/xfs_alloc_btree.c +0 -3

-	xfs_extent_busy_insert(cur->bc_tp, be32_to_cpu(agf->agf_seqno), bno, 1,
-			      XFS_EXTENT_BUSY_SKIP_DISCARD);
-	xfs_trans_agbtree_delta(cur->bc_tp, -1);

Was this supposed to go away?

Quoted 4 lines

-/*
* Add the extent to the list of extents to be free at transaction end.
* The list is maintained sorted (by block number).
*/

This comment could be fixed now that the sort is deferred.

Quoted 7 lines

fs/xfs/xfs_bmap_util.c +7 -0

+STATIC int
+xfs_freed_extent_cmp(
+	void			*priv,
+	struct list_head	*la,
+	struct list_head	*lb)
+{
+	struct xfs_freed_extent *a =

fs/xfs/xfs_bmap_util.c xfs_fsb_to_db(struct xfs_inode *ip, xfs_fsblock_t fsb) · line 74

Quoted 6 lines

fs/xfs/xfs_bmap_util.c +6 -0

+		container_of(la, struct xfs_freed_extent, list);
+	struct xfs_freed_extent *b =
+		container_of(lb, struct xfs_freed_extent, list);
+
+	if (a->agno == b->agno)
+		return a->bno - b->bno;

Could we just do a comparison here and return +/-1?

Quoted 7 lines

fs/xfs/xfs_bmap_util.c +3 -4

-	for (free = flist->xbf_first; free != NULL; free = next) {
-		next = free->xbfi_next;
-		if ((error = xfs_free_extent(ntp, free->xbfi_startblock,
-				free->xbfi_blockcount))) {
+
+	list_for_each_entry(free, &flist->xbf_list, list) {
+		error = __xfs_free_extent(ntp, free);

fs/xfs/xfs_bmap_util.c xfs_bmap_finish( · line 147

Quoted 4 lines

fs/xfs/xfs_bmap_util.c +1 -0

+		if (error) {
 			/*
 			 * The bmap free list will be cleaned up at a
 			 * higher level.  The EFI will be canceled when

So it seems like technically we could get away with still doing the list migration here an extent at a time, but that would turn this code kind of ugly (e.g., to remove each entry from xbf_list as we go).

Also, it appears we no longer do the xfs_extent_busy_insert() in this path..?

Quoted 4 lines

void
xfs_extent_busy_insert(

fs/xfs/xfs_extent_busy.c +0 -1

 	struct xfs_trans	*tp,
-	xfs_agnumber_t		agno,

fs/xfs/xfs_extent_busy.c line 39

Quoted 5 lines

fs/xfs/xfs_extent_busy.c +1 -3

-	xfs_agblock_t		bno,
-	xfs_extlen_t		len,
-	unsigned int		flags)
+	struct xfs_freed_extent	*new)
 {

tp is only used for the mount now, so we can probably replace tp with mp.

Quoted 6 lines · Brian Foster

Review

Quoted 2 lines

(quoted diff) +2 -0

+		xfs_extent_busy_insert(tp, free);
+		list_add(&free->list, &tp->t_busy);

If I follow correctly, the list_add() is removed from xfs_extent_busy_insert() because we use the list field for the bmap flist as well as the t_busy list.

Indeed. And in the case of the normal bmap free path we just splice the list from the bmap flist into the transaction, so we remove the add inside xfs_extent_busy_insert and move it to the callers, so thast the bmap free path can batch it.

Quoted 6 lines

It appears we've lost an error check associated with allocation failure in xfs_freed_extent_alloc() (here and at other callers). The current code looks like it handles this by marking the transaction as synchronous. Have we avoided the need for this by using kmem_zone_alloc()? I guess it looks like the sleep param will cause it to continue to retry...

Indeed. That's what the old bmap freelist path did, and for that case we can't really handle a failure as we are in a dirty transaction that we would have to abort. For the old extent_busy structure the failure wasn't fatal, but we got rid of that allocation entirely for the fast path.

Quoted 7 lines

fs/xfs/xfs_alloc.h typedef struct xfs_alloc_arg { · line 129

Quoted 5 lines

fs/xfs/xfs_alloc.h +5 -0

+struct xfs_freed_extent {
+	struct rb_node	rb_node;	/* ag by-bno indexed search tree */
+	struct list_head list;		/* transaction busy extent list */
+	xfs_agnumber_t	agno;
+	xfs_agblock_t	bno;

agbno?

Maybe that would be a bit more clear, although we use bno for an agblock_t in lots of places.

Quoted 5 lines

fs/xfs/xfs_alloc_btree.c xfs_allocbt_free_block( · line 122

Quoted 3 lines

fs/xfs/xfs_alloc_btree.c +0 -3

-	xfs_extent_busy_insert(cur->bc_tp, be32_to_cpu(agf->agf_seqno), bno, 1,
-			      XFS_EXTENT_BUSY_SKIP_DISCARD);
-	xfs_trans_agbtree_delta(cur->bc_tp, -1);

Was this supposed to go away?

The xfs_trans_agbtree_delta call wasn't supposed to go away. Kinda surprised it still passed testing despite this.

Quoted 6 lines

Quoted 4 lines

-/*
* Add the extent to the list of extents to be free at transaction end.
* The list is maintained sorted (by block number).
*/

This comment could be fixed now that the sort is deferred.

I'll fix it.

Quoted 7 lines

Quoted 7 lines

fs/xfs/xfs_bmap_util.c +7 -0

+STATIC int
+xfs_freed_extent_cmp(
+	void			*priv,
+	struct list_head	*la,
+	struct list_head	*lb)
+{
+	struct xfs_freed_extent *a =

Quoted 8 lines

fs/xfs/xfs_bmap_util.c xfs_fsb_to_db(struct xfs_inode *ip, xfs_fsblock_t fsb) · line 74

Quoted 6 lines

fs/xfs/xfs_bmap_util.c +6 -0

+		container_of(la, struct xfs_freed_extent, list);
+	struct xfs_freed_extent *b =
+		container_of(lb, struct xfs_freed_extent, list);
+
+	if (a->agno == b->agno)
+		return a->bno - b->bno;

Could we just do a comparison here and return +/-1?

Because we the compare callback returns an int, and type promotion in C will give us a wrong result if we "simplify" compare to 64-bit values. We already ran into this with the list_sort in xfs_buf.c. I'll add a comment to explain it.

Quoted 16 lines

fs/xfs/xfs_bmap_util.c xfs_bmap_finish( · line 147

Quoted 12 lines

fs/xfs/xfs_bmap_util.c +4 -4

-	for (free = flist->xbf_first; free != NULL; free = next) {
-		next = free->xbfi_next;
-		if ((error = xfs_free_extent(ntp, free->xbfi_startblock,
-				free->xbfi_blockcount))) {
+
+	list_for_each_entry(free, &flist->xbf_list, list) {
+		error = __xfs_free_extent(ntp, free);
+		if (error) {
 			/*
 			 * The bmap free list will be cleaned up at a
 			 * higher level.  The EFI will be canceled when

So it seems like technically we could get away with still doing the list migration here an extent at a time, but that would turn this code kind of ugly (e.g., to remove each entry from xbf_list as we go).

And there's not point.

Quoted 2 lines

Also, it appears we no longer do the xfs_extent_busy_insert() in this path..?

It did before I messed up a rebase..

Quoted 4 lines

Quoted 4 lines

void
xfs_extent_busy_insert(

fs/xfs/xfs_extent_busy.c +0 -1

 	struct xfs_trans	*tp,
-	xfs_agnumber_t		agno,

Quoted 8 lines

fs/xfs/xfs_extent_busy.c line 39

Quoted 5 lines

fs/xfs/xfs_extent_busy.c +1 -3

-	xfs_agblock_t		bno,
-	xfs_extlen_t		len,
-	unsigned int		flags)
+	struct xfs_freed_extent	*new)
 {

tp is only used for the mount now, so we can probably replace tp with mp.

I'll update it.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs