On 06/24/2016 12:43 AM, Fam Zheng wrote:
> On Thu, 06/23 16:37, Eric Blake wrote:
>> Sector-based limits are awkward to think about; in our on-going
>> quest to move to byte-based interfaces, convert max_discard and
>> discard_alignment.  Rename them, using 'pdiscard' as an aid to
>> track which remaining discard interfaces need conversion, and so
>> that the compiler will help us catch the change in semantics
>> across any rebased code.  The BlockLimits type is now completely
>> byte-based; and in iscsi.c, sector_limits_lun2qemu() is no
>> longer needed.
>>
>> pdiscard_alignment is made unsigned (we use power-of-2 alignments
>> as bitmasks, where unsigned is easier to think about) while
>> leaving max_pdiscard signed (since we still have an 'int'
>> interface); this is comparable to what commit cf081fc did for
>> write zeroes limits.  We may later want to make everything an
>> unsigned 64-bit limit - but that requires a bigger code audit.
>>

>> -    /* optimal alignment for discard requests in sectors */
>> -    int64_t discard_alignment;
>> +    /* optimal alignment for discard requests in bytes, must be power
>> +     * of 2, less than max_discard if that is set, and multiple of
> 
> s/max_discard/max_pdiscard/

Maintainer could touch it up on pull request.


>>          /* align request */
>> -        if (bs->bl.discard_alignment &&
>> -            num >= bs->bl.discard_alignment &&
>> -            sector_num % bs->bl.discard_alignment) {
>> -            if (num > bs->bl.discard_alignment) {
>> -                num = bs->bl.discard_alignment;
>> +        if (discard_alignment &&
>> +            num >= discard_alignment &&
>> +            sector_num % discard_alignment) {
>> +            if (num > discard_alignment) {
>> +                num = discard_alignment;
>>              }
>> -            num -= sector_num % bs->bl.discard_alignment;
>> +            num -= sector_num % discard_alignment;
> 
> Or just
> 
>                num = discard_alignment - sector_num % discard_alignment;
> 
> without the if.
> 

Sure. It all gets simplified later when I switch to bdrv_co_pdiscard().
 Up to the maintainer.

> Otherwise looks good,
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>
> 


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