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 <ebl...@redhat.com> > >> @@ -1177,7 +1176,7 @@ static int coroutine_fn >> bdrv_co_do_pwrite_zeroes(BlockDriverState *bs, >> >> if (ret == -ENOTSUP) { >> /* Fall back to bounce buffer if write zeroes is unsupported */ >> - int max_xfer_len = MIN_NON_ZERO(bs->bl.max_transfer_length, >> + int max_xfer_len = MIN_NON_ZERO(bs->bl.max_transfer, >> MAX_WRITE_ZEROES_BOUNCE_BUFFER); > > 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(BlockDriverState >> *bs, Error **errp) >> * iscsi_open(): iscsi targets don't change their limits. */ >> >> IscsiLun *iscsilun = bs->opaque; >> - uint32_t max_xfer_len = iscsilun->use_16_for_rw ? 0xffffffff : 0xffff; >> + uint64_t max_xfer_len = iscsilun->use_16_for_rw ? 0xffffffff : 0xffff; >> >> if (iscsilun->bl.max_xfer_len) { >> max_xfer_len = MIN(max_xfer_len, iscsilun->bl.max_xfer_len); >> } >> >> - bs->bl.max_transfer_length = sector_limits_lun2qemu(max_xfer_len, >> iscsilun); >> + bs->bl.max_transfer = MIN(BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS, >> + max_xfer_len * iscsilun->block_size); > > Why don't you simply use 0 when you defined it as no limit? > > If we make some drivers put 0 there and others INT_MAX, chances are that > 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 = 0; } else { bs->bl.max_transfer = 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 = UINT32_MAX >> BDRV_SECTOR_BITS; >> - bs->bl.max_transfer_length = UINT32_MAX >> BDRV_SECTOR_BITS; >> + bs->bl.max_transfer = NBD_MAX_BUFFER_SIZE; >> } > > This introduces a semantic difference and should therefore be a separate > patch. Here, it should become UINT32_MAX through mechanical conversion. > > 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). > >> 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 *bs, >> Error **errp) >> if (S_ISBLK(st.st_mode)) { >> int ret = hdev_get_max_transfer_length(s->fd); >> if (ret >= 0) { >> - bs->bl.max_transfer_length = ret; >> + bs->bl.max_transfer = ret << BDRV_SECTOR_BITS; > > I assume that we don't care about overflows here because in practice the > 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 = blk_get_max_transfer_length(mrb->reqs[0]->dev->blk); >> - max_xfer_len = MIN_NON_ZERO(max_xfer_len, BDRV_REQUEST_MAX_SECTORS); >> + max_xfer_len = blk_get_max_transfer(mrb->reqs[0]->dev->blk); >> + assert(max_xfer_len && >> + max_xfer_len >> BDRV_SECTOR_BITS <= BDRV_REQUEST_MAX_SECTORS); > > 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 and > 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, int ret) >> if (s->type == TYPE_DISK && >> r->req.cmd.buf[0] == INQUIRY && >> r->req.cmd.buf[2] == 0xb0) { >> - uint32_t max_xfer_len = blk_get_max_transfer_length(s->conf.blk); >> - 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 = >> + 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); >> } >> } > > This is another hidden semantic change. Can we have a separate > scsi-generic patch first that changes the handling for the > max_transfer == 0 case and only then make the change in > blk_get_max_transfer() as pure refactoring without a change in > behaviour? Will split. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature