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