From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38407) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGrB5-00062y-5n for qemu-devel@nongnu.org; Wed, 06 Jan 2016 11:43:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGrB3-0005Iz-Su for qemu-devel@nongnu.org; Wed, 06 Jan 2016 11:43:15 -0500 References: <1451981476-29390-1-git-send-email-famz@redhat.com> <1451981476-29390-2-git-send-email-famz@redhat.com> From: Max Reitz Message-ID: <568D4417.4040903@redhat.com> Date: Wed, 6 Jan 2016 17:43:03 +0100 MIME-Version: 1.0 In-Reply-To: <1451981476-29390-2-git-send-email-famz@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="adXvwXsT0CDopb2C4rh8np0fOkSNWHnoM" Subject: Re: [Qemu-devel] [PATCH v5 01/15] block: Add "file" output parameter to block status query functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , pbonzini@redhat.com, Stefan Hajnoczi , qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --adXvwXsT0CDopb2C4rh8np0fOkSNWHnoM Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 05.01.2016 09:11, Fam Zheng wrote: > The added parameter can be used to return the BDS pointer which the > valid offset is referring to. Its value should be ignored unless > BDRV_BLOCK_OFFSET_VALID in ret is set. >=20 > Until block drivers fill in the right value, let's clear it explicitly > right before calling .bdrv_get_block_status. >=20 > The "bs->file" condition in bdrv_co_get_block_status is kept now to kee= p iotest > case 102 passing, and will be fixed once all drivers return the right f= ile > pointer. >=20 > Signed-off-by: Fam Zheng > --- > block/io.c | 42 ++++++++++++++++++++++++++++-----------= --- > block/iscsi.c | 6 ++++-- > block/mirror.c | 3 ++- > block/parallels.c | 2 +- > block/qcow.c | 2 +- > block/qcow2.c | 2 +- > block/qed.c | 3 ++- > block/raw-posix.c | 3 ++- > block/raw_bsd.c | 3 ++- > block/sheepdog.c | 2 +- > block/vdi.c | 2 +- > block/vmdk.c | 2 +- > block/vpc.c | 2 +- > block/vvfat.c | 2 +- > include/block/block.h | 9 ++++++--- > include/block/block_int.h | 3 ++- > qemu-img.c | 7 +++++-- > 17 files changed, 61 insertions(+), 34 deletions(-) >=20 > diff --git a/block/io.c b/block/io.c > index 63e3678..492c291 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -663,6 +663,7 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t= sector_num, > int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags) > { > int64_t target_sectors, ret, nb_sectors, sector_num =3D 0; > + BlockDriverState *file; > int n; > =20 > target_sectors =3D bdrv_nb_sectors(bs); > @@ -675,7 +676,7 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvReques= tFlags flags) > if (nb_sectors <=3D 0) { > return 0; > } > - ret =3D bdrv_get_block_status(bs, sector_num, nb_sectors, &n);= > + ret =3D bdrv_get_block_status(bs, sector_num, nb_sectors, &n, = &file); > if (ret < 0) { > error_report("error getting block status at sector %" PRId= 64 ": %s", > sector_num, strerror(-ret)); > @@ -1464,6 +1465,7 @@ int bdrv_flush_all(void) > typedef struct BdrvCoGetBlockStatusData { > BlockDriverState *bs; > BlockDriverState *base; > + BlockDriverState **file; > int64_t sector_num; > int nb_sectors; > int *pnum; > @@ -1488,7 +1490,8 @@ typedef struct BdrvCoGetBlockStatusData { > */ > static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState = *bs, > int64_t sector_nu= m, > - int nb_sectors, i= nt *pnum) > + int nb_sectors, i= nt *pnum, > + BlockDriverState = **file) > { > int64_t total_sectors; > int64_t n; > @@ -1518,16 +1521,19 @@ static int64_t coroutine_fn bdrv_co_get_block_s= tatus(BlockDriverState *bs, > return ret; > } > =20 > - ret =3D bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_secto= rs, pnum); > + *file =3D NULL; > + ret =3D bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_secto= rs, pnum, > + file); > if (ret < 0) { > *pnum =3D 0; > + *file =3D NULL; > return ret; > } > =20 > if (ret & BDRV_BLOCK_RAW) { > assert(ret & BDRV_BLOCK_OFFSET_VALID); > return bdrv_get_block_status(bs->file->bs, ret >> BDRV_SECTOR_= BITS, > - *pnum, pnum); > + *pnum, pnum, file); > } > =20 > if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) { > @@ -1544,13 +1550,14 @@ static int64_t coroutine_fn bdrv_co_get_block_s= tatus(BlockDriverState *bs, > } > } > =20 > - if (bs->file && > + if (bs->file && *file && *file !=3D bs && In order to keep iotest 102 working, this line should not be changed at all in this patch. Because *file is NULL and not changed by any of the block drivers, the condition will always be false now (breaking iotest 10= 2). > (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && > (ret & BDRV_BLOCK_OFFSET_VALID)) { > + BlockDriverState *file2; > int file_pnum; > =20 > - ret2 =3D bdrv_co_get_block_status(bs->file->bs, ret >> BDRV_SE= CTOR_BITS, > - *pnum, &file_pnum); > + ret2 =3D bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BI= TS, And instead of *file, this should remain bs->file->bs. > + *pnum, &file_pnum, &file2); > if (ret2 >=3D 0) { > /* Ignore errors. This is just providing extra informatio= n, it > * is useful but not necessary. [...] > diff --git a/include/block/block.h b/include/block/block.h > index db8e096..ab966ad 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -113,7 +113,8 @@ typedef struct HDGeometry { > * Allocation status flags > * BDRV_BLOCK_DATA: data is read from bs->file or another file > * BDRV_BLOCK_ZERO: sectors read as zero > - * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data > + * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file or another file > + * as raw data Hm, this seems so unspecific. We do know exactly where the data is stored, and that is in the *file BDS returned by bdrv_get_block_status(). Can we make some reference to that? I don't know whether "sector stored in *file as raw data" will be clear enough, though. Maybe "sector stored in the BDS returned as *file by bdrv_get_block_status()" will suffice. Max > * BDRV_BLOCK_ALLOCATED: the content of the block is determined by thi= s > * layer (as opposed to the backing file) > * BDRV_BLOCK_RAW: used internally to indicate that the request --adXvwXsT0CDopb2C4rh8np0fOkSNWHnoM Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWjUQXAAoJEDuxQgLoOKytDZwH/A7tY16x9qWkJ23xuXs7/P3B ePsNorJMWLPPGOCUULTRa1BUOt5YzuBcrTcPhr2XlKW9kCLgkPUXo/8wrYWMA6uV 21tCGhNF6YoevtHegnaNNflh9G+nHjCFH0JL2xopYNG0zhMecGRSF+Q25wKPcEY9 D4gcCk5JG5uqfV6c7TB2LDUKcxj+crm05cF0jFdVJrmh2naPfwx37jOMrps1FfHr 5L/f56awTwnrzLtYbQPoduln/6/ZOvN+z9VcJpLGlrZd1pgybmIJXcYL+vFIeXYk xfLDPolT7uGf+dcWRghmxiYX4tahaCnSR5aeEPwRAymKsPn49jUb7Ime1t7+3qA= =dbR4 -----END PGP SIGNATURE----- --adXvwXsT0CDopb2C4rh8np0fOkSNWHnoM--