Re: [Qemu-block] [PATCH] qemu-img: align is_allocated_sectors to 4k

[Peter Lieven]

Am 11.06.2018 um 15:30 schrieb Max Reitz:
> On 2018-06-07 14:46, Peter Lieven wrote:
> > We currently don't enforce that the sparse segments we detect during convert are
> > aligned. This leads to unnecessary and costly read-modify-write cycles either
> > internally in Qemu or in the background on the storage device as nearly all
> > modern filesystems or hardware has a 4k alignment internally.
> >
> > As we per default set the min_sparse size to 4k it makes perfectly sense to 
> > ensure
> > that these sparse holes in the file are placed at 4k boundaries.
> >
> > The number of RMW cycles when converting an example image [1] to a raw device 
> > that
> > has 4k sector size is about 4600 4k read requests to perform a total of about 
> > 15000
> > write requests. With this path the 4600 additional read requests are eliminated.
> >
> > [1] 
> > https://cloud-images.ubuntu.com/releases/16.04/release/ubuntu-16.04-server-cloudimg-amd64-disk1.vmdk
> >
> > Signed-off-by: Peter Lieven <address@hidden>
> > ---
> >   qemu-img.c | 21 +++++++++++++++------
> >   1 file changed, 15 insertions(+), 6 deletions(-)
> I like the idea, but it doesn't seem guaranteed that
> is_allocated_sectors() is called on aligned offsets, so this alignment
> work may still leave things unaligned.

I can't image why this should happen. As long as the alignment devides the 
buffer size we either
write or skip aligned bytes. Maybe get_block_status returns an unaligned number 
of sectors?

> Furthermore, we should probably not blindly assume 4k but instead use
> some block limit of the target, like pwrite_zeroes_alignment, or
> pdiscard_alignment, depending on the case.  (Or probably still
> min_sparse, if that's less.)
>
> Since is_allocated_sectors_min() (the only caller of
> is_allocated_sectors()) is called from just a single place, taking those
> factors into account should be possible.

I also thought of this, but for instance for raw-posix I always get a 
request_alignment of 1.
But maybe the alignments you proposed produce a better result. I will check 
that.

Thanks,
Peter