All of
 help / color / mirror / Atom feed
From: Christoph Hellwig <>
To: Dmitry Monakhov <>
Subject: Re: [PATCH 1/4] block: implement compatible DISCARD support
Date: Thu, 11 Feb 2010 07:21:54 -0500	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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.


> 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. 

  parent reply	other threads:[~2010-02-11 12:21 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-02-11 10:53 Dmitry Monakhov
2010-02-11 10:57 ` [PATCH 2/4] block: support compat discard mode by default Dmitry Monakhov
2010-02-11 11:25   ` Dmitry Monakhov
2010-02-11 12:22   ` Christoph Hellwig
2010-02-11 10:59 ` [PATCH 3/4] ext4: convert extent zeroout to generic function Dmitry Monakhov
2010-02-11 10:59   ` Dmitry Monakhov
2010-02-11 11:01 ` [PATCH 4/4] btrfs: add discard_compat support Dmitry Monakhov
2010-02-11 11:15   ` Dmitry Monakhov
2010-02-11 12:21 ` Christoph Hellwig [this message]
2010-02-11 12:59   ` [PATCH 1/4] block: implement compatible DISCARD support Dmitry Monakhov
2010-02-11 13:09     ` Christoph Hellwig
2010-02-11 13:45       ` Dmitry Monakhov
2010-02-11 14:06         ` Christoph Hellwig
2010-02-11 14:40   ` Martin K. Petersen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \
    --subject='Re: [PATCH 1/4] block: implement compatible DISCARD support' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.