| Message ID | 20200821141123.28538-7-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | preallocate filter | expand |
On 21.08.20 16:11, Vladimir Sementsov-Ogievskiy wrote: > Add flag to make serialising request no wait: if there are conflicting > requests, just return error immediately. It's will be used in upcoming > preallocate filter. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/block/block.h | 9 ++++++++- > block/io.c | 11 ++++++++++- > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/include/block/block.h b/include/block/block.h > index b8f4e86e8d..877fda06a4 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -67,8 +67,15 @@ typedef enum { > * written to qiov parameter which may be NULL. > */ > BDRV_REQ_PREFETCH = 0x200, > + > + /* > + * If we need to wait for other requests, just fail immediately. Used > + * only together with BDRV_REQ_SERIALISING. > + */ > + BDRV_REQ_NO_WAIT = 0x400, > + > /* Mask of valid flags */ > - BDRV_REQ_MASK = 0x3ff, > + BDRV_REQ_MASK = 0x7ff, > } BdrvRequestFlags; > > typedef struct BlockSizes { > diff --git a/block/io.c b/block/io.c > index dd28befb08..c93b1e98a3 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -1912,9 +1912,18 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, uint64_t bytes, > assert(!(bs->open_flags & BDRV_O_INACTIVE)); > assert((bs->open_flags & BDRV_O_NO_IO) == 0); > assert(!(flags & ~BDRV_REQ_MASK)); > + assert(!((flags & BDRV_REQ_NO_WAIT) && !(flags & BDRV_REQ_SERIALISING))); > > if (flags & BDRV_REQ_SERIALISING) { > - bdrv_make_request_serialising(req, bdrv_get_cluster_size(bs)); > + QEMU_LOCK_GUARD(&bs->reqs_lock); > + > + tracked_request_set_serialising(req, bdrv_get_cluster_size(bs)); > + > + if ((flags & BDRV_REQ_NO_WAIT) && bdrv_find_conflicting_request(req)) { bdrv_find_conflicting_request() will return NULL even if there are conflicting requests, but those have a non-NULL waiting_for. Is that something to consider? (I would like to think that will never have a real impact because then we must find some other conflicting request; but isn’t is possible that we find an overlapping request that waits for another request with which it overlaps, but our request does not?) Max > + return -EBUSY; > + } > + > + bdrv_wait_serialising_requests_locked(req); > } else { > bdrv_wait_serialising_requests(req); > } >
25.08.2020 16:10, Max Reitz wrote: > On 21.08.20 16:11, Vladimir Sementsov-Ogievskiy wrote: >> Add flag to make serialising request no wait: if there are conflicting >> requests, just return error immediately. It's will be used in upcoming >> preallocate filter. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> include/block/block.h | 9 ++++++++- >> block/io.c | 11 ++++++++++- >> 2 files changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/include/block/block.h b/include/block/block.h >> index b8f4e86e8d..877fda06a4 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -67,8 +67,15 @@ typedef enum { >> * written to qiov parameter which may be NULL. >> */ >> BDRV_REQ_PREFETCH = 0x200, >> + >> + /* >> + * If we need to wait for other requests, just fail immediately. Used >> + * only together with BDRV_REQ_SERIALISING. >> + */ >> + BDRV_REQ_NO_WAIT = 0x400, >> + >> /* Mask of valid flags */ >> - BDRV_REQ_MASK = 0x3ff, >> + BDRV_REQ_MASK = 0x7ff, >> } BdrvRequestFlags; >> >> typedef struct BlockSizes { >> diff --git a/block/io.c b/block/io.c >> index dd28befb08..c93b1e98a3 100644 >> --- a/block/io.c >> +++ b/block/io.c >> @@ -1912,9 +1912,18 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, uint64_t bytes, >> assert(!(bs->open_flags & BDRV_O_INACTIVE)); >> assert((bs->open_flags & BDRV_O_NO_IO) == 0); >> assert(!(flags & ~BDRV_REQ_MASK)); >> + assert(!((flags & BDRV_REQ_NO_WAIT) && !(flags & BDRV_REQ_SERIALISING))); >> >> if (flags & BDRV_REQ_SERIALISING) { >> - bdrv_make_request_serialising(req, bdrv_get_cluster_size(bs)); >> + QEMU_LOCK_GUARD(&bs->reqs_lock); >> + >> + tracked_request_set_serialising(req, bdrv_get_cluster_size(bs)); >> + >> + if ((flags & BDRV_REQ_NO_WAIT) && bdrv_find_conflicting_request(req)) { > > bdrv_find_conflicting_request() will return NULL even if there are > conflicting requests, but those have a non-NULL waiting_for. Is that > something to consider? > > (I would like to think that will never have a real impact because then > we must find some other conflicting request; but isn’t is possible that > we find an overlapping request that waits for another request with which > it overlaps, but our request does not?) > Actually check in bdrv_find_conflicting_request() is the same like in the following bdrv_wait_serialising_requests_locked(), so, if bdrv_find_conflicting_request() returns NULL, it means that in bdrv_wait_serialising_requests_locked() it will return NULL again (as there are no yield points) and we will not wait, so all is OK. And, why is it OK to ignore already waiting requests in bdrv_wait_serialising_requests_locked(): just because if we proceed now with our request, these waiting requests will have to wait for us, when they wake and go to the next iteration of waiting loop. > >> + return -EBUSY; >> + } >> + >> + bdrv_wait_serialising_requests_locked(req); >> } else { >> bdrv_wait_serialising_requests(req); >> } >> > >
On 26.08.20 08:26, Vladimir Sementsov-Ogievskiy wrote: > 25.08.2020 16:10, Max Reitz wrote: >> On 21.08.20 16:11, Vladimir Sementsov-Ogievskiy wrote: >>> Add flag to make serialising request no wait: if there are conflicting >>> requests, just return error immediately. It's will be used in upcoming >>> preallocate filter. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> include/block/block.h | 9 ++++++++- >>> block/io.c | 11 ++++++++++- >>> 2 files changed, 18 insertions(+), 2 deletions(-) >>> >>> diff --git a/include/block/block.h b/include/block/block.h >>> index b8f4e86e8d..877fda06a4 100644 >>> --- a/include/block/block.h >>> +++ b/include/block/block.h >>> @@ -67,8 +67,15 @@ typedef enum { >>> * written to qiov parameter which may be NULL. >>> */ >>> BDRV_REQ_PREFETCH = 0x200, >>> + >>> + /* >>> + * If we need to wait for other requests, just fail immediately. >>> Used >>> + * only together with BDRV_REQ_SERIALISING. >>> + */ >>> + BDRV_REQ_NO_WAIT = 0x400, >>> + >>> /* Mask of valid flags */ >>> - BDRV_REQ_MASK = 0x3ff, >>> + BDRV_REQ_MASK = 0x7ff, >>> } BdrvRequestFlags; >>> typedef struct BlockSizes { >>> diff --git a/block/io.c b/block/io.c >>> index dd28befb08..c93b1e98a3 100644 >>> --- a/block/io.c >>> +++ b/block/io.c >>> @@ -1912,9 +1912,18 @@ bdrv_co_write_req_prepare(BdrvChild *child, >>> int64_t offset, uint64_t bytes, >>> assert(!(bs->open_flags & BDRV_O_INACTIVE)); >>> assert((bs->open_flags & BDRV_O_NO_IO) == 0); >>> assert(!(flags & ~BDRV_REQ_MASK)); >>> + assert(!((flags & BDRV_REQ_NO_WAIT) && !(flags & >>> BDRV_REQ_SERIALISING))); >>> if (flags & BDRV_REQ_SERIALISING) { >>> - bdrv_make_request_serialising(req, bdrv_get_cluster_size(bs)); >>> + QEMU_LOCK_GUARD(&bs->reqs_lock); >>> + >>> + tracked_request_set_serialising(req, >>> bdrv_get_cluster_size(bs)); >>> + >>> + if ((flags & BDRV_REQ_NO_WAIT) && >>> bdrv_find_conflicting_request(req)) { >> >> bdrv_find_conflicting_request() will return NULL even if there are >> conflicting requests, but those have a non-NULL waiting_for. Is that >> something to consider? >> >> (I would like to think that will never have a real impact because then >> we must find some other conflicting request; but isn’t is possible that >> we find an overlapping request that waits for another request with which >> it overlaps, but our request does not?) >> > > Actually check in bdrv_find_conflicting_request() is the same like in > the following > bdrv_wait_serialising_requests_locked(), so, if > bdrv_find_conflicting_request() returns > NULL, it means that in bdrv_wait_serialising_requests_locked() it will > return NULL > again (as there are no yield points) and we will not wait, so all is OK. OK. I thought that maybe we would want to avoid that other requests might have to wait for the preallocation write. (Of course, we can’t avoid that altogether, but if we already know of such requests at the beginning of the request...) Well, if the only thing to look out for is that preallocation writes themselves do not wait: Reviewed-by: Max Reitz <mreitz@redhat.com> > And, why is it OK to ignore already waiting requests in > bdrv_wait_serialising_requests_locked(): just because if we proceed now > with our request, > these waiting requests will have to wait for us, when they wake and go > to the next iteration > of waiting loop. Sure.
26.08.2020 11:36, Max Reitz wrote: > On 26.08.20 08:26, Vladimir Sementsov-Ogievskiy wrote: >> 25.08.2020 16:10, Max Reitz wrote: >>> On 21.08.20 16:11, Vladimir Sementsov-Ogievskiy wrote: >>>> Add flag to make serialising request no wait: if there are conflicting >>>> requests, just return error immediately. It's will be used in upcoming >>>> preallocate filter. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> include/block/block.h | 9 ++++++++- >>>> block/io.c | 11 ++++++++++- >>>> 2 files changed, 18 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/include/block/block.h b/include/block/block.h >>>> index b8f4e86e8d..877fda06a4 100644 >>>> --- a/include/block/block.h >>>> +++ b/include/block/block.h >>>> @@ -67,8 +67,15 @@ typedef enum { >>>> * written to qiov parameter which may be NULL. >>>> */ >>>> BDRV_REQ_PREFETCH = 0x200, >>>> + >>>> + /* >>>> + * If we need to wait for other requests, just fail immediately. >>>> Used >>>> + * only together with BDRV_REQ_SERIALISING. >>>> + */ >>>> + BDRV_REQ_NO_WAIT = 0x400, >>>> + >>>> /* Mask of valid flags */ >>>> - BDRV_REQ_MASK = 0x3ff, >>>> + BDRV_REQ_MASK = 0x7ff, >>>> } BdrvRequestFlags; >>>> typedef struct BlockSizes { >>>> diff --git a/block/io.c b/block/io.c >>>> index dd28befb08..c93b1e98a3 100644 >>>> --- a/block/io.c >>>> +++ b/block/io.c >>>> @@ -1912,9 +1912,18 @@ bdrv_co_write_req_prepare(BdrvChild *child, >>>> int64_t offset, uint64_t bytes, >>>> assert(!(bs->open_flags & BDRV_O_INACTIVE)); >>>> assert((bs->open_flags & BDRV_O_NO_IO) == 0); >>>> assert(!(flags & ~BDRV_REQ_MASK)); >>>> + assert(!((flags & BDRV_REQ_NO_WAIT) && !(flags & >>>> BDRV_REQ_SERIALISING))); >>>> if (flags & BDRV_REQ_SERIALISING) { >>>> - bdrv_make_request_serialising(req, bdrv_get_cluster_size(bs)); >>>> + QEMU_LOCK_GUARD(&bs->reqs_lock); >>>> + >>>> + tracked_request_set_serialising(req, >>>> bdrv_get_cluster_size(bs)); >>>> + >>>> + if ((flags & BDRV_REQ_NO_WAIT) && >>>> bdrv_find_conflicting_request(req)) { >>> >>> bdrv_find_conflicting_request() will return NULL even if there are >>> conflicting requests, but those have a non-NULL waiting_for. Is that >>> something to consider? >>> >>> (I would like to think that will never have a real impact because then >>> we must find some other conflicting request; but isn’t is possible that >>> we find an overlapping request that waits for another request with which >>> it overlaps, but our request does not?) >>> >> >> Actually check in bdrv_find_conflicting_request() is the same like in >> the following >> bdrv_wait_serialising_requests_locked(), so, if >> bdrv_find_conflicting_request() returns >> NULL, it means that in bdrv_wait_serialising_requests_locked() it will >> return NULL >> again (as there are no yield points) and we will not wait, so all is OK. > > OK. I thought that maybe we would want to avoid that other requests > might have to wait for the preallocation write. (Of course, we can’t > avoid that altogether, but if we already know of such requests at the > beginning of the request...) > Hmm, I see your point now. Hmm actually, what we want: 1. make preallocation on write 2. serialize the request 3. if there are conflicting requests do nothing, as (most probably) the conflicting request will do preallocation itself So, what we can say about intersecting requests, which are waiting for something (and therefore are not "conflicting")? Aha, for sure, they don't preallocate (otherwise they don't wait). So [3] is still satisfied.. There was not original aim to not make the other parallel request wait for preallocation. So, it may be done later.. Intuitively, I don't expect the benefit: if there is no concurrent preallocate request, who are these intersecting requests? For example, EOF-crossing READS, waiting for some serialized not preallocating (so fit into file-size) WRITE. But this shouldn't happen often :) > Well, if the only thing to look out for is that preallocation writes > themselves do not wait: So, let's assume that yes, we care that they don't wait themselves (mostly to avoid concurrent preallocation requests) and don't care about other intersecting requests. > > Reviewed-by: Max Reitz <mreitz@redhat.com> Thanks! > >> And, why is it OK to ignore already waiting requests in >> bdrv_wait_serialising_requests_locked(): just because if we proceed now >> with our request, >> these waiting requests will have to wait for us, when they wake and go >> to the next iteration >> of waiting loop. > > Sure. >
diff --git a/include/block/block.h b/include/block/block.h index b8f4e86e8d..877fda06a4 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -67,8 +67,15 @@ typedef enum { * written to qiov parameter which may be NULL. */ BDRV_REQ_PREFETCH = 0x200, + + /* + * If we need to wait for other requests, just fail immediately. Used + * only together with BDRV_REQ_SERIALISING. + */ + BDRV_REQ_NO_WAIT = 0x400, + /* Mask of valid flags */ - BDRV_REQ_MASK = 0x3ff, + BDRV_REQ_MASK = 0x7ff, } BdrvRequestFlags; typedef struct BlockSizes { diff --git a/block/io.c b/block/io.c index dd28befb08..c93b1e98a3 100644 --- a/block/io.c +++ b/block/io.c @@ -1912,9 +1912,18 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, uint64_t bytes, assert(!(bs->open_flags & BDRV_O_INACTIVE)); assert((bs->open_flags & BDRV_O_NO_IO) == 0); assert(!(flags & ~BDRV_REQ_MASK)); + assert(!((flags & BDRV_REQ_NO_WAIT) && !(flags & BDRV_REQ_SERIALISING))); if (flags & BDRV_REQ_SERIALISING) { - bdrv_make_request_serialising(req, bdrv_get_cluster_size(bs)); + QEMU_LOCK_GUARD(&bs->reqs_lock); + + tracked_request_set_serialising(req, bdrv_get_cluster_size(bs)); + + if ((flags & BDRV_REQ_NO_WAIT) && bdrv_find_conflicting_request(req)) { + return -EBUSY; + } + + bdrv_wait_serialising_requests_locked(req); } else { bdrv_wait_serialising_requests(req); }
Add flag to make serialising request no wait: if there are conflicting requests, just return error immediately. It's will be used in upcoming preallocate filter. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/block/block.h | 9 ++++++++- block/io.c | 11 ++++++++++- 2 files changed, 18 insertions(+), 2 deletions(-)