xfs
[Top] [All Lists]

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

To: Alex Elder <aelder@xxxxxxx>
Subject: Re: [PATCH 3/6] xfs: exact busy extent tracking
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Mon, 28 Mar 2011 08:10:54 -0400
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <1301087060.2537.689.camel@doink>
References: <20110322195550.260682574@xxxxxxxxxxxxxxxxxxxxxx> <20110322200137.657110761@xxxxxxxxxxxxxxxxxxxxxx> <1301087060.2537.689.camel@doink>
User-agent: Mutt/1.5.21 (2010-09-15)
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.

<Prev in Thread] Current Thread [Next in Thread>