From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34510) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bGKpo-0005G9-AZ for qemu-devel@nongnu.org; Fri, 24 Jun 2016 02:43:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bGKpl-0000yK-Ld for qemu-devel@nongnu.org; Fri, 24 Jun 2016 02:43:23 -0400 Date: Fri, 24 Jun 2016 14:43:10 +0800 From: Fam Zheng Message-ID: <20160624064310.GC13266@ad.usersys.redhat.com> References: <1466721446-27737-1-git-send-email-eblake@redhat.com> <1466721446-27737-18-git-send-email-eblake@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1466721446-27737-18-git-send-email-eblake@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 17/22] block: Switch discard length bounds to byte-based List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, kwolf@redhat.com, stefanha@redhat.com, Max Reitz , Ronnie Sahlberg , Paolo Bonzini , Peter Lieven On Thu, 06/23 16:37, Eric Blake wrote: > 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. > > pdiscard_alignment is made unsigned (we use power-of-2 alignments > as bitmasks, where unsigned is easier to think about) while > leaving max_pdiscard signed (since we still have an 'int' > interface); this is comparable to what commit cf081fc did for > write zeroes limits. We may later want to make everything an > unsigned 64-bit limit - but that requires a bigger code audit. > > Signed-off-by: Eric Blake > > --- > v3: split out write_zeroes wording tweaks, improve commit message > v2: rebase nbd and iscsi limits across earlier improvements > --- > include/block/block_int.h | 14 ++++++++++---- > block/io.c | 16 +++++++++------- > block/iscsi.c | 19 ++++++------------- > block/nbd.c | 2 +- > qemu-img.c | 3 ++- > 5 files changed, 28 insertions(+), 26 deletions(-) > > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 7a4a00f..388ef80 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; > + /* 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 sectors */ > - int64_t discard_alignment; > + /* optimal alignment for discard requests in bytes, must be power > + * of 2, less than max_discard if that is set, and multiple of s/max_discard/max_pdiscard/ > + * 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/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; Or just num = discard_alignment - sector_num % discard_alignment; without the if. Otherwise looks good, Reviewed-by: Fam Zheng > } > > /* limit request size */ > diff --git a/block/iscsi.c b/block/iscsi.c > index 368687d..0d16c31 100644 > --- a/block/iscsi.c > +++ b/block/iscsi.c > @@ -1696,13 +1696,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 > @@ -1722,14 +1715,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) {