Re: [Qemu-block] [PATCH v2 5/9] block: Pass unaligned discard requests to drivers

[Max Reitz]

On 18.11.2016 00:01, Eric Blake wrote:
> On 11/17/2016 04:26 PM, Max Reitz wrote:
>> On 17.11.2016 21:13, Eric Blake wrote:
>>> 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.
>>>
> 
>>>      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 small requests to get to alignment boundaries. */
>>> +            num = MIN(count, align - head);
>>> +            if (!QEMU_IS_ALIGNED(num, bs->bl.request_alignment)) {
>>> +                num %= bs->bl.request_alignment;
>>> +            }
>>
>> Could be written as
>>
>> num = num % bs->bl.request_alignment ?: num;
>>
>> But that's up to you.
>>
>> More importantly, is it possible that request_alignment >
>> pdiscard_alignment? In that case, align would be request_alignment, head
>> would be less than request_alignment but could be more than
>> pdiscard_alignment.
> 
> pdiscard_alignment can be 0 (no inherent limit); but it if it is
> nonzero, it must be at least request_alignment.  The block layer (should
> be, if it is not already) enforcing that as part of the
> .bdrv_refresh_limits() callback contract; at any rate, it is documented
> that way in block_int.h

Yes, you're right. Thus:

Reviewed-by: Max Reitz <address@hidden>

signature.asc
Description: OpenPGP digital signature