Re: [Qemu-devel] [PATCH v3 17/22] block: Switch discard length bounds to byte-based

[Kevin Wolf <kwolf@redhat.com>]

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

Am 24.06.2016 um 16:15 hat Eric Blake geschrieben:
> 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.

Okay, no problem.

> >>          /* 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.

This is an actual code change and not a bug fix, so I'll leave this one
alone. We can always have a follow-up patch, but as you say your other
work will simplify it anyway, I guess that's not necessary.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]