| Message ID | 20210518100757.31243-8-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block-copy: protect block-copy internal structures | expand |
18.05.2021 13:07, Emanuele Giuseppe Esposito wrote: > With tasks and calls lock protecting all State fields, > .method is the last BlockCopyState field left unprotected. > Set it as atomic. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> OK, in 06 some things are out of coroutine. Here could we just reuse mutex? I believe, that we don't need any kind of protection for .method inside block_copy_state_new(), as it's just a creation and initialization of new structure. And other things are called from coroutines. So, seems no reasons for additional atomic access logic? > --- > block/block-copy.c | 37 ++++++++++++++++++------------------- > 1 file changed, 18 insertions(+), 19 deletions(-) > > diff --git a/block/block-copy.c b/block/block-copy.c > index 573e96fefb..ebccb7fbc6 100644 > --- a/block/block-copy.c > +++ b/block/block-copy.c > @@ -108,7 +108,7 @@ typedef struct BlockCopyState { > > /* State */ > int64_t in_flight_bytes; /* protected by tasks_lock */ > - BlockCopyMethod method; > + BlockCopyMethod method; /* atomic */ > CoMutex tasks_lock; > QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls */ > QemuMutex calls_lock; > @@ -184,7 +184,7 @@ static bool coroutine_fn block_copy_wait_one(BlockCopyState *s, int64_t offset, > > static inline int64_t block_copy_chunk_size(BlockCopyState *s) > { > - switch (s->method) { > + switch (qatomic_read(&s->method)) { > case COPY_READ_WRITE_CLUSTER: > return s->cluster_size; > case COPY_READ_WRITE: > @@ -338,16 +338,17 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target, > * buffered copying (read and write respect max_transfer on their > * behalf). > */ > - s->method = COPY_READ_WRITE_CLUSTER; > + qatomic_set(&s->method, COPY_READ_WRITE_CLUSTER); > } else if (write_flags & BDRV_REQ_WRITE_COMPRESSED) { > /* Compression supports only cluster-size writes and no copy-range. */ > - s->method = COPY_READ_WRITE_CLUSTER; > + qatomic_set(&s->method, COPY_READ_WRITE_CLUSTER); > } else { > /* > * We enable copy-range, but keep small copy_size, until first > * successful copy_range (look at block_copy_do_copy). > */ > - s->method = use_copy_range ? COPY_RANGE_SMALL : COPY_READ_WRITE; > + qatomic_set(&s->method, use_copy_range ? COPY_RANGE_SMALL : > + COPY_READ_WRITE); > } > > ratelimit_init(&s->rate_limit); > @@ -432,26 +433,24 @@ static int coroutine_fn block_copy_do_copy(BlockCopyState *s, > return ret; > } > > - if (s->method >= COPY_RANGE_SMALL) { > + if (qatomic_read(&s->method) >= COPY_RANGE_SMALL) { > ret = bdrv_co_copy_range(s->source, offset, s->target, offset, nbytes, > 0, s->write_flags); > if (ret < 0) { > trace_block_copy_copy_range_fail(s, offset, ret); > - s->method = COPY_READ_WRITE; > + qatomic_set(&s->method, COPY_READ_WRITE); > /* Fallback to read+write with allocated buffer */ > } else { > - if (s->method == COPY_RANGE_SMALL) { > - /* > - * Successful copy-range. Now increase copy_size. copy_range > - * does not respect max_transfer (it's a TODO), so we factor > - * that in here. > - * > - * Note: we double-check s->method for the case when > - * parallel block-copy request unsets it during previous > - * bdrv_co_copy_range call. > - */ > - s->method = COPY_RANGE_FULL; > - } > + /* > + * Successful copy-range. Now increase copy_size. copy_range > + * does not respect max_transfer (it's a TODO), so we factor > + * that in here. > + * > + * Note: we double-check s->method for the case when > + * parallel block-copy request unsets it during previous > + * bdrv_co_copy_range call. > + */ > + qatomic_cmpxchg(&s->method, COPY_RANGE_SMALL, COPY_RANGE_FULL); > goto out; > } > } >
On 21/05/2021 19:10, Vladimir Sementsov-Ogievskiy wrote: > 18.05.2021 13:07, Emanuele Giuseppe Esposito wrote: >> With tasks and calls lock protecting all State fields, >> .method is the last BlockCopyState field left unprotected. >> Set it as atomic. >> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > > OK, in 06 some things are out of coroutine. Here could we just reuse mutex? > > I believe, that we don't need any kind of protection for .method inside > block_copy_state_new(), as it's just a creation and initialization of > new structure. I agree here, will remove the atomic_set in block_copy_state_new. > > And other things are called from coroutines. So, seems no reasons for > additional atomic access logic? But... why should I use a mutex? I think the .method usage is pretty straightforward, adding a lock (which one, tasks_lock? does not seem appropriate) would just cover also functions that do not need it, since the field is modified in if-else statements (see block_copy_do_copy). It looks to me that an atomic here won't hurt, and it's pretty straightforward to understand. Thank you, Emanuele
25.05.2021 13:18, Emanuele Giuseppe Esposito wrote: > > > On 21/05/2021 19:10, Vladimir Sementsov-Ogievskiy wrote: >> 18.05.2021 13:07, Emanuele Giuseppe Esposito wrote: >>> With tasks and calls lock protecting all State fields, >>> .method is the last BlockCopyState field left unprotected. >>> Set it as atomic. >>> >>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> >> OK, in 06 some things are out of coroutine. Here could we just reuse mutex? >> >> I believe, that we don't need any kind of protection for .method inside block_copy_state_new(), as it's just a creation and initialization of new structure. > > I agree here, will remove the atomic_set in block_copy_state_new. >> >> And other things are called from coroutines. So, seems no reasons for additional atomic access logic? > > But... why should I use a mutex? I think the .method usage is pretty > straightforward, adding a lock (which one, tasks_lock? does not seem appropriate) Paolo said patch 05 should go away. So, we have only one mutex. We can name it just "lock" and use for all the needs, like in qcow2. > would just cover also functions that do not need it, since the field is modified in if-else statements (see block_copy_do_copy). > It looks to me that an atomic here won't hurt, and it's pretty straightforward to understand. > > Thank you, > Emanuele > Hmm. OK, let me think: First look at block_copy_do_copy(). It's called only from block_copy_task_entry. block_copy_task_entry() has mutex-critical-section anyway around handling return value. That means that we can simply move s->method modification logic to this already existing critical section. Next, block_copy_chunk_size() is called only from block_copy_task_create(), where we should have critical section too. So, no reason for atomics, as we already have critical sections. I think it's significant: Drawbacks of atomics: 1. Code becomes harder to read. Just because instead of simple access to variable, we have to call atomic access functions. And the code become the mess of different qatomic_* calls. 2. The thread-safety is harder to analyze. You argue that use is straightforward: yes, it's obvious that atomic access protect the variable itself. But what about the logic? It's the same as questions I asked about critical sections in a patch 04. With critical sections things are clear: just protect the whole logic with a critical sections and you are sure that no other critical section intersects. With atomics you should analyze for example: are existing critical sections OK with the fact that the variable may be atomically changed by other thread not locking the mutex. It's not a simple question in general. Probable benefits of atomics: 1. Performance.. anything else? So, if we have some lockless performance-critical mechanism, atomics are needed. Still, in general lockless algorithms are a lot trickier and harder to understand than simple critical sections. Still, sometimes it worth the complexity. But, if we already have the mutex to protect most of the logic inside some subsystem (block-copy for example), it's better to just protect the remaining bit of things in the subsystem by same mutex, than to add drawbacks of atomics with no reason. Especially when this remaining bit of accesses follows or goes directly before existing critical section. I don't believe that in this case atomics may bring better performance. I even think, that performance may become worse (remember atomic operations are not free, and simple accesses to variable may be faster). And I really doubt, that someone can demonstrate a performance benefit of atomic accesses in block-layer. IO operations are a lot longer anyway than all these simple variable accesses. So, I'm against adding atomics just because they won't hurt :)
On 25/05/21 13:00, Vladimir Sementsov-Ogievskiy wrote: > > Hmm. OK, let me think: > > First look at block_copy_do_copy(). It's called only from > block_copy_task_entry. block_copy_task_entry() has > mutex-critical-section anyway around handling return value. That means > that we can simply move s->method modification logic to this already > existing critical section. > > Next, block_copy_chunk_size() is called only from > block_copy_task_create(), where we should have critical section too. block_copy_do_copy would have to release the mutex around the reads/writes (including the bdrv_co_pwrite_zeroes that has nothing to do with s->method). There's also the "goto out" if the copy-range operation succeeds, which makes things a bit more complicated. The goto suggests using QEMU_WITH_LOCK_GUARD, but that doesn't work too well either, because there are two accesses (one before the bdrv_co_copy_range and one after). So I understand why you want to avoid atomics and I agree that they are more complicated than the other solutions, on the other hand I think this patch is the simplest code. Paolo > So, no reason for atomics, as we already have critical sections.
26.05.2021 17:48, Paolo Bonzini wrote: > On 25/05/21 13:00, Vladimir Sementsov-Ogievskiy wrote: >> >> Hmm. OK, let me think: >> >> First look at block_copy_do_copy(). It's called only from block_copy_task_entry. block_copy_task_entry() has mutex-critical-section anyway around handling return value. That means that we can simply move s->method modification logic to this already existing critical section. >> >> Next, block_copy_chunk_size() is called only from block_copy_task_create(), where we should have critical section too. > > block_copy_do_copy would have to release the mutex around the reads/writes (including the bdrv_co_pwrite_zeroes that has nothing to do with s->method). > > There's also the "goto out" if the copy-range operation succeeds, which makes things a bit more complicated. The goto suggests using QEMU_WITH_LOCK_GUARD, but that doesn't work too well either, because there are two accesses (one before the bdrv_co_copy_range and one after). Hmm, this "goto out" makes no sense actually, and should be removed.. It's a mistake or a kind of forgotten thing to refactor after some changes. > > So I understand why you want to avoid atomics and I agree that they are more complicated than the other solutions, on the other hand I think this patch is the simplest code. > I think it's better to pass s->method as parameter to block_copy_do_copy. Then block_copy_do_copy() doesn't need any kind of synchronization. In block_copy_task_entry() we'll have the whole logic of handling result of block_copy_do_copy (including changing of s->method). And then, only one question is: before calling block_copy_do_copy() we should get s->method. Either by atomic operation or under separate critical section.. I'd be OK either way. It's actually the original idea of block_copy_do_copy() function: do only simple copy, don't interact with the state. It's a kind of wrapper on top of bdrv_co<io functions>. So, actually updating s->use_copy_range here was a bad idea. Note also the comment above block_copy_do_copy(): " No sync here: nor bitmap neighter intersecting requests handling, only copy." Somehow, the comment conflicts with introducing synchronization primitives inside the function, although it was about another things :))
On 26/05/21 19:18, Vladimir Sementsov-Ogievskiy wrote: > > It's actually the original idea of block_copy_do_copy() function: do > only simple copy, don't interact with the state. It's a kind of wrapper > on top of bdrv_co<io functions>. > > So, actually updating s->use_copy_range here was a bad idea. It's still more complicated, because you need to add some kind of method = s->method; ret = block_copy_do_copy(..., method); if (ret < 0 && method <= COPY_RANGE_SMALL) { method = COPY_RANGE_READ_WRITE; ret = block_copy_do_copy(..., method); } lock(); if (method == s->method) { /* compute new method */ } which makes it more complicated than this patch IMO. But yeah at least it's a viable alternative to the atomics. Paolo
On 28/05/21 12:24, Paolo Bonzini wrote: > > It's still more complicated, because you need to add some kind of > > method = s->method; This would even have to be a separate, one-line critical section... Paolo > ret = block_copy_do_copy(..., method); > if (ret < 0 && method <= COPY_RANGE_SMALL) { > method = COPY_RANGE_READ_WRITE; > ret = block_copy_do_copy(..., method); > } > lock(); > if (method == s->method) { > /* compute new method */ > } > > which makes it more complicated than this patch IMO. But yeah at least > it's a viable alternative to the atomics.
28.05.2021 14:01, Paolo Bonzini wrote: > On 28/05/21 12:24, Paolo Bonzini wrote: >> >> It's still more complicated, because you need to add some kind of >> >> method = s->method; > > This would even have to be a separate, one-line critical section... > Or atomic operation.. What I don't like that all troubles are for unused code. Many things may change to the moment when we actually reuse this for qemu-img convert. And, qemu-img convert probably don't need this complicated logic with different methods. It may be better just return error if copy_range failed. What's a good reason for fall-back to normal write if copy-range is explicitly requested by user? > >> ret = block_copy_do_copy(..., method); >> if (ret < 0 && method <= COPY_RANGE_SMALL) { >> method = COPY_RANGE_READ_WRITE; >> ret = block_copy_do_copy(..., method); >> } >> lock(); >> if (method == s->method) { >> /* compute new method */ >> } >> >> which makes it more complicated than this patch IMO. But yeah at least it's a viable alternative to the atomics. > >
28.05.2021 14:01, Paolo Bonzini wrote: > On 28/05/21 12:24, Paolo Bonzini wrote: >> >> It's still more complicated, because you need to add some kind of >> >> method = s->method; > > This would even have to be a separate, one-line critical section... > Hm, so, we should set .use_copy_range in task, when it is initialized. > >> ret = block_copy_do_copy(..., method); >> if (ret < 0 && method <= COPY_RANGE_SMALL) { >> method = COPY_RANGE_READ_WRITE; >> ret = block_copy_do_copy(..., method); >> } >> lock(); >> if (method == s->method) { >> /* compute new method */ >> } >> >> which makes it more complicated than this patch IMO. But yeah at least it's a viable alternative to the atomics. > > OK, I'm OK with patch as is. Finally I can refactor it later on top if needed.. I'll try now do some refactoring, you'll probably want to base on it, or vise-versa, I'll rebase it later on top of these patches.
diff --git a/block/block-copy.c b/block/block-copy.c index 573e96fefb..ebccb7fbc6 100644 --- a/block/block-copy.c +++ b/block/block-copy.c @@ -108,7 +108,7 @@ typedef struct BlockCopyState { /* State */ int64_t in_flight_bytes; /* protected by tasks_lock */ - BlockCopyMethod method; + BlockCopyMethod method; /* atomic */ CoMutex tasks_lock; QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls */ QemuMutex calls_lock; @@ -184,7 +184,7 @@ static bool coroutine_fn block_copy_wait_one(BlockCopyState *s, int64_t offset, static inline int64_t block_copy_chunk_size(BlockCopyState *s) { - switch (s->method) { + switch (qatomic_read(&s->method)) { case COPY_READ_WRITE_CLUSTER: return s->cluster_size; case COPY_READ_WRITE: @@ -338,16 +338,17 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target, * buffered copying (read and write respect max_transfer on their * behalf). */ - s->method = COPY_READ_WRITE_CLUSTER; + qatomic_set(&s->method, COPY_READ_WRITE_CLUSTER); } else if (write_flags & BDRV_REQ_WRITE_COMPRESSED) { /* Compression supports only cluster-size writes and no copy-range. */ - s->method = COPY_READ_WRITE_CLUSTER; + qatomic_set(&s->method, COPY_READ_WRITE_CLUSTER); } else { /* * We enable copy-range, but keep small copy_size, until first * successful copy_range (look at block_copy_do_copy). */ - s->method = use_copy_range ? COPY_RANGE_SMALL : COPY_READ_WRITE; + qatomic_set(&s->method, use_copy_range ? COPY_RANGE_SMALL : + COPY_READ_WRITE); } ratelimit_init(&s->rate_limit); @@ -432,26 +433,24 @@ static int coroutine_fn block_copy_do_copy(BlockCopyState *s, return ret; } - if (s->method >= COPY_RANGE_SMALL) { + if (qatomic_read(&s->method) >= COPY_RANGE_SMALL) { ret = bdrv_co_copy_range(s->source, offset, s->target, offset, nbytes, 0, s->write_flags); if (ret < 0) { trace_block_copy_copy_range_fail(s, offset, ret); - s->method = COPY_READ_WRITE; + qatomic_set(&s->method, COPY_READ_WRITE); /* Fallback to read+write with allocated buffer */ } else { - if (s->method == COPY_RANGE_SMALL) { - /* - * Successful copy-range. Now increase copy_size. copy_range - * does not respect max_transfer (it's a TODO), so we factor - * that in here. - * - * Note: we double-check s->method for the case when - * parallel block-copy request unsets it during previous - * bdrv_co_copy_range call. - */ - s->method = COPY_RANGE_FULL; - } + /* + * Successful copy-range. Now increase copy_size. copy_range + * does not respect max_transfer (it's a TODO), so we factor + * that in here. + * + * Note: we double-check s->method for the case when + * parallel block-copy request unsets it during previous + * bdrv_co_copy_range call. + */ + qatomic_cmpxchg(&s->method, COPY_RANGE_SMALL, COPY_RANGE_FULL); goto out; } }
With tasks and calls lock protecting all State fields, .method is the last BlockCopyState field left unprotected. Set it as atomic. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- block/block-copy.c | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-)