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