[PATCH] xfs: add discard support (at transaction commit)

[Dave Chinner]

On Sun, May 09, 2010 at 01:50:48PM -0400, Christoph Hellwig wrote:
> Now that we have reliably tracking of deleted extents in a transaction
> we can easily implement "online" discard support which calls
> blkdev_issue_discard once a transaction commits.  We simply have to
> walk the list of busy extents after transaction commit, but before deleting
> it from the rbtree tracking these busy extents.
> 
> This does not replace by background discard support patch which is probably
> better for thin provisioned arrays - I will updated it to apply ontop of
> this patch when I'm ready to re-submit it.

I think this can  be made to work, but I don't really like it that
much, especially the barrier flush part. Is there any particular
reason we need to issue discards at this level apart from "other
filesystems are doing it" rather than doing it lazily in a
non-performance critical piece of code?

Regardless of this, some questions about the patch come to mind:

	1. is it safe to block the xfslogd in the block layer in,
	say, get_request()? i.e. should we be issuing IO from an IO
	completion handler? That raises red flags in my head...

	2. issuing discards will block xfslogd and potentially stall
	the log if there are lots of discards to issue.

	3. DISCARD_FL_BARRIER appears to be used to allow async
	issuing of the discard to ensure any followup write has the
	discard processed first. What happens if the device does not
	support barriers or barriers are turned off?

	To me it appears that a lack of barriers could result in a
	write being reordered in front of the discard.  e.g.
	delalloc results in btree block freed, marked busy. New
	delalloc occurs, allocates block, marked sync, forces log,
	issues async discard, completes transaction and then writes
	data async.  Which operation does the drive see and complete
	first - the discard or the data write?

	4. A barrier IO on every discard? In a word: Ouch.

Cheers,

Dave.
-- 
Dave Chinner
david at fromorbit.com