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

[Max Reitz <mreitz@redhat.com>]

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

On 2018-06-11 15:59, Peter Lieven wrote:
> 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 <pl@kamp.de>
>>> ---
>>>   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?

Yes, because the source medium does not need to be the same as the
destination (so the source may have e.g. 512-byte clusters).

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

Yes, because request_alignment is a hard requirement.  With caching, you
can send requests with any alignment, so it's 1.

pwrite_zeroes_alignment and pdiscard_alignment are described as "Optimal
alignment", so those should contain the values we/you want.  If they are
0, then you should probably fall back to opt_transfer instead of
request_alignment.

Max

> But maybe the alignments you proposed produce a better result. I will
> check that.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]