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@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
