[Qemu-devel] [PATCH for-2.8 0/2] pass partial discard requests all the way down

[Qemu-devel] [PATCH for-2.8 0/2] pass partial discard requests all the way down

[Eric Blake]

Peter reported a mild regression in qemu 2.7 when targetting the
Dell Equallogic iSCSI, which advertizes a preferred and maximum
unmap alignment at 15M.  In qemu 2.6, trims not aligned to those
boundaries still made it to the device, but in 2.7, the block
layer is ignoring unaligned portions of guest requests, resulting
in fewer blocks being actually trimmed.

Since discard is advisary, it is borderline if this counts as a
bug fix worthy for inclusion in 2.8, or if it should be deferred
to 2.9.  At any rate, while I was able to test that the patch
didn't misbehave for me when I tweaked an NBD setup to force
15M alignment, I am unable to test that it makes an actual
difference for Peter's hardware, and that needs to happen before
anyone even thinks of applying this.

Eric Blake (2):
  block: Return -ENOTSUP rather than assert on unaligned discards
  block: Pass unaligned discard requests to drivers

 block/io.c       | 36 +++++++++++++++++++++++-------------
 block/iscsi.c    |  4 +++-
 block/qcow2.c    |  4 ++++
 block/sheepdog.c |  5 +++--
 4 files changed, 33 insertions(+), 16 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] block: Return -ENOTSUP rather than assert on unaligned discards

[Eric Blake]

Right now, the block layer rounds discard requests, so that
individual drivers are able to assert that discard requests
will never be unaligned.  But there are some ISCSI devices
that track and coalesce multiple unaligned requests, turning it
into an actual discard if the requests eventually cover an
entire page, which implies that it is better to always pass
discard requests as low down the stack as possible.

In isolation, this patch has no semantic effect, since the
block layer currently never passes an unaligned request down.
But the block layer already has code that silently ignores
drivers that return -ENOTSUP for a discard request that cannot
be honored (as well as drivers that return 0 even when nothing
was done).  So the next patch will update the block layer to
fragment discard requests, so that clients are guaranteed that
they are either dealing with an unaligned head or tail, or an
aligned body, of the overall request, making it similar to the
block layer semantics of write zero fragmentation.

Signed-off-by: Eric Blake <eblake@redhat.com>
CC: qemu-stable@nongnu.org
---
 block/iscsi.c    | 4 +++-
 block/qcow2.c    | 4 ++++
 block/sheepdog.c | 5 +++--
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 71bd523..0960929 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1083,7 +1083,9 @@ coroutine_fn iscsi_co_pdiscard(BlockDriverState *bs, int64_t offset, int count)
     struct IscsiTask iTask;
     struct unmap_list list;

-    assert(is_byte_request_lun_aligned(offset, count, iscsilun));
+    if (!is_byte_request_lun_aligned(offset, count, iscsilun)) {
+        return -ENOTSUP;
+    }

     if (!iscsilun->lbp.lbpu) {
         /* UNMAP is not supported by the target */
diff --git a/block/qcow2.c b/block/qcow2.c
index 6d5689a..9af00fd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2490,6 +2490,10 @@ static coroutine_fn int qcow2_co_pdiscard(BlockDriverState *bs,
     int ret;
     BDRVQcow2State *s = bs->opaque;

+    if (!QEMU_IS_ALIGNED(offset | count, s->cluster_size)) {
+        return -ENOTSUP;
+    }
+
     qemu_co_mutex_lock(&s->lock);
     ret = qcow2_discard_clusters(bs, offset, count >> BDRV_SECTOR_BITS,
                                  QCOW2_DISCARD_REQUEST, false);
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 1fb9173..4c9af89 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2829,8 +2829,9 @@ static coroutine_fn int sd_co_pdiscard(BlockDriverState *bs, int64_t offset,
     iov.iov_len = sizeof(zero);
     discard_iov.iov = &iov;
     discard_iov.niov = 1;
-    assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
-    assert((count & (BDRV_SECTOR_SIZE - 1)) == 0);
+    if (!QEMU_IS_ALIGNED(offset | count, BDRV_SECTOR_SIZE)) {
+        return -ENOTSUP;
+    }
     acb = sd_aio_setup(bs, &discard_iov, offset >> BDRV_SECTOR_BITS,
                        count >> BDRV_SECTOR_BITS);
     acb->aiocb_type = AIOCB_DISCARD_OBJ;
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] block: Pass unaligned discard requests to drivers

[Eric Blake]

Discard is advisory, so rounding the requests to alignment
boundaries is never semantically wrong from the data that
the guest sees.  But at least the Dell Equallogic iSCSI SANs
has an interesting property that its advertised discard
alignment is 15M, yet documents that discarding a sequence
of 1M slices will eventually result in the 15M page being
marked as discarded, and it is possible to observe which
pages have been discarded.

Between commits 9f1963b and b8d0a980, we converted the block
layer to a byte-based interface that ultimately ignores any
unaligned head or tail based on the driver's advertised
discard granularity, which means that qemu 2.7 refuses to
pass any discard request smaller than 15M down to the Dell
Equallogic hardware.  This is a slight regression in behavior
compared to earlier qemu, where a guest executing discards
in power-of-2 chunks used to be able to get every page
discarded, but is now left with various pages still allocated
because the guest requests did not align with the hardware's
15M pages.

Since the SCSI specification says nothing about a minimum
discard granularity, and only documents the preferred
alignment, it is best if the block layer gives the driver
every bit of information about discard requests, rather than
rounding it to alignment boundaries early.

Rework the block layer discard algorithm to mirror the write
zero algorithm: always peel off any unaligned head or tail
and manage that in isolation, then do the bulk of the request
on an aligned boundary.  The fallback when the driver returns
-ENOTSUP for an unaligned request is to silently ignore that
portion of the discard request; but for devices that can pass
the partial request all the way down to hardware, this can
result in the hardware coalescing requests and discarding
aligned pages after all.

Reported by: Peter Lieven <pl@kamp.de>
Signed-off-by: Eric Blake <eblake@redhat.com>
CC: qemu-stable@nongnu.org
---
 block/io.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/block/io.c b/block/io.c
index aa532a5..76c3030 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2421,7 +2421,7 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset,
 {
     BdrvTrackedRequest req;
     int max_pdiscard, ret;
-    int head, align;
+    int head, tail, align;

     if (!bs->drv) {
         return -ENOMEDIUM;
@@ -2444,19 +2444,15 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset,
         return 0;
     }

-    /* Discard is advisory, so ignore any unaligned head or tail */
+    /* Discard is advisory, but some devices track and coalesce
+     * unaligned requests, so we must pass everything down rather than
+     * round here.  Still, most devices will just silently ignore
+     * unaligned requests (by returning -ENOTSUP), so we must fragment
+     * the request accordingly.  */
     align = MAX(bs->bl.pdiscard_alignment, bs->bl.request_alignment);
     assert(align % bs->bl.request_alignment == 0);
     head = offset % align;
-    if (head) {
-        head = MIN(count, align - head);
-        count -= head;
-        offset += head;
-    }
-    count = QEMU_ALIGN_DOWN(count, align);
-    if (!count) {
-        return 0;
-    }
+    tail = (offset + count) % align;

     bdrv_inc_in_flight(bs);
     tracked_request_begin(&req, bs, offset, count, BDRV_TRACKED_DISCARD);
@@ -2468,11 +2464,25 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset,

     max_pdiscard = QEMU_ALIGN_DOWN(MIN_NON_ZERO(bs->bl.max_pdiscard, INT_MAX),
                                    align);
-    assert(max_pdiscard);
+    assert(max_pdiscard >= bs->bl.request_alignment);

     while (count > 0) {
         int ret;
-        int num = MIN(count, max_pdiscard);
+        int num = count;
+
+        if (head) {
+            /* Make a small request up to the first aligned sector. */
+            num = MIN(count, align - head);
+            head = (head + num) % align;
+            assert(num < max_pdiscard);
+        } else if (tail && num > align) {
+            /* Shorten the request to the last aligned sector.  */
+            num -= tail;
+        }
+        /* limit request size */
+        if (num > max_pdiscard) {
+            num = max_pdiscard;
+        }

         if (bs->drv->bdrv_co_pdiscard) {
             ret = bs->drv->bdrv_co_pdiscard(bs, offset, num);
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.8 0/2] pass partial discard requests all the way down

[Laszlo Ersek]

On 11/11/16 00:10, Eric Blake wrote:
> Peter reported a mild regression in qemu 2.7 when targetting the
> Dell Equallogic iSCSI, which advertizes a preferred and maximum
> unmap alignment at 15M.  In qemu 2.6, trims not aligned to those
> boundaries still made it to the device, but in 2.7, the block
> layer is ignoring unaligned portions of guest requests, resulting
> in fewer blocks being actually trimmed.
> 
> Since discard is advisary,

advisory?

> it is borderline if this counts as a
> bug fix worthy for inclusion in 2.8, or if it should be deferred
> to 2.9.  At any rate, while I was able to test that the patch
> didn't misbehave for me when I tweaked an NBD setup to force
> 15M alignment, I am unable to test that it makes an actual
> difference for Peter's hardware, and that needs to happen before
> anyone even thinks of applying this.
> 
> Eric Blake (2):
>   block: Return -ENOTSUP rather than assert on unaligned discards
>   block: Pass unaligned discard requests to drivers
> 
>  block/io.c       | 36 +++++++++++++++++++++++-------------
>  block/iscsi.c    |  4 +++-
>  block/qcow2.c    |  4 ++++
>  block/sheepdog.c |  5 +++--
>  4 files changed, 33 insertions(+), 16 deletions(-)
> 

Re: [Qemu-devel] [PATCH for-2.8 0/2] pass partial discard requests all the way down

[Laszlo Ersek]

On 11/11/16 11:09, Laszlo Ersek wrote:
> On 11/11/16 00:10, Eric Blake wrote:
>> Peter reported a mild regression in qemu 2.7 when targetting the
>> Dell Equallogic iSCSI, which advertizes a preferred and maximum
>> unmap alignment at 15M.  In qemu 2.6, trims not aligned to those
>> boundaries still made it to the device, but in 2.7, the block
>> layer is ignoring unaligned portions of guest requests, resulting
>> in fewer blocks being actually trimmed.
>>
>> Since discard is advisary,
> 
> advisory?

Ehh, sorry, I missed that this msg wouldn't be committed; it's just the
blurb for the series. The actual commits and the code have the right
spelling. Not gratuitously trying to be a smart-ass...

Laszlo

>> it is borderline if this counts as a
>> bug fix worthy for inclusion in 2.8, or if it should be deferred
>> to 2.9.  At any rate, while I was able to test that the patch
>> didn't misbehave for me when I tweaked an NBD setup to force
>> 15M alignment, I am unable to test that it makes an actual
>> difference for Peter's hardware, and that needs to happen before
>> anyone even thinks of applying this.
>>
>> Eric Blake (2):
>>   block: Return -ENOTSUP rather than assert on unaligned discards
>>   block: Pass unaligned discard requests to drivers
>>
>>  block/io.c       | 36 +++++++++++++++++++++++-------------
>>  block/iscsi.c    |  4 +++-
>>  block/qcow2.c    |  4 ++++
>>  block/sheepdog.c |  5 +++--
>>  4 files changed, 33 insertions(+), 16 deletions(-)
>>
> 

Re: [Qemu-devel] [Qemu-stable] [PATCH 2/2] block: Pass unaligned discard requests to drivers

[Peter Lieven]

Am 11.11.2016 um 00:10 schrieb Eric Blake:
> Discard is advisory, so rounding the requests to alignment
> boundaries is never semantically wrong from the data that
> the guest sees.  But at least the Dell Equallogic iSCSI SANs
> has an interesting property that its advertised discard
> alignment is 15M, yet documents that discarding a sequence
> of 1M slices will eventually result in the 15M page being
> marked as discarded, and it is possible to observe which
> pages have been discarded.
>
> Between commits 9f1963b and b8d0a980, we converted the block
> layer to a byte-based interface that ultimately ignores any
> unaligned head or tail based on the driver's advertised
> discard granularity, which means that qemu 2.7 refuses to
> pass any discard request smaller than 15M down to the Dell
> Equallogic hardware.  This is a slight regression in behavior
> compared to earlier qemu, where a guest executing discards
> in power-of-2 chunks used to be able to get every page
> discarded, but is now left with various pages still allocated
> because the guest requests did not align with the hardware's
> 15M pages.
>
> Since the SCSI specification says nothing about a minimum
> discard granularity, and only documents the preferred
> alignment, it is best if the block layer gives the driver
> every bit of information about discard requests, rather than
> rounding it to alignment boundaries early.
>
> Rework the block layer discard algorithm to mirror the write
> zero algorithm: always peel off any unaligned head or tail
> and manage that in isolation, then do the bulk of the request
> on an aligned boundary.  The fallback when the driver returns
> -ENOTSUP for an unaligned request is to silently ignore that
> portion of the discard request; but for devices that can pass
> the partial request all the way down to hardware, this can
> result in the hardware coalescing requests and discarding
> aligned pages after all.
>
> Reported by: Peter Lieven <pl@kamp.de>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> CC: qemu-stable@nongnu.org
> ---
>  block/io.c | 36 +++++++++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index aa532a5..76c3030 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2421,7 +2421,7 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset,
>  {
>      BdrvTrackedRequest req;
>      int max_pdiscard, ret;
> -    int head, align;
> +    int head, tail, align;
>
>      if (!bs->drv) {
>          return -ENOMEDIUM;
> @@ -2444,19 +2444,15 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset,
>          return 0;
>      }
>
> -    /* Discard is advisory, so ignore any unaligned head or tail */
> +    /* Discard is advisory, but some devices track and coalesce
> +     * unaligned requests, so we must pass everything down rather than
> +     * round here.  Still, most devices will just silently ignore
> +     * unaligned requests (by returning -ENOTSUP), so we must fragment
> +     * the request accordingly.  */
>      align = MAX(bs->bl.pdiscard_alignment, bs->bl.request_alignment);
>      assert(align % bs->bl.request_alignment == 0);
>      head = offset % align;
> -    if (head) {
> -        head = MIN(count, align - head);
> -        count -= head;
> -        offset += head;
> -    }
> -    count = QEMU_ALIGN_DOWN(count, align);
> -    if (!count) {
> -        return 0;
> -    }
> +    tail = (offset + count) % align;
>
>      bdrv_inc_in_flight(bs);
>      tracked_request_begin(&req, bs, offset, count, BDRV_TRACKED_DISCARD);
> @@ -2468,11 +2464,25 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset,
>
>      max_pdiscard = QEMU_ALIGN_DOWN(MIN_NON_ZERO(bs->bl.max_pdiscard, INT_MAX),
>                                     align);
> -    assert(max_pdiscard);
> +    assert(max_pdiscard >= bs->bl.request_alignment);
>
>      while (count > 0) {
>          int ret;
> -        int num = MIN(count, max_pdiscard);
> +        int num = count;
> +
> +        if (head) {
> +            /* Make a small request up to the first aligned sector. */
> +            num = MIN(count, align - head);
> +            head = (head + num) % align;

Is there any way that head is != 0 after this?

Peter

Re: [Qemu-devel] [PATCH for-2.8 0/2] pass partial discard requests all the way down

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 995 bytes --]

On 11/11/2016 04:11 AM, Laszlo Ersek wrote:
> On 11/11/16 11:09, Laszlo Ersek wrote:
>> On 11/11/16 00:10, Eric Blake wrote:
>>> Peter reported a mild regression in qemu 2.7 when targetting the
>>> Dell Equallogic iSCSI, which advertizes a preferred and maximum
>>> unmap alignment at 15M.  In qemu 2.6, trims not aligned to those
>>> boundaries still made it to the device, but in 2.7, the block
>>> layer is ignoring unaligned portions of guest requests, resulting
>>> in fewer blocks being actually trimmed.
>>>
>>> Since discard is advisary,
>>
>> advisory?
> 
> Ehh, sorry, I missed that this msg wouldn't be committed; it's just the
> blurb for the series. The actual commits and the code have the right
> spelling. Not gratuitously trying to be a smart-ass...

On the other hand, it's not every day you get to call me out on a typo,
so have fun doing it :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [Qemu-stable] [PATCH 2/2] block: Pass unaligned discard requests to drivers

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2458 bytes --]

On 11/11/2016 04:58 AM, Peter Lieven wrote:
> Am 11.11.2016 um 00:10 schrieb Eric Blake:
>> Discard is advisory, so rounding the requests to alignment
>> boundaries is never semantically wrong from the data that
>> the guest sees.  But at least the Dell Equallogic iSCSI SANs
>> has an interesting property that its advertised discard
>> alignment is 15M, yet documents that discarding a sequence
>> of 1M slices will eventually result in the 15M page being
>> marked as discarded, and it is possible to observe which
>> pages have been discarded.
>>

>> @@ -2468,11 +2464,25 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset,
>>
>>      max_pdiscard = QEMU_ALIGN_DOWN(MIN_NON_ZERO(bs->bl.max_pdiscard, INT_MAX),
>>                                     align);
>> -    assert(max_pdiscard);
>> +    assert(max_pdiscard >= bs->bl.request_alignment);
>>
>>      while (count > 0) {
>>          int ret;
>> -        int num = MIN(count, max_pdiscard);
>> +        int num = count;
>> +
>> +        if (head) {
>> +            /* Make a small request up to the first aligned sector. */
>> +            num = MIN(count, align - head);
>> +            head = (head + num) % align;
> 
> Is there any way that head is != 0 after this?

The corresponding write_zero code (after my other pending patch) is:

num = MIN(MIN(count, max_transfer), align - head);

where it is indeed possible that head is still nonzero after this.  But
you are correct that for discard, as written, head is always zero after
this assignment.

On the other hand, I'm wondering if I should additionally be prepared to
split twice: suppose you have a device with a 512 request_alignment, but
the discard request is byte-aligned.  If the device can discard 512
bytes at a time (qcow2 can, if the file was configured with 512-byte
clusters), but the request is not on a 512-byte boundary, it may make
sense to do a really small request up to request_alignment, then a
larger request up to pdiscard_alignment, before doing the bulk at proper
alignment.

Should I send a v2 along those lines?  I'm still working up a blkdebug
patch that lets us add qemu-iotests, that will be my ultimate proof of
whether I can indeed come up with a case that differs by whether I
subdivide the head into as many as two parts.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]