Re: [Qemu-devel] [PATCH V4] qemu-img: align result of is_allocated_sectors

[Kevin Wolf <kwolf@redhat.com>]

Am 07.07.2018 um 13:42 hat Peter Lieven geschrieben:
> 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 have a 4k alignment internally.
> 
> This patch modifies is_allocated_sectors so that its *pnum result will always
> end at an alignment boundary. This way all requests will end at an alignment
> boundary. The start of all requests will also be aligned as long as the results
> of get_block_status do not lead to an unaligned offset.
> 
> 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 additional 4600 read requests are eliminated while
> the number of total write requests stays constant.
> 
> [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>
> ---
> V3->V4: - only focus on the end offset in is_allocated_sectors [Kevin]
> V2->V3: - ensure that s.alignment is a power of 2
>         - correctly handle n < alignment in is_allocated_sectors if
>           sector_num % alignment > 0.
> V1->V2: - take the current sector offset into account [Max]
>         - try to figure out the target alignment [Max]
> 
>  qemu-img.c | 44 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index e1a506f..20e3236 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1105,11 +1105,15 @@ static int64_t find_nonzero(const uint8_t *buf, int64_t n)
>   *
>   * 'pnum' is set to the number of sectors (including and immediately following
>   * the first one) that are known to be in the same allocated/unallocated state.
> + * The function will try to align the end offset to alignment boundaries so
> + * that the request will at least end aligned and consequtive requests will
> + * also start at an aligned offset.
>   */
> -static int is_allocated_sectors(const uint8_t *buf, int n, int *pnum)
> +static int is_allocated_sectors(const uint8_t *buf, int n, int *pnum,
> +                                int64_t sector_num, int alignment)
>  {
>      bool is_zero;
> -    int i;
> +    int i, tail;
>  
>      if (n <= 0) {
>          *pnum = 0;
> @@ -1122,6 +1126,23 @@ static int is_allocated_sectors(const uint8_t *buf, int n, int *pnum)
>              break;
>          }
>      }
> +
> +    tail = (sector_num + i) & (alignment - 1);
> +    if (tail) {
> +        if (is_zero && i == tail) {

Should this be i <= tail for the case where sector_num is unaligned?

For example:

    Bytes 0     - 1024:     zero
    Bytes 1024  - 4096:     non-zero

    /* Check from 512 to 4096, alignment 2048 */
    is_allocated_sectors(buf, 7, &pnum, 1, 4)

    -> is_zero = true
    -> i = 1
    -> tail = (sector_num + i) & (alignment - 1)
            = (1 + 1) & (4 - 1)
            = 2
            != i

> +            /* treat unallocated areas which only consist
> +             * of a small tail as allocated. */
> +            is_zero = 0;

(This should be false rather than 0, is_zero is a bool)

> +        }
> +        if (!is_zero) {
> +            /* align up end offset of allocated areas. */
> +            i += alignment - tail;
> +            i = MIN(i, n);
> +        } else {
> +            /* align down end offset of zero areas. */
> +            i -= tail;

So our example above will end up in this branch and we get:

    i = i - tail
      = 1 - 2
      = -1

I'm not sure what callers will do with a negative *pnum, but I expect it
won't be anything good.

Kevin