Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] block: Add blk_truncate_for_formatting()

[Maxim Levitsky <mlevitsk@redhat.com>]

On Fri, 2019-07-12 at 19:35 +0200, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/sysemu/block-backend.h | 12 ++++++++
>  block/block-backend.c          | 54 ++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index 733c4957eb..cd9ec8bf52 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -236,6 +236,18 @@ int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
>                            int bytes);
>  int blk_truncate(BlockBackend *blk, int64_t offset, PreallocMode prealloc,
>                   Error **errp);
> +
> +/**
> + * Wrapper of blk_truncate() for format drivers that need to truncate
> + * their protocol node before formatting it.
> + * Invoke blk_truncate() to truncate the file to @offset; if that
> + * fails with -ENOTSUP (and the file is already big enough), try to
> + * overwrite the first sector with zeroes.  If that succeeds, return
> + * success.
> + */
> +int blk_truncate_for_formatting(BlockBackend *blk, int64_t offset,
> +                                Error **errp);
> +
>  int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes);
>  int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
>                       int64_t pos, int size);
> diff --git a/block/block-backend.c b/block/block-backend.c
> index a8d160fd5d..c0e64b1ee1 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -2041,6 +2041,60 @@ int blk_truncate(BlockBackend *blk, int64_t offset, PreallocMode prealloc,
>      return bdrv_truncate(blk->root, offset, prealloc, errp);
>  }
>  
> +int blk_truncate_for_formatting(BlockBackend *blk, int64_t offset, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    int64_t current_size;
> +    int bytes_to_clear;
> +    int ret;
> +
> +    ret = blk_truncate(blk, offset, PREALLOC_MODE_OFF, &local_err);
> +    if (ret < 0 && ret != -ENOTSUP) {
> +        error_propagate(errp, local_err);
> +        return ret;
> +    } else if (ret >= 0) {
> +        return ret;
> +    }

What if the truncate does succeed? For example the current implementation of raw_co_truncate,
does return zero when you truncate to less that block device size 
(and this is kind of wrong since you can't really change the block device size)

Even more, I see is that in the later patch, you call this with offset == 0 which
I think will always succeed on a raw block device, thus skipping the zeroing code.

How about just doing the zeroing in the bdrv_create_file_fallback?


Another idea:

blk_truncate_for_formatting would first truncate the file to 0, then
check if the size of the file became zero in addition to the successful return value.

If the file size became zero, truncate the file to the requested size - this should make sure that file is empty.
Otherwise, zero the first sector.

It might also be nice to add a check that if the size didn't became zero, that it remained the same
to avoid strange situations of semi broken truncate.


Also I would rename the function to something like blk_raw_format_file,
basically a function which tries its best to erase an existing file contents


Yet another idea would to drop the lying in the raw_co_truncate (on block devices), and fail always,
unless asked to truncate to the exact file size, and let the callers deal with that.
Callers where it is not critical for the truncate to work can just ignore this failure.
That is probably hard to implement 

Or we can add a truncate 'mode' to .bdrv_co_truncate, which would let the caller indicate its intention,
that is if the caller must truncate to that size or it can accept truncate ending up in bigger file that it asked for. 

As we once discussed on IRC, the fact that truncate on a block device 'succeeds',
despite not really beeing able to change the block device size, causes other issues,
like not beeing able to use preallocation=full when creating a qcow2 image on a block device.

Best regards,
	Maxim Levitsky

> +
> +    current_size = blk_getlength(blk);
> +    if (current_size < 0) {
> +        error_free(local_err);
> +        error_setg_errno(errp, -current_size,
> +                         "Failed to inquire new image file's current length");
> +        return current_size;
> +    }
> +
> +    if (current_size < offset) {
> +        /* Need to grow the image, but we failed to do that */
> +        error_propagate(errp, local_err);
> +        return -ENOTSUP;
> +    }
> +
> +    error_free(local_err);
> +    /*
> +     * We can deal with images that are too big.  We just need to
> +     * clear the first sector.
> +     */
> +
> +    bytes_to_clear = MIN(current_size, BDRV_SECTOR_SIZE) - offset;
> +    if (bytes_to_clear) {
> +        if (!(blk->root->perm & BLK_PERM_WRITE)) {
> +            error_setg(errp, "Cannot clear first sector of new image: "
> +                       "Write permission missing");
> +            return -EPERM;
> +        }
> +
> +        ret = blk_pwrite_zeroes(blk, offset, bytes_to_clear, 0);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "Failed to clear the first sector of "
> +                             "the new image");
> +            return ret;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static void blk_pdiscard_entry(void *opaque)
>  {
>      BlkRwCo *rwco = opaque;