From: Eric Blake <eblake@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
qemu-block@nongnu.org
Cc: fam@euphon.net, kwolf@redhat.com, qemu-stable@nongnu.org,
qemu-devel@nongnu.org, mreitz@redhat.com, stefanha@redhat.com,
den@openvz.org
Subject: Re: [PATCH 4/5] block/io: fix bdrv_co_do_pwrite_zeroes head calculation
Date: Fri, 13 Mar 2020 16:47:58 -0500 [thread overview]
Message-ID: <67a739c0-61c2-2134-50d5-559ed5f3a7e0@redhat.com> (raw)
In-Reply-To: <20200302100537.29058-5-vsementsov@virtuozzo.com>
On 3/2/20 4:05 AM, Vladimir Sementsov-Ogievskiy wrote:
> It's wrong to update head using num in this place, as num may be
> reduced during the iteration, and we'll have wrong head value on next
> iteration.
>
> Instead update head at iteration end.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/io.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
Offhand, I don't see how this fixes any bug....
/me reads on
>
> diff --git a/block/io.c b/block/io.c
> index 75fd5600c2..c64566b4cf 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1785,7 +1785,6 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> * convenience, limit this request to max_transfer even if
> * we don't need to fall back to writes. */
> num = MIN(MIN(bytes, max_transfer), alignment - head);
> - head = (head + num) % alignment;
> assert(num < max_write_zeroes);
Here, we've asserted that if head was non-zero, num was already smaller
than max_write_zeroes. The rest of the loop does indeed have code that
appears like it can reduce num, but that code is guarded:
/* limit request size */
if (num > max_write_zeroes) {
num = max_write_zeroes;
}
...
if (ret == -ENOTSUP && !(flags & BDRV_REQ_NO_FALLBACK)) {
/* Fall back to bounce buffer if write zeroes is unsupported */
BdrvRequestFlags write_flags = flags & ~BDRV_REQ_ZERO_WRITE;
if ((flags & BDRV_REQ_FUA) &&
!(bs->supported_write_flags & BDRV_REQ_FUA)) {
/* No need for bdrv_driver_pwrite() to do a fallback
* flush on each chunk; use just one at the end */
write_flags &= ~BDRV_REQ_FUA;
need_flush = true;
}
num = MIN(num, max_transfer);
Oh. Now I see. If max_write_zeroes is > max_transfer, but we fall back
to a bounce buffer, it is indeed possible that a misaligned request that
forces fallbacks to writes may indeed require more than one write to get
to the point where it is then using a buffer aligned to max_write_zeroes.
Do we have an iotest provoking this, or is it theoretical? With an
iotest, this one is material for 5.0 even if the rest of the series
misses soft freeze.
> } else if (tail && num > alignment) {
> /* Shorten the request to the last aligned sector. */
> @@ -1844,6 +1843,9 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>
> offset += num;
> bytes -= num;
> + if (head) {
> + head = offset % alignment;
> + }
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
next prev parent reply other threads:[~2020-03-13 21:49 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-02 10:05 [PATCH 0/5] nbd: reduce max_block restrictions Vladimir Sementsov-Ogievskiy
2020-03-02 10:05 ` [PATCH 1/5] block/nbd-client: drop max_block restriction from block_status Vladimir Sementsov-Ogievskiy
2020-03-02 20:53 ` Eric Blake
2020-03-02 10:05 ` [PATCH 2/5] block/nbd-client: drop max_block restriction from discard Vladimir Sementsov-Ogievskiy
2020-03-02 20:53 ` Eric Blake
2020-03-02 10:05 ` [PATCH 3/5] block: add max_pwrite_zeroes_no_fallback to BlockLimits Vladimir Sementsov-Ogievskiy
2020-03-13 21:07 ` Eric Blake
2020-03-24 8:32 ` Vladimir Sementsov-Ogievskiy
2020-03-31 6:52 ` Vladimir Sementsov-Ogievskiy
2020-04-01 14:09 ` Vladimir Sementsov-Ogievskiy
2020-03-02 10:05 ` [PATCH 4/5] block/io: fix bdrv_co_do_pwrite_zeroes head calculation Vladimir Sementsov-Ogievskiy
2020-03-13 21:47 ` Eric Blake [this message]
2020-03-24 9:22 ` Vladimir Sementsov-Ogievskiy
2020-03-02 10:05 ` [PATCH 5/5] block/io: auto-no-fallback for write-zeroes Vladimir Sementsov-Ogievskiy
2020-03-13 21:56 ` Eric Blake
2020-04-01 14:35 ` Vladimir Sementsov-Ogievskiy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=67a739c0-61c2-2134-50d5-559ed5f3a7e0@redhat.com \
--to=eblake@redhat.com \
--cc=den@openvz.org \
--cc=fam@euphon.net \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-stable@nongnu.org \
--cc=stefanha@redhat.com \
--cc=vsementsov@virtuozzo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.