Re: [PATCH 2/4] xfs: improve handling of busy extents in the low-level allocator

[Christoph Hellwig <hch@lst.de>]

On Tue, Feb 07, 2017 at 08:13:54AM -0500, Brian Foster wrote:
> If discard is not enabled, the behavior is straightforward in that a
> busy_gen returned for a particular extent is triggered when that extent
> is made unbusy. A retry call to xfs_extent_busy_[trim|flush]() is never
> necessary. If discard is enabled, the pagb_gen returned for a particular
> extent is triggered when one of the busy extents in the list is made
> unbusy. It might be the extent I passed to xfs_extent_busy_trim(), it
> might not. I have to retry the call to know for sure.

pagb_gen is always incremented when _a_ busy extent is freed.
Without online discard this does indeed only happen once per
log commit / AG.  With discards it could happen twice, but this
difference in semantics shouldn't really matter.

> I think it's pretty straightforward how easy that may be to misuse in
> the future. My concern is that somebody down the road adds code to trim
> a particular extent, waits on the pagb_gen, doesn't retry and thus
> introduces a very subtle bug into the code.
> 
> I can pretty much guarantee after a month or so I'm going to forget all
> about this constraint, never mind longer than that. Hence, I'm asking
> for a brief comment to clarify how pagb_gen actually works. The more I
> think about it, I think above xfs_busy_extent_trim() is the right place

>
> since that is where we introduce/return the gen, we have a comment
> update there already, and it apparently already mistakenly refers to
> online discard. How about we update it to something like the following?

Sure, I'll add it.