[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-stable] [PATCH v2 5/9] block: Pass unaligned discard requests
From: |
Max Reitz |
Subject: |
Re: [Qemu-stable] [PATCH v2 5/9] block: Pass unaligned discard requests to drivers |
Date: |
Sat, 19 Nov 2016 23:05:14 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 |
On 18.11.2016 02:13, Eric Blake wrote:
> On 11/17/2016 05:44 PM, Max Reitz wrote:
>>>
>>> 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.
>>
>> Is this series supposed to address this issue? Because if so, I fail to
>> see where it does. If the device advertises 15 MB as the discard
>> granularity, then the iscsi driver will still drop all discard requests
>> that are not aligned to 15 MB boundaries, no?
>>
>
> I don't have access to the device in question, so I'm hoping Peter
> chimes in (oops, how'd I miss him on original CC?). Here's all the more
> he said on v1:
> https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg02223.html
>
> My gut feel, however, is that the iscsi code is NOT rounding
Why are you relying on your gut feel when you can simply look into the
code in question? As a matter of fact, I know that you have looked at
the very piece of code in question because you touch it in patch 4.
Now that I looked at it again, though, I see my mistake, though: I
assumed that is_byte_request_lun_aligned() would check whether the
request is aligned to the advertised discard granularity. Of course, it
does not, though, it only checks whether it's aligned to the device's
block_size.
So with the device in question, block_size is something like, say, 1 MB
and the pdiscard_alignment is 15 MB. With your series, the block layer
will first split off the head of an unaligned discard request (with the
rest being aligned to the pdiscard_alignment) and then again the head
off that head (with regards to the request_alignment).
The iscsi driver will discard the head of the head (which isn't aligned
to the request_alignment), but pass the part of the head that is aligned
to request_alignment and of course pass everything that's aligned to
pdiscard_alignment, too.
(And the same then happens for the tail.)
OK, now I see.
> (the qcow2
> code rounded, but that's different); the regression happened in 2.7
> because the block layer also started rounding, and this patch gets the
> block layer rounding out of the way. If nothing changed in the iscsi
> code in the meantime, then the iscsi code should now (once again) be
> discarding all sizes, regardless of the 15M advertisement.
Well, all sizes that are aligned to the request_alignment.
> Meanwhile, I _did_ test this patch with blkdebug (see 9/9), as well as
> on a slightly modified NBD client that forced 15M alignments, and for
> those cases, it definitely made the difference on whether all bytes were
> passed (spread across several calls), vs. just the aligned bytes in the
> middle of a request larger than 15M.
>
>> The only difference is that it's now the iscsi driver that drops the
>> request instead of the generic block layer.
>
> If the iscsi driver was ever dropping it in the first place.
It wasn't dropping them, it was asserting that it was dropped; yes, my
mistake for thinking that is_byte_request_lun_aligned() would check
whether the request is aligned to pdiscard_alignment.
Max
signature.asc
Description: OpenPGP digital signature