From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45527) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bBr2u-0008NM-Dj for qemu-devel@nongnu.org; Sat, 11 Jun 2016 18:06:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bBr2r-0005HB-PD for qemu-devel@nongnu.org; Sat, 11 Jun 2016 18:06:23 -0400 References: <1464973388-15821-1-git-send-email-eblake@redhat.com> <1464973388-15821-4-git-send-email-eblake@redhat.com> <20160607124522.GH4684@noname.str.redhat.com> From: Eric Blake Message-ID: <575C8B48.7030909@redhat.com> Date: Sat, 11 Jun 2016 16:06:00 -0600 MIME-Version: 1.0 In-Reply-To: <20160607124522.GH4684@noname.str.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="NWb0Sx0O3TSUkUiiMwLGCR4nuBcxVU59q" Subject: Re: [Qemu-devel] [PATCH 3/5] block: Switch transfer length bounds to byte-based List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, mreitz@redhat.com, qemu-block@nongnu.org, Stefan Hajnoczi , Fam Zheng , Ronnie Sahlberg , Paolo Bonzini , Peter Lieven , "Michael S. Tsirkin" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --NWb0Sx0O3TSUkUiiMwLGCR4nuBcxVU59q Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 06/07/2016 06:45 AM, Kevin Wolf wrote: > Am 03.06.2016 um 19:03 hat Eric Blake geschrieben: >> Sector-based limits are awkward to think about; in our on-going >> quest to move to byte-based interfaces, convert max_transfer_length >> and opt_transfer_length. Rename them (dropping the _length suffix) >> so that the compiler will help us catch the change in semantics >> across any rebased code. Improve the documentation, and guarantee >> that blk_get_max_transfer() returns a non-zero value. Use unsigned >> values, so that we don't have to worry about negative values and >> so that bit-twiddling is easier; however, we are still constrained >> by 2^31 of signed int in most APIs. >> >> Of note: the iscsi calculations use a 64-bit number internally, >> but the final result is guaranteed to fit in 32 bits. NBD is >> fixed to advertise the maximum length of 32M that the qemu and >> kernel servers enforce, rather than a number of sectors that >> would overflow int when converted back to bytes. scsi-generic >> now advertises a maximum always, rather than just when the >> underlying device advertised a maximum. >> >> Signed-off-by: Eric Blake >=20 >> @@ -1177,7 +1176,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes= (BlockDriverState *bs, >> >> if (ret =3D=3D -ENOTSUP) { >> /* Fall back to bounce buffer if write zeroes is unsuppor= ted */ >> - int max_xfer_len =3D MIN_NON_ZERO(bs->bl.max_transfer_len= gth, >> + int max_xfer_len =3D MIN_NON_ZERO(bs->bl.max_transfer, >> MAX_WRITE_ZEROES_BOUNCE_B= UFFER); >=20 > You could consider renaming the variable to max_transfer to keep it > consistent with the BlockLimits field name and to make it even more > obvious that you converted all uses. Good idea. >> @@ -1706,13 +1708,14 @@ static void iscsi_refresh_limits(BlockDriverSt= ate *bs, Error **errp) >> * iscsi_open(): iscsi targets don't change their limits. */ >> >> IscsiLun *iscsilun =3D bs->opaque; >> - uint32_t max_xfer_len =3D iscsilun->use_16_for_rw ? 0xffffffff : = 0xffff; >> + uint64_t max_xfer_len =3D iscsilun->use_16_for_rw ? 0xffffffff : = 0xffff; >> >> if (iscsilun->bl.max_xfer_len) { >> max_xfer_len =3D MIN(max_xfer_len, iscsilun->bl.max_xfer_len)= ; >> } >> >> - bs->bl.max_transfer_length =3D sector_limits_lun2qemu(max_xfer_le= n, iscsilun); >> + bs->bl.max_transfer =3D MIN(BDRV_REQUEST_MAX_SECTORS << BDRV_SECT= OR_BITS, >> + max_xfer_len * iscsilun->block_size); >=20 > Why don't you simply use 0 when you defined it as no limit? >=20 > If we make some drivers put 0 there and others INT_MAX, chances are tha= t > we'll introduce places where we fail to handle 0 correctly. So if I'm understanding correctly, we want something like: if (max_xfer_len * iscsilun->block_size > INT_MAX) { bs->bl.max_transfer =3D 0; } else { bs->bl.max_transfer =3D max_xfer_len * iscsilun->block_size; } and make sure that 0 continues to mean 'no signed 32-bit limit'. >> +++ b/block/nbd.c >> @@ -363,7 +363,7 @@ static int nbd_co_flush(BlockDriverState *bs) >> static void nbd_refresh_limits(BlockDriverState *bs, Error **errp) >> { >> bs->bl.max_discard =3D UINT32_MAX >> BDRV_SECTOR_BITS; >> - bs->bl.max_transfer_length =3D UINT32_MAX >> BDRV_SECTOR_BITS; >> + bs->bl.max_transfer =3D NBD_MAX_BUFFER_SIZE; >> } >=20 > This introduces a semantic difference and should therefore be a separat= e > patch. Here, it should become UINT32_MAX through mechanical conversion.= >=20 > Or actually, it can't because that's not a power of two. So maybe have > the NBD patch first... Will split. I don't see any problem with a max_transfer that is not a power of two, as long as it is a multiple of request_alignment (iscsi is a case in point - the max_transfer can be 0xffff blocks). >=20 >> static int nbd_co_discard(BlockDriverState *bs, int64_t sector_num, >> diff --git a/block/raw-posix.c b/block/raw-posix.c >> index ce2e20f..f3bd5ef 100644 >> --- a/block/raw-posix.c >> +++ b/block/raw-posix.c >> @@ -752,7 +752,7 @@ static void raw_refresh_limits(BlockDriverState *b= s, Error **errp) >> if (S_ISBLK(st.st_mode)) { >> int ret =3D hdev_get_max_transfer_length(s->fd); >> if (ret >=3D 0) { >> - bs->bl.max_transfer_length =3D ret; >> + bs->bl.max_transfer =3D ret << BDRV_SECTOR_BITS; >=20 > I assume that we don't care about overflows here because in practice th= e > values are very low? Should we check (or maybe better just cap) it > anyway? Will add a check. >> @@ -391,8 +391,9 @@ void virtio_blk_submit_multireq(BlockBackend *blk,= MultiReqBuffer *mrb) >> return; >> } >> >> - max_xfer_len =3D blk_get_max_transfer_length(mrb->reqs[0]->dev->b= lk); >> - max_xfer_len =3D MIN_NON_ZERO(max_xfer_len, BDRV_REQUEST_MAX_SECT= ORS); >> + max_xfer_len =3D blk_get_max_transfer(mrb->reqs[0]->dev->blk); >> + assert(max_xfer_len && >> + max_xfer_len >> BDRV_SECTOR_BITS <=3D BDRV_REQUEST_MAX_SEC= TORS); >=20 > Why can we assert this here? The comment for BlockLimits.max_xfer_len > doesn't document any maximum. Of course, as I already said above, it > might not happen in practice, but INT_MAX + 1 is theoretically valid an= d > would fail the assertion. As part of this patch, blk_get_max_transfer() guarantees that its result is non-zero and no larger than INT_MAX. >> +++ b/hw/scsi/scsi-generic.c >> @@ -225,13 +225,13 @@ static void scsi_read_complete(void * opaque, in= t ret) >> if (s->type =3D=3D TYPE_DISK && >> r->req.cmd.buf[0] =3D=3D INQUIRY && >> r->req.cmd.buf[2] =3D=3D 0xb0) { >> - uint32_t max_xfer_len =3D blk_get_max_transfer_length(s->conf= =2Eblk); >> - if (max_xfer_len) { >> - stl_be_p(&r->buf[8], max_xfer_len); >> - /* Also take care of the opt xfer len. */ >> - if (ldl_be_p(&r->buf[12]) > max_xfer_len) { >> - stl_be_p(&r->buf[12], max_xfer_len); >> - } >> + uint32_t max_xfer_len =3D >> + blk_get_max_transfer(s->conf.blk) >> BDRV_SECTOR_BITS; >> + >> + stl_be_p(&r->buf[8], max_xfer_len); >> + /* Also take care of the opt xfer len. */ >> + if (ldl_be_p(&r->buf[12]) > max_xfer_len) { >> + stl_be_p(&r->buf[12], max_xfer_len); >> } >> } >=20 > This is another hidden semantic change. Can we have a separate > scsi-generic patch first that changes the handling for the > max_transfer =3D=3D 0 case and only then make the change in > blk_get_max_transfer() as pure refactoring without a change in > behaviour? Will split. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --NWb0Sx0O3TSUkUiiMwLGCR4nuBcxVU59q 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 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJXXItIAAoJEKeha0olJ0NqJaIH/14eFMrTmRoZzvQG9F2orO36 HPR/nl32r3sCElGpV1xBOTRXuoNH+ILsA0Y8zMea/0dvaaJ7dyZCkqtHfUtdW4vB HlR0CAkQCsb5/lQf3U8NEXFKKginoQ927C+Cf+xp1fhXa/7WbKMEfFYx6beMrAVi y2BpGBIXFMRPGJvYWIgk/U8ofNkgv3+xtAwCftLqQJjS7JGf/95qPvboCQAHnbKt 6TyVluPOTA1mH9Q+QeR90Q5paSVuXeeJS5m3fw6KJXMagiXJv5s7mY0zDwQQTtnA 79NFlilINopuOpoXj8xi6Nq50kSqOBLaji3TzxZKoAdvFWc/njt6qm6n6TTGyuo= =xd9b -----END PGP SIGNATURE----- --NWb0Sx0O3TSUkUiiMwLGCR4nuBcxVU59q--