[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-stable] [PATCH v2 4/9] block: Return -ENOTSUP rather than asse
From: |
Max Reitz |
Subject: |
Re: [Qemu-stable] [PATCH v2 4/9] block: Return -ENOTSUP rather than assert on unaligned discards |
Date: |
Thu, 17 Nov 2016 23:01:36 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 |
On 17.11.2016 21:13, Eric Blake wrote:
> Right now, the block layer rounds discard requests, so that
> individual drivers are able to assert that discard requests
> will never be unaligned. But there are some ISCSI devices
> that track and coalesce multiple unaligned requests, turning it
> into an actual discard if the requests eventually cover an
> entire page, which implies that it is better to always pass
> discard requests as low down the stack as possible.
>
> In isolation, this patch has no semantic effect, since the
> block layer currently never passes an unaligned request through.
> But the block layer already has code that silently ignores
> drivers that return -ENOTSUP for a discard request that cannot
> be honored (as well as drivers that return 0 even when nothing
> was done). But the next patch will update the block layer to
> fragment discard requests, so that clients are guaranteed that
> they are either dealing with an unaligned head or tail, or an
> aligned core, making it similar to the block layer semantics of
> write zero fragmentation.
>
> CC: address@hidden
> Signed-off-by: Eric Blake <address@hidden>
> ---
> block/iscsi.c | 4 +++-
> block/qcow2.c | 5 +++++
> block/sheepdog.c | 5 +++--
> 3 files changed, 11 insertions(+), 3 deletions(-)
OK, so much about my remark that the comment describing
pdiscard_alignment only says it's an "optimal alignment"...
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 71bd523..0960929 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1083,7 +1083,9 @@ coroutine_fn iscsi_co_pdiscard(BlockDriverState *bs,
> int64_t offset, int count)
> struct IscsiTask iTask;
> struct unmap_list list;
>
> - assert(is_byte_request_lun_aligned(offset, count, iscsilun));
> + if (!is_byte_request_lun_aligned(offset, count, iscsilun)) {
> + return -ENOTSUP;
> + }
>
> if (!iscsilun->lbp.lbpu) {
> /* UNMAP is not supported by the target */
Next line is:
> return 0;
Hmm... -ENOTSUP would be the obvious return value here, too. That might
interfere with your next patch, though.
> diff --git a/block/qcow2.c b/block/qcow2.c
> index e22f6dc..7cfcd84 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2491,6 +2491,11 @@ static coroutine_fn int
> qcow2_co_pdiscard(BlockDriverState *bs,
> int ret;
> BDRVQcow2State *s = bs->opaque;
>
> + if (!QEMU_IS_ALIGNED(offset | count, s->cluster_size)) {
Ha! I like "offset | count".
> + assert(count < s->cluster_size);
Maybe add a comment for this assertion? E.g. "The block layer will only
generate unaligned discard requests that are smaller than the alignment".
> + return -ENOTSUP;
> + }
> +
> qemu_co_mutex_lock(&s->lock);
> ret = qcow2_discard_clusters(bs, offset, count >> BDRV_SECTOR_BITS,
> QCOW2_DISCARD_REQUEST, false);
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 1fb9173..4c9af89 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -2829,8 +2829,9 @@ static coroutine_fn int sd_co_pdiscard(BlockDriverState
> *bs, int64_t offset,
> iov.iov_len = sizeof(zero);
> discard_iov.iov = &iov;
> discard_iov.niov = 1;
> - assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> - assert((count & (BDRV_SECTOR_SIZE - 1)) == 0);
> + if (!QEMU_IS_ALIGNED(offset | count, BDRV_SECTOR_SIZE)) {
> + return -ENOTSUP;
> + }
Out of interest: Where does sheepdog tell the block layer that requests
have to be aligned to this value?
With this patch, it doesn't matter though, it only did before, so:
Reviewed-by: Max Reitz <address@hidden>
> acb = sd_aio_setup(bs, &discard_iov, offset >> BDRV_SECTOR_BITS,
> count >> BDRV_SECTOR_BITS);
> acb->aiocb_type = AIOCB_DISCARD_OBJ;
>
signature.asc
Description: OpenPGP digital signature