Am 12.06.2016 um 00:06 hat Eric Blake geschrieben:
> 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'.Forget that, brain fart. Somehow I was thinking that just doing the assignment without MIN() would do the right thing. Which it very obviously doesn't. Kevin
pgpBBqFxDRsTj.pgp
Description: PGP signature
