From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51312) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJ4Gs-0000Ob-II for qemu-devel@nongnu.org; Wed, 04 Feb 2015 13:01:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YJ4Gm-0005jf-TP for qemu-devel@nongnu.org; Wed, 04 Feb 2015 13:01:50 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55685) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJ4Gm-0005jP-J6 for qemu-devel@nongnu.org; Wed, 04 Feb 2015 13:01:44 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t14I1dSq018578 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 4 Feb 2015 13:01:43 -0500 Message-ID: <54D25E82.8010000@redhat.com> Date: Wed, 04 Feb 2015 11:01:38 -0700 From: Eric Blake MIME-Version: 1.0 References: <1423063902-4926-1-git-send-email-stefanha@redhat.com> <1423072297-1842-1-git-send-email-eblake@redhat.com> In-Reply-To: <1423072297-1842-1-git-send-email-eblake@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="aX4X69R3SSt54H7IKEO3cNRChaKMjQJnO" Subject: Re: [Qemu-devel] [PATCH reformat] block: move I/O request processing to block/io.c List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: kwolf@redhat.com, stefanha@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --aX4X69R3SSt54H7IKEO3cNRChaKMjQJnO Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 02/04/2015 10:51 AM, Eric Blake wrote: > From: Stefan Hajnoczi >=20 > The block.c file has grown to over 6000 lines. It is time to split thi= s > file so there are fewer conflicts and the code is easier to maintain. >=20 > Extract I/O request processing code: > * Read > * Write > * Flush > * Discard > * ioctl > * Tracked requests and queuing > * Throttling and copy-on-read >=20 > The patch simply moves code from block.c into block/io.c. >=20 > 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(). >=20 > 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. >=20 > Signed-off-by: Stefan Hajnoczi > --- >=20 > This patch produces identical results to Stefan's email, but is > MUCH more readable (hint: git config diff.algorithm patience) >=20 > 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_s= ectors) > +void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr= _sectors) > +++ b/block/Makefile.objs > +block-obj-y +=3D 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 obtaini= ng a copy > + * of this software and associated documentation files (the "Software"= ), to deal > + * in the Software without restriction, including without limitation t= he rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/o= r 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 incl= uded in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXP= RESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABI= LITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT S= HALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES O= R OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARI= SING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALI= NGS IN > + * THE SOFTWARE. > + */ > + > +#include "trace.h" > +#include "block/block_int.h" > + > +#define NOT_DONE 0x7fffffff /* used while emulated sync operation in p= rogress */ > + > 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); > =20 > -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 =3D bdrv_co_readv_em; > + bdrv->bdrv_co_writev =3D 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 =3D bdrv_aio_readv_em; > + bdrv->bdrv_aio_writev =3D 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]); > } > =20 > - /* Block drivers without coroutine functions need emulation */ > - if (!bdrv->bdrv_co_readv) { > - bdrv->bdrv_co_readv =3D bdrv_co_readv_em; > - bdrv->bdrv_co_writev =3D 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 =3D bdrv_aio_readv_em; > - bdrv->bdrv_aio_writev =3D bdrv_aio_writev_em; > - } > - } > /** > * The copy-on-read flag is actually a reference count so multiple use= rs may > * use the feature without worrying about clobbering its previous stat= e. > @@ -183,7 +217,7 @@ > return false; > } > =20 > -static bool bdrv_drain_one(BlockDriverState *bs) > +bool bdrv_drain_one(BlockDriverState *bs) > { > bool bs_busy; > =20 > @@ -286,19 +320,6 @@ > } > } > =20 > -static int bdrv_get_cluster_size(BlockDriverState *bs) > -{ > - BlockDriverInfo bdi; > - int ret; > - > - ret =3D bdrv_get_info(bs, &bdi); > - if (ret < 0 || bdi.cluster_size =3D=3D 0) { > - return bs->request_alignment; > - } else { > - return bdi.cluster_size; > - } > -} > - > static bool tracked_request_overlaps(BdrvTrackedRequest *req, > int64_t offset, unsigned int byte= s) > { > @@ -357,7 +378,6 @@ > return waited; > } > =20 > - > static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offse= t, > size_t size) > { > @@ -598,6 +618,22 @@ > return 0; > } > =20 > +int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num, > + const uint8_t *buf, int nb_sectors) > +{ > + BlockDriver *drv =3D 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; > } > =20 > +static int bdrv_get_cluster_size(BlockDriverState *bs) > +{ > + BlockDriverInfo bdi; > + int ret; > + > + ret =3D bdrv_get_info(bs, &bdi); > + if (ret < 0 || bdi.cluster_size =3D=3D 0) { > + return bs->request_alignment; > + } else { > + return bdi.cluster_size; > + } > +} > + > /* > * Forwards an already correctly aligned request to the BlockDriver. T= his > * handles copy on read and zeroing after EOF; any other features must= be > @@ -1161,22 +1210,6 @@ > BDRV_REQ_ZERO_WRITE | flags); > } > =20 > -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num, > - const uint8_t *buf, int nb_sectors) > -{ > - BlockDriver *drv =3D 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 */ > =20 > @@ -1211,7 +1244,6 @@ > cb, opaque, true); > } > =20 > - > typedef struct MultiwriteCB { > int error; > int num_requests; > @@ -1501,7 +1533,6 @@ > return bdrv_aio_rw_vector(bs, sector_num, qiov, nb_sectors, cb, op= aque, 1); > } > =20 > - > typedef struct BlockAIOCBCoroutine { > BlockAIOCB common; > BlockRequest req; > @@ -1620,7 +1651,6 @@ > =20 > return &acb->common; > } > - > void *qemu_aio_get(const AIOCBInfo *aiocb_info, BlockDriverState *bs, > BlockCompletionFunc *cb, void *opaque) > { > @@ -1937,10 +1967,6 @@ > return NULL; > } > =20 > -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 fun= ctions > + * 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_s= ectors); > +void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, > + int nr_sectors); > =20 > --- a/block/Makefile.objs > --- /dev/null > --- a/include/block/block_int.h > --=20 >=20 Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --aX4X69R3SSt54H7IKEO3cNRChaKMjQJnO Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJU0l6CAAoJEKeha0olJ0NqpJ0H/37Rl3MOohsawANw8HGphAZ+ yStioJLd/TsFUJ5UICRae3qSIrmd0NHT2irwQfoWX+wUfWy5PwKJ298dv8Jdkom1 Ro3W0P12xcXYaReTNev1vUUlXofOila9jslZS1SP40+spBux/Y9ioMFRdRGUzc1k 8jGUjfMDZhov1rb3aaauQRqBbeyt3B+G3liigz2asI1uOEfL19HJXg+JJZez8UCh D7GVMuUeWYwatyhpnLINa9sB46cT3qg4Zyk/Aqh/O8VGQdrIef6fqHnG7CMWrTwx U3nIYZmLnKFZwHLoyb/dLRI7GYjN+yX/518Fc2QU3iMYQnuD7/cCl9L7pe+v0ak= =SvJZ -----END PGP SIGNATURE----- --aX4X69R3SSt54H7IKEO3cNRChaKMjQJnO--