[Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests

[Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests

[Peter Lieven]

in a read-modify-write cycle a small request might cause
head and tail to fall into the same aligned block. Currently
QEMU reads the same block twice in this case which is
not necessary.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
v1->v2: following Paolos suggestions to simplify the if condition and
        adjusting the comment

 block/io.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/io.c b/block/io.c
index 60a6bd8..7459dfb 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1430,6 +1430,14 @@ int coroutine_fn bdrv_co_pwritev(BlockDriverState *bs,
 
         bytes += offset & (align - 1);
         offset = offset & ~(align - 1);
+
+        /* We have read the tail already if the request is smaller
+         * than one aligned block.
+         */
+        if (bytes < align) {
+            qemu_iovec_add(&local_qiov, head_buf + bytes, align - bytes);
+            bytes = align;
+        }
     }
 
     if ((offset + bytes) & (align - 1)) {
-- 
1.9.1

Re: [Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests

[Kevin Wolf]

Am 24.05.2016 um 16:30 hat Peter Lieven geschrieben:
> in a read-modify-write cycle a small request might cause
> head and tail to fall into the same aligned block. Currently
> QEMU reads the same block twice in this case which is
> not necessary.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests

[Fam Zheng]

On Tue, 05/24 16:30, Peter Lieven wrote:
> in a read-modify-write cycle a small request might cause
> head and tail to fall into the same aligned block. Currently
> QEMU reads the same block twice in this case which is
> not necessary.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>

Thanks, applied to my block branch:

https://github.com/famz/qemu/tree/block

Re: [Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests

[Fam Zheng]

On Thu, 05/26 14:50, Fam Zheng wrote:
> On Tue, 05/24 16:30, Peter Lieven wrote:
> > in a read-modify-write cycle a small request might cause
> > head and tail to fall into the same aligned block. Currently
> > QEMU reads the same block twice in this case which is
> > not necessary.
> > 
> > Signed-off-by: Peter Lieven <pl@kamp.de>
> 
> Thanks, applied to my block branch:
> 
> https://github.com/famz/qemu/tree/block
> 

Looks like this breaks iotests 077 (hang), the blkdebug break points expected
by the script are not hit now. While squashing in below patch fixes the case, I
think it is more appropriate to keep the patch as is and fix the case itself.

Dropped from my queue, please send another version with test case update so I
can apply together.

diff --git a/block/io.c b/block/io.c
index d480097..a6523cf 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1435,8 +1435,10 @@ int coroutine_fn bdrv_co_pwritev(BlockDriverState *bs,
          * than one aligned block.
          */
         if (bytes < align) {
+            bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_TAIL);
             qemu_iovec_add(&local_qiov, head_buf + bytes, align - bytes);
             bytes = align;
+            bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
         }
     }

Re: [Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests

[Paolo Bonzini]

On 26/05/2016 09:10, Fam Zheng wrote:
> 
> diff --git a/block/io.c b/block/io.c
> index d480097..a6523cf 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1435,8 +1435,10 @@ int coroutine_fn bdrv_co_pwritev(BlockDriverState *bs,
>           * than one aligned block.
>           */
>          if (bytes < align) {
> +            bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_TAIL);
>              qemu_iovec_add(&local_qiov, head_buf + bytes, align - bytes);
>              bytes = align;
> +            bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
>          }
>      }

This doesn't look too wrong...  Should the right sequence of events be
head/after_head or head/after_tail?  It's probably simplest to just emit
all four events.

Paolo

Re: [Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests

[Fam Zheng]

On Thu, 05/26 09:55, Paolo Bonzini wrote:
> 
> 
> On 26/05/2016 09:10, Fam Zheng wrote:
> > 
> > diff --git a/block/io.c b/block/io.c
> > index d480097..a6523cf 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -1435,8 +1435,10 @@ int coroutine_fn bdrv_co_pwritev(BlockDriverState *bs,
> >           * than one aligned block.
> >           */
> >          if (bytes < align) {
> > +            bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_TAIL);
> >              qemu_iovec_add(&local_qiov, head_buf + bytes, align - bytes);
> >              bytes = align;
> > +            bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
> >          }
> >      }
> 
> This doesn't look too wrong...  Should the right sequence of events be
> head/after_head or head/after_tail?  It's probably simplest to just emit
> all four events.

I've no idea. (That's why I leaned towards fixing the test case).  But if Kevin
can ack, I'd be happy with this way.

Fam

Re: [Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests

[Paolo Bonzini]

On 26/05/2016 10:30, Fam Zheng wrote:
>> > 
>> > This doesn't look too wrong...  Should the right sequence of events be
>> > head/after_head or head/after_tail?  It's probably simplest to just emit
>> > all four events.
> I've no idea. (That's why I leaned towards fixing the test case).

Well, fixing the testcase means knowing what events should be emitted.

QEMU with Peter's patch emits head/after_head.  If the right one is
head/after_tail, _both QEMU and the testcase_ need to be adjusted.  Your
patch keeps the backwards-compatible route.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests

[Fam Zheng]

On Thu, 05/26 11:20, Paolo Bonzini wrote:
> 
> 
> On 26/05/2016 10:30, Fam Zheng wrote:
> >> > 
> >> > This doesn't look too wrong...  Should the right sequence of events be
> >> > head/after_head or head/after_tail?  It's probably simplest to just emit
> >> > all four events.
> > I've no idea. (That's why I leaned towards fixing the test case).
> 
> Well, fixing the testcase means knowing what events should be emitted.
> 
> QEMU with Peter's patch emits head/after_head.  If the right one is
> head/after_tail, _both QEMU and the testcase_ need to be adjusted.  Your
> patch keeps the backwards-compatible route.

Yes, I mean I was not very convinced in tweaking the events at all: each pair
of them has been emitted around bdrv_aligned_preadv(), and the new branch
doesn't do it anymore. So I don't see a reason to add events here.

Fam

Re: [Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests

[Kevin Wolf]

Am 27.05.2016 um 02:36 hat Fam Zheng geschrieben:
> On Thu, 05/26 11:20, Paolo Bonzini wrote:
> > On 26/05/2016 10:30, Fam Zheng wrote:
> > >> > 
> > >> > This doesn't look too wrong...  Should the right sequence of events be
> > >> > head/after_head or head/after_tail?  It's probably simplest to just emit
> > >> > all four events.
> > > I've no idea. (That's why I leaned towards fixing the test case).
> > 
> > Well, fixing the testcase means knowing what events should be emitted.
> > 
> > QEMU with Peter's patch emits head/after_head.  If the right one is
> > head/after_tail, _both QEMU and the testcase_ need to be adjusted.  Your
> > patch keeps the backwards-compatible route.
> 
> Yes, I mean I was not very convinced in tweaking the events at all: each pair
> of them has been emitted around bdrv_aligned_preadv(), and the new branch
> doesn't do it anymore. So I don't see a reason to add events here.

Yes, if you can assume that anyone who uses the debug events know
exactly what the code looks like, adding the events here is pointless
because TAIL, AFTER_TAIL and for the greatest part also AFTER_HEAD are
essentially the same then.

Having TAIL before the qiov change and AFTER_TAIL afterwards doesn't
make any difference, they could (and should) be called immediately one
after another if we wanted to keep the behaviour.

I would agree that we should take a look at the test case and what it
actually wants to achieve before we can decide whether AFTER_HEAD and
TAIL/AFTER_TAIL would be the same (the former could trigger earlier if
there are two requests and only one is unaligned at the tail). Maybe we
even need to extend the test case now so that both paths (explicit read
of the tail and the shortcut) are covered.

Kevin

Re: [Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests

[Peter Lieven]

Am 27.05.2016 um 10:55 schrieb Kevin Wolf:
> Am 27.05.2016 um 02:36 hat Fam Zheng geschrieben:
>> On Thu, 05/26 11:20, Paolo Bonzini wrote:
>>> On 26/05/2016 10:30, Fam Zheng wrote:
>>>>>> This doesn't look too wrong...  Should the right sequence of events be
>>>>>> head/after_head or head/after_tail?  It's probably simplest to just emit
>>>>>> all four events.
>>>> I've no idea. (That's why I leaned towards fixing the test case).
>>> Well, fixing the testcase means knowing what events should be emitted.
>>>
>>> QEMU with Peter's patch emits head/after_head.  If the right one is
>>> head/after_tail, _both QEMU and the testcase_ need to be adjusted.  Your
>>> patch keeps the backwards-compatible route.
>> Yes, I mean I was not very convinced in tweaking the events at all: each pair
>> of them has been emitted around bdrv_aligned_preadv(), and the new branch
>> doesn't do it anymore. So I don't see a reason to add events here.
> Yes, if you can assume that anyone who uses the debug events know
> exactly what the code looks like, adding the events here is pointless
> because TAIL, AFTER_TAIL and for the greatest part also AFTER_HEAD are
> essentially the same then.
>
> Having TAIL before the qiov change and AFTER_TAIL afterwards doesn't
> make any difference, they could (and should) be called immediately one
> after another if we wanted to keep the behaviour.
>
> I would agree that we should take a look at the test case and what it
> actually wants to achieve before we can decide whether AFTER_HEAD and
> TAIL/AFTER_TAIL would be the same (the former could trigger earlier if
> there are two requests and only one is unaligned at the tail). Maybe we
> even need to extend the test case now so that both paths (explicit read
> of the tail and the shortcut) are covered.

The part that actually blocks in 077 is

# Sequential RMW requests on the same physical sector

its expecting all 4 events around the RMW cycle.

However, it seems that also other parts of 077 would need an adjustment
and the output might differ depending on the alignment. So I guess we
have to emit the events if we don't want to recode the whole 077 and make
it aware of the alignment.

Peter

Re: [Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests

[Kevin Wolf]

Am 30.05.2016 um 08:25 hat Peter Lieven geschrieben:
> Am 27.05.2016 um 10:55 schrieb Kevin Wolf:
> >Am 27.05.2016 um 02:36 hat Fam Zheng geschrieben:
> >>On Thu, 05/26 11:20, Paolo Bonzini wrote:
> >>>On 26/05/2016 10:30, Fam Zheng wrote:
> >>>>>>This doesn't look too wrong...  Should the right sequence of events be
> >>>>>>head/after_head or head/after_tail?  It's probably simplest to just emit
> >>>>>>all four events.
> >>>>I've no idea. (That's why I leaned towards fixing the test case).
> >>>Well, fixing the testcase means knowing what events should be emitted.
> >>>
> >>>QEMU with Peter's patch emits head/after_head.  If the right one is
> >>>head/after_tail, _both QEMU and the testcase_ need to be adjusted.  Your
> >>>patch keeps the backwards-compatible route.
> >>Yes, I mean I was not very convinced in tweaking the events at all: each pair
> >>of them has been emitted around bdrv_aligned_preadv(), and the new branch
> >>doesn't do it anymore. So I don't see a reason to add events here.
> >Yes, if you can assume that anyone who uses the debug events know
> >exactly what the code looks like, adding the events here is pointless
> >because TAIL, AFTER_TAIL and for the greatest part also AFTER_HEAD are
> >essentially the same then.
> >
> >Having TAIL before the qiov change and AFTER_TAIL afterwards doesn't
> >make any difference, they could (and should) be called immediately one
> >after another if we wanted to keep the behaviour.
> >
> >I would agree that we should take a look at the test case and what it
> >actually wants to achieve before we can decide whether AFTER_HEAD and
> >TAIL/AFTER_TAIL would be the same (the former could trigger earlier if
> >there are two requests and only one is unaligned at the tail). Maybe we
> >even need to extend the test case now so that both paths (explicit read
> >of the tail and the shortcut) are covered.
> 
> The part that actually blocks in 077 is
> 
> # Sequential RMW requests on the same physical sector
> 
> its expecting all 4 events around the RMW cycle.
> 
> However, it seems that also other parts of 077 would need an adjustment
> and the output might differ depending on the alignment. So I guess we
> have to emit the events if we don't want to recode the whole 077 and make
> it aware of the alignment.

Yes, but my point is that we may need to rework 077 anyway if we don't
only want to make it pass again, but to cover all relevant paths, too.
We got a new code path and it's unlikely that the existing tests covered
both the old code path and the new one.

Kevin

Re: [Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests

[Peter Lieven]

Am 30.05.2016 um 10:24 schrieb Kevin Wolf:
> Am 30.05.2016 um 08:25 hat Peter Lieven geschrieben:
>> Am 27.05.2016 um 10:55 schrieb Kevin Wolf:
>>> Am 27.05.2016 um 02:36 hat Fam Zheng geschrieben:
>>>> On Thu, 05/26 11:20, Paolo Bonzini wrote:
>>>>> On 26/05/2016 10:30, Fam Zheng wrote:
>>>>>>>> This doesn't look too wrong...  Should the right sequence of events be
>>>>>>>> head/after_head or head/after_tail?  It's probably simplest to just emit
>>>>>>>> all four events.
>>>>>> I've no idea. (That's why I leaned towards fixing the test case).
>>>>> Well, fixing the testcase means knowing what events should be emitted.
>>>>>
>>>>> QEMU with Peter's patch emits head/after_head.  If the right one is
>>>>> head/after_tail, _both QEMU and the testcase_ need to be adjusted.  Your
>>>>> patch keeps the backwards-compatible route.
>>>> Yes, I mean I was not very convinced in tweaking the events at all: each pair
>>>> of them has been emitted around bdrv_aligned_preadv(), and the new branch
>>>> doesn't do it anymore. So I don't see a reason to add events here.
>>> Yes, if you can assume that anyone who uses the debug events know
>>> exactly what the code looks like, adding the events here is pointless
>>> because TAIL, AFTER_TAIL and for the greatest part also AFTER_HEAD are
>>> essentially the same then.
>>>
>>> Having TAIL before the qiov change and AFTER_TAIL afterwards doesn't
>>> make any difference, they could (and should) be called immediately one
>>> after another if we wanted to keep the behaviour.
>>>
>>> I would agree that we should take a look at the test case and what it
>>> actually wants to achieve before we can decide whether AFTER_HEAD and
>>> TAIL/AFTER_TAIL would be the same (the former could trigger earlier if
>>> there are two requests and only one is unaligned at the tail). Maybe we
>>> even need to extend the test case now so that both paths (explicit read
>>> of the tail and the shortcut) are covered.
>> The part that actually blocks in 077 is
>>
>> # Sequential RMW requests on the same physical sector
>>
>> its expecting all 4 events around the RMW cycle.
>>
>> However, it seems that also other parts of 077 would need an adjustment
>> and the output might differ depending on the alignment. So I guess we
>> have to emit the events if we don't want to recode the whole 077 and make
>> it aware of the alignment.
> Yes, but my point is that we may need to rework 077 anyway if we don't
> only want to make it pass again, but to cover all relevant paths, too.
> We got a new code path and it's unlikely that the existing tests covered
> both the old code path and the new one.

So you would postpone this patch until 077 is reworked?
I found this one a nice improvement and 077 might take some time.

Peter

Re: [Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests

[Kevin Wolf]

Am 30.05.2016 um 11:30 hat Peter Lieven geschrieben:
> Am 30.05.2016 um 10:24 schrieb Kevin Wolf:
> >Am 30.05.2016 um 08:25 hat Peter Lieven geschrieben:
> >>Am 27.05.2016 um 10:55 schrieb Kevin Wolf:
> >>>Am 27.05.2016 um 02:36 hat Fam Zheng geschrieben:
> >>>>On Thu, 05/26 11:20, Paolo Bonzini wrote:
> >>>>>On 26/05/2016 10:30, Fam Zheng wrote:
> >>>>>>>>This doesn't look too wrong...  Should the right sequence of events be
> >>>>>>>>head/after_head or head/after_tail?  It's probably simplest to just emit
> >>>>>>>>all four events.
> >>>>>>I've no idea. (That's why I leaned towards fixing the test case).
> >>>>>Well, fixing the testcase means knowing what events should be emitted.
> >>>>>
> >>>>>QEMU with Peter's patch emits head/after_head.  If the right one is
> >>>>>head/after_tail, _both QEMU and the testcase_ need to be adjusted.  Your
> >>>>>patch keeps the backwards-compatible route.
> >>>>Yes, I mean I was not very convinced in tweaking the events at all: each pair
> >>>>of them has been emitted around bdrv_aligned_preadv(), and the new branch
> >>>>doesn't do it anymore. So I don't see a reason to add events here.
> >>>Yes, if you can assume that anyone who uses the debug events know
> >>>exactly what the code looks like, adding the events here is pointless
> >>>because TAIL, AFTER_TAIL and for the greatest part also AFTER_HEAD are
> >>>essentially the same then.
> >>>
> >>>Having TAIL before the qiov change and AFTER_TAIL afterwards doesn't
> >>>make any difference, they could (and should) be called immediately one
> >>>after another if we wanted to keep the behaviour.
> >>>
> >>>I would agree that we should take a look at the test case and what it
> >>>actually wants to achieve before we can decide whether AFTER_HEAD and
> >>>TAIL/AFTER_TAIL would be the same (the former could trigger earlier if
> >>>there are two requests and only one is unaligned at the tail). Maybe we
> >>>even need to extend the test case now so that both paths (explicit read
> >>>of the tail and the shortcut) are covered.
> >>The part that actually blocks in 077 is
> >>
> >># Sequential RMW requests on the same physical sector
> >>
> >>its expecting all 4 events around the RMW cycle.
> >>
> >>However, it seems that also other parts of 077 would need an adjustment
> >>and the output might differ depending on the alignment. So I guess we
> >>have to emit the events if we don't want to recode the whole 077 and make
> >>it aware of the alignment.
> >Yes, but my point is that we may need to rework 077 anyway if we don't
> >only want to make it pass again, but to cover all relevant paths, too.
> >We got a new code path and it's unlikely that the existing tests covered
> >both the old code path and the new one.
> 
> So you would postpone this patch until 077 is reworked?
> I found this one a nice improvement and 077 might take some time.

The problem with "we'll rework the tests later" is always that it
doesn't happen if the patches for the functional parts and a workaround
for the test case are merged.

I don't think that making 077 cover both cases should be hard or take
much time, it just needs to be done. If all the time for writing emails
in this thread had been used to work on the test case, it would already
be done.

Kevin

Re: [Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests

[Peter Lieven]

Am 30.05.2016 um 11:47 schrieb Kevin Wolf:
> Am 30.05.2016 um 11:30 hat Peter Lieven geschrieben:
>> Am 30.05.2016 um 10:24 schrieb Kevin Wolf:
>>> Am 30.05.2016 um 08:25 hat Peter Lieven geschrieben:
>>>> Am 27.05.2016 um 10:55 schrieb Kevin Wolf:
>>>>> Am 27.05.2016 um 02:36 hat Fam Zheng geschrieben:
>>>>>> On Thu, 05/26 11:20, Paolo Bonzini wrote:
>>>>>>> On 26/05/2016 10:30, Fam Zheng wrote:
>>>>>>>>>> This doesn't look too wrong...  Should the right sequence of events be
>>>>>>>>>> head/after_head or head/after_tail?  It's probably simplest to just emit
>>>>>>>>>> all four events.
>>>>>>>> I've no idea. (That's why I leaned towards fixing the test case).
>>>>>>> Well, fixing the testcase means knowing what events should be emitted.
>>>>>>>
>>>>>>> QEMU with Peter's patch emits head/after_head.  If the right one is
>>>>>>> head/after_tail, _both QEMU and the testcase_ need to be adjusted.  Your
>>>>>>> patch keeps the backwards-compatible route.
>>>>>> Yes, I mean I was not very convinced in tweaking the events at all: each pair
>>>>>> of them has been emitted around bdrv_aligned_preadv(), and the new branch
>>>>>> doesn't do it anymore. So I don't see a reason to add events here.
>>>>> Yes, if you can assume that anyone who uses the debug events know
>>>>> exactly what the code looks like, adding the events here is pointless
>>>>> because TAIL, AFTER_TAIL and for the greatest part also AFTER_HEAD are
>>>>> essentially the same then.
>>>>>
>>>>> Having TAIL before the qiov change and AFTER_TAIL afterwards doesn't
>>>>> make any difference, they could (and should) be called immediately one
>>>>> after another if we wanted to keep the behaviour.
>>>>>
>>>>> I would agree that we should take a look at the test case and what it
>>>>> actually wants to achieve before we can decide whether AFTER_HEAD and
>>>>> TAIL/AFTER_TAIL would be the same (the former could trigger earlier if
>>>>> there are two requests and only one is unaligned at the tail). Maybe we
>>>>> even need to extend the test case now so that both paths (explicit read
>>>>> of the tail and the shortcut) are covered.
>>>> The part that actually blocks in 077 is
>>>>
>>>> # Sequential RMW requests on the same physical sector
>>>>
>>>> its expecting all 4 events around the RMW cycle.
>>>>
>>>> However, it seems that also other parts of 077 would need an adjustment
>>>> and the output might differ depending on the alignment. So I guess we
>>>> have to emit the events if we don't want to recode the whole 077 and make
>>>> it aware of the alignment.
>>> Yes, but my point is that we may need to rework 077 anyway if we don't
>>> only want to make it pass again, but to cover all relevant paths, too.
>>> We got a new code path and it's unlikely that the existing tests covered
>>> both the old code path and the new one.
>> So you would postpone this patch until 077 is reworked?
>> I found this one a nice improvement and 077 might take some time.
> The problem with "we'll rework the tests later" is always that it
> doesn't happen if the patches for the functional parts and a workaround
> for the test case are merged.
>
> I don't think that making 077 cover both cases should be hard or take
> much time, it just needs to be done. If all the time for writing emails
> in this thread had been used to work on the test case, it would already
> be done.

Understood. If you can give a hint how to get the value of the align
parameter into the test script I can try. Otherwise the test will fail
also if any block driver has an align value that is not equal to 512.

Peter

Re: [Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests

[Kevin Wolf]

Am 30.05.2016 um 11:53 hat Peter Lieven geschrieben:
> Am 30.05.2016 um 11:47 schrieb Kevin Wolf:
> >Am 30.05.2016 um 11:30 hat Peter Lieven geschrieben:
> >>Am 30.05.2016 um 10:24 schrieb Kevin Wolf:
> >>>Am 30.05.2016 um 08:25 hat Peter Lieven geschrieben:
> >>>>Am 27.05.2016 um 10:55 schrieb Kevin Wolf:
> >>>>>Am 27.05.2016 um 02:36 hat Fam Zheng geschrieben:
> >>>>>>On Thu, 05/26 11:20, Paolo Bonzini wrote:
> >>>>>>>On 26/05/2016 10:30, Fam Zheng wrote:
> >>>>>>>>>>This doesn't look too wrong...  Should the right sequence of events be
> >>>>>>>>>>head/after_head or head/after_tail?  It's probably simplest to just emit
> >>>>>>>>>>all four events.
> >>>>>>>>I've no idea. (That's why I leaned towards fixing the test case).
> >>>>>>>Well, fixing the testcase means knowing what events should be emitted.
> >>>>>>>
> >>>>>>>QEMU with Peter's patch emits head/after_head.  If the right one is
> >>>>>>>head/after_tail, _both QEMU and the testcase_ need to be adjusted.  Your
> >>>>>>>patch keeps the backwards-compatible route.
> >>>>>>Yes, I mean I was not very convinced in tweaking the events at all: each pair
> >>>>>>of them has been emitted around bdrv_aligned_preadv(), and the new branch
> >>>>>>doesn't do it anymore. So I don't see a reason to add events here.
> >>>>>Yes, if you can assume that anyone who uses the debug events know
> >>>>>exactly what the code looks like, adding the events here is pointless
> >>>>>because TAIL, AFTER_TAIL and for the greatest part also AFTER_HEAD are
> >>>>>essentially the same then.
> >>>>>
> >>>>>Having TAIL before the qiov change and AFTER_TAIL afterwards doesn't
> >>>>>make any difference, they could (and should) be called immediately one
> >>>>>after another if we wanted to keep the behaviour.
> >>>>>
> >>>>>I would agree that we should take a look at the test case and what it
> >>>>>actually wants to achieve before we can decide whether AFTER_HEAD and
> >>>>>TAIL/AFTER_TAIL would be the same (the former could trigger earlier if
> >>>>>there are two requests and only one is unaligned at the tail). Maybe we
> >>>>>even need to extend the test case now so that both paths (explicit read
> >>>>>of the tail and the shortcut) are covered.
> >>>>The part that actually blocks in 077 is
> >>>>
> >>>># Sequential RMW requests on the same physical sector
> >>>>
> >>>>its expecting all 4 events around the RMW cycle.
> >>>>
> >>>>However, it seems that also other parts of 077 would need an adjustment
> >>>>and the output might differ depending on the alignment. So I guess we
> >>>>have to emit the events if we don't want to recode the whole 077 and make
> >>>>it aware of the alignment.
> >>>Yes, but my point is that we may need to rework 077 anyway if we don't
> >>>only want to make it pass again, but to cover all relevant paths, too.
> >>>We got a new code path and it's unlikely that the existing tests covered
> >>>both the old code path and the new one.
> >>So you would postpone this patch until 077 is reworked?
> >>I found this one a nice improvement and 077 might take some time.
> >The problem with "we'll rework the tests later" is always that it
> >doesn't happen if the patches for the functional parts and a workaround
> >for the test case are merged.
> >
> >I don't think that making 077 cover both cases should be hard or take
> >much time, it just needs to be done. If all the time for writing emails
> >in this thread had been used to work on the test case, it would already
> >be done.
> 
> Understood. If you can give a hint how to get the value of the align
> parameter into the test script I can try. Otherwise the test will fail
> also if any block driver has an align value that is not equal to 512.

The test case already uses blkdebug to enforce a specific align value
(which is 4096 in this test case, not 512):

    echo "open -o driver=$IMGFMT,file.align=4k blkdebug::$TEST_IMG"

Kevin

Re: [Qemu-devel] [PATCH V2] block/io: optimize bdrv_co_pwritev for small requests

[Peter Lieven]

Am 30.05.2016 um 12:06 schrieb Kevin Wolf:
> Am 30.05.2016 um 11:53 hat Peter Lieven geschrieben:
>> Am 30.05.2016 um 11:47 schrieb Kevin Wolf:
>>> Am 30.05.2016 um 11:30 hat Peter Lieven geschrieben:
>>>> Am 30.05.2016 um 10:24 schrieb Kevin Wolf:
>>>>> Am 30.05.2016 um 08:25 hat Peter Lieven geschrieben:
>>>>>> Am 27.05.2016 um 10:55 schrieb Kevin Wolf:
>>>>>>> Am 27.05.2016 um 02:36 hat Fam Zheng geschrieben:
>>>>>>>> On Thu, 05/26 11:20, Paolo Bonzini wrote:
>>>>>>>>> On 26/05/2016 10:30, Fam Zheng wrote:
>>>>>>>>>>>> This doesn't look too wrong...  Should the right sequence of events be
>>>>>>>>>>>> head/after_head or head/after_tail?  It's probably simplest to just emit
>>>>>>>>>>>> all four events.
>>>>>>>>>> I've no idea. (That's why I leaned towards fixing the test case).
>>>>>>>>> Well, fixing the testcase means knowing what events should be emitted.
>>>>>>>>>
>>>>>>>>> QEMU with Peter's patch emits head/after_head.  If the right one is
>>>>>>>>> head/after_tail, _both QEMU and the testcase_ need to be adjusted.  Your
>>>>>>>>> patch keeps the backwards-compatible route.
>>>>>>>> Yes, I mean I was not very convinced in tweaking the events at all: each pair
>>>>>>>> of them has been emitted around bdrv_aligned_preadv(), and the new branch
>>>>>>>> doesn't do it anymore. So I don't see a reason to add events here.
>>>>>>> Yes, if you can assume that anyone who uses the debug events know
>>>>>>> exactly what the code looks like, adding the events here is pointless
>>>>>>> because TAIL, AFTER_TAIL and for the greatest part also AFTER_HEAD are
>>>>>>> essentially the same then.
>>>>>>>
>>>>>>> Having TAIL before the qiov change and AFTER_TAIL afterwards doesn't
>>>>>>> make any difference, they could (and should) be called immediately one
>>>>>>> after another if we wanted to keep the behaviour.
>>>>>>>
>>>>>>> I would agree that we should take a look at the test case and what it
>>>>>>> actually wants to achieve before we can decide whether AFTER_HEAD and
>>>>>>> TAIL/AFTER_TAIL would be the same (the former could trigger earlier if
>>>>>>> there are two requests and only one is unaligned at the tail). Maybe we
>>>>>>> even need to extend the test case now so that both paths (explicit read
>>>>>>> of the tail and the shortcut) are covered.
>>>>>> The part that actually blocks in 077 is
>>>>>>
>>>>>> # Sequential RMW requests on the same physical sector
>>>>>>
>>>>>> its expecting all 4 events around the RMW cycle.
>>>>>>
>>>>>> However, it seems that also other parts of 077 would need an adjustment
>>>>>> and the output might differ depending on the alignment. So I guess we
>>>>>> have to emit the events if we don't want to recode the whole 077 and make
>>>>>> it aware of the alignment.
>>>>> Yes, but my point is that we may need to rework 077 anyway if we don't
>>>>> only want to make it pass again, but to cover all relevant paths, too.
>>>>> We got a new code path and it's unlikely that the existing tests covered
>>>>> both the old code path and the new one.
>>>> So you would postpone this patch until 077 is reworked?
>>>> I found this one a nice improvement and 077 might take some time.
>>> The problem with "we'll rework the tests later" is always that it
>>> doesn't happen if the patches for the functional parts and a workaround
>>> for the test case are merged.
>>>
>>> I don't think that making 077 cover both cases should be hard or take
>>> much time, it just needs to be done. If all the time for writing emails
>>> in this thread had been used to work on the test case, it would already
>>> be done.
>> Understood. If you can give a hint how to get the value of the align
>> parameter into the test script I can try. Otherwise the test will fail
>> also if any block driver has an align value that is not equal to 512.
> The test case already uses blkdebug to enforce a specific align value
> (which is 4096 in this test case, not 512):
>
>      echo "open -o driver=$IMGFMT,file.align=4k blkdebug::$TEST_IMG"

Sorry, I missed that. Then I will try to fix 077.

Peter