PROBLEM: SSD access time with dm-crypt is way too high

PROBLEM: SSD access time with dm-crypt is way too high

[Michael Zugelder]

Hello,

from kernel 2.6.33 on (at least up to 2.6.37) the access time when using
dm-crypt on SSD is way too high. I would expect it to be a bit worse
than directly accessing the device (~0.2ms here), but currently it's at
exactly 10ms (except a few outlines). This corresponds to CONFIG_HZ=100.

The high access time makes the system feel very slow (if you're
expecting SSD performance). On kernel 2.6.32 this wasn't an issue,
access time stayed below 0.3ms. Those numbers are from palimpsest, but
the difference is so drastic, that pretty much everything you can think
of shows a difference (e. g. find-ing all files on the disk goes up from
tens of seconds to multiple minutes). 
Just to be clear: This isn't about a dm-crypt encrypted disk being
slower that an unencrypted disk. It's about the access time of the same
encrypted disk rising to >= 10ms, which is at least an order of
magnitude for an SSD. I suspect with an HDD, the difference isn't much
noticeable.

I filed a bugreport on the Ubuntu bugtracker with a few pretty graphs,
vmstat and LatencyTOP output. If that would be useful, see
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/653611 .

A bisect between .32 and .33 revealed some more information. I arrived
at 79da064, which is a revert of fb1e753 (block: improve
queue_should_plug() by looking at IO depths), so I compiled fb1e753.
With this 2.6.31 kernel, the problem was gone. One step back to 1f98a13,
the access time was high again.

So, it seems that fb1e753 (block: improve queue_should_plug() by looking
at IO depths) improves dm-crypt access time on SSDs (by a factor of over
40 here), but (as in the revert commit stated) reduces sequential
throughput in some cases. I applied fb1e753 to 2.6.37 and the access
time went down to the .32 value.

For me, it seems that dm-crypt interferes with queuing, which totally
destroys access time. If the disk empties the queue after a few
fractions of a millisecond, and it only gets refilled every tick (10ms),
wouldn't this explain the observed behavior? Correspondingly, a 300HZ
kernel achieves about 3ms access time Going all the way up to 1000HZ
hardly improves that number.

Please keep me on CC, since I am not subscribed.

Thanks
Michael

Re: [dm-devel] PROBLEM: SSD access time with dm-crypt is way too high

[Milan Broz]

On 01/09/2011 02:35 PM, Michael Zugelder wrote:

Hi,

> Just to be clear: This isn't about a dm-crypt encrypted disk being
> slower that an unencrypted disk. It's about the access time of the same
> encrypted disk rising to >= 10ms, which is at least an order of
> magnitude for an SSD. I suspect with an HDD, the difference isn't much
> noticeable.

yep, it was reported several times, thanks for that bisect.
We will need to retest it with per-cpu dm-crypt patches
(should be in linux-next soon) but I expect that this problem remains.

> I filed a bugreport on the Ubuntu bugtracker with a few pretty graphs,
> vmstat and LatencyTOP output. If that would be useful, see
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/653611 .

(Maybe Ubuntu people can forward issue to upstream list next time?)

> A bisect between .32 and .33 revealed some more information. I arrived
> at 79da064, which is a revert of fb1e753 (block: improve
> queue_should_plug() by looking at IO depths), so I compiled fb1e753.
> With this 2.6.31 kernel, the problem was gone. One step back to 1f98a13,
> the access time was high again.
> 
> So, it seems that fb1e753 (block: improve queue_should_plug() by looking
> at IO depths) improves dm-crypt access time on SSDs (by a factor of over
> 40 here), but (as in the revert commit stated) reduces sequential
> throughput in some cases. I applied fb1e753 to 2.6.37 and the access
> time went down to the .32 value.
> 
> For me, it seems that dm-crypt interferes with queuing, which totally
> destroys access time.

Yes, dm-crypt clones requests and process them in its own queue so there
is some interference.

I am not sure if this is fixable by block layer only patch (like fb1e753),

Maybe dm-crypt should call unplug explicitly, flush crypt workqueue
explicitly or something like that... dunno

Jens, do you have an idea how to improve it and not lose throughput?

Milan

[PATCH] PROBLEM: SSD access time with dm-crypt is way too high

[Mikulas Patocka]

On Sun, 9 Jan 2011, Milan Broz wrote:

> On 01/09/2011 02:35 PM, Michael Zugelder wrote:
> 
> Hi,
> 
> > Just to be clear: This isn't about a dm-crypt encrypted disk being
> > slower that an unencrypted disk. It's about the access time of the same
> > encrypted disk rising to >= 10ms, which is at least an order of
> > magnitude for an SSD. I suspect with an HDD, the difference isn't much
> > noticeable.
> 
> yep, it was reported several times, thanks for that bisect.
> We will need to retest it with per-cpu dm-crypt patches
> (should be in linux-next soon) but I expect that this problem remains.
> 
> > I filed a bugreport on the Ubuntu bugtracker with a few pretty graphs,
> > vmstat and LatencyTOP output. If that would be useful, see
> > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/653611 .
> 
> (Maybe Ubuntu people can forward issue to upstream list next time?)
> 
> > A bisect between .32 and .33 revealed some more information. I arrived
> > at 79da064, which is a revert of fb1e753 (block: improve
> > queue_should_plug() by looking at IO depths), so I compiled fb1e753.
> > With this 2.6.31 kernel, the problem was gone. One step back to 1f98a13,
> > the access time was high again.
> > 
> > So, it seems that fb1e753 (block: improve queue_should_plug() by looking
> > at IO depths) improves dm-crypt access time on SSDs (by a factor of over
> > 40 here), but (as in the revert commit stated) reduces sequential
> > throughput in some cases. I applied fb1e753 to 2.6.37 and the access
> > time went down to the .32 value.
> > 
> > For me, it seems that dm-crypt interferes with queuing, which totally
> > destroys access time.
> 
> Yes, dm-crypt clones requests and process them in its own queue so there
> is some interference.
> 
> I am not sure if this is fixable by block layer only patch (like fb1e753),
> 
> Maybe dm-crypt should call unplug explicitly, flush crypt workqueue
> explicitly or something like that... dunno
> 
> Jens, do you have an idea how to improve it and not lose throughput?
> 
> Milan
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
> 

Hi

Try this patch. I coded it long time ago, but it was forgotten somehow.

Mikulas

---

Unplug the queue when all IOs are finished.

If we don't unplug the queue, there will be 3ms delay (specified in
block/blk-settings.c, blk_queue_make_request) before the requests is submitted
to the device. So we must unplug the queue.

To avoid excessive unplugging, we must unplug only when we finish processing the
last request.

dm-crypt uses workqueue to queue the IOs. Unfortunatelly, the workqueue API
doesn't allow to query whether there are some more requests pending on the
queue. There API provides function to query "is this work queued on any
workqueue?", but it doesn't provide a function to query "is there any work
pending on this workqueue?".

There are two approaches that I considered:

1. make a special work for unplug. After queuing each IO's work, cancel the
unplug work and queue it again (so that it will always be queued as a last
entry). Unfortunatelly, canceling a work is rather slow operation so I decided
to not use this approach.

2. use a special pointer that points to the last IO. When the IO is finished
and the pointer matches this IO, we know that it was the last IO and we should
unplug. This patch implements this approach.

The pointer is not protected by any locks. So there may be race conditions with
it's manipulation (for example, on architectures with non-atomic memory writes,
simultaneous writes to the pointer may make it to point to garbage). However,
the pointer is never dereferenced, so this shouldn't cause crashes. The race
condition may only cause that unplug is missed and this triggers the 3ms delay.

Write requests are dispatched directly from kcryptd workqueue, so wee need an
unplug for it as well.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-crypt.c |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

Index: linux-2.6.36-rc3-fast/drivers/md/dm-crypt.c
===================================================================
--- linux-2.6.36-rc3-fast.orig/drivers/md/dm-crypt.c	2010-09-02 22:50:52.000000000 +0200
+++ linux-2.6.36-rc3-fast/drivers/md/dm-crypt.c	2010-09-02 22:59:01.000000000 +0200
@@ -107,6 +107,16 @@ struct crypt_config {
 	struct workqueue_struct *io_queue;
 	struct workqueue_struct *crypt_queue;
 
+  	/*
+	 * The last io submitted to io_queue and crypt_queue.
+	 * This pointers are not protected by any locks and thus must not be
+	 * dereferenced --- they may contain garbage. The only allowed operation
+	 * is to compare these pointers with current io being processed. If they
+	 * match, the block device queue should be unplugged.
+	 */
+	struct dm_crypt_io *io_queue_last_io;
+	struct dm_crypt_io *crypt_queue_last_io;
+
 	char *cipher;
 	char *cipher_mode;
 
@@ -731,17 +741,29 @@ static void kcryptd_io_write(struct dm_c
 static void kcryptd_io(struct work_struct *work)
 {
 	struct dm_crypt_io *io = container_of(work, struct dm_crypt_io, work);
+	struct crypt_config *cc = io->target->private;
 
 	if (bio_data_dir(io->base_bio) == READ)
 		kcryptd_io_read(io);
 	else
 		kcryptd_io_write(io);
+
+	/*
+	 * Neither io nor cc->io_queue_last_io may be dereferenced here.
+	 * This check just checks if it was the last io.
+	 */
+	if (io == cc->io_queue_last_io) {
+		struct request_queue *q = bdev_get_queue(cc->dev->bdev);
+		blk_unplug(q);
+		cc->io_queue_last_io = NULL;
+	}
 }
 
 static void kcryptd_queue_io(struct dm_crypt_io *io)
 {
 	struct crypt_config *cc = io->target->private;
 
+	cc->io_queue_last_io = io;
 	INIT_WORK(&io->work, kcryptd_io);
 	queue_work(cc->io_queue, &io->work);
 }
@@ -915,17 +937,31 @@ static void kcryptd_async_done(struct cr
 static void kcryptd_crypt(struct work_struct *work)
 {
 	struct dm_crypt_io *io = container_of(work, struct dm_crypt_io, work);
+	struct crypt_config *cc = io->target->private;
 
 	if (bio_data_dir(io->base_bio) == READ)
 		kcryptd_crypt_read_convert(io);
 	else
 		kcryptd_crypt_write_convert(io);
+
+	/*
+	 * Neither io nor cc->crypt_queue_last_io may be dereferenced here.
+	 * This check just checks if it was the last writing io.
+	 */
+	if (io == cc->crypt_queue_last_io) {
+		struct request_queue *q = bdev_get_queue(cc->dev->bdev);
+		blk_unplug(q);
+		cc->crypt_queue_last_io = NULL;
+	}
 }
 
 static void kcryptd_queue_crypt(struct dm_crypt_io *io)
 {
 	struct crypt_config *cc = io->target->private;
 
+	/* Write ios need unplugging in kcryptd work thread */
+	if (bio_data_dir(io->base_bio) == WRITE)
+		cc->crypt_queue_last_io = io;
 	INIT_WORK(&io->work, kcryptd_crypt);
 	queue_work(cc->crypt_queue, &io->work);
 }

Re: [PATCH] PROBLEM: SSD access time with dm-crypt is way too high

[Milan Broz]

On 01/10/2011 06:19 PM, Mikulas Patocka wrote:
> Try this patch. I coded it long time ago, but it was forgotten somehow.

No, it was not forgotten, I just do not like this approach :)

> +  	/*
> +	 * The last io submitted to io_queue and crypt_queue.
> +	 * This pointers are not protected by any locks and thus must not be
> +	 * dereferenced --- they may contain garbage. The only allowed operation
> +	 * is to compare these pointers with current io being processed. If they
> +	 * match, the block device queue should be unplugged.
> +	 */
> +	struct dm_crypt_io *io_queue_last_io;
> +	struct dm_crypt_io *crypt_queue_last_io;

We can now process ios in parallel on different CPUs, that code will
rewrite the context struct on every IO and at least it will cause cache bouncing.
(it will not apply on current code anyway, see patches in linux-next)

And I think it can kill performance is some cases (if the requests are submitted
such way that you will call unplug after every io.)

We need something clever here - I wonder if fixing the reverted fb1e753 can
help here....

Milan

Re: [dm-devel] [PATCH] PROBLEM: SSD access time with dm-crypt is way too high

[Christoph Hellwig]

On Mon, Jan 10, 2011 at 12:19:25PM -0500, Mikulas Patocka wrote:
> There are two approaches that I considered:
> 
> 1. make a special work for unplug. After queuing each IO's work, cancel the
> unplug work and queue it again (so that it will always be queued as a last
> entry). Unfortunatelly, canceling a work is rather slow operation so I decided
> to not use this approach.
> 
> 2. use a special pointer that points to the last IO. When the IO is finished
> and the pointer matches this IO, we know that it was the last IO and we should
> unplug. This patch implements this approach.

What about keeping a reference count of pending I/O requests in a shared
structure, with the last decrement doing the unplug?

Re: [PATCH] PROBLEM: SSD access time with dm-crypt is way too high

[Michael Zugelder]

On Mon, 2011-01-10 at 12:19 -0500, Mikulas Patocka wrote: 
> Try this patch.

Hi,

just compiled it on top of 2.6.37 without the fb1e753 and ran seeker [1]
for 100 seconds:

raw disk:       0.2173ms
fb1e753 patch:  0.2643ms
your patch:	0.2676ms

Throughput is within 5% of max, even with the 10ms access time.

On Mon, 2011-01-10 at 18:50 +0100, Milan Broz wrote:
> We can now process ios in parallel on different CPUs, that code will
> rewrite the context struct on every IO and at least it will cause cache bouncing.

I should say that those numbers come from a CRYPTO_PCRYPT=n kernel.


Anyone got an idea for a good testcase? I tried replicating the issues
with with a loop'd file on a tmpfs and failed. Using a usb-attached HDD
also didn't work.


Michael

[1] http://www.linuxinsight.com/how_fast_is_your_disk.html

Re: PROBLEM: SSD access time with dm-crypt is way too high

[Yakov Hrebtov]

09.01.2011 18:35, Michael Zugelder wrote:
> from kernel 2.6.33 on (at least up to 2.6.37) the access time when using
> dm-crypt on SSD is way too high. I would expect it to be a bit worse
> than directly accessing the device (~0.2ms here), but currently it's at
> exactly 10ms (except a few outlines). This corresponds to CONFIG_HZ=100.

I have similar problems, but 2.6.32 is also affected for me.
Look details here: https://bugzilla.kernel.org/show_bug.cgi?id=17892