Re: [Qemu-devel] [PATCH v6 09/20] parallels: Switch to .bdrv_co_block_status()

[Vladimir Sementsov-Ogievskiy]

09.12.2017 19:39, Eric Blake wrote:
> On 12/09/2017 06:31 AM, Vladimir Sementsov-Ogievskiy wrote:
> > 07.12.2017 23:30, Eric Blake wrote:
> > > We are gradually moving away from sector-based interfaces, towards
> > > byte-based.  Update the parallels driver accordingly.  Note that
> > > the internal function block_status() is still sector-based, because
> > > it is still in use by other sector-based functions; but that's okay
> > > because request_alignment is 512 as a result of those functions.
> > > For now, no optimizations are added based on the mapping hint.
> > >
> > > Signed-off-by: Eric Blake <address@hidden>
> > >
> > >    {
> > >        BDRVParallelsState *s = bs->opaque;
> > > -    int64_t offset;
> > > +    int count;
> > >
> > > +    assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
> > >        qemu_co_mutex_lock(&s->lock);
> > > -    offset = block_status(s, sector_num, nb_sectors, pnum);
> > > +    offset = block_status(s, offset >> BDRV_SECTOR_BITS,
> > > +                          bytes >> BDRV_SECTOR_BITS, &count);
> > >        qemu_co_mutex_unlock(&s->lock);
> > >
> > > +    *pnum = count * BDRV_SECTOR_SIZE;
> > >        if (offset < 0) {
> > >            return 0;
> > >        }
> > >
> > > +    *map = offset;
> > oh, didn't notice previous time :(, should be
> >
> > *map = offset * BDRV_SECTOR_SIZE
> Oh, right.
>
> > (also, if you already use ">> BDRV_SECTOR_BITS" in this function,
> > would not it be more consistent to use "<< BDRV_SECTOR_BITS"
> > for sector-to-byte conversion?)
> >
> > with that fixed (with "<<" or "*", up to you),
> '>> BDRV_SECTOR_BITS' always works.  You could also write '/
> BDRV_SECTOR_SIZE' (the compiler should figure out that you are dividing
> by a power of 2, and use the shift instruction under the hood), but I
> find that a bit harder to reason about.
>
> On the other hand, '<< BDRV_SECTOR_BITS' only produces the same size
> output as the input; if the left side is 32 bits, it risks overflowing.
> But '* BDRV_SECTOR_SIZE' always produces a 64-bit value.  So I've
> learned (from past mistakes in other byte-conversion series) that the
> multiply form is less likely to introduce unintended truncation bugs.

hm, now I doubt. I've wrote tiny program:

#include <stdint.h>
#include <stdio.h>

int main() {
    uint32_t blocks = 1 << 20;
    int block_bits = 15;
    uint32_t block_size = 1 << block_bits;
    uint64_t a = blocks * block_size;
    uint64_t b = blocks << block_bits;
    uint64_t c = (uint64_t)blocks * block_size;
    uint64_t d = (uint64_t)blocks << block_bits;
    printf("%lx %lx %lx %lx\n", a, b, c, d);
    return 0;
}


and it prints
0 0 800000000 800000000
for me. So, no difference between * and <<...

> > Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


--
Best regards,
Vladimir