[PATCH 0/2] block-copy: small fix and refactor

[PATCH 0/2] block-copy: small fix and refactor

[Vladimir Sementsov-Ogievskiy]

Hi all!

This is my suggestion how to refactor block-copy to avoid extra atomic
operations in 
"[PATCH v2 0/7] block-copy: protect block-copy internal structures"

Vladimir Sementsov-Ogievskiy (2):
  block-copy: fix block_copy_task_entry() progress update
  block-copy: refactor copy_range handling

 block/block-copy.c | 79 +++++++++++++++++++++++++++++++---------------
 1 file changed, 53 insertions(+), 26 deletions(-)

-- 
2.29.2

[PATCH 1/2] block-copy: fix block_copy_task_entry() progress update

[Vladimir Sementsov-Ogievskiy]

Don't report successful progress on failure, when call_state->ret is
set.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/block-copy.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index c2e5090412..f9e871b64f 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -439,9 +439,11 @@ static coroutine_fn int block_copy_task_entry(AioTask *task)
 
     ret = block_copy_do_copy(t->s, t->offset, t->bytes, t->zeroes,
                              &error_is_read);
-    if (ret < 0 && !t->call_state->ret) {
-        t->call_state->ret = ret;
-        t->call_state->error_is_read = error_is_read;
+    if (ret < 0) {
+        if (!t->call_state->ret) {
+            t->call_state->ret = ret;
+            t->call_state->error_is_read = error_is_read;
+        }
     } else {
         progress_work_done(t->s->progress, t->bytes);
     }
-- 
2.29.2

[PATCH 2/2] block-copy: refactor copy_range handling

[Vladimir Sementsov-Ogievskiy]

Currently we update s->use_copy_range and s->copy_size in
block_copy_do_copy().

It's not very good:

1. block_copy_do_copy() is intended to be a simple function, that wraps
bdrv_co_<io> functions for need of block copy. That's why we don't pass
BlockCopyTask into it. So, block_copy_do_copy() is bad place for
manipulation with generic state of block-copy process

2. We are going to make block-copy thread-safe. So, it's good to move
manipulation with state of block-copy to the places where we'll need
critical sections anyway, to not introduce extra synchronization
primitives in block_copy_do_copy().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/block-copy.c | 71 +++++++++++++++++++++++++++++++---------------
 1 file changed, 48 insertions(+), 23 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index f9e871b64f..c96fe31054 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -65,6 +65,7 @@ typedef struct BlockCopyTask {
     int64_t offset;
     int64_t bytes;
     bool zeroes;
+    bool copy_range;
     QLIST_ENTRY(BlockCopyTask) list;
     CoQueue wait_queue; /* coroutines blocked on this task */
 } BlockCopyTask;
@@ -183,6 +184,7 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
         .call_state = call_state,
         .offset = offset,
         .bytes = bytes,
+        .copy_range = s->use_copy_range,
     };
     qemu_co_queue_init(&task->wait_queue);
     QLIST_INSERT_HEAD(&s->tasks, task, list);
@@ -342,11 +344,17 @@ static coroutine_fn int block_copy_task_run(AioTaskPool *pool,
  *
  * No sync here: nor bitmap neighter intersecting requests handling, only copy.
  *
+ * @copy_range is in-out argument: if *copy_range is false, copy_range is not
+ * done. If *copy_range is true, copy_range attempt is done. If copy_range
+ * attempt failed, the function fallback to usual read+write and *copy_range is
+ * set to false. *copy_range and zeroes must not be true simultaneously.
+ *
  * Returns 0 on success.
  */
 static int coroutine_fn block_copy_do_copy(BlockCopyState *s,
                                            int64_t offset, int64_t bytes,
-                                           bool zeroes, bool *error_is_read)
+                                           bool zeroes, bool *copy_range,
+                                           bool *error_is_read)
 {
     int ret;
     int64_t nbytes = MIN(offset + bytes, s->len) - offset;
@@ -359,6 +367,7 @@ static int coroutine_fn block_copy_do_copy(BlockCopyState *s,
     assert(offset + bytes <= s->len ||
            offset + bytes == QEMU_ALIGN_UP(s->len, s->cluster_size));
     assert(nbytes < INT_MAX);
+    assert(!(*copy_range && zeroes));
 
     if (zeroes) {
         ret = bdrv_co_pwrite_zeroes(s->target, offset, nbytes, s->write_flags &
@@ -370,32 +379,15 @@ static int coroutine_fn block_copy_do_copy(BlockCopyState *s,
         return ret;
     }
 
-    if (s->use_copy_range) {
+    if (*copy_range) {
         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->use_copy_range = false;
-            s->copy_size = MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER);
+            *copy_range = false;
             /* Fallback to read+write with allocated buffer */
         } else {
-            if (s->use_copy_range) {
-                /*
-                 * 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->use_copy_range for the case when
-                 * parallel block-copy request unsets it during previous
-                 * bdrv_co_copy_range call.
-                 */
-                s->copy_size =
-                        MIN(MAX(s->cluster_size, BLOCK_COPY_MAX_COPY_RANGE),
-                            QEMU_ALIGN_DOWN(block_copy_max_transfer(s->source,
-                                                                    s->target),
-                                            s->cluster_size));
-            }
-            goto out;
+            return 0;
         }
     }
 
@@ -431,14 +423,44 @@ out:
     return ret;
 }
 
+static void block_copy_handle_copy_range_result(BlockCopyState *s,
+                                                bool is_success)
+{
+    if (!s->use_copy_range) {
+        /* already disabled */
+        return;
+    }
+
+    if (is_success) {
+        /*
+         * Successful copy-range. Now increase copy_size.  copy_range
+         * does not respect max_transfer (it's a TODO), so we factor
+         * that in here.
+         */
+        s->copy_size =
+                MIN(MAX(s->cluster_size, BLOCK_COPY_MAX_COPY_RANGE),
+                    QEMU_ALIGN_DOWN(block_copy_max_transfer(s->source,
+                                                            s->target),
+                                    s->cluster_size));
+    } else {
+        /* Copy-range failed, disable it. */
+        s->use_copy_range = false;
+        s->copy_size = MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER);
+    }
+}
+
 static coroutine_fn int block_copy_task_entry(AioTask *task)
 {
     BlockCopyTask *t = container_of(task, BlockCopyTask, task);
     bool error_is_read = false;
+    bool copy_range = t->copy_range;
     int ret;
 
     ret = block_copy_do_copy(t->s, t->offset, t->bytes, t->zeroes,
-                             &error_is_read);
+                             &copy_range, &error_is_read);
+    if (t->copy_range) {
+        block_copy_handle_copy_range_result(t->s, copy_range);
+    }
     if (ret < 0) {
         if (!t->call_state->ret) {
             t->call_state->ret = ret;
@@ -619,7 +641,10 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
             g_free(task);
             continue;
         }
-        task->zeroes = ret & BDRV_BLOCK_ZERO;
+        if (ret & BDRV_BLOCK_ZERO) {
+            task->zeroes = true;
+            task->copy_range = false;
+        }
 
         if (s->speed) {
             if (!call_state->ignore_ratelimit) {
-- 
2.29.2

Re: [PATCH 2/2] block-copy: refactor copy_range handling

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1022 bytes --]

On Fri, May 28, 2021 at 05:16:28PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> @@ -183,6 +184,7 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
>          .call_state = call_state,
>          .offset = offset,
>          .bytes = bytes,
> +        .copy_range = s->use_copy_range,
>      };
>      qemu_co_queue_init(&task->wait_queue);
>      QLIST_INSERT_HEAD(&s->tasks, task, list);
> @@ -342,11 +344,17 @@ static coroutine_fn int block_copy_task_run(AioTaskPool *pool,
>   *
>   * No sync here: nor bitmap neighter intersecting requests handling, only copy.
>   *
> + * @copy_range is in-out argument: if *copy_range is false, copy_range is not

s/is in-out argument/is an in-out argument/

> + * done. If *copy_range is true, copy_range attempt is done. If copy_range

s/copy_range attempt is done/copy_range is attempted/

> + * attempt failed, the function fallback to usual read+write and *copy_range is

If the copy_range attempt fails, the function falls back to the usual
read+write and ...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] block-copy: refactor copy_range handling

[Vladimir Sementsov-Ogievskiy]

02.06.2021 12:12, Stefan Hajnoczi wrote:
> On Fri, May 28, 2021 at 05:16:28PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> @@ -183,6 +184,7 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
>>           .call_state = call_state,
>>           .offset = offset,
>>           .bytes = bytes,
>> +        .copy_range = s->use_copy_range,
>>       };
>>       qemu_co_queue_init(&task->wait_queue);
>>       QLIST_INSERT_HEAD(&s->tasks, task, list);
>> @@ -342,11 +344,17 @@ static coroutine_fn int block_copy_task_run(AioTaskPool *pool,
>>    *
>>    * No sync here: nor bitmap neighter intersecting requests handling, only copy.
>>    *
>> + * @copy_range is in-out argument: if *copy_range is false, copy_range is not
> 
> s/is in-out argument/is an in-out argument/
> 
>> + * done. If *copy_range is true, copy_range attempt is done. If copy_range
> 
> s/copy_range attempt is done/copy_range is attempted/
> 
>> + * attempt failed, the function fallback to usual read+write and *copy_range is
> 
> If the copy_range attempt fails, the function falls back to the usual
> read+write and ...
> 

That's better, thanks!

-- 
Best regards,
Vladimir

Re: [PATCH 0/2] block-copy: small fix and refactor

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

On Fri, May 28, 2021 at 05:16:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> This is my suggestion how to refactor block-copy to avoid extra atomic
> operations in 
> "[PATCH v2 0/7] block-copy: protect block-copy internal structures"
> 
> Vladimir Sementsov-Ogievskiy (2):
>   block-copy: fix block_copy_task_entry() progress update
>   block-copy: refactor copy_range handling
> 
>  block/block-copy.c | 79 +++++++++++++++++++++++++++++++---------------
>  1 file changed, 53 insertions(+), 26 deletions(-)

I posted suggestions for the doc comment on Patch 2, otherwise:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] block-copy: small fix and refactor

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

Am 02.06.2021 um 11:13 hat Stefan Hajnoczi geschrieben:
> On Fri, May 28, 2021 at 05:16:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > Hi all!
> > 
> > This is my suggestion how to refactor block-copy to avoid extra atomic
> > operations in 
> > "[PATCH v2 0/7] block-copy: protect block-copy internal structures"
> > 
> > Vladimir Sementsov-Ogievskiy (2):
> >   block-copy: fix block_copy_task_entry() progress update
> >   block-copy: refactor copy_range handling
> > 
> >  block/block-copy.c | 79 +++++++++++++++++++++++++++++++---------------
> >  1 file changed, 53 insertions(+), 26 deletions(-)
> 
> I posted suggestions for the doc comment on Patch 2, otherwise:
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Thanks, fixed up the comment accordingly and applied to the block
branch.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] block-copy: small fix and refactor

[Paolo Bonzini]

On 02/06/21 14:21, Kevin Wolf wrote:
> Am 02.06.2021 um 11:13 hat Stefan Hajnoczi geschrieben:
>> On Fri, May 28, 2021 at 05:16:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> This is my suggestion how to refactor block-copy to avoid extra atomic
>>> operations in
>>> "[PATCH v2 0/7] block-copy: protect block-copy internal structures"
>>>
>>> Vladimir Sementsov-Ogievskiy (2):
>>>    block-copy: fix block_copy_task_entry() progress update
>>>    block-copy: refactor copy_range handling
>>>
>>>   block/block-copy.c | 79 +++++++++++++++++++++++++++++++---------------
>>>   1 file changed, 53 insertions(+), 26 deletions(-)
>>
>> I posted suggestions for the doc comment on Patch 2, otherwise:
>>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Thanks, fixed up the comment accordingly and applied to the block
> branch.

I'm a bit confused.  Vladimir said in his review of Emanuele's patches 
that he was okay with patch 7 and that he would rebase this refactoring 
on top of it.

Vladimir's main complaint for the s->method state machine was the extra 
lines of code.  Here we have just as many new lines of code and new 
parameters that are passed by reference.  Kevin, can you please look at 
Emanuele's patches and possibly unqueue the second patch here?  It seems 
to me that it should have been tagged as RFC.

Paolo

[1] 
https://patchew.org/QEMU/20210518100757.31243-1-eesposit@redhat.com/20210518100757.31243-8-eesposit@redhat.com/

Re: [PATCH 0/2] block-copy: small fix and refactor

[Kevin Wolf]

Am 03.06.2021 um 09:38 hat Paolo Bonzini geschrieben:
> On 02/06/21 14:21, Kevin Wolf wrote:
> > Am 02.06.2021 um 11:13 hat Stefan Hajnoczi geschrieben:
> > > On Fri, May 28, 2021 at 05:16:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > Hi all!
> > > > 
> > > > This is my suggestion how to refactor block-copy to avoid extra atomic
> > > > operations in
> > > > "[PATCH v2 0/7] block-copy: protect block-copy internal structures"
> > > > 
> > > > Vladimir Sementsov-Ogievskiy (2):
> > > >    block-copy: fix block_copy_task_entry() progress update
> > > >    block-copy: refactor copy_range handling
> > > > 
> > > >   block/block-copy.c | 79 +++++++++++++++++++++++++++++++---------------
> > > >   1 file changed, 53 insertions(+), 26 deletions(-)
> > > 
> > > I posted suggestions for the doc comment on Patch 2, otherwise:
> > > 
> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > Thanks, fixed up the comment accordingly and applied to the block
> > branch.
> 
> I'm a bit confused.  Vladimir said in his review of Emanuele's patches
> that he was okay with patch 7 and that he would rebase this
> refactoring on top of it.
> 
> Vladimir's main complaint for the s->method state machine was the
> extra lines of code.  Here we have just as many new lines of code and
> new parameters that are passed by reference.  Kevin, can you please
> look at Emanuele's patches and possibly unqueue the second patch here?
> It seems to me that it should have been tagged as RFC.

Sorry, I was not aware that Vladimir intended to rebase this one. This
has already landed in master, so if rebasing the other patch is a real
problem, we'd have to revert this one first.

Kevin

Re: [PATCH 0/2] block-copy: small fix and refactor

[Emanuele Giuseppe Esposito]

On 07/06/2021 17:10, Kevin Wolf wrote:
> Am 03.06.2021 um 09:38 hat Paolo Bonzini geschrieben:
>> On 02/06/21 14:21, Kevin Wolf wrote:
>>> Am 02.06.2021 um 11:13 hat Stefan Hajnoczi geschrieben:
>>>> On Fri, May 28, 2021 at 05:16:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Hi all!
>>>>>
>>>>> This is my suggestion how to refactor block-copy to avoid extra atomic
>>>>> operations in
>>>>> "[PATCH v2 0/7] block-copy: protect block-copy internal structures"
>>>>>
>>>>> Vladimir Sementsov-Ogievskiy (2):
>>>>>     block-copy: fix block_copy_task_entry() progress update
>>>>>     block-copy: refactor copy_range handling
>>>>>
>>>>>    block/block-copy.c | 79 +++++++++++++++++++++++++++++++---------------
>>>>>    1 file changed, 53 insertions(+), 26 deletions(-)
>>>>
>>>> I posted suggestions for the doc comment on Patch 2, otherwise:
>>>>
>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>
>>> Thanks, fixed up the comment accordingly and applied to the block
>>> branch.
>>
>> I'm a bit confused.  Vladimir said in his review of Emanuele's patches
>> that he was okay with patch 7 and that he would rebase this
>> refactoring on top of it.
>>
>> Vladimir's main complaint for the s->method state machine was the
>> extra lines of code.  Here we have just as many new lines of code and
>> new parameters that are passed by reference.  Kevin, can you please
>> look at Emanuele's patches and possibly unqueue the second patch here?
>> It seems to me that it should have been tagged as RFC.
> 
> Sorry, I was not aware that Vladimir intended to rebase this one. This
> has already landed in master, so if rebasing the other patch is a real
> problem, we'd have to revert this one first.
>
It shouldn't be a problem, I have already rebased on top of it. I will 
re-spin a new series with this and other minor (and hopefully final) 
fixes soon.

Emanuele

Re: [PATCH 0/2] block-copy: small fix and refactor

[Vladimir Sementsov-Ogievskiy]

07.06.2021 18:16, Emanuele Giuseppe Esposito wrote:
> 
> 
> On 07/06/2021 17:10, Kevin Wolf wrote:
>> Am 03.06.2021 um 09:38 hat Paolo Bonzini geschrieben:
>>> On 02/06/21 14:21, Kevin Wolf wrote:
>>>> Am 02.06.2021 um 11:13 hat Stefan Hajnoczi geschrieben:
>>>>> On Fri, May 28, 2021 at 05:16:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Hi all!
>>>>>>
>>>>>> This is my suggestion how to refactor block-copy to avoid extra atomic
>>>>>> operations in
>>>>>> "[PATCH v2 0/7] block-copy: protect block-copy internal structures"
>>>>>>
>>>>>> Vladimir Sementsov-Ogievskiy (2):
>>>>>>     block-copy: fix block_copy_task_entry() progress update
>>>>>>     block-copy: refactor copy_range handling
>>>>>>
>>>>>>    block/block-copy.c | 79 +++++++++++++++++++++++++++++++---------------
>>>>>>    1 file changed, 53 insertions(+), 26 deletions(-)
>>>>>
>>>>> I posted suggestions for the doc comment on Patch 2, otherwise:
>>>>>
>>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>>
>>>> Thanks, fixed up the comment accordingly and applied to the block
>>>> branch.
>>>
>>> I'm a bit confused.  Vladimir said in his review of Emanuele's patches
>>> that he was okay with patch 7 and that he would rebase this
>>> refactoring on top of it.
>>>
>>> Vladimir's main complaint for the s->method state machine was the
>>> extra lines of code.  Here we have just as many new lines of code and
>>> new parameters that are passed by reference.  Kevin, can you please
>>> look at Emanuele's patches and possibly unqueue the second patch here?
>>> It seems to me that it should have been tagged as RFC.
>>
>> Sorry, I was not aware that Vladimir intended to rebase this one. This
>> has already landed in master, so if rebasing the other patch is a real
>> problem, we'd have to revert this one first.
>>
> It shouldn't be a problem, I have already rebased on top of it. I will re-spin a new series with this and other minor (and hopefully final) fixes soon.
> 

Thanks, and sorry for the mess!

Hmm, actually, I said

> 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. 

So, I considered both variants. Then I sent patches, everybody in CC, everybody were silent.


Honestly, I'm a bit confused too. I find my complains valid (independently of me being "I'm OK and can refactor later") and you agreed with them in general. I'm an author and maintainer of the component. I do refactoring that makes it simple to follow my suggestion. So for me it's a bit like doing your work for you. And you ask to roll-back it.


Still, misunderstanding and the mess with two parallel conflicting series is my fault, sorry for this. At least I should have answered to your series when Stefan gave an r-b to my series.

-- 
Best regards,
Vladimir

Re: [PATCH 0/2] block-copy: small fix and refactor

[Emanuele Giuseppe Esposito]

On 07/06/2021 18:18, Vladimir Sementsov-Ogievskiy wrote:
> 07.06.2021 18:16, Emanuele Giuseppe Esposito wrote:
>>
>>
>> On 07/06/2021 17:10, Kevin Wolf wrote:
>>> Am 03.06.2021 um 09:38 hat Paolo Bonzini geschrieben:
>>>> On 02/06/21 14:21, Kevin Wolf wrote:
>>>>> Am 02.06.2021 um 11:13 hat Stefan Hajnoczi geschrieben:
>>>>>> On Fri, May 28, 2021 at 05:16:26PM +0300, Vladimir 
>>>>>> Sementsov-Ogievskiy wrote:
>>>>>>> Hi all!
>>>>>>>
>>>>>>> This is my suggestion how to refactor block-copy to avoid extra 
>>>>>>> atomic
>>>>>>> operations in
>>>>>>> "[PATCH v2 0/7] block-copy: protect block-copy internal structures"
>>>>>>>
>>>>>>> Vladimir Sementsov-Ogievskiy (2):
>>>>>>>     block-copy: fix block_copy_task_entry() progress update
>>>>>>>     block-copy: refactor copy_range handling
>>>>>>>
>>>>>>>    block/block-copy.c | 79 
>>>>>>> +++++++++++++++++++++++++++++++---------------
>>>>>>>    1 file changed, 53 insertions(+), 26 deletions(-)
>>>>>>
>>>>>> I posted suggestions for the doc comment on Patch 2, otherwise:
>>>>>>
>>>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>>>
>>>>> Thanks, fixed up the comment accordingly and applied to the block
>>>>> branch.
>>>>
>>>> I'm a bit confused.  Vladimir said in his review of Emanuele's patches
>>>> that he was okay with patch 7 and that he would rebase this
>>>> refactoring on top of it.
>>>>
>>>> Vladimir's main complaint for the s->method state machine was the
>>>> extra lines of code.  Here we have just as many new lines of code and
>>>> new parameters that are passed by reference.  Kevin, can you please
>>>> look at Emanuele's patches and possibly unqueue the second patch here?
>>>> It seems to me that it should have been tagged as RFC.
>>>
>>> Sorry, I was not aware that Vladimir intended to rebase this one. This
>>> has already landed in master, so if rebasing the other patch is a real
>>> problem, we'd have to revert this one first.
>>>
>> It shouldn't be a problem, I have already rebased on top of it. I will 
>> re-spin a new series with this and other minor (and hopefully final) 
>> fixes soon.
>>
> 
> Thanks, and sorry for the mess!
> 
> Hmm, actually, I said
> 
>> 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. 
> 
> So, I considered both variants. Then I sent patches, everybody in CC, 
> everybody were silent.
> 
> 
> Honestly, I'm a bit confused too. I find my complains valid 
> (independently of me being "I'm OK and can refactor later") and you 
> agreed with them in general. I'm an author and maintainer of the 
> component. I do refactoring that makes it simple to follow my 
> suggestion. So for me it's a bit like doing your work for you. And you 
> ask to roll-back it.

I think it's useless to discuss about these things now. I rebased, all 
is clear and I am positive that in the next version we will have 
something that makes everyone happy :) and if not, feel free to comment it!

Emanuele

> 
> Still, misunderstanding and the mess with two parallel conflicting 
> series is my fault, sorry for this. At least I should have answered to 
> your series when Stefan gave an r-b to my series.
>