Re: [PATCH 3/6] xfs: exact busy extent tracking

[Christoph Hellwig <hch@infradead.org>]

On Fri, Mar 25, 2011 at 04:04:20PM -0500, Alex Elder wrote:
> > - * Insert a new extent into the busy tree.
> 
> OK, to be honest I haven't re-read this entire block of
> comment text to identify what might be of value.  But
> is there really *nothing* worth saving?  Is the busy
> extent tree documented adequately elsewhere?

The old text doesn't really apply to the new code anymore.  I'll see
if there's a place I can insert some more documentaion, but it already
has quite a lot of comments.

> >  		if (new->bno < busyp->bno) {
> >  			/* may overlap, but exact start block is lower */
>
> This comment isn't really right any more (BUG_ON that condition).

> >  		} else if (new->bno > busyp->bno) {
> >  			/* may overlap, but exact start block is higher */
> 
> This one too.

Removed.

> > +			continue;
> > +		} else if (fbno >= bend) {
> > +			rbp = rbp->rb_right;
> > +			continue;
> > +		}
> > +
> 
> I was going to suggest:
> 		/* Extent overlaps busy extent */
> 
> here, but I think that may not be the case any more if
> busyp->length is zero.  Or rather, the extent may
> surround the zero-length busy extent (which I suppose
> could be considered overlap).
> 
> If busyp->length is zero, I think the call to
> xfs_alloc_busy_try_reuse() is not needed; in fact,
> if it is already 0, that function will call
> rb_erase() on the entry again.

We will never see zero-length busy extents here.  While they have
to remain on the per-transaction/cil_context list they are properly
removed from the rbtree.

> ...therefore this branch is always taken, and the code
> below this block to the end of the loop is not reached.
> 
> Since this is the only place it's used, xfs_alloc_busy_try_reuse()
> might as well be defined as a void function.
> 
> (Ahh, but now that I've looked at the later patches
> I see it gets used again later.  I'm leaving my
> comments here nevertheless.)

Yes, it's changing in the next patch.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs