Re: [Qemu-devel] [PATCH v3 01/29] block: Move initialisation of BlockLimits to bdrv_refresh_limits()

[Peter Lieven <pl@kamp.de>]

On 20.01.2014 10:31, Kevin Wolf wrote:
> Am 17.01.2014 um 23:39 hat Benoît Canet geschrieben:
>> Le Friday 17 Jan 2014 à 15:14:51 (+0100), Kevin Wolf a écrit :
>>> 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>
>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>   block.c                   | 19 +++++++++++++++++++
>>>   block/iscsi.c             | 46 +++++++++++++++++++++++++++++-----------------
>>>   block/qcow2.c             | 11 ++++++++++-
>>>   block/qed.c               | 11 ++++++++++-
>>>   block/vmdk.c              | 22 ++++++++++++++++++----
>>>   include/block/block_int.h |  2 ++
>>>   6 files changed, 88 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 64e7d22..99e69da 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -479,6 +479,19 @@ 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 && 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 +846,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 +1033,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 c0ea0c4..3202dc5 100644
>>> --- a/block/iscsi.c
>>> +++ b/block/iscsi.c
>>> @@ -1265,23 +1265,6 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>>>                  sizeof(struct scsi_inquiry_block_limits));
>>>           scsi_free_scsi_task(task);
>>>           task = NULL;
>>> -
>>> -        if (iscsilun->bl.max_unmap < 0xffffffff) {
>>> -            bs->bl.max_discard = sector_lun2qemu(iscsilun->bl.max_unmap,
>>> -                                                 iscsilun);
>>> -        }
>>> -        bs->bl.discard_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran,
>>> -                                                   iscsilun);
>>> -
>>> -        if (iscsilun->bl.max_ws_len < 0xffffffff) {
>>> -            bs->bl.max_write_zeroes = sector_lun2qemu(iscsilun->bl.max_ws_len,
>>> -                                                      iscsilun);
>>> -        }
>>> -        bs->bl.write_zeroes_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran,
>>> -                                                        iscsilun);
>>> -
>>> -        bs->bl.opt_transfer_length = sector_lun2qemu(iscsilun->bl.opt_xfer_len,
>>> -                                                     iscsilun);
>>>       }
>>>   
>>>   #if defined(LIBISCSI_FEATURE_NOP_COUNTER)
>>> @@ -1326,6 +1309,34 @@ static void iscsi_close(BlockDriverState *bs)
>>>       memset(iscsilun, 0, sizeof(IscsiLun));
>>>   }
>>>   
>>> +static int iscsi_refresh_limits(BlockDriverState *bs)
>>> +{
>>> +    IscsiLun *iscsilun = bs->opaque;
>>> +
>>> +    /* We don't actually refresh here, but just return data queried in
>>> +     * iscsi_open(): iscsi targets don't change their limits. */
>>> +    if (iscsilun->lbp.lbpu || iscsilun->lbp.lbpws) {
>>> +        if (iscsilun->bl.max_unmap < 0xffffffff) {
>>> +            bs->bl.max_discard = sector_lun2qemu(iscsilun->bl.max_unmap,
>>> +                                                 iscsilun);
>>> +        }
>>> +        bs->bl.discard_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran,
>>> +                                                   iscsilun);
>>> +
>>> +        if (iscsilun->bl.max_ws_len < 0xffffffff) {
>>> +            bs->bl.max_write_zeroes = sector_lun2qemu(iscsilun->bl.max_ws_len,
>>> +                                                      iscsilun);
>>> +        }
>>> +        bs->bl.write_zeroes_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran,
>>> +                                                        iscsilun);
>>> +
>>> +        bs->bl.opt_transfer_length = sector_lun2qemu(iscsilun->bl.opt_xfer_len,
>>> +                                                     iscsilun);
>> Why is bl.opt_transfer_length filled only inside the test for unmap and write same ?
>> Is there a relationship ?
> Good question. The only thing I can say is that I'm moving existing code
> here, so this aspect doesnt' change. I suspect this needs a fix on top
> of this series. Peter?
Yes, the opt_transfer_length is independend. It got there because, if either lbp.lbpu or lbp.lbpws
is set, we can be sure that the Block Limits page is supported. I will change this
when Kevins series is merged.

Peter