Re: [Qemu-devel] [PATCH v4 19/21] block: Make bdrv_is_allocated() byte-based

[Kevin Wolf <kwolf@redhat.com>]

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

Am 07.07.2017 um 04:55 hat Eric Blake geschrieben:
> On 07/06/2017 11:02 AM, Kevin Wolf wrote:
> 
> >> +++ b/qemu-img.c
> >> @@ -3229,6 +3229,7 @@ static int img_rebase(int argc, char **argv)
> >>          int64_t new_backing_num_sectors = 0;
> >>          uint64_t sector;
> >>          int n;
> >> +        int64_t count;
> >>          float local_progress = 0;
> >>
> >>          buf_old = blk_blockalign(blk, IO_BUF_SIZE);
> >> @@ -3276,12 +3277,14 @@ static int img_rebase(int argc, char **argv)
> >>              }
> >>
> >>              /* If the cluster is allocated, we don't need to take action */
> >> -            ret = bdrv_is_allocated(bs, sector, n, &n);
> >> +            ret = bdrv_is_allocated(bs, sector << BDRV_SECTOR_BITS,
> >> +                                    n << BDRV_SECTOR_BITS, &count);
> >>              if (ret < 0) {
> >>                  error_report("error while reading image metadata: %s",
> >>                               strerror(-ret));
> >>                  goto out;
> >>              }
> >> +            n = DIV_ROUND_UP(count, BDRV_SECTOR_SIZE);
> >>              if (ret) {
> >>                  continue;
> >>              }
> > 
> > Same thing. Sectors that are half allocated and half free must be
> > considered completely allocated if the caller can't do byte alignment.
> > Just rounding up without looking at ret isn't correct.
> 
> In reality, qemu-img rebase should probably be rewritten to exploit
> bdrv_get_block_status() instead of the weaker bdrv_is_allocated(). Right
> now, it is manually calling buffer_is_zero() on every sector to learn
> what sectors are candidates for optimizing, rather than being able to
> exploit BDRV_BLOCK_ZERO where possible

Yes, that's true, but also unrelated. As soon as you convert
bdrv_get_block_status() to byte granularity, you get the same thing
again.

Actually, having another look at this code, the solution for this
specific case is to convert the whole loop (or even function) to byte
granularity. The bdrv_is_allocated() call was the only thing that
required sectors.

> (yes, we may STILL want to call buffer_is_zero() on BDRV_BLOCK_DATA
> when we are trying to squeeze out all parts of an image that can be
> compressed, but maybe that should be an option, just as GNU dd has
> options for creating as much sparseness as possible (slow, because it
> reads data) vs. preserving existing sparseness (faster, because it
> relies on hole detection but leaves explicit written zeroes as
> non-sparse).

I'd expect that typically buffer_is_zero() is only slow if it's also
effective. Most data blocks start with non-zero data in the first few
bytes, so that's really quick. And I'm sure that checking whether a
buffer is zero is faster than doing disk I/O for the same buffer, so the
case of actual zero buffers should benefit, too. The only problem is
with cases where the buffer starts with many zeros and then ends in some
real data. I'm not sure how common this is.

> I guess there's still lots of ground for improving qemu-img, so for
> THIS series, I'll just leave it at an assertion that things are
> sector-aligned.

Sounds okay. If you want, you can add another patch to convert the
function to bytes, or we can do it later on top.

Kevin

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