[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v3 17/22] block: Switch discard length bounds to
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [PATCH v3 17/22] block: Switch discard length bounds to byte-based |
Date: |
Fri, 24 Jun 2016 16:29:51 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
pgpSlrwfwHNaY.pgp
Description: PGP signature