[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 4/9] block: treat BDRV_REQ_ALLOCATE as serial
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v7 4/9] block: treat BDRV_REQ_ALLOCATE as serialising |
Date: |
Wed, 31 Jan 2018 18:35:41 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 |
On 2018-01-30 13:36, Anton Nefedov wrote:
>
>
> On 29/1/2018 10:48 PM, Max Reitz wrote:
>> On 2018-01-18 18:49, Anton Nefedov wrote:
>>> The idea is that ALLOCATE requests may overlap with other requests.
>>> Reuse the existing block layer infrastructure for serialising requests.
>>> Use the following approach:
>>> - mark ALLOCATE serialising, so subsequent requests to the area wait
>>> - ALLOCATE request itself must never wait if another request is in
>>> flight
>>> already. Return EAGAIN, let the caller reconsider.
>>>
>>> Signed-off-by: Anton Nefedov <address@hidden>
>>> Reviewed-by: Eric Blake <address@hidden>
>>> ---
>>> block/io.c | 27 +++++++++++++++++++--------
>>> 1 file changed, 19 insertions(+), 8 deletions(-)
>>
>> The basic principle looks good to me.
>>
>>> diff --git a/block/io.c b/block/io.c
>>> index cf2f84c..4b0d34f 100644
>>> --- a/block/io.c
>>> +++ b/block/io.c
>>
>> [...]
>>
>>> @@ -1717,7 +1728,7 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
>>> struct iovec head_iov;
>>> mark_request_serialising(&req, align);
>>> - wait_serialising_requests(&req);
>>> + wait_serialising_requests(&req, false);
>>
>> What if someone calls bdrv_co_pwritev() with BDRV_REQ_ZERO_WRITE |
>> BDRV_REQ_ALLOCATE?
>
> Either
>
> assert(!(qiov && (flags & BDRV_REQ_ALLOCATE)));
>
> will fail or bdrv_co_do_zero_pwritev() will be used.
Ah, right, I didn't see that condition there.
>> .. Then this should do exactly the same as
>> bdrv_co_do_zero_pwritev(), which it currently does not -- besides this
>> serialization, this includes returning -ENOTSUP if there is a head or
>> tail to write.
>>
>
> Another question is if that assertion is ok.
> In other words: should (qiov!=NULL && REQ_ALLOCATE) be a valid case?
> e.g. with qiov filled with zeroes?
I think it's OK to leave the assertion that way. But maybe move it
down, under the bdrv_co_do_zero_pwritev() call (and then omit the qiov
!= NULL, because that's guaranteed then)?
(But maybe not everyone's as blind as me.)
> I'd rather document that not supported (and leave the assertion).
>
> Actually, even (qiov!=NULL && REQ_ZERO_WRITE) looks kind of
> unsupported/broken? Alignment code in bdrv_co_pwritev() zeroes out the
> head and tail by passing the flag down bdrv_aligned_pwritev()
Yes. Maybe we should have an assertion that you aren't allowed to pass
a qiov with REQ_ZERO_WRITE...
Max
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v7 0/9] qcow2: cluster space preallocation, Anton Nefedov, 2018/01/18
- [Qemu-devel] [PATCH v7 6/9] block: support BDRV_REQ_ALLOCATE in passthrough drivers, Anton Nefedov, 2018/01/18
- [Qemu-devel] [PATCH v7 5/9] file-posix: support BDRV_REQ_ALLOCATE, Anton Nefedov, 2018/01/18
- [Qemu-devel] [PATCH v7 2/9] blkverify: set supported write/zero flags, Anton Nefedov, 2018/01/18
- [Qemu-devel] [PATCH v7 8/9] qcow2: skip writing zero buffers to empty COW areas, Anton Nefedov, 2018/01/18