Re: [Qemu-devel] [PATCH v2] block: move I/O request processing to block/io.c

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3040 bytes --]

On 02/17/2015 07:39 AM, Stefan Hajnoczi wrote:
> The block.c file has grown to over 6000 lines.  It is time to split this
> file so there are fewer conflicts and the code is easier to maintain.
> 
> Extract I/O request processing code:
>  * Read
>  * Write
>  * Zero writes and making the image empty
>  * Flush
>  * Discard
>  * ioctl
>  * Tracked requests and queuing
>  * Throttling and copy-on-read
>  * Block status and allocated functions
>  * Refreshing block limits
>  * Reading/writing vmstate
>  * qemu_blockalign() and friends
> 
> The patch simply moves code from block.c into block/io.c.
> 
> No code changes are made except adding the following block_int.h
> functions so they can be called across block.c and block/io.c:
> bdrv_set_dirty(), bdrv_reset_dirty().

I spotted a couple of other changes that you may not have intended (or
if you did, you should also call them out here).

> 
> I/O request processing needs to set up BlockDriver coroutine and AIO
> emulation function pointers, so add bdrv_setup_io_funcs(bdrv) interface
> that block.c calls.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> This patch is based on the last block pull request from Fri 13th February 2015.
> This is necessary since the patch is prone to conflicts in block.c - we need to
> use the latest code!
> 
> If anyone wants to move more stuff, please consider sending follow-up patches
> instead because it is painful to rebase this.
> 
> v2:
>  * Move bdrv_drain_all() [Kevin]
>  * Move bdrv_make_zero() [Kevin]
>  * Move bdrv_flush_all() [Kevin]
>  * Move bdrv_is_allocated/get_block_status family of functions [Kevin]
>  * Move bdrv_refresh_limits() [Kevin]
>  * Move load/save_vmstate() [Kevin]
>  * Move qemu_blockalign() and friends [Kevin]

> -void bdrv_drain_all(void)
> -{
> -    /* Always run first iteration so any pending completion BHs run */
> -    bool busy = true;
> -    BlockDriverState *bs;
> -
> -    while (busy) {
> -        busy = false;
> -
> -        QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> -            AioContext *aio_context = bdrv_get_aio_context(bs);
> -

> +void bdrv_drain_all(void)
> +{
> +    /* Always run first iteration so any pending completion BHs run */
> +    bool busy = true;
> +    BlockDriverState *bs = NULL;
> +
> +    while (busy) {
> +        busy = false;
> +
> +        while ((bs = bdrv_next(bs))) {
> +            AioContext *aio_context = bdrv_get_aio_context(bs);
> +

You switched iteration from QTAILQ_FOREACH to a while(bdrv_next()) loop,
here, and in bdrv_flush_all.

I think the change is safe, but it would be smarter to split it into a
separate patch and/or document it in the commit message as intentional.
 But I also agree that we want this patch in sooner rather than later to
minimize conflict churn.  So:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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