[Qemu-devel] [PATCH +stable] block: don't attempt to merge overlapping requests

[Qemu-devel] [PATCH +stable] block: don't attempt to merge overlapping requests

[Avi Kivity]

The block multiwrite code pretends to be able to merge overlapping requests,
but doesn't do so in fact.  This leads to I/O errors (for example on mkfs
of a large virtio disk).

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 block.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 48305b7..0e44e26 100644
--- a/block.c
+++ b/block.c
@@ -1956,8 +1956,8 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs,
         int64_t oldreq_last = reqs[outidx].sector + reqs[outidx].nb_sectors;
 
         // This handles the cases that are valid for all block drivers, namely
-        // exactly sequential writes and overlapping writes.
-        if (reqs[i].sector <= oldreq_last) {
+        // exactly sequential writes
+        if (reqs[i].sector == oldreq_last) {
             merge = 1;
         }
 
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH +stable] block: don't attempt to merge overlapping requests

[Stefan Hajnoczi]

On Tue, May 18, 2010 at 6:18 PM, Avi Kivity <avi@redhat.com> wrote:
> The block multiwrite code pretends to be able to merge overlapping requests,
> but doesn't do so in fact.  This leads to I/O errors (for example on mkfs
> of a large virtio disk).

Are overlapping write requests correct guest behavior?  I thought the
ordering semantics require a flush between overlapping writes to
ensure A is written before B.

What cache= mode are you running?

Stefan

[Qemu-devel] Re: [PATCH +stable] block: don't attempt to merge overlapping requests

[Stefan Hajnoczi]

I just caught up on mails and saw you had already mentioned that
overlapping writes from the guest look fishy in the "the >1Tb block
issue".  Cache mode might still be interesting because it affects how
guest virtio-blk chooses queue ordering mode.

Stefan

[Qemu-devel] Re: [PATCH +stable] block: don't attempt to merge overlapping requests

[Michael Tokarev]

18.05.2010 23:37, Stefan Hajnoczi wrote:
> I just caught up on mails and saw you had already mentioned that
> overlapping writes from the guest look fishy in the "the>1Tb block
> issue".  Cache mode might still be interesting because it affects how
> guest virtio-blk chooses queue ordering mode.

What I can say for sure is that the issue mentioned (>1Tb block)
occurs regardless of the cache mode.  I tried all 3, with exactly
the same results (well, not entirely exactly, since the prob
depends on timing too, and timing is different depending on the
cache mode), and performed all further tests with cache=writeback
since it's the mode which lets the guest to finish mkfs'ing the
1.5Tb "disk" in a reasonable time (or else it takes hours).

But actually I don't quite see where that dependency is: guest
does not know how its data is cached on host...

/mjt

[Qemu-devel] Re: [PATCH +stable] block: don't attempt to merge overlapping requests

[Stefan Hajnoczi]

On Tue, May 18, 2010 at 8:41 PM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> But actually I don't quite see where that dependency is: guest
> does not know how its data is cached on host...

I was thinking of this bit in linux-2.6:drivers/block/virtio_blk.c:

/* If barriers are supported, tell block layer that queue is ordered */
if (virtio_has_feature(vdev, VIRTIO_BLK_F_FLUSH))
    blk_queue_ordered(q, QUEUE_ORDERED_DRAIN_FLUSH,
                                  virtblk_prepare_flush);
else if (virtio_has_feature(vdev, VIRTIO_BLK_F_BARRIER))
    blk_queue_ordered(q, QUEUE_ORDERED_TAG, NULL);

I was wondering whether we have something wrong, causing the guest to
perform overlapping write requests without draining the queue or
flushing.

Stefan

[Qemu-devel] Re: [PATCH +stable] block: don't attempt to merge overlapping requests

[Avi Kivity]

On 05/18/2010 10:22 PM, Stefan Hajnoczi wrote:
> On Tue, May 18, 2010 at 6:18 PM, Avi Kivity<avi@redhat.com>  wrote:
>    
>> The block multiwrite code pretends to be able to merge overlapping requests,
>> but doesn't do so in fact.  This leads to I/O errors (for example on mkfs
>> of a large virtio disk).
>>      
> Are overlapping write requests correct guest behavior?  I thought the
> ordering semantics require a flush between overlapping writes to
> ensure A is written before B.
>
> What cache= mode are you running?
>    

writeback.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH +stable] block: don't attempt to merge overlapping requests

[Stefan Hajnoczi]

On Wed, May 19, 2010 at 9:09 AM, Avi Kivity <avi@redhat.com> wrote:
> On 05/18/2010 10:22 PM, Stefan Hajnoczi wrote:
>> What cache= mode are you running?
>
> writeback.

In the cache=writeback case the virtio-blk guest driver does:

blk_queue_ordered(q, QUEUE_ORDERED_DRAIN_FLUSH, ...)

Stefan

[Qemu-devel] Re: [PATCH +stable] block: don't attempt to merge overlapping requests

[Avi Kivity]

On 05/19/2010 12:01 PM, Stefan Hajnoczi wrote:
> On Wed, May 19, 2010 at 9:09 AM, Avi Kivity<avi@redhat.com>  wrote:
>    
>> On 05/18/2010 10:22 PM, Stefan Hajnoczi wrote:
>>      
>>> What cache= mode are you running?
>>>        
>> writeback.
>>      
> In the cache=writeback case the virtio-blk guest driver does:
>
> blk_queue_ordered(q, QUEUE_ORDERED_DRAIN_FLUSH, ...)
>    

I don't follow.  What's the implication?

btw I really dislike how the cache attribute (which I see as a pure host 
choice) is exposed to the guest.  It means we can't change caching mode 
on the fly (for example after live migration), or that changing caching 
mode during a restart may expose a previously hidden guest bug.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH +stable] block: don't attempt to merge overlapping requests

[Stefan Hajnoczi]

On Wed, May 19, 2010 at 10:06 AM, Avi Kivity <avi@redhat.com> wrote:
>> In the cache=writeback case the virtio-blk guest driver does:
>>
>> blk_queue_ordered(q, QUEUE_ORDERED_DRAIN_FLUSH, ...)
>>
>
> I don't follow.  What's the implication?

I was wondering whether the queue is incorrectly set to a mode where
overlapping write requests aren't being ordered.  Anyway, Christoph
says overlapping write requests can be issued so my theory is broken.

> btw I really dislike how the cache attribute (which I see as a pure host
> choice) is exposed to the guest.  It means we can't change caching mode on
> the fly (for example after live migration), or that changing caching mode
> during a restart may expose a previously hidden guest bug.

Christoph has mentioned wanting to make the write cache switchable
from inside the guest.

Stefan

[Qemu-devel] Re: [PATCH +stable] block: don't attempt to merge overlapping requests

[Christoph Hellwig]

On Wed, May 19, 2010 at 10:23:44AM +0100, Stefan Hajnoczi wrote:
> On Wed, May 19, 2010 at 10:06 AM, Avi Kivity <avi@redhat.com> wrote:
> >> In the cache=writeback case the virtio-blk guest driver does:
> >>
> >> blk_queue_ordered(q, QUEUE_ORDERED_DRAIN_FLUSH, ...)
> >>
> >
> > I don't follow. ?What's the implication?
> 
> I was wondering whether the queue is incorrectly set to a mode where
> overlapping write requests aren't being ordered.  Anyway, Christoph
> says overlapping write requests can be issued so my theory is broken.

They can happen, but won't during usual pagecache based writeback.  So
this should not happen for the pagecache based mke2fs workload.  It
could happen for a workload with mkfs that uses O_DIRECT.  And we
need to handle it either way.

And btw, our barrier handling for devices using multiwrite (aka virtio)
is utterly busted right now, patch will follow ASAP.