Re: [Qemu-devel] [PATCH v3 14/23] hw: Convert from BlockDriverState to BlockBackend, mostly

[Kevin Wolf]

Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben:
> Just four uses of BlockDriverState are left:
> 
> * The Xen paravirtual block device backend (xen_disk.c) opens images
>   itself when set up via xenbus, bypassing blockdev.c.  I figure it
>   should go through qmp_blockdev_add() instead.
> 
> * Device model "usb-storage" prompts for keys.  No other device model
>   does, and this one probably shouldn't do it, either.
> 
> * ide_issue_trim_cb() uses bdrv_aio_discard() instead of
>   blk_aio_discard() because it fishes its backend out of a BlockAIOCB,
>   which has only the BlockDriverState.
> 
> * PC87312State has an unused BlockDriverState[] member.
> 
> The next two commits take care of the latter two.
> 
> Signed-off-by: Markus Armbruster <address@hidden>

Okay. This patch gives us a chance to have a look at each of the
functions that we'll expose in the BlockBackend layer.

> diff --git a/block/block-backend.c b/block/block-backend.c
> index 7fd832d..5f796b4 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -235,3 +235,260 @@ void blk_hide_on_behalf_of_do_drive_del(BlockBackend 
> *blk)
>          bdrv_make_anon(blk->bs);
>      }
>  }
> +
> +void blk_iostatus_enable(BlockBackend *blk)
> +{
> +    bdrv_iostatus_enable(blk->bs);
> +}

Even at the end of the series, iostatus (i.e. rerror/werror handling) is
still at the BDS level. I think our earlier conclusion was that
it's really a BB thing, so I would expect it to be implemented in
block-backend.c with no calls into block.c.

Is this on your todo list?

> +int blk_attach_dev(BlockBackend *blk, void *dev)
> +{
> +    return bdrv_attach_dev(blk->bs, dev);
> +}
> +
> +void blk_attach_dev_nofail(BlockBackend *blk, void *dev)
> +{
> +    bdrv_attach_dev_nofail(blk->bs, dev);
> +}
> +
> +void blk_detach_dev(BlockBackend *blk, void *dev)
> +{
> +    bdrv_detach_dev(blk->bs, dev);
> +}
> +
> +void *blk_get_attached_dev(BlockBackend *blk)
> +{
> +    return bdrv_get_attached_dev(blk->bs);
> +}
> +
> +void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque)
> +{
> +    bdrv_set_dev_ops(blk->bs, ops, opaque);
> +}

At the end of the series, all of these are BB-only code. Good.

> +int blk_read(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
> +             int nb_sectors)
> +{
> +    return bdrv_read(blk->bs, sector_num, buf, nb_sectors);
> +}
> +
> +int blk_read_unthrottled(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
> +                         int nb_sectors)
> +{
> +    return bdrv_read_unthrottled(blk->bs, sector_num, buf, nb_sectors);
> +}

This one is a hack that I'd rather see disappear. Unrelated to this
series, though.

> +int blk_write(BlockBackend *blk, int64_t sector_num, const uint8_t *buf,
> +              int nb_sectors)
> +{
> +    return bdrv_write(blk->bs, sector_num, buf, nb_sectors);
> +}
> +
> +BlockAIOCB *blk_aio_write_zeroes(BlockBackend *blk, int64_t sector_num,
> +                                 int nb_sectors, BdrvRequestFlags flags,
> +                                 BlockCompletionFunc *cb, void *opaque)
> +{
> +    return bdrv_aio_write_zeroes(blk->bs, sector_num, nb_sectors, flags,
> +                                 cb, opaque);
> +}
> +
> +int blk_pread(BlockBackend *blk, int64_t offset, void *buf, int count)
> +{
> +    return bdrv_pread(blk->bs, offset, buf, count);
> +}
> +
> +int blk_pwrite(BlockBackend *blk, int64_t offset, const void *buf, int count)
> +{
> +    return bdrv_pwrite(blk->bs, offset, buf, count);
> +}

Any particular reason for the ordering of the functions here? Is this
just how they were in block.c? I would have expected all read function
and all write functions in one place.

> +int64_t blk_getlength(BlockBackend *blk)
> +{
> +    return bdrv_getlength(blk->bs);
> +}
> +
> +void blk_get_geometry(BlockBackend *blk, uint64_t *nb_sectors_ptr)
> +{
> +    bdrv_get_geometry(blk->bs, nb_sectors_ptr);
> +}
> +
> +BlockAIOCB *blk_aio_readv(BlockBackend *blk, int64_t sector_num,
> +                          QEMUIOVector *iov, int nb_sectors,
> +                          BlockCompletionFunc *cb, void *opaque)
> +{
> +    return bdrv_aio_readv(blk->bs, sector_num, iov, nb_sectors, cb, opaque);
> +}
> +
> +BlockAIOCB *blk_aio_writev(BlockBackend *blk, int64_t sector_num,
> +                           QEMUIOVector *iov, int nb_sectors,
> +                           BlockCompletionFunc *cb, void *opaque)
> +{
> +    return bdrv_aio_writev(blk->bs, sector_num, iov, nb_sectors, cb, opaque);
> +}
> +
> +BlockAIOCB *blk_aio_flush(BlockBackend *blk,
> +                          BlockCompletionFunc *cb, void *opaque)
> +{
> +    return bdrv_aio_flush(blk->bs, cb, opaque);
> +}
> +
> +BlockAIOCB *blk_aio_discard(BlockBackend *blk,
> +                            int64_t sector_num, int nb_sectors,
> +                            BlockCompletionFunc *cb, void *opaque)
> +{
> +    return bdrv_aio_discard(blk->bs, sector_num, nb_sectors, cb, opaque);
> +}
> +
> +void blk_aio_cancel(BlockAIOCB *acb)
> +{
> +    bdrv_aio_cancel(acb);
> +}
> +
> +int blk_aio_multiwrite(BlockBackend *blk, BlockRequest *reqs, int num_reqs)
> +{
> +    return bdrv_aio_multiwrite(blk->bs, reqs, num_reqs);
> +}
> +
> +int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf)
> +{
> +    return bdrv_ioctl(blk->bs, req, buf);
> +}
> +
> +BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void 
> *buf,
> +                          BlockCompletionFunc *cb, void *opaque)
> +{
> +    return bdrv_aio_ioctl(blk->bs, req, buf, cb, opaque);
> +}
> +
> +int blk_flush(BlockBackend *blk)
> +{
> +    return bdrv_flush(blk->bs);
> +}
> +
> +int blk_flush_all(void)
> +{
> +    return bdrv_flush_all();
> +}
> +
> +void blk_drain_all(void)
> +{
> +    bdrv_drain_all();
> +}

> +BlockdevOnError blk_get_on_error(BlockBackend *blk, bool is_read)
> +{
> +    return bdrv_get_on_error(blk->bs, is_read);
> +}
> +
> +BlockErrorAction blk_get_error_action(BlockBackend *blk, bool is_read,
> +                                      int error)
> +{
> +    return bdrv_get_error_action(blk->bs, is_read, error);
> +}
> +
> +void blk_error_action(BlockBackend *blk, BlockErrorAction action,
> +                      bool is_read, int error)
> +{
> +    bdrv_error_action(blk->bs, action, is_read, error);
> +}

Like iostatus, this probably should be a BB-only thing in the end.

> +int blk_is_read_only(BlockBackend *blk)
> +{
> +    return bdrv_is_read_only(blk->bs);
> +}
> +
> +int blk_is_sg(BlockBackend *blk)
> +{
> +    return bdrv_is_sg(blk->bs);
> +}
> +
> +int blk_enable_write_cache(BlockBackend *blk)
> +{
> +    return bdrv_enable_write_cache(blk->bs);
> +}
> +
> +void blk_set_enable_write_cache(BlockBackend *blk, bool wce)
> +{
> +    bdrv_set_enable_write_cache(blk->bs, wce);
> +}
> +
> +int blk_is_inserted(BlockBackend *blk)
> +{
> +    return bdrv_is_inserted(blk->bs);
> +}
> +
> +void blk_lock_medium(BlockBackend *blk, bool locked)
> +{
> +    bdrv_lock_medium(blk->bs, locked);
> +}
> +
> +void blk_eject(BlockBackend *blk, bool eject_flag)
> +{
> +    bdrv_eject(blk->bs, eject_flag);
> +}
> +
> +int blk_get_flags(BlockBackend *blk)
> +{
> +    return bdrv_get_flags(blk->bs);
> +}
> +
> +void blk_set_guest_block_size(BlockBackend *blk, int align)
> +{
> +    bdrv_set_guest_block_size(blk->bs, align);
> +}
> +
> +void *blk_blockalign(BlockBackend *blk, size_t size)

Perhaps better blk_memalign?

> +{
> +    return qemu_blockalign(blk ? blk->bs : NULL, size);
> +}
> +
> +bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp)
> +{
> +    return bdrv_op_is_blocked(blk->bs, op, errp);
> +}
> +
> +void blk_op_unblock(BlockBackend *blk, BlockOpType op, Error *reason)
> +{
> +    bdrv_op_unblock(blk->bs, op, reason);
> +}
> +
> +void blk_op_block_all(BlockBackend *blk, Error *reason)
> +{
> +    bdrv_op_block_all(blk->bs, reason);
> +}
> +
> +void blk_op_unblock_all(BlockBackend *blk, Error *reason)
> +{
> +    bdrv_op_unblock_all(blk->bs, reason);
> +}
> +
> +AioContext *blk_get_aio_context(BlockBackend *blk)
> +{
> +    return bdrv_get_aio_context(blk->bs);
> +}
> +
> +void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
> +{
> +    bdrv_set_aio_context(blk->bs, new_context);
> +}
> +
> +void blk_io_plug(BlockBackend *blk)
> +{
> +    bdrv_io_plug(blk->bs);
> +}
> +
> +void blk_io_unplug(BlockBackend *blk)
> +{
> +    bdrv_io_unplug(blk->bs);
> +}
> +
> +BlockAcctStats *blk_get_stats(BlockBackend *blk)
> +{
> +    return bdrv_get_stats(blk->bs);
> +}
> +
> +void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk,
> +                  BlockCompletionFunc *cb, void *opaque)
> +{
> +    return qemu_aio_get(aiocb_info, blk_bs(blk), cb, opaque);
> +}

There are a few more in this list that look dubious, but this patch
series isn't the right place to discuss them.

Found nothing noteworthy in the rest of the patch, though I only
quickly scanned it, so that wasn't thorough enough for a R-b.

Kevin