[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/5] block: Switch transfer length bounds to byt
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 3/5] block: Switch transfer length bounds to byte-based |
Date: |
Tue, 14 Jun 2016 10:20:38 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
Re: [Qemu-devel] [PATCH 5/5] block: Move request_alignment into BlockLimit, Kevin Wolf, 2016/06/07
[Qemu-devel] [PATCH 3/5] block: Switch transfer length bounds to byte-based, Eric Blake, 2016/06/03
[Qemu-devel] [PATCH 1/5] block: Tighter assertions on bdrv_aligned_preadv(), Eric Blake, 2016/06/03
[Qemu-devel] [PATCH 2/5] block: Honor flags during bdrv_aligned_preadv(), Eric Blake, 2016/06/03
[Qemu-devel] [PATCH 6/5] block: Fix harmless off-by-one in bdrv_aligned_preadv(), Eric Blake, 2016/06/03
[Qemu-devel] [PATCH 7/5] block: Refactor zero_beyond_eof hack in bdrv_aligned_preadv(), Eric Blake, 2016/06/03