xfs
[Top] [All Lists]

Re: [PATCH 4/4] xfs: reduce the number of pagb_lock roundtrips in xfs_al

To: Alex Elder <aelder@xxxxxxx>
Subject: Re: [PATCH 4/4] xfs: reduce the number of pagb_lock roundtrips in xfs_alloc_clear_busy
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 29 Apr 2011 11:13:40 +1000
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <1303923381.2056.58.camel@doink>
References: <20110424190613.404786382@xxxxxxxxxxxxxxxxxxxxxx> <20110424190657.249817918@xxxxxxxxxxxxxxxxxxxxxx> <1303923381.2056.58.camel@doink>
User-agent: Mutt/1.5.20 (2009-06-14)
On Wed, Apr 27, 2011 at 11:56:21AM -0500, Alex Elder wrote:
> On Sun, 2011-04-24 at 15:06 -0400, Christoph Hellwig wrote:
> > Instead of finding the per-ag and then taking and releasing the pagb_lock
> > for every single busy extent completed sort the list of busy extents and
> > only switch betweens AGs where nessecary.  This becomes especially important
> > with the online discard support which will hit this lock more often.
> > 
> > Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> 
> I said this looked good before, but since you haven't
> indicated that here yet, I'll make one more comment.
> 
> Both places that call (the new) xfs_alloc_busy_clear()
> precede the call with a call to xfs_alloc_busy_sort().
> Considering that, and the fact that xfs_alloc_busy_clear()
> depends on the list being sorted for correct (or at least
> efficient) operation, I think you should just embed the
> list_sort() call in xfs_alloc_busy_clear().
> 
> There would then be no real need to define the
> xfs_alloc_busy_sort() helper (just call list_sort()
> directly), and you can move the definition of
> xfs_busy_extent_ag_cmp() up in the file and give
> it private scope.

I agree with this - sorting the list externally just seems to
complicate the API and means callers need to remember to sort the
list first...

Otherwise,

Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>

-- 
Dave Chinner
david@xxxxxxxxxxxxx

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