Message ID | 1467733852-27097-19-git-send-email-kwolf@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 07/05/2016 09:50 AM, Kevin Wolf wrote: > From: Eric Blake <eblake@redhat.com> > > Sector-based limits are awkward to think about; in our on-going > quest to move to byte-based interfaces, convert max_discard and > discard_alignment. Rename them, using 'pdiscard' as an aid to > track which remaining discard interfaces need conversion, and so > that the compiler will help us catch the change in semantics > across any rebased code. The BlockLimits type is now completely > byte-based; and in iscsi.c, sector_limits_lun2qemu() is no > longer needed. > > +++ b/include/block/block_int.h > @@ -324,11 +324,17 @@ struct BlockDriver { > }; > > typedef struct BlockLimits { > - /* maximum number of sectors that can be discarded at once */ > - int max_discard; > - > - /* optimal alignment for discard requests in sectors */ > - int64_t discard_alignment; > + /* maximum number of bytes that can be discarded at once (since it > + * is signed, it must be < 2G, if set), should be multiple of > + * pdiscard_alignment, but need not be power of 2. May be 0 if no > + * inherent 32-bit limit */ > + int32_t max_pdiscard; > + > + /* optimal alignment for discard requests in bytes, must be power > + * of 2, less than max_pdiscard if that is set, and multiple of > + * bs->request_alignment. May be 0 if bs->request_alignment is > + * good enough */ > + uint32_t pdiscard_alignment; Given the recent thread on an iscsi device with 15M optimum alignment for zero and discards, I guess I have some followup patches to write if we don't want to stall this pull request.
Am 06.07.2016 um 04:14 hat Eric Blake geschrieben: > On 07/05/2016 09:50 AM, Kevin Wolf wrote: > > From: Eric Blake <eblake@redhat.com> > > > > Sector-based limits are awkward to think about; in our on-going > > quest to move to byte-based interfaces, convert max_discard and > > discard_alignment. Rename them, using 'pdiscard' as an aid to > > track which remaining discard interfaces need conversion, and so > > that the compiler will help us catch the change in semantics > > across any rebased code. The BlockLimits type is now completely > > byte-based; and in iscsi.c, sector_limits_lun2qemu() is no > > longer needed. > > > > > +++ b/include/block/block_int.h > > @@ -324,11 +324,17 @@ struct BlockDriver { > > }; > > > > typedef struct BlockLimits { > > - /* maximum number of sectors that can be discarded at once */ > > - int max_discard; > > - > > - /* optimal alignment for discard requests in sectors */ > > - int64_t discard_alignment; > > + /* maximum number of bytes that can be discarded at once (since it > > + * is signed, it must be < 2G, if set), should be multiple of > > + * pdiscard_alignment, but need not be power of 2. May be 0 if no > > + * inherent 32-bit limit */ > > + int32_t max_pdiscard; > > + > > + /* optimal alignment for discard requests in bytes, must be power > > + * of 2, less than max_pdiscard if that is set, and multiple of > > + * bs->request_alignment. May be 0 if bs->request_alignment is > > + * good enough */ > > + uint32_t pdiscard_alignment; > > Given the recent thread on an iscsi device with 15M optimum alignment > for zero and discards, I guess I have some followup patches to write if > we don't want to stall this pull request. Please send a followup patch (series). Does this one actually change the behaviour or just document the behaviour that we already expected? Kevin
diff --git a/block/io.c b/block/io.c index 8ca9d43..0f15d05 100644 --- a/block/io.c +++ b/block/io.c @@ -2368,19 +2368,21 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, goto out; } - max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS); + max_discard = MIN_NON_ZERO(bs->bl.max_pdiscard >> BDRV_SECTOR_BITS, + BDRV_REQUEST_MAX_SECTORS); while (nb_sectors > 0) { int ret; int num = nb_sectors; + int discard_alignment = bs->bl.pdiscard_alignment >> BDRV_SECTOR_BITS; /* align request */ - if (bs->bl.discard_alignment && - num >= bs->bl.discard_alignment && - sector_num % bs->bl.discard_alignment) { - if (num > bs->bl.discard_alignment) { - num = bs->bl.discard_alignment; + if (discard_alignment && + num >= discard_alignment && + sector_num % discard_alignment) { + if (num > discard_alignment) { + num = discard_alignment; } - num -= sector_num % bs->bl.discard_alignment; + num -= sector_num % discard_alignment; } /* limit request size */ diff --git a/block/iscsi.c b/block/iscsi.c index bde4a04..342f6b8 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -1697,13 +1697,6 @@ static void iscsi_close(BlockDriverState *bs) memset(iscsilun, 0, sizeof(IscsiLun)); } -static int sector_limits_lun2qemu(int64_t sector, IscsiLun *iscsilun) -{ - int limit = MIN(sector_lun2qemu(sector, iscsilun), INT_MAX / 2 + 1); - - return limit < BDRV_REQUEST_MAX_SECTORS ? limit : 0; -} - static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) { /* We don't actually refresh here, but just return data queried in @@ -1723,14 +1716,14 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) } if (iscsilun->lbp.lbpu) { - if (iscsilun->bl.max_unmap < 0xffffffff) { - bs->bl.max_discard = - sector_limits_lun2qemu(iscsilun->bl.max_unmap, iscsilun); + if (iscsilun->bl.max_unmap < 0xffffffff / iscsilun->block_size) { + bs->bl.max_pdiscard = + iscsilun->bl.max_unmap * iscsilun->block_size; } - bs->bl.discard_alignment = - sector_limits_lun2qemu(iscsilun->bl.opt_unmap_gran, iscsilun); + bs->bl.pdiscard_alignment = + iscsilun->bl.opt_unmap_gran * iscsilun->block_size; } else { - bs->bl.discard_alignment = iscsilun->block_size >> BDRV_SECTOR_BITS; + bs->bl.pdiscard_alignment = iscsilun->block_size; } if (iscsilun->bl.max_ws_len < 0xffffffff / iscsilun->block_size) { diff --git a/block/nbd.c b/block/nbd.c index f5511ea..08e5b67 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -362,7 +362,7 @@ static int nbd_co_flush(BlockDriverState *bs) static void nbd_refresh_limits(BlockDriverState *bs, Error **errp) { - bs->bl.max_discard = NBD_MAX_SECTORS; + bs->bl.max_pdiscard = NBD_MAX_BUFFER_SIZE; bs->bl.max_transfer = NBD_MAX_BUFFER_SIZE; } diff --git a/include/block/block_int.h b/include/block/block_int.h index 7a4a00f..a3e69fd 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -324,11 +324,17 @@ struct BlockDriver { }; typedef struct BlockLimits { - /* maximum number of sectors that can be discarded at once */ - int max_discard; - - /* optimal alignment for discard requests in sectors */ - int64_t discard_alignment; + /* maximum number of bytes that can be discarded at once (since it + * is signed, it must be < 2G, if set), should be multiple of + * pdiscard_alignment, but need not be power of 2. May be 0 if no + * inherent 32-bit limit */ + int32_t max_pdiscard; + + /* optimal alignment for discard requests in bytes, must be power + * of 2, less than max_pdiscard if that is set, and multiple of + * bs->request_alignment. May be 0 if bs->request_alignment is + * good enough */ + uint32_t pdiscard_alignment; /* maximum number of bytes that can zeroized at once (since it is * signed, it must be < 2G, if set), should be multiple of diff --git a/qemu-img.c b/qemu-img.c index 046b267..3a7c162 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -2091,7 +2091,8 @@ static int img_convert(int argc, char **argv) bufsectors = MIN(32768, MAX(bufsectors, MAX(out_bs->bl.opt_transfer >> BDRV_SECTOR_BITS, - out_bs->bl.discard_alignment))); + out_bs->bl.pdiscard_alignment >> + BDRV_SECTOR_BITS))); if (skip_create) { int64_t output_sectors = blk_nb_sectors(out_blk);