| Message ID | 1478819426-7564-3-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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);
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(-)