Re: [PATCH 1/4] block: implement compatible DISCARD support

[Christoph Hellwig <hch@infradead.org>]

On Thu, Feb 11, 2010 at 01:53:45PM +0300, Dmitry Monakhov wrote:
> Currently there are no many discs has native TRIM (aka) discard
> feature support. But in fact this is good feature. We can easily
> simlulate it for devices which has not native support.
> In compat mode discard dequest transforms in to simple zerofiled
> write request.
> In fact currently blkdev_issue_discard function implemented
> incorrectly.
> 1) Whait flags not optimal we dont have to wait for each bio in flight.

Indeed.

> 2) Not wait by default. Which makes it fairly useless.

Not sure what you mean with that.  We do not need to wait for the
discard request for the "online" type of use - the barrier flag
means we can't re-order I/O before and after the request so there
is no reason to wait - it stays in the places where it needs to be
in the I/O stream.

> 3) Send each bio with barrier flag(if requested). Which result in
>    bad performance. In fact caller just want to make shure that full
>    request is completed and ordered against other requests.

Yes, we need to ensure ordering only against reordering with other
requests.  Your patch only issues a cache flush, which means that
we miss the queue drain before submitting the discard bios

> 5) It use allocated_page instead of ZERO_PAGE.

That's incorrect - both the scsi WRITE SAME and ATA UNMAP
implementations write to the payload.  I have some plans to change that
an get rid of the payload entirely, but I need to get back to the
discard work and look at it in more detail.

> This patch introduce generic blkdev_issue_zeroout() function which also
> may be used for native discard request support, in this case zero payload
> simply ignored.

Which is a bit different from fixing efficiency issues in discard, I'd
prefer that to be split into a separate patch, especially as there might
be quite a bit of discussion on the zeroout behaviour.

Note that a discard is not actually required to zero out data it
has discarded, it's an optional feature in the command sets.