xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 4/4] xfs: reduce the number of pagb_lock roundtrips in xfs_alloc_clear_busy
From: Alex Elder <aelder@xxxxxxx>
Date: Wed, 27 Apr 2011 11:56:21 -0500
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <20110424190657.249817918@xxxxxxxxxxxxxxxxxxxxxx>
References: <20110424190613.404786382@xxxxxxxxxxxxxxxxxxxxxx> <20110424190657.249817918@xxxxxxxxxxxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
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.

Perhaps you have other plans that make the model you
have here more appropriate though.  Let me know
if you intend to re-submit, otherwise I'll just take
this version.

Either way, this looks good to me.  I have been
testing with these patches.  I'll wait another day
to give others a chance to review, but I'll plan
to incorporate this tomorrow.

Reviewed-by: Alex Elder <aelder@xxxxxxx>


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