* [PATCH] xfs: merge xfs_bmap_free_item and xfs_extent_busy
@ 2014-03-07 10:55 Christoph Hellwig
2014-03-11 15:31 ` Brian Foster
0 siblings, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2014-03-07 10:55 UTC (permalink / raw)
To: xfs
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 <hch@lst.de>
diff --git a/fs/xfs/xfs_alloc.c b/fs/xfs/xfs_alloc.c
index c1cf6a3..656991d 100644
--- a/fs/xfs/xfs_alloc.c
+++ b/fs/xfs/xfs_alloc.c
@@ -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.
+ */
+ if (agno >= mp->m_sb.sb_agcount)
+ return EFSCORRUPTED;
+ if (agbno >= mp->m_sb.sb_agblocks)
+ return EFSCORRUPTED;
+
+ free = xfs_freed_extent_alloc(agno, agbno, len, 0);
+ error = __xfs_free_extent(tp, free);
+ if (!error) {
+ xfs_extent_busy_insert(tp, free);
+ list_add(&free->list, &tp->t_busy);
+ }
+ return error;
+}
diff --git a/fs/xfs/xfs_alloc.h b/fs/xfs/xfs_alloc.h
index feacb06..4aa7f8c 100644
--- a/fs/xfs/xfs_alloc.h
+++ b/fs/xfs/xfs_alloc.h
@@ -122,6 +122,17 @@ typedef struct xfs_alloc_arg {
xfs_fsblock_t firstblock; /* io first block allocated */
} xfs_alloc_arg_t;
+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;
+ 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__ */
diff --git a/fs/xfs/xfs_alloc_btree.c b/fs/xfs/xfs_alloc_btree.c
index cc1eadc..ce3041a 100644
--- a/fs/xfs/xfs_alloc_btree.c
+++ b/fs/xfs/xfs_alloc_btree.c
@@ -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;
bno = xfs_daddr_to_agbno(cur->bc_mp, XFS_BUF_ADDR(bp));
- error = xfs_alloc_put_freelist(cur->bc_tp, agbp, NULL, bno, 1);
+ error = xfs_alloc_put_freelist(tp, agbp, NULL, bno, 1);
if (error)
return error;
- 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);
+ 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;
}
diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index 5b6092e..9c2c00c 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -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
@@ -588,10 +588,6 @@ xfs_bmap_validate_ret(
#endif /* DEBUG */
/*
- * bmap free list manipulation functions
- */
-
-/*
* Add the extent to the list of extents to be free at transaction end.
* The list is maintained sorted (by block number).
*/
@@ -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);
}
diff --git a/fs/xfs/xfs_bmap.h b/fs/xfs/xfs_bmap.h
index f84bd7a..73cedc4 100644
--- a/fs/xfs/xfs_bmap.h
+++ b/fs/xfs/xfs_bmap.h
@@ -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;
}
/*
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 01f6a64..ade325f 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -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,
+ struct list_head *la,
+ struct list_head *lb)
+{
+ struct xfs_freed_extent *a =
+ 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;
+ 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) {
- 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
@@ -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--;
diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
index 935ed2b..ffb26ea 100644
--- a/fs/xfs/xfs_bmap_util.h
+++ b/fs/xfs/xfs_bmap_util.h
@@ -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,
diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index 4f11ef0..e3d0f18 100644
--- a/fs/xfs/xfs_discard.c
+++ b/fs/xfs/xfs_discard.c
@@ -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) {
diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
index fd22f69..f4711ee 100644
--- a/fs/xfs/xfs_extent_busy.c
+++ b/fs/xfs/xfs_extent_busy.c
@@ -35,51 +35,29 @@
void
xfs_extent_busy_insert(
struct xfs_trans *tp,
- xfs_agnumber_t agno,
- xfs_agblock_t bno,
- xfs_extlen_t len,
- unsigned int flags)
+ struct xfs_freed_extent *new)
{
- 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;
}
diff --git a/fs/xfs/xfs_extent_busy.h b/fs/xfs/xfs_extent_busy.h
index bfff284..ccc8a13 100644
--- a/fs/xfs/xfs_extent_busy.h
+++ b/fs/xfs/xfs_extent_busy.h
@@ -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,
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index f317488..a674664 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -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);
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index a4ae41c..2dfe819 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: merge xfs_bmap_free_item and xfs_extent_busy
2014-03-07 10:55 [PATCH] xfs: merge xfs_bmap_free_item and xfs_extent_busy Christoph Hellwig
@ 2014-03-11 15:31 ` Brian Foster
2014-03-12 10:40 ` Christoph Hellwig
0 siblings, 1 reply; 4+ messages in thread
From: Brian Foster @ 2014-03-11 15:31 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Fri, Mar 07, 2014 at 02:55:35AM -0800, Christoph Hellwig wrote:
> 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 <hch@lst.de>
>
> diff --git a/fs/xfs/xfs_alloc.c b/fs/xfs/xfs_alloc.c
> index c1cf6a3..656991d 100644
> --- a/fs/xfs/xfs_alloc.c
> +++ b/fs/xfs/xfs_alloc.c
> @@ -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.
> + */
> + if (agno >= mp->m_sb.sb_agcount)
> + return EFSCORRUPTED;
> + if (agbno >= mp->m_sb.sb_agblocks)
> + return EFSCORRUPTED;
> +
> + free = xfs_freed_extent_alloc(agno, agbno, len, 0);
> + error = __xfs_free_extent(tp, free);
> + if (!error) {
> + 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...
> + }
> + return error;
> +}
> diff --git a/fs/xfs/xfs_alloc.h b/fs/xfs/xfs_alloc.h
> index feacb06..4aa7f8c 100644
> --- a/fs/xfs/xfs_alloc.h
> +++ b/fs/xfs/xfs_alloc.h
> @@ -122,6 +122,17 @@ typedef struct xfs_alloc_arg {
> xfs_fsblock_t firstblock; /* io first block allocated */
> } xfs_alloc_arg_t;
>
> +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?
> + 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__ */
> diff --git a/fs/xfs/xfs_alloc_btree.c b/fs/xfs/xfs_alloc_btree.c
> index cc1eadc..ce3041a 100644
> --- a/fs/xfs/xfs_alloc_btree.c
> +++ b/fs/xfs/xfs_alloc_btree.c
> @@ -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;
>
> bno = xfs_daddr_to_agbno(cur->bc_mp, XFS_BUF_ADDR(bp));
> - error = xfs_alloc_put_freelist(cur->bc_tp, agbp, NULL, bno, 1);
> + error = xfs_alloc_put_freelist(tp, agbp, NULL, bno, 1);
> if (error)
> return error;
>
> - 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?
> + 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;
> }
>
> diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
> index 5b6092e..9c2c00c 100644
> --- a/fs/xfs/xfs_bmap.c
> +++ b/fs/xfs/xfs_bmap.c
> @@ -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
> @@ -588,10 +588,6 @@ xfs_bmap_validate_ret(
> #endif /* DEBUG */
>
> /*
> - * bmap free list manipulation functions
> - */
> -
> -/*
> * 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.
> @@ -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);
> }
>
> diff --git a/fs/xfs/xfs_bmap.h b/fs/xfs/xfs_bmap.h
> index f84bd7a..73cedc4 100644
> --- a/fs/xfs/xfs_bmap.h
> +++ b/fs/xfs/xfs_bmap.h
> @@ -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;
> }
>
> /*
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 01f6a64..ade325f 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -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,
> + struct list_head *la,
> + struct list_head *lb)
> +{
> + struct xfs_freed_extent *a =
> + 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?
> + 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) {
> - 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).
Also, it appears we no longer do the xfs_extent_busy_insert() in this
path..?
> @@ -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--;
> diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
> index 935ed2b..ffb26ea 100644
> --- a/fs/xfs/xfs_bmap_util.h
> +++ b/fs/xfs/xfs_bmap_util.h
> @@ -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,
> diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> index 4f11ef0..e3d0f18 100644
> --- a/fs/xfs/xfs_discard.c
> +++ b/fs/xfs/xfs_discard.c
> @@ -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) {
> diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
> index fd22f69..f4711ee 100644
> --- a/fs/xfs/xfs_extent_busy.c
> +++ b/fs/xfs/xfs_extent_busy.c
> @@ -35,51 +35,29 @@
> void
> xfs_extent_busy_insert(
> struct xfs_trans *tp,
> - xfs_agnumber_t agno,
> - 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.
Brian
> - 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;
> }
> diff --git a/fs/xfs/xfs_extent_busy.h b/fs/xfs/xfs_extent_busy.h
> index bfff284..ccc8a13 100644
> --- a/fs/xfs/xfs_extent_busy.h
> +++ b/fs/xfs/xfs_extent_busy.h
> @@ -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,
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index f317488..a674664 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -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);
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index a4ae41c..2dfe819 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -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
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: merge xfs_bmap_free_item and xfs_extent_busy
2014-03-11 15:31 ` Brian Foster
@ 2014-03-12 10:40 ` Christoph Hellwig
2014-03-12 12:35 ` Brian Foster
0 siblings, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2014-03-12 10:40 UTC (permalink / raw)
To: Brian Foster; +Cc: Christoph Hellwig, xfs
On Tue, Mar 11, 2014 at 11:31:38AM -0400, Brian Foster wrote:
> > + 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.
> 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.
> > +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.
> > - 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.
> > -/*
> > * 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.
> > +STATIC int
> > +xfs_freed_extent_cmp(
> > + void *priv,
> > + struct list_head *la,
> > + struct list_head *lb)
> > +{
> > + struct xfs_freed_extent *a =
> > + 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.
> > - 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.
> Also, it appears we no longer do the xfs_extent_busy_insert() in this
> path..?
It did before I messed up a rebase..
> > void
> > xfs_extent_busy_insert(
> > struct xfs_trans *tp,
> > - xfs_agnumber_t agno,
> > - 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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: merge xfs_bmap_free_item and xfs_extent_busy
2014-03-12 10:40 ` Christoph Hellwig
@ 2014-03-12 12:35 ` Brian Foster
0 siblings, 0 replies; 4+ messages in thread
From: Brian Foster @ 2014-03-12 12:35 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Wed, Mar 12, 2014 at 03:40:58AM -0700, Christoph Hellwig wrote:
> On Tue, Mar 11, 2014 at 11:31:38AM -0400, Brian Foster wrote:
> > > + 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.
>
> > 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.
>
Ok, thanks for the explanation.
> > > +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.
>
Sure, bno is just a little less clear in the context of a struct being
modified/handled in different contexts.
> > > - 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.
>
> > > -/*
> > > * 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.
>
> > > +STATIC int
> > > +xfs_freed_extent_cmp(
> > > + void *priv,
> > > + struct list_head *la,
> > > + struct list_head *lb)
> > > +{
> > > + struct xfs_freed_extent *a =
> > > + 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.
>
Interesting. Well, I could still be missing something but fwiw my
concern here is that we're subtracting two unsigned 32-bit values.
Brian
>
> > > - 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.
>
> > Also, it appears we no longer do the xfs_extent_busy_insert() in this
> > path..?
>
> It did before I messed up a rebase..
>
> > > void
> > > xfs_extent_busy_insert(
> > > struct xfs_trans *tp,
> > > - xfs_agnumber_t agno,
> > > - 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
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-03-12 12:36 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-07 10:55 [PATCH] xfs: merge xfs_bmap_free_item and xfs_extent_busy Christoph Hellwig
2014-03-11 15:31 ` Brian Foster
2014-03-12 10:40 ` Christoph Hellwig
2014-03-12 12:35 ` Brian Foster
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).