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 <anton.nefe...@virtuozzo.com> >>> Reviewed-by: Eric Blake <ebl...@redhat.com> >>> --- >>> 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
