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

[Eric Blake <eblake@redhat.com>]

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

On 02/04/2015 10:51 AM, Eric Blake wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> 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
>  * Flush
>  * Discard
>  * ioctl
>  * Tracked requests and queuing
>  * Throttling and copy-on-read
> 
> 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_drain_one(), bdrv_set_dirty(), bdrv_reset_dirty().
> 
> 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 produces identical results to Stefan's email, but is
> MUCH more readable (hint: git config diff.algorithm patience)
> 
>  block.c                   | 1980 +-------------------------------------------
>  block/Makefile.objs       |    1 +
>  block/io.c                | 1997 +++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block_int.h |   14 +
>  4 files changed, 2015 insertions(+), 1977 deletions(-)
>  create mode 100644 block/io.c

And here's how I reviewed it:
$ git format-patch --stdout -1 > patch
$ diff -u <(sed -n 's/^-//p' patch) <(sed -n 's/^\+//p' patch)

> --- /dev/fd/63	2015-02-04 10:59:08.221371351 -0700
> +++ /dev/fd/62	2015-02-04 10:59:08.221371351 -0700
> @@ -1,5 +1,39 @@
> ---
> --- a/block.c
> +++ b/block.c
> +    bdrv_setup_io_funcs(bdrv);
> +void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors)
> +void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors)
> +++ b/block/Makefile.objs
> +block-obj-y += io.o
> +++ b/block/io.c
> +/*
> + * QEMU System Emulator block driver
> + *
> + * Copyright (c) 2003 Fabrice Bellard
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "trace.h"
> +#include "block/block_int.h"
> +
> +#define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */
> +
>  static BlockAIOCB *bdrv_aio_readv_em(BlockDriverState *bs,
>          int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
>          BlockCompletionFunc *cb, void *opaque);
> @@ -30,10 +64,24 @@
>  static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
>      int64_t sector_num, int nb_sectors, BdrvRequestFlags flags);
>  
> -static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> -                           int nr_sectors);
> -static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
> -                             int nr_sectors);
> +void bdrv_setup_io_funcs(BlockDriver *bdrv)
> +{
> +    /* Block drivers without coroutine functions need emulation */
> +    if (!bdrv->bdrv_co_readv) {
> +        bdrv->bdrv_co_readv = bdrv_co_readv_em;
> +        bdrv->bdrv_co_writev = bdrv_co_writev_em;
> +
> +        /* bdrv_co_readv_em()/brdv_co_writev_em() work in terms of aio, so if
> +         * the block driver lacks aio we need to emulate that too.
> +         */
> +        if (!bdrv->bdrv_aio_readv) {
> +            /* add AIO emulation layer */
> +            bdrv->bdrv_aio_readv = bdrv_aio_readv_em;
> +            bdrv->bdrv_aio_writev = bdrv_aio_writev_em;
> +        }
> +    }
> +}
> +
>  /* throttling disk I/O limits */
>  void bdrv_set_io_limits(BlockDriverState *bs,
>                          ThrottleConfig *cfg)
> @@ -132,20 +180,6 @@
>      qemu_co_queue_next(&bs->throttled_reqs[is_write]);
>  }
>  
> -    /* Block drivers without coroutine functions need emulation */
> -    if (!bdrv->bdrv_co_readv) {
> -        bdrv->bdrv_co_readv = bdrv_co_readv_em;
> -        bdrv->bdrv_co_writev = bdrv_co_writev_em;
> -
> -        /* bdrv_co_readv_em()/brdv_co_writev_em() work in terms of aio, so if
> -         * the block driver lacks aio we need to emulate that too.
> -         */
> -        if (!bdrv->bdrv_aio_readv) {
> -            /* add AIO emulation layer */
> -            bdrv->bdrv_aio_readv = bdrv_aio_readv_em;
> -            bdrv->bdrv_aio_writev = bdrv_aio_writev_em;
> -        }
> -    }
>  /**
>   * The copy-on-read flag is actually a reference count so multiple users may
>   * use the feature without worrying about clobbering its previous state.
> @@ -183,7 +217,7 @@
>      return false;
>  }
>  
> -static bool bdrv_drain_one(BlockDriverState *bs)
> +bool bdrv_drain_one(BlockDriverState *bs)
>  {
>      bool bs_busy;
>  
> @@ -286,19 +320,6 @@
>      }
>  }
>  
> -static int bdrv_get_cluster_size(BlockDriverState *bs)
> -{
> -    BlockDriverInfo bdi;
> -    int ret;
> -
> -    ret = bdrv_get_info(bs, &bdi);
> -    if (ret < 0 || bdi.cluster_size == 0) {
> -        return bs->request_alignment;
> -    } else {
> -        return bdi.cluster_size;
> -    }
> -}
> -
>  static bool tracked_request_overlaps(BdrvTrackedRequest *req,
>                                       int64_t offset, unsigned int bytes)
>  {
> @@ -357,7 +378,6 @@
>      return waited;
>  }
>  
> -
>  static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
>                                     size_t size)
>  {
> @@ -598,6 +618,22 @@
>      return 0;
>  }
>  
> +int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
> +                          const uint8_t *buf, int nb_sectors)
> +{
> +    BlockDriver *drv = bs->drv;
> +    if (!drv)
> +        return -ENOMEDIUM;
> +    if (!drv->bdrv_write_compressed)
> +        return -ENOTSUP;
> +    if (bdrv_check_request(bs, sector_num, nb_sectors))
> +        return -EIO;
> +
> +    assert(QLIST_EMPTY(&bs->dirty_bitmaps));
> +
> +    return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
> +}
> +
>  static int coroutine_fn bdrv_co_do_copy_on_readv(BlockDriverState *bs,
>          int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
>  {
> @@ -669,6 +705,19 @@
>      return ret;
>  }
>  
> +static int bdrv_get_cluster_size(BlockDriverState *bs)
> +{
> +    BlockDriverInfo bdi;
> +    int ret;
> +
> +    ret = bdrv_get_info(bs, &bdi);
> +    if (ret < 0 || bdi.cluster_size == 0) {
> +        return bs->request_alignment;
> +    } else {
> +        return bdi.cluster_size;
> +    }
> +}
> +
>  /*
>   * Forwards an already correctly aligned request to the BlockDriver. This
>   * handles copy on read and zeroing after EOF; any other features must be
> @@ -1161,22 +1210,6 @@
>                               BDRV_REQ_ZERO_WRITE | flags);
>  }
>  
> -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
> -                          const uint8_t *buf, int nb_sectors)
> -{
> -    BlockDriver *drv = bs->drv;
> -    if (!drv)
> -        return -ENOMEDIUM;
> -    if (!drv->bdrv_write_compressed)
> -        return -ENOTSUP;
> -    if (bdrv_check_request(bs, sector_num, nb_sectors))
> -        return -EIO;
> -
> -    assert(QLIST_EMPTY(&bs->dirty_bitmaps));
> -
> -    return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
> -}
> -
>  /**************************************************************/
>  /* async I/Os */
>  
> @@ -1211,7 +1244,6 @@
>                                   cb, opaque, true);
>  }
>  
> -
>  typedef struct MultiwriteCB {
>      int error;
>      int num_requests;
> @@ -1501,7 +1533,6 @@
>      return bdrv_aio_rw_vector(bs, sector_num, qiov, nb_sectors, cb, opaque, 1);
>  }
>  
> -
>  typedef struct BlockAIOCBCoroutine {
>      BlockAIOCB common;
>      BlockRequest req;
> @@ -1620,7 +1651,6 @@
>  
>      return &acb->common;
>  }
> -
>  void *qemu_aio_get(const AIOCBInfo *aiocb_info, BlockDriverState *bs,
>                     BlockCompletionFunc *cb, void *opaque)
>  {
> @@ -1937,10 +1967,6 @@
>      return NULL;
>  }
>  
> -static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> -                           int nr_sectors)
> -static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
> -                             int nr_sectors)
>  void bdrv_add_before_write_notifier(BlockDriverState *bs,
>                                      NotifierWithReturn *notifier)
>  {
> @@ -1976,8 +2002,18 @@
>          bdrv_flush_io_queue(bs->file);
>      }
>  }
> +++ b/include/block/block_int.h
> +/**
> + * bdrv_setup_io_funcs:
> + *
> + * Prepare a #BlockDriver for I/O request processing by populating
> + * unimplemented coroutine and AIO interfaces with generic wrapper functions
> + * that fall back to implemented interfaces.
> + */
> +void bdrv_setup_io_funcs(BlockDriver *bdrv);
> +bool bdrv_drain_one(BlockDriverState *bs);
> +
> +void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
> +void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
> +                      int nr_sectors);
>  
> --- a/block/Makefile.objs
> --- /dev/null
> --- a/include/block/block_int.h
> -- 
> 

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 --]