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

[Peter Lieven]

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 <address@hidden>
> > > Reviewed-by: Max Reitz <address@hidden>
> > > ---
> > >   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