Patchwork [01/22] block: Move initialisation of BlockLimits to bdrv_refresh_limits()

login
register
mail settings
Submitter Kevin Wolf
Date Dec. 11, 2013, 9:08 p.m.
Message ID <1386796109-15264-2-git-send-email-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/300420/
State New
Headers show

Comments

Kevin Wolf - Dec. 11, 2013, 9:08 p.m.
This function separates filling the BlockLimits from bdrv_open(), which
allows it to call it from other operations which may change the limits
(e.g. modifications to the backing file chain or bdrv_reopen)

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                   | 21 ++++++++++++
 block/iscsi.c             | 87 ++++++++++++++++++++++++++++-------------------
 block/qcow2.c             | 11 +++++-
 block/qed.c               | 11 +++++-
 block/vmdk.c              | 22 +++++++++---
 include/block/block_int.h |  2 ++
 6 files changed, 113 insertions(+), 41 deletions(-)
Wayne Xia - Dec. 12, 2013, 2:54 a.m.
于 2013/12/12 5:08, Kevin Wolf 写道:
> This function separates filling the BlockLimits from bdrv_open(), which
> allows it to call it from other operations which may change the limits
> (e.g. modifications to the backing file chain or bdrv_reopen)
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block.c                   | 21 ++++++++++++
>   block/iscsi.c             | 87 ++++++++++++++++++++++++++++-------------------
>   block/qcow2.c             | 11 +++++-
>   block/qed.c               | 11 +++++-
>   block/vmdk.c              | 22 +++++++++---
>   include/block/block_int.h |  2 ++
>   6 files changed, 113 insertions(+), 41 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 13f001a..c32d856 100644
> --- a/block.c
> +++ b/block.c
> @@ -479,6 +479,21 @@ int bdrv_create_file(const char* filename, QEMUOptionParameter *options,
>       return ret;
>   }
> 
> +static int bdrv_refresh_limits(BlockDriverState *bs)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    memset(&bs->bl, 0, sizeof(bs->bl));
> +
> +    if (!drv) {
> +        return 0;
> +    } else if (drv->bdrv_refresh_limits) {
> +        return drv->bdrv_refresh_limits(bs);
> +    }
> +
> +    return 0;
    It seems this line can be removed.

> +}
> +
>   /*
>    * Create a uniquely-named empty temporary file.
>    * Return 0 upon success, otherwise a negative errno value.
> @@ -833,6 +848,8 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>           goto free_and_fail;
>       }
> 
> +    bdrv_refresh_limits(bs);
> +
>   #ifndef _WIN32
>       if (bs->is_temporary) {
>           assert(bs->filename[0] != '\0');
> @@ -1018,6 +1035,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>       }
>       pstrcpy(bs->backing_file, sizeof(bs->backing_file),
>               bs->backing_hd->file->filename);
> +
> +    /* Recalculate the BlockLimits with the backing file */
> +    bdrv_refresh_limits(bs);
> +
>       return 0;
>   }
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 829d444..f3ded8c 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1425,6 +1425,56 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>           task = NULL;
>       }
> 
> +#if defined(LIBISCSI_FEATURE_NOP_COUNTER)
> +    /* Set up a timer for sending out iSCSI NOPs */
> +    iscsilun->nop_timer = timer_new_ms(QEMU_CLOCK_REALTIME, iscsi_nop_timed_event, iscsilun);
> +    timer_mod(iscsilun->nop_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + NOP_INTERVAL);
> +#endif
> +
> +out:
> +    qemu_opts_del(opts);
> +    if (initiator_name != NULL) {
> +        g_free(initiator_name);
> +    }
> +    if (iscsi_url != NULL) {
> +        iscsi_destroy_url(iscsi_url);
> +    }
> +    if (task != NULL) {
> +        scsi_free_scsi_task(task);
> +    }
> +
> +    if (ret) {
> +        if (iscsi != NULL) {
> +            iscsi_destroy_context(iscsi);
> +        }
> +        memset(iscsilun, 0, sizeof(IscsiLun));
> +    }
> +    return ret;
> +}
> +
> +static void iscsi_close(BlockDriverState *bs)
> +{
> +    IscsiLun *iscsilun = bs->opaque;
> +    struct iscsi_context *iscsi = iscsilun->iscsi;
> +
> +    if (iscsilun->nop_timer) {
> +        timer_del(iscsilun->nop_timer);
> +        timer_free(iscsilun->nop_timer);
> +    }
> +    qemu_aio_set_fd_handler(iscsi_get_fd(iscsi), NULL, NULL, NULL);
> +    iscsi_destroy_context(iscsi);
> +    g_free(iscsilun->zeroblock);
> +    memset(iscsilun, 0, sizeof(IscsiLun));
> +}
> +
> +static int iscsi_refresh_limits(BlockDriverState *bs)
> +{
> +    IscsiLun *iscsilun = bs->opaque;
> +    struct scsi_task *task = NULL;
> +    int ret;
> +
> +    memset(&bs->bl, 0, sizeof(bs->bl));
> +
>       if (iscsilun->lbp.lbpu || iscsilun->lbp.lbpws) {
>           struct scsi_inquiry_block_limits *inq_bl;
>           task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
> @@ -1462,48 +1512,14 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>                                                        iscsilun);
>       }
> 
> -#if defined(LIBISCSI_FEATURE_NOP_COUNTER)
> -    /* Set up a timer for sending out iSCSI NOPs */
> -    iscsilun->nop_timer = timer_new_ms(QEMU_CLOCK_REALTIME, iscsi_nop_timed_event, iscsilun);
> -    timer_mod(iscsilun->nop_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + NOP_INTERVAL);
> -#endif
> -
> +    ret = 0;
>   out:
> -    qemu_opts_del(opts);
> -    if (initiator_name != NULL) {
> -        g_free(initiator_name);
> -    }
> -    if (iscsi_url != NULL) {
> -        iscsi_destroy_url(iscsi_url);
> -    }
>       if (task != NULL) {
>           scsi_free_scsi_task(task);
>       }
> -
> -    if (ret) {
> -        if (iscsi != NULL) {
> -            iscsi_destroy_context(iscsi);
> -        }
> -        memset(iscsilun, 0, sizeof(IscsiLun));
> -    }
>       return ret;
>   }
> 
> -static void iscsi_close(BlockDriverState *bs)
> -{
> -    IscsiLun *iscsilun = bs->opaque;
> -    struct iscsi_context *iscsi = iscsilun->iscsi;
> -
> -    if (iscsilun->nop_timer) {
> -        timer_del(iscsilun->nop_timer);
> -        timer_free(iscsilun->nop_timer);
> -    }
> -    qemu_aio_set_fd_handler(iscsi_get_fd(iscsi), NULL, NULL, NULL);
> -    iscsi_destroy_context(iscsi);
> -    g_free(iscsilun->zeroblock);
> -    memset(iscsilun, 0, sizeof(IscsiLun));
> -}
> -
>   static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
>   {
>       IscsiLun *iscsilun = bs->opaque;
> @@ -1616,6 +1632,7 @@ static BlockDriver bdrv_iscsi = {
>       .bdrv_getlength  = iscsi_getlength,
>       .bdrv_get_info   = iscsi_get_info,
>       .bdrv_truncate   = iscsi_truncate,
> +    .bdrv_refresh_limits = iscsi_refresh_limits,
> 
>   #if defined(LIBISCSI_FEATURE_IOVECTOR)
>       .bdrv_co_get_block_status = iscsi_co_get_block_status,
> diff --git a/block/qcow2.c b/block/qcow2.c
> index f29aa88..f40355c 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -718,7 +718,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>       }
> 
>       qemu_opts_del(opts);
> -    bs->bl.write_zeroes_alignment = s->cluster_sectors;
> 
>       if (s->use_lazy_refcounts && s->qcow_version < 3) {
>           error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
> @@ -751,6 +750,15 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>       return ret;
>   }
> 
> +static int qcow2_refresh_limits(BlockDriverState *bs)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +
> +    bs->bl.write_zeroes_alignment = s->cluster_sectors;
> +
> +    return 0;
> +}
> +
>   static int qcow2_set_key(BlockDriverState *bs, const char *key)
>   {
>       BDRVQcowState *s = bs->opaque;
> @@ -2268,6 +2276,7 @@ static BlockDriver bdrv_qcow2 = {
> 
>       .bdrv_change_backing_file   = qcow2_change_backing_file,
> 
> +    .bdrv_refresh_limits        = qcow2_refresh_limits,
>       .bdrv_invalidate_cache      = qcow2_invalidate_cache,
> 
>       .create_options = qcow2_create_options,
> diff --git a/block/qed.c b/block/qed.c
> index 450a1fa..d35bd1c 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -495,7 +495,6 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
>           }
>       }
> 
> -    bs->bl.write_zeroes_alignment = s->header.cluster_size >> BDRV_SECTOR_BITS;
>       s->need_check_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>                                               qed_need_check_timer_cb, s);
> 
> @@ -507,6 +506,15 @@ out:
>       return ret;
>   }
> 
> +static int bdrv_qed_refresh_limits(BlockDriverState *bs)
> +{
> +    BDRVQEDState *s = bs->opaque;
> +
> +    bs->bl.write_zeroes_alignment = s->header.cluster_size >> BDRV_SECTOR_BITS;
> +
> +    return 0;
> +}
> +
>   /* We have nothing to do for QED reopen, stubs just return
>    * success */
>   static int bdrv_qed_reopen_prepare(BDRVReopenState *state,
> @@ -1616,6 +1624,7 @@ static BlockDriver bdrv_qed = {
>       .bdrv_truncate            = bdrv_qed_truncate,
>       .bdrv_getlength           = bdrv_qed_getlength,
>       .bdrv_get_info            = bdrv_qed_get_info,
> +    .bdrv_refresh_limits      = bdrv_qed_refresh_limits,
>       .bdrv_change_backing_file = bdrv_qed_change_backing_file,
>       .bdrv_invalidate_cache    = bdrv_qed_invalidate_cache,
>       .bdrv_check               = bdrv_qed_check,
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 0734bc2..f8a387f 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -428,10 +428,6 @@ static int vmdk_add_extent(BlockDriverState *bs,
>       extent->l2_size = l2_size;
>       extent->cluster_sectors = flat ? sectors : cluster_sectors;
> 
> -    if (!flat) {
> -        bs->bl.write_zeroes_alignment =
> -            MAX(bs->bl.write_zeroes_alignment, cluster_sectors);
> -    }
>       if (s->num_extents > 1) {
>           extent->end_sector = (*(extent - 1)).end_sector + extent->sectors;
>       } else {
> @@ -886,6 +882,23 @@ fail:
>       return ret;
>   }
> 
> +
> +static int vmdk_refresh_limits(BlockDriverState *bs)
> +{
> +    BDRVVmdkState *s = bs->opaque;
> +    int i;
> +
> +    for (i = 0; i < s->num_extents; i++) {
> +        if (!s->extents[i].flat) {
> +            bs->bl.write_zeroes_alignment =
> +                MAX(bs->bl.write_zeroes_alignment,
> +                    s->extents[i].cluster_sectors);
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>   static int get_whole_cluster(BlockDriverState *bs,
>                   VmdkExtent *extent,
>                   uint64_t cluster_offset,
> @@ -1971,6 +1984,7 @@ static BlockDriver bdrv_vmdk = {
>       .bdrv_get_allocated_file_size = vmdk_get_allocated_file_size,
>       .bdrv_has_zero_init           = vmdk_has_zero_init,
>       .bdrv_get_specific_info       = vmdk_get_specific_info,
> +    .bdrv_refresh_limits          = vmdk_refresh_limits,
> 
>       .create_options               = vmdk_create_options,
>   };
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 8b132d7..c49fa6b 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -226,6 +226,8 @@ struct BlockDriver {
>       int (*bdrv_debug_resume)(BlockDriverState *bs, const char *tag);
>       bool (*bdrv_debug_is_suspended)(BlockDriverState *bs, const char *tag);
> 
> +    int (*bdrv_refresh_limits)(BlockDriverState *bs);
> +
>       /*
>        * Returns 1 if newly created images are guaranteed to contain only
>        * zeros, 0 otherwise.
>
Wayne Xia - Dec. 12, 2013, 2:57 a.m.
>> +static int bdrv_refresh_limits(BlockDriverState *bs)
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +
>> +    memset(&bs->bl, 0, sizeof(bs->bl));
>> +
>> +    if (!drv) {
>> +        return 0;
>> +    } else if (drv->bdrv_refresh_limits) {
>> +        return drv->bdrv_refresh_limits(bs);
>> +    }
>> +
>> +    return 0;
>      It seems this line can be removed.
> 
  I missed the "else if", then the patch is OK.
Peter Lieven - Dec. 12, 2013, 6:17 a.m.
Am 11.12.2013 22:08, schrieb Kevin Wolf:
> This function separates filling the BlockLimits from bdrv_open(), which
> allows it to call it from other operations which may change the limits
> (e.g. modifications to the backing file chain or bdrv_reopen)
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                   | 21 ++++++++++++
>  block/iscsi.c             | 87 ++++++++++++++++++++++++++++-------------------
>  block/qcow2.c             | 11 +++++-
>  block/qed.c               | 11 +++++-
>  block/vmdk.c              | 22 +++++++++---
>  include/block/block_int.h |  2 ++
>  6 files changed, 113 insertions(+), 41 deletions(-)
>
> diff --git a/block.c b/block.c
> index 13f001a..c32d856 100644
> --- a/block.c
> +++ b/block.c
> @@ -479,6 +479,21 @@ int bdrv_create_file(const char* filename, QEMUOptionParameter *options,
>      return ret;
>  }
>  
> +static int bdrv_refresh_limits(BlockDriverState *bs)
> +{
> +    BlockDriver *drv = bs->drv;
> +
> +    memset(&bs->bl, 0, sizeof(bs->bl));
> +
> +    if (!drv) {
> +        return 0;
> +    } else if (drv->bdrv_refresh_limits) {
> +        return drv->bdrv_refresh_limits(bs);
> +    }
> +
> +    return 0;
> +}
> +
>  /*
>   * Create a uniquely-named empty temporary file.
>   * Return 0 upon success, otherwise a negative errno value.
> @@ -833,6 +848,8 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>          goto free_and_fail;
>      }
>  
> +    bdrv_refresh_limits(bs);
> +
>  #ifndef _WIN32
>      if (bs->is_temporary) {
>          assert(bs->filename[0] != '\0');
> @@ -1018,6 +1035,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>      }
>      pstrcpy(bs->backing_file, sizeof(bs->backing_file),
>              bs->backing_hd->file->filename);
> +
> +    /* Recalculate the BlockLimits with the backing file */
> +    bdrv_refresh_limits(bs);
> +
>      return 0;
>  }
>  
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 829d444..f3ded8c 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1425,6 +1425,56 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>          task = NULL;
>      }
>  
> +#if defined(LIBISCSI_FEATURE_NOP_COUNTER)
> +    /* Set up a timer for sending out iSCSI NOPs */
> +    iscsilun->nop_timer = timer_new_ms(QEMU_CLOCK_REALTIME, iscsi_nop_timed_event, iscsilun);
> +    timer_mod(iscsilun->nop_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + NOP_INTERVAL);
> +#endif
> +
> +out:
> +    qemu_opts_del(opts);
> +    if (initiator_name != NULL) {
> +        g_free(initiator_name);
> +    }
> +    if (iscsi_url != NULL) {
> +        iscsi_destroy_url(iscsi_url);
> +    }
> +    if (task != NULL) {
> +        scsi_free_scsi_task(task);
> +    }
> +
> +    if (ret) {
> +        if (iscsi != NULL) {
> +            iscsi_destroy_context(iscsi);
> +        }
> +        memset(iscsilun, 0, sizeof(IscsiLun));
> +    }
> +    return ret;
> +}
> +
> +static void iscsi_close(BlockDriverState *bs)
> +{
> +    IscsiLun *iscsilun = bs->opaque;
> +    struct iscsi_context *iscsi = iscsilun->iscsi;
> +
> +    if (iscsilun->nop_timer) {
> +        timer_del(iscsilun->nop_timer);
> +        timer_free(iscsilun->nop_timer);
> +    }
> +    qemu_aio_set_fd_handler(iscsi_get_fd(iscsi), NULL, NULL, NULL);
> +    iscsi_destroy_context(iscsi);
> +    g_free(iscsilun->zeroblock);
> +    memset(iscsilun, 0, sizeof(IscsiLun));
> +}
> +
> +static int iscsi_refresh_limits(BlockDriverState *bs)
> +{
> +    IscsiLun *iscsilun = bs->opaque;
> +    struct scsi_task *task = NULL;
> +    int ret;
> +
> +    memset(&bs->bl, 0, sizeof(bs->bl));
> +
you do that memset in bdrv_refresh_limits already.
>      if (iscsilun->lbp.lbpu || iscsilun->lbp.lbpws) {
>          struct scsi_inquiry_block_limits *inq_bl;
>          task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
> @@ -1462,48 +1512,14 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,

iscsi_do_inquiry does a sync call to request the pages from the target.
you can only do that here if you can guarantee that iscsi_refresh_limits will
be called with no pending requests in-flight otherwise this will end in a mess.

otherwise i would propose to leave the inquiry call ins iscsi_open and just fill
the bs->bl struc tin iscsi_refresh_limits. the problem is if we make this async
we have to handle all possible I/O callbacks whiile in iscsi_refresh_limits.
i do not see how an iscsi target can change its limits while a target is mounted.

Peter

>                                                       iscsilun);
>      }
>  
> -#if defined(LIBISCSI_FEATURE_NOP_COUNTER)
> -    /* Set up a timer for sending out iSCSI NOPs */
> -    iscsilun->nop_timer = timer_new_ms(QEMU_CLOCK_REALTIME, iscsi_nop_timed_event, iscsilun);
> -    timer_mod(iscsilun->nop_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + NOP_INTERVAL);
> -#endif
> -
> +    ret = 0;
>  out:
> -    qemu_opts_del(opts);
> -    if (initiator_name != NULL) {
> -        g_free(initiator_name);
> -    }
> -    if (iscsi_url != NULL) {
> -        iscsi_destroy_url(iscsi_url);
> -    }
>      if (task != NULL) {
>          scsi_free_scsi_task(task);
>      }
> -
> -    if (ret) {
> -        if (iscsi != NULL) {
> -            iscsi_destroy_context(iscsi);
> -        }
> -        memset(iscsilun, 0, sizeof(IscsiLun));
> -    }
>      return ret;
>  }
>  
> -static void iscsi_close(BlockDriverState *bs)
> -{
> -    IscsiLun *iscsilun = bs->opaque;
> -    struct iscsi_context *iscsi = iscsilun->iscsi;
> -
> -    if (iscsilun->nop_timer) {
> -        timer_del(iscsilun->nop_timer);
> -        timer_free(iscsilun->nop_timer);
> -    }
> -    qemu_aio_set_fd_handler(iscsi_get_fd(iscsi), NULL, NULL, NULL);
> -    iscsi_destroy_context(iscsi);
> -    g_free(iscsilun->zeroblock);
> -    memset(iscsilun, 0, sizeof(IscsiLun));
> -}
> -
>  static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
>  {
>      IscsiLun *iscsilun = bs->opaque;
> @@ -1616,6 +1632,7 @@ static BlockDriver bdrv_iscsi = {
>      .bdrv_getlength  = iscsi_getlength,
>      .bdrv_get_info   = iscsi_get_info,
>      .bdrv_truncate   = iscsi_truncate,
> +    .bdrv_refresh_limits = iscsi_refresh_limits,
>  
>  #if defined(LIBISCSI_FEATURE_IOVECTOR)
>      .bdrv_co_get_block_status = iscsi_co_get_block_status,
> diff --git a/block/qcow2.c b/block/qcow2.c
> index f29aa88..f40355c 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -718,7 +718,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>  
>      qemu_opts_del(opts);
> -    bs->bl.write_zeroes_alignment = s->cluster_sectors;
>  
>      if (s->use_lazy_refcounts && s->qcow_version < 3) {
>          error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
> @@ -751,6 +750,15 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>      return ret;
>  }
>  
> +static int qcow2_refresh_limits(BlockDriverState *bs)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +
> +    bs->bl.write_zeroes_alignment = s->cluster_sectors;
> +
> +    return 0;
> +}
> +
>  static int qcow2_set_key(BlockDriverState *bs, const char *key)
>  {
>      BDRVQcowState *s = bs->opaque;
> @@ -2268,6 +2276,7 @@ static BlockDriver bdrv_qcow2 = {
>  
>      .bdrv_change_backing_file   = qcow2_change_backing_file,
>  
> +    .bdrv_refresh_limits        = qcow2_refresh_limits,
>      .bdrv_invalidate_cache      = qcow2_invalidate_cache,
>  
>      .create_options = qcow2_create_options,
> diff --git a/block/qed.c b/block/qed.c
> index 450a1fa..d35bd1c 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -495,7 +495,6 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
>          }
>      }
>  
> -    bs->bl.write_zeroes_alignment = s->header.cluster_size >> BDRV_SECTOR_BITS;
>      s->need_check_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>                                              qed_need_check_timer_cb, s);
>  
> @@ -507,6 +506,15 @@ out:
>      return ret;
>  }
>  
> +static int bdrv_qed_refresh_limits(BlockDriverState *bs)
> +{
> +    BDRVQEDState *s = bs->opaque;
> +
> +    bs->bl.write_zeroes_alignment = s->header.cluster_size >> BDRV_SECTOR_BITS;
> +
> +    return 0;
> +}
> +
>  /* We have nothing to do for QED reopen, stubs just return
>   * success */
>  static int bdrv_qed_reopen_prepare(BDRVReopenState *state,
> @@ -1616,6 +1624,7 @@ static BlockDriver bdrv_qed = {
>      .bdrv_truncate            = bdrv_qed_truncate,
>      .bdrv_getlength           = bdrv_qed_getlength,
>      .bdrv_get_info            = bdrv_qed_get_info,
> +    .bdrv_refresh_limits      = bdrv_qed_refresh_limits,
>      .bdrv_change_backing_file = bdrv_qed_change_backing_file,
>      .bdrv_invalidate_cache    = bdrv_qed_invalidate_cache,
>      .bdrv_check               = bdrv_qed_check,
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 0734bc2..f8a387f 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -428,10 +428,6 @@ static int vmdk_add_extent(BlockDriverState *bs,
>      extent->l2_size = l2_size;
>      extent->cluster_sectors = flat ? sectors : cluster_sectors;
>  
> -    if (!flat) {
> -        bs->bl.write_zeroes_alignment =
> -            MAX(bs->bl.write_zeroes_alignment, cluster_sectors);
> -    }
>      if (s->num_extents > 1) {
>          extent->end_sector = (*(extent - 1)).end_sector + extent->sectors;
>      } else {
> @@ -886,6 +882,23 @@ fail:
>      return ret;
>  }
>  
> +
> +static int vmdk_refresh_limits(BlockDriverState *bs)
> +{
> +    BDRVVmdkState *s = bs->opaque;
> +    int i;
> +
> +    for (i = 0; i < s->num_extents; i++) {
> +        if (!s->extents[i].flat) {
> +            bs->bl.write_zeroes_alignment =
> +                MAX(bs->bl.write_zeroes_alignment,
> +                    s->extents[i].cluster_sectors);
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static int get_whole_cluster(BlockDriverState *bs,
>                  VmdkExtent *extent,
>                  uint64_t cluster_offset,
> @@ -1971,6 +1984,7 @@ static BlockDriver bdrv_vmdk = {
>      .bdrv_get_allocated_file_size = vmdk_get_allocated_file_size,
>      .bdrv_has_zero_init           = vmdk_has_zero_init,
>      .bdrv_get_specific_info       = vmdk_get_specific_info,
> +    .bdrv_refresh_limits          = vmdk_refresh_limits,
>  
>      .create_options               = vmdk_create_options,
>  };
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 8b132d7..c49fa6b 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -226,6 +226,8 @@ struct BlockDriver {
>      int (*bdrv_debug_resume)(BlockDriverState *bs, const char *tag);
>      bool (*bdrv_debug_is_suspended)(BlockDriverState *bs, const char *tag);
>  
> +    int (*bdrv_refresh_limits)(BlockDriverState *bs);
> +
>      /*
>       * Returns 1 if newly created images are guaranteed to contain only
>       * zeros, 0 otherwise.
Thomas Huth - Dec. 12, 2013, 7:51 a.m.
On Thu, 12 Dec 2013 10:57:49 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> 
> >> +static int bdrv_refresh_limits(BlockDriverState *bs)
> >> +{
> >> +    BlockDriver *drv = bs->drv;
> >> +
> >> +    memset(&bs->bl, 0, sizeof(bs->bl));
> >> +
> >> +    if (!drv) {
> >> +        return 0;
> >> +    } else if (drv->bdrv_refresh_limits) {
> >> +        return drv->bdrv_refresh_limits(bs);
> >> +    }
> >> +
> >> +    return 0;
> >      It seems this line can be removed.
> > 
>   I missed the "else if", then the patch is OK.
 
But it could also be written in a shorter way:

    if (drv && drv->bdrv_refresh_limits) {
        return drv->bdrv_refresh_limits(bs);
    }

    return 0;

 Regards,
  Thomas
Kevin Wolf - Dec. 12, 2013, 9:32 a.m.
Am 12.12.2013 um 08:51 hat Thomas Huth geschrieben:
> On Thu, 12 Dec 2013 10:57:49 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> 
> > 
> > >> +static int bdrv_refresh_limits(BlockDriverState *bs)
> > >> +{
> > >> +    BlockDriver *drv = bs->drv;
> > >> +
> > >> +    memset(&bs->bl, 0, sizeof(bs->bl));
> > >> +
> > >> +    if (!drv) {
> > >> +        return 0;
> > >> +    } else if (drv->bdrv_refresh_limits) {
> > >> +        return drv->bdrv_refresh_limits(bs);
> > >> +    }
> > >> +
> > >> +    return 0;
> > >      It seems this line can be removed.
> > > 
> >   I missed the "else if", then the patch is OK.
>  
> But it could also be written in a shorter way:
> 
>     if (drv && drv->bdrv_refresh_limits) {
>         return drv->bdrv_refresh_limits(bs);
>     }
> 
>     return 0;

Indeed, with some code changes, this has become a bit more complicated
than necessary. I need to touch the patch anyway for Peter's comments,
so I'll change it, even though it disappears anyway later in the series.

Kevin
Kevin Wolf - Dec. 12, 2013, 9:35 a.m.
Am 12.12.2013 um 07:17 hat Peter Lieven geschrieben:
> Am 11.12.2013 22:08, schrieb Kevin Wolf:
> > This function separates filling the BlockLimits from bdrv_open(), which
> > allows it to call it from other operations which may change the limits
> > (e.g. modifications to the backing file chain or bdrv_reopen)
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>

> > +static int iscsi_refresh_limits(BlockDriverState *bs)
> > +{
> > +    IscsiLun *iscsilun = bs->opaque;
> > +    struct scsi_task *task = NULL;
> > +    int ret;
> > +
> > +    memset(&bs->bl, 0, sizeof(bs->bl));
> > +
> you do that memset in bdrv_refresh_limits already.

Right, I'll drop it here.

> >      if (iscsilun->lbp.lbpu || iscsilun->lbp.lbpws) {
> >          struct scsi_inquiry_block_limits *inq_bl;
> >          task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
> > @@ -1462,48 +1512,14 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
> 
> iscsi_do_inquiry does a sync call to request the pages from the target.
> you can only do that here if you can guarantee that iscsi_refresh_limits will
> be called with no pending requests in-flight otherwise this will end in a mess.
> 
> otherwise i would propose to leave the inquiry call ins iscsi_open and just fill
> the bs->bl struc tin iscsi_refresh_limits. the problem is if we make this async
> we have to handle all possible I/O callbacks whiile in iscsi_refresh_limits.
> i do not see how an iscsi target can change its limits while a target is mounted.

Yes, I'll just split the block in two, leaving the actual request in
iscsi_open().

Thanks for your review.

Kevin

Patch

diff --git a/block.c b/block.c
index 13f001a..c32d856 100644
--- a/block.c
+++ b/block.c
@@ -479,6 +479,21 @@  int bdrv_create_file(const char* filename, QEMUOptionParameter *options,
     return ret;
 }
 
+static int bdrv_refresh_limits(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+
+    memset(&bs->bl, 0, sizeof(bs->bl));
+
+    if (!drv) {
+        return 0;
+    } else if (drv->bdrv_refresh_limits) {
+        return drv->bdrv_refresh_limits(bs);
+    }
+
+    return 0;
+}
+
 /*
  * Create a uniquely-named empty temporary file.
  * Return 0 upon success, otherwise a negative errno value.
@@ -833,6 +848,8 @@  static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
         goto free_and_fail;
     }
 
+    bdrv_refresh_limits(bs);
+
 #ifndef _WIN32
     if (bs->is_temporary) {
         assert(bs->filename[0] != '\0');
@@ -1018,6 +1035,10 @@  int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
     }
     pstrcpy(bs->backing_file, sizeof(bs->backing_file),
             bs->backing_hd->file->filename);
+
+    /* Recalculate the BlockLimits with the backing file */
+    bdrv_refresh_limits(bs);
+
     return 0;
 }
 
diff --git a/block/iscsi.c b/block/iscsi.c
index 829d444..f3ded8c 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1425,6 +1425,56 @@  static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
         task = NULL;
     }
 
+#if defined(LIBISCSI_FEATURE_NOP_COUNTER)
+    /* Set up a timer for sending out iSCSI NOPs */
+    iscsilun->nop_timer = timer_new_ms(QEMU_CLOCK_REALTIME, iscsi_nop_timed_event, iscsilun);
+    timer_mod(iscsilun->nop_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + NOP_INTERVAL);
+#endif
+
+out:
+    qemu_opts_del(opts);
+    if (initiator_name != NULL) {
+        g_free(initiator_name);
+    }
+    if (iscsi_url != NULL) {
+        iscsi_destroy_url(iscsi_url);
+    }
+    if (task != NULL) {
+        scsi_free_scsi_task(task);
+    }
+
+    if (ret) {
+        if (iscsi != NULL) {
+            iscsi_destroy_context(iscsi);
+        }
+        memset(iscsilun, 0, sizeof(IscsiLun));
+    }
+    return ret;
+}
+
+static void iscsi_close(BlockDriverState *bs)
+{
+    IscsiLun *iscsilun = bs->opaque;
+    struct iscsi_context *iscsi = iscsilun->iscsi;
+
+    if (iscsilun->nop_timer) {
+        timer_del(iscsilun->nop_timer);
+        timer_free(iscsilun->nop_timer);
+    }
+    qemu_aio_set_fd_handler(iscsi_get_fd(iscsi), NULL, NULL, NULL);
+    iscsi_destroy_context(iscsi);
+    g_free(iscsilun->zeroblock);
+    memset(iscsilun, 0, sizeof(IscsiLun));
+}
+
+static int iscsi_refresh_limits(BlockDriverState *bs)
+{
+    IscsiLun *iscsilun = bs->opaque;
+    struct scsi_task *task = NULL;
+    int ret;
+
+    memset(&bs->bl, 0, sizeof(bs->bl));
+
     if (iscsilun->lbp.lbpu || iscsilun->lbp.lbpws) {
         struct scsi_inquiry_block_limits *inq_bl;
         task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
@@ -1462,48 +1512,14 @@  static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
                                                      iscsilun);
     }
 
-#if defined(LIBISCSI_FEATURE_NOP_COUNTER)
-    /* Set up a timer for sending out iSCSI NOPs */
-    iscsilun->nop_timer = timer_new_ms(QEMU_CLOCK_REALTIME, iscsi_nop_timed_event, iscsilun);
-    timer_mod(iscsilun->nop_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + NOP_INTERVAL);
-#endif
-
+    ret = 0;
 out:
-    qemu_opts_del(opts);
-    if (initiator_name != NULL) {
-        g_free(initiator_name);
-    }
-    if (iscsi_url != NULL) {
-        iscsi_destroy_url(iscsi_url);
-    }
     if (task != NULL) {
         scsi_free_scsi_task(task);
     }
-
-    if (ret) {
-        if (iscsi != NULL) {
-            iscsi_destroy_context(iscsi);
-        }
-        memset(iscsilun, 0, sizeof(IscsiLun));
-    }
     return ret;
 }
 
-static void iscsi_close(BlockDriverState *bs)
-{
-    IscsiLun *iscsilun = bs->opaque;
-    struct iscsi_context *iscsi = iscsilun->iscsi;
-
-    if (iscsilun->nop_timer) {
-        timer_del(iscsilun->nop_timer);
-        timer_free(iscsilun->nop_timer);
-    }
-    qemu_aio_set_fd_handler(iscsi_get_fd(iscsi), NULL, NULL, NULL);
-    iscsi_destroy_context(iscsi);
-    g_free(iscsilun->zeroblock);
-    memset(iscsilun, 0, sizeof(IscsiLun));
-}
-
 static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
 {
     IscsiLun *iscsilun = bs->opaque;
@@ -1616,6 +1632,7 @@  static BlockDriver bdrv_iscsi = {
     .bdrv_getlength  = iscsi_getlength,
     .bdrv_get_info   = iscsi_get_info,
     .bdrv_truncate   = iscsi_truncate,
+    .bdrv_refresh_limits = iscsi_refresh_limits,
 
 #if defined(LIBISCSI_FEATURE_IOVECTOR)
     .bdrv_co_get_block_status = iscsi_co_get_block_status,
diff --git a/block/qcow2.c b/block/qcow2.c
index f29aa88..f40355c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -718,7 +718,6 @@  static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     qemu_opts_del(opts);
-    bs->bl.write_zeroes_alignment = s->cluster_sectors;
 
     if (s->use_lazy_refcounts && s->qcow_version < 3) {
         error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
@@ -751,6 +750,15 @@  static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     return ret;
 }
 
+static int qcow2_refresh_limits(BlockDriverState *bs)
+{
+    BDRVQcowState *s = bs->opaque;
+
+    bs->bl.write_zeroes_alignment = s->cluster_sectors;
+
+    return 0;
+}
+
 static int qcow2_set_key(BlockDriverState *bs, const char *key)
 {
     BDRVQcowState *s = bs->opaque;
@@ -2268,6 +2276,7 @@  static BlockDriver bdrv_qcow2 = {
 
     .bdrv_change_backing_file   = qcow2_change_backing_file,
 
+    .bdrv_refresh_limits        = qcow2_refresh_limits,
     .bdrv_invalidate_cache      = qcow2_invalidate_cache,
 
     .create_options = qcow2_create_options,
diff --git a/block/qed.c b/block/qed.c
index 450a1fa..d35bd1c 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -495,7 +495,6 @@  static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
-    bs->bl.write_zeroes_alignment = s->header.cluster_size >> BDRV_SECTOR_BITS;
     s->need_check_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                             qed_need_check_timer_cb, s);
 
@@ -507,6 +506,15 @@  out:
     return ret;
 }
 
+static int bdrv_qed_refresh_limits(BlockDriverState *bs)
+{
+    BDRVQEDState *s = bs->opaque;
+
+    bs->bl.write_zeroes_alignment = s->header.cluster_size >> BDRV_SECTOR_BITS;
+
+    return 0;
+}
+
 /* We have nothing to do for QED reopen, stubs just return
  * success */
 static int bdrv_qed_reopen_prepare(BDRVReopenState *state,
@@ -1616,6 +1624,7 @@  static BlockDriver bdrv_qed = {
     .bdrv_truncate            = bdrv_qed_truncate,
     .bdrv_getlength           = bdrv_qed_getlength,
     .bdrv_get_info            = bdrv_qed_get_info,
+    .bdrv_refresh_limits      = bdrv_qed_refresh_limits,
     .bdrv_change_backing_file = bdrv_qed_change_backing_file,
     .bdrv_invalidate_cache    = bdrv_qed_invalidate_cache,
     .bdrv_check               = bdrv_qed_check,
diff --git a/block/vmdk.c b/block/vmdk.c
index 0734bc2..f8a387f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -428,10 +428,6 @@  static int vmdk_add_extent(BlockDriverState *bs,
     extent->l2_size = l2_size;
     extent->cluster_sectors = flat ? sectors : cluster_sectors;
 
-    if (!flat) {
-        bs->bl.write_zeroes_alignment =
-            MAX(bs->bl.write_zeroes_alignment, cluster_sectors);
-    }
     if (s->num_extents > 1) {
         extent->end_sector = (*(extent - 1)).end_sector + extent->sectors;
     } else {
@@ -886,6 +882,23 @@  fail:
     return ret;
 }
 
+
+static int vmdk_refresh_limits(BlockDriverState *bs)
+{
+    BDRVVmdkState *s = bs->opaque;
+    int i;
+
+    for (i = 0; i < s->num_extents; i++) {
+        if (!s->extents[i].flat) {
+            bs->bl.write_zeroes_alignment =
+                MAX(bs->bl.write_zeroes_alignment,
+                    s->extents[i].cluster_sectors);
+        }
+    }
+
+    return 0;
+}
+
 static int get_whole_cluster(BlockDriverState *bs,
                 VmdkExtent *extent,
                 uint64_t cluster_offset,
@@ -1971,6 +1984,7 @@  static BlockDriver bdrv_vmdk = {
     .bdrv_get_allocated_file_size = vmdk_get_allocated_file_size,
     .bdrv_has_zero_init           = vmdk_has_zero_init,
     .bdrv_get_specific_info       = vmdk_get_specific_info,
+    .bdrv_refresh_limits          = vmdk_refresh_limits,
 
     .create_options               = vmdk_create_options,
 };
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8b132d7..c49fa6b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -226,6 +226,8 @@  struct BlockDriver {
     int (*bdrv_debug_resume)(BlockDriverState *bs, const char *tag);
     bool (*bdrv_debug_is_suspended)(BlockDriverState *bs, const char *tag);
 
+    int (*bdrv_refresh_limits)(BlockDriverState *bs);
+
     /*
      * Returns 1 if newly created images are guaranteed to contain only
      * zeros, 0 otherwise.