[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 0/4] block: Avoid copy-on-read assertions
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH 0/4] block: Avoid copy-on-read assertions |
Date: |
Mon, 2 Oct 2017 10:10:02 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
On 10/02/2017 09:50 AM, Kevin Wolf wrote:
>> The warning is a false negative (the error message is actually pointing
>> to a line in bdrv_co_do_copy_on_readv - but the compiler must have
>> inlined it into bdrv_aligned_preadv) - the function is only ever called
>> with non-zero bytes, and therefore the 'while (cluster_bytes)' loop will
>> execute at least once, and ret always gets assigned. But the compiler
>> can't see that, so I'll squash this in:
>
> Well, you could help the compiler with this:
>
> assert(cluster_bytes > 0);
>
> Then it compiles. Unfortunately, the compiler was right and you weren't:
>
> $ ./qemu-io -C -c 'read 0 0' /tmp/test.qcow2
> qemu-io: block/io.c:988: bdrv_co_do_copy_on_readv: Assertion
> `cluster_bytes > 0' failed.
> Abgebrochen (Speicherabzug geschrieben)
>
> Maybe a case to add to the test?
Of course - that's a good test to have.
So it turns out the reason we get in with 0 length is:
ret = bdrv_is_allocated(bs, offset, bytes, &pnum);
if (ret < 0) {
goto out;
}
if (!ret || pnum != bytes) {
ret = bdrv_co_do_copy_on_readv(child, offset, bytes, qiov);
goto out;
}
because bdrv_is_allocated() returns 0 for a 0-length query. I wonder if
we should instead assert that bdrv_is_allocated/bdrv_get_block_status()
are given a non-zero size, and fix callers to avoid a 0-length query
(after all, it's tough to argue that the read is allocated in the
current layer or defers to a backing layer, when there is nothing to be
read).
>> +++ b/block/io.c
>> @@ -952,7 +952,7 @@ static int coroutine_fn
>> bdrv_co_do_copy_on_readv(BdrvChild *child,
>> int64_t cluster_offset;
>> unsigned int cluster_bytes;
>> size_t skip_bytes;
>> - int ret;
>> + int ret = 0;
>> int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer,
>> BDRV_REQUEST_MAX_BYTES);
>> unsigned int progress = 0;
>
> I would prefer a ret = 0 immediately before err: so that we'll still get
> warning if we forget assigning ret in any future error path.
Sure, I can take that approach instead. v2 coming up, after a bit more
of a wait for any other review comments on the main patch rather than
just this fixup.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature