From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57157) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1am2jH-000331-HJ for qemu-devel@nongnu.org; Fri, 01 Apr 2016 13:19:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1am2jG-0008Gp-Dx for qemu-devel@nongnu.org; Fri, 01 Apr 2016 13:19:27 -0400 References: <1459513321-3776-1-git-send-email-olaf@aepfle.de> From: Max Reitz Message-ID: <56FEAD92.5010802@redhat.com> Date: Fri, 1 Apr 2016 19:19:14 +0200 MIME-Version: 1.0 In-Reply-To: <1459513321-3776-1-git-send-email-olaf@aepfle.de> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="AKsiWDbRrWGFbJIrv36CbaVCNbJ9QE5F6" Subject: Re: [Qemu-devel] [Qemu-block] [PATCH] block: split large discard requests from block frontend List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Olaf Hering , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --AKsiWDbRrWGFbJIrv36CbaVCNbJ9QE5F6 Content-Type: multipart/mixed; boundary="ThQqISrkRLGk6cXQQ0CTe9pjtNac86tQc" From: Max Reitz To: Olaf Hering , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi Message-ID: <56FEAD92.5010802@redhat.com> Subject: Re: [Qemu-block] [PATCH] block: split large discard requests from block frontend References: <1459513321-3776-1-git-send-email-olaf@aepfle.de> In-Reply-To: <1459513321-3776-1-git-send-email-olaf@aepfle.de> --ThQqISrkRLGk6cXQQ0CTe9pjtNac86tQc Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 01.04.2016 14:22, Olaf Hering wrote: > Large discard requests lead to sign expansion errors in qemu. > Since there is no API to tell a guest about the limitations qmeu > has to split a large request itself. >=20 > Signed-off-by: Olaf Hering > Cc: Stefan Hajnoczi > Cc: Kevin Wolf > --- > block/io.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) Hi! Let me first nag about some style problems, and then I'll come to the technical/design aspect. :-) > diff --git a/block/io.c b/block/io.c > index c4869b9..5b6ed58 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -2442,7 +2442,7 @@ static void coroutine_fn bdrv_discard_co_entry(vo= id *opaque) > rwco->ret =3D bdrv_co_discard(rwco->bs, rwco->sector_num, rwco->nb= _sectors); > } > =20 > -int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_= num, > +static int __bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,= Two things: First, I think we do not like identifiers to start with underscores, especially not with two underscores or with underscore and a capital letter, because in C those combinations are generally reserved for compiler/language extensions or for the operating system (think of __attribute__(), __asm__(), or _Bool). Second, the coroutine_fn should stay. This is just an empty macro (I think) that signifies that a certain function is run inside of a coroutine. That fact will not change if you put a wrapper around it. > int nb_sectors) This should be aligned to the opening paranthesis of the line above. > { > BdrvTrackedRequest req; > @@ -2524,6 +2524,26 @@ out: > return ret; > } > =20 > +int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_= num, > + int nb_sectors) > +{ > + int num, ret; > + int limit =3D BDRV_REQUEST_MAX_SECTORS; > + int remaining =3D nb_sectors; > + int64_t sector_offset =3D sector_num; > + > + do { > + num =3D remaining > limit ? limit : remaining; num =3D MIN(limit, remaining); would work just as fine and maybe look a bit better. > + ret =3D __bdrv_co_discard(bs, sector_offset, num); > + if (ret < 0) > + break; In qemu, every block is enclosed by {} braces, even if it contains only a single statement. This is something that the checkpatch script (scripts/checkpatch.pl in the qemu tree) can detect. It is advisible to run that scripts on patches before they are sent to the mailing list. > + remaining -=3D num; > + sector_offset +=3D num; > + } while (remaining > 0); > + > + return ret; > +} > + > int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sect= ors) > { > Coroutine *co; >=20 So, now about the technical aspect: Guest requests are not issued to BlockDriverStates directly, they pass through a BlockBackend first. The check whether the request is too large is done there (in blk_check_request() called by blk_co_discard() and blk_aio_discard()). Thus, if a discard request exceeds BDRV_REQUEST_MAX_SECTORS, it will be denied at the BlockBackend level and not reach bdrv_co_discard() at all. At least that is how it's supposed to be. If it isn't, that's a bug. ;-) Finally, I'm not sure whether this is something that should be done in the block layer. I personally feel like it is the device models' responsibility to only submit requests that adhere to the limits established by the block layer. In any case, do you have a test case where a guest was able to submit a request that led to the overflow error you described in the commit messag= e? Max --ThQqISrkRLGk6cXQQ0CTe9pjtNac86tQc-- --AKsiWDbRrWGFbJIrv36CbaVCNbJ9QE5F6 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 iQEcBAEBCAAGBQJW/q2SAAoJEDuxQgLoOKytBVoIAKY7W7tUUDlj8pGAQqRnujgy HpfXrzm0VXgXaIz8WmCLB94UvOfPhzIv0mgywLGfEGb9++jguHqsu63Ev0pyvc8P iXW0P391yBf75mhUfM8UevjZHi/lOXUu6/oRwSWRzu+JyLyDBGB1EGZCnZGNm6rM baIxujKbgnQCYBm8Z10zigR9rrv22nJ5ZU1bJ2FKbi9pBMGOghn6Rp9EnIvp3v4k RLPRikheyd6i8DZbFutNckPXnoQKgqXgQtgZfiD4y3yya76aOxYOQi21+ibY8GQ5 /96aIhyCa2vPNHdE+EIBNqCEE/X5S29GJUuDQt1iR0MqyjjfjL52TqxZnB0Z0KE= =7QgG -----END PGP SIGNATURE----- --AKsiWDbRrWGFbJIrv36CbaVCNbJ9QE5F6--