Re: [PATCH v5 06/10] block: introduce BDRV_REQ_NO_WAIT flag
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
---
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(>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
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.
--
Best regards,
Vladimir
Re: [PATCH v5 06/10] block: introduce BDRV_REQ_NO_WAIT flag
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
>>> ---
>>> 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(>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
> 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.
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v5 06/10] block: introduce BDRV_REQ_NO_WAIT flag
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
---
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(>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);
}
--
Best regards,
Vladimir
Re: [PATCH v5 06/10] block: introduce BDRV_REQ_NO_WAIT flag
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
> ---
> 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(>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);
> }
>
signature.asc
Description: OpenPGP digital signature
[PATCH v5 06/10] block: introduce BDRV_REQ_NO_WAIT flag
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
---
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(>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);
}
--
2.21.3
