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: qemu-sta...@nongnu.org > Signed-off-by: Eric Blake <ebl...@redhat.com> > --- > 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 <mre...@redhat.com>
> 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
