Re: [PATCH v2 4/4] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3292 bytes --]

On 09.01.20 20:13, Alberto Garcia wrote:
> This replaces all remaining instances in the qcow2 code.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2-cluster.c |  2 +-
>  block/qcow2.c         | 10 ++++++----
>  2 files changed, 7 insertions(+), 5 deletions(-)

The question of course is why we would even have such instances still.

> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 932fc48919..777ca2d409 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -219,7 +219,7 @@ static int l2_load(BlockDriverState *bs, uint64_t offset,
>   * Writes one sector of the L1 table to the disk (can't update single entries
>   * and we really don't want bdrv_pread to perform a read-modify-write)
>   */
> -#define L1_ENTRIES_PER_SECTOR (512 / 8)
> +#define L1_ENTRIES_PER_SECTOR (BDRV_SECTOR_SIZE / 8)
>  int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index)

Here it’s because the comment is wrong: “Can’t update single entries” –
yes, we can.  We’d just have to do a bdrv_pwrite() to a single entry.

>  {
>      BDRVQcow2State *s = bs->opaque;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 783d2b9578..c0f3e715ef 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3280,7 +3280,8 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
>  
>      /* Validate options and set default values */
>      if (!QEMU_IS_ALIGNED(qcow2_opts->size, BDRV_SECTOR_SIZE)) {
> -        error_setg(errp, "Image size must be a multiple of 512 bytes");
> +        error_setg(errp, "Image size must be a multiple of %u bytes",
> +                   (unsigned) BDRV_SECTOR_SIZE);

Either way how patch 1 goes, this is due to a limitation in qemu.

>          ret = -EINVAL;
>          goto out;
>      }
> @@ -3836,7 +3837,7 @@ qcow2_co_copy_range_from(BlockDriverState *bs,
>          case QCOW2_CLUSTER_NORMAL:
>              child = s->data_file;
>              copy_offset += offset_into_cluster(s, src_offset);
> -            if ((copy_offset & 511) != 0) {
> +            if (!QEMU_IS_ALIGNED(copy_offset, BDRV_SECTOR_SIZE)) {

Hm.  I don’t get this one.

>                  ret = -EIO;
>                  goto out;
>              }
> @@ -3958,8 +3959,9 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>          return -ENOTSUP;
>      }
>  
> -    if (offset & 511) {
> -        error_setg(errp, "The new size must be a multiple of 512");
> +    if (!QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE)) {
> +        error_setg(errp, "The new size must be a multiple of %u",
> +                   (unsigned) BDRV_SECTOR_SIZE);

This too is due to the limitation in qemu that BDS lengths are only
stored in multiples of BDRV_SECTOR_SIZE, so we disallow any other image
sizes.

>          return -EINVAL;
>      }
So in 3/4 instances this patch looks good to me (it’s really about the
block layers concept of a “sector”, even though that notion is outdated
in the first hunk), but I don’t quite get the third hunk.  (I’m sure it
has to do something with the block layer, so it’s OK to change in this
patch, but I’m still interested in why we have that limitation there.)

Max


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