On Tue, Feb 07, 2017 at 10:43:32AM +0100, Christoph Hellwig wrote: > On Mon, Feb 06, 2017 at 11:47:50AM -0500, Brian Foster wrote: > > That said, that limitation should be noted somewhere. Can we add a > > comment in xfs_extent_busy_clear() right above the hunk where we do the > > SKIP_DISCARD wake? E.g., something that points out the gen number for > > any particular extent could be bumped in such a situation.. (or > > something along those lines)? > > I could add a comment, but given that the tree tracks busy extents > and is not in any way specific to discard I think it's more confusing > than not having it. The tree is generic to busy extents, sure. The behavior of the generation number definitely changes depending on whether discards are enabled or not. 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. 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? /* * ... * * Return the current busy generation for the AG if the extent is busy. This * value can be used to wait for at least one of the currently busy extents to * be cleared. Note that the busy list is not guaranteed to be empty after the * gen is woken. The state of a specific extent must always be confirmed with * another call to xfs_extent_busy_trim() before it can be used. */ Brian > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
