Re: [Qemu-devel] [PATCH 3/5] block: Switch transfer length bounds to byte-based

[Kevin Wolf]

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 <address@hidden>
> > 
> >> @@ -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

pgpM1wtRWLt7T.pgp
Description: PGP signature