[Qemu-devel] Fwd: qemu drive mirror assert fault

[Qemu-devel] Fwd: qemu drive mirror assert fault

[Paolo Bonzini]

John, Fam,

I got this report offlist.  This happens if a bit in the hbitmap is
cleared and the HBitmap has _not_ yet reached the bit.  See this comment
in include/qemu/hbitmap.h:

 * Resetting bits before the current
 * position of the iterator is also okay.  However, concurrent
 * resetting of bits can lead to unexpected behavior if the iterator
 * has not yet reached those bits.

Can you please take a look?

Thanks,

Paolo

-------- Forwarded Message --------
Subject: 	qemu drive mirror assert fault
Date: 	Wed, 29 Apr 2015 10:50:28 +0800
From: 	wangxiaolong <wangxiaolong@ucloud.cn>
To: 	pbonzini <pbonzini@redhat.com>

hello,

I used drive mirror to do live migration, and I run into such an assert
fault:

(gdb) bt

#0  0x00007fd2c6e678a5 in raise (sig=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64

#1  0x00007fd2c6e69085 in abort () at abort.c:92

#2  0x00007fd2c6e60a1e in __assert_fail_base (fmt=<value optimized out>,
assertion=0x7fd2ca215aa0 "cur", file=0x7fd2ca215a78 "util/hbitmap.c",
line=<value optimized out>,

    function=<value optimized out>) at assert.c:96

#3  0x00007fd2c6e60ae0 in __assert_fail (assertion=0x7fd2ca215aa0 "cur",
file=0x7fd2ca215a78 "util/hbitmap.c", line=129, function=0x7fd2ca215bf0
"hbitmap_iter_skip_words")

    at assert.c:105

#4  0x00007fd2ca1b3bb8 in hbitmap_iter_skip_words (hbi=<value optimized
out>) at util/hbitmap.c:129

#5  0x00007fd2c9f8f8e0 in hbitmap_iter_next (opaque=0x7fd2cc59c730) at
/usr/src/debug/qemu-kvm-1.5.3/include/qemu/hbitmap.h:166

#6  mirror_iteration (opaque=0x7fd2cc59c730) at block/mirror.c:163

#7  mirror_run (opaque=0x7fd2cc59c730) at block/mirror.c:407

#8  0x00007fd2c9fc45bb in coroutine_trampoline (i0=<value optimized
out>, i1=<value optimized out>) at coroutine-ucontext.c:118

#9  0x00007fd2c6e78b70 in ?? () from /lib64/libc-2.12.so

#10 0x00007fff53eede80 in ?? ()

#11 0x0000000000000000 in ?? ()


and I just can’t figure out what is the cause of this situation,
could you help me figure it out, thanks!

Re: [Qemu-devel] Fwd: qemu drive mirror assert fault

[Paolo Bonzini]

On 30/04/2015 17:50, Paolo Bonzini wrote:
> John, Fam,
> 
> I got this report offlist.  This happens if a bit in the hbitmap is
> cleared and the HBitmap has _not_ yet reached the bit.  See this comment
> in include/qemu/hbitmap.h:
> 
>  * Resetting bits before the current
>  * position of the iterator is also okay.  However, concurrent
>  * resetting of bits can lead to unexpected behavior if the iterator
>  * has not yet reached those bits.
> 
> Can you please take a look?

Oh, it's related to discard.  The reported pointed out later that he is
using virtio-scsi and has discard=unmap enabled.

Paolo

Re: [Qemu-devel] Fwd: qemu drive mirror assert fault

[Fam Zheng]

On Thu, 04/30 17:50, Paolo Bonzini wrote:
> John, Fam,
> 
> I got this report offlist.  This happens if a bit in the hbitmap is
> cleared and the HBitmap has _not_ yet reached the bit.  See this comment
> in include/qemu/hbitmap.h:
> 
>  * Resetting bits before the current
>  * position of the iterator is also okay.  However, concurrent
>  * resetting of bits can lead to unexpected behavior if the iterator
>  * has not yet reached those bits.
> 
> Can you please take a look?

Since the gdb output is suggesting 1.5.3, it's worth to trying 2.3 which has
this:

    commit c4237dfa635900e4d1cdc6038d5efe3507f45f0c
    Author: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
    Date:   Thu Nov 27 12:40:46 2014 +0300

        block: fix spoiling all dirty bitmaps by mirror and migration

        Mirror and migration use dirty bitmaps for their purposes, and since
        commit [block: per caller dirty bitmap] they use their own bitmaps, not
        the global one. But they use old functions bdrv_set_dirty and
        bdrv_reset_dirty, which change all dirty bitmaps.

        Named dirty bitmaps series by Fam and Snow are affected: mirroring and
        migration will spoil all (not related to this mirroring or migration)
        named dirty bitmaps.

        This patch fixes this by adding bdrv_set_dirty_bitmap and
        bdrv_reset_dirty_bitmap, which change concrete bitmap. Also, to prevent
        such mistakes in future, old functions bdrv_(set,reset)_dirty are made
        static, for internal block usage.

        Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
        CC: John Snow <jsnow@redhat.com>
        CC: Fam Zheng <famz@redhat.com>
        CC: Denis V. Lunev <den@openvz.org>
        CC: Stefan Hajnoczi <stefanha@redhat.com>
        CC: Kevin Wolf <kwolf@redhat.com>
        Reviewed-by: John Snow <jsnow@redhat.com>
        Reviewed-by: Fam Zheng <famz@redhat.com>
        Message-id: 1417081246-3593-1-git-send-email-vsementsov@parallels.com
        Signed-off-by: Max Reitz <mreitz@redhat.com>

Fam

> 
> Thanks,
> 
> Paolo
> 
> -------- Forwarded Message --------
> Subject: 	qemu drive mirror assert fault
> Date: 	Wed, 29 Apr 2015 10:50:28 +0800
> From: 	wangxiaolong <wangxiaolong@ucloud.cn>
> To: 	pbonzini <pbonzini@redhat.com>
> 
> hello,
> 
> I used drive mirror to do live migration, and I run into such an assert
> fault:
> 
> (gdb) bt
> 
> #0  0x00007fd2c6e678a5 in raise (sig=6) at
> ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> 
> #1  0x00007fd2c6e69085 in abort () at abort.c:92
> 
> #2  0x00007fd2c6e60a1e in __assert_fail_base (fmt=<value optimized out>,
> assertion=0x7fd2ca215aa0 "cur", file=0x7fd2ca215a78 "util/hbitmap.c",
> line=<value optimized out>,
> 
>     function=<value optimized out>) at assert.c:96
> 
> #3  0x00007fd2c6e60ae0 in __assert_fail (assertion=0x7fd2ca215aa0 "cur",
> file=0x7fd2ca215a78 "util/hbitmap.c", line=129, function=0x7fd2ca215bf0
> "hbitmap_iter_skip_words")
> 
>     at assert.c:105
> 
> #4  0x00007fd2ca1b3bb8 in hbitmap_iter_skip_words (hbi=<value optimized
> out>) at util/hbitmap.c:129
> 
> #5  0x00007fd2c9f8f8e0 in hbitmap_iter_next (opaque=0x7fd2cc59c730) at
> /usr/src/debug/qemu-kvm-1.5.3/include/qemu/hbitmap.h:166
> 
> #6  mirror_iteration (opaque=0x7fd2cc59c730) at block/mirror.c:163
> 
> #7  mirror_run (opaque=0x7fd2cc59c730) at block/mirror.c:407
> 
> #8  0x00007fd2c9fc45bb in coroutine_trampoline (i0=<value optimized
> out>, i1=<value optimized out>) at coroutine-ucontext.c:118
> 
> #9  0x00007fd2c6e78b70 in ?? () from /lib64/libc-2.12.so
> 
> #10 0x00007fff53eede80 in ?? ()
> 
> #11 0x0000000000000000 in ?? ()
> 
> 
> and I just can’t figure out what is the cause of this situation,
> could you help me figure it out, thanks!
> 
> 
> 
> 

Re: [Qemu-devel] Fwd: qemu drive mirror assert fault

[Paolo Bonzini]

On 05/05/2015 09:36, Fam Zheng wrote:
> Since the gdb output is suggesting 1.5.3, it's worth to trying 2.3 which has
> this:
> 
>     commit c4237dfa635900e4d1cdc6038d5efe3507f45f0c
>     Author: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>     Date:   Thu Nov 27 12:40:46 2014 +0300
> 
>         block: fix spoiling all dirty bitmaps by mirror and migration
> 
>         Mirror and migration use dirty bitmaps for their purposes, and since
>         commit [block: per caller dirty bitmap] they use their own bitmaps, not
>         the global one. But they use old functions bdrv_set_dirty and
>         bdrv_reset_dirty, which change all dirty bitmaps.
> 
>         Named dirty bitmaps series by Fam and Snow are affected: mirroring and
>         migration will spoil all (not related to this mirroring or migration)
>         named dirty bitmaps.
> 
>         This patch fixes this by adding bdrv_set_dirty_bitmap and
>         bdrv_reset_dirty_bitmap, which change concrete bitmap. Also, to prevent
>         such mistakes in future, old functions bdrv_(set,reset)_dirty are made
>         static, for internal block usage.
> 
>         Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>         CC: John Snow <jsnow@redhat.com>
>         CC: Fam Zheng <famz@redhat.com>
>         CC: Denis V. Lunev <den@openvz.org>
>         CC: Stefan Hajnoczi <stefanha@redhat.com>
>         CC: Kevin Wolf <kwolf@redhat.com>
>         Reviewed-by: John Snow <jsnow@redhat.com>
>         Reviewed-by: Fam Zheng <famz@redhat.com>
>         Message-id: 1417081246-3593-1-git-send-email-vsementsov@parallels.com
>         Signed-off-by: Max Reitz <mreitz@redhat.com>

I don't think this commit is relevant.  The bug is caused by
bdrv_co_discard clearing the bitmap (which is not supported during
iteration, and is caught by the assertion).  That bdrv_reset_dirty is
not removed by the commit you pointed out.

Paolo

Re: [Qemu-devel] Fwd: qemu drive mirror assert fault

[Fam Zheng]

On Tue, 05/05 12:13, Paolo Bonzini wrote:
> 
> 
> On 05/05/2015 09:36, Fam Zheng wrote:
> I don't think this commit is relevant.  The bug is caused by
> bdrv_co_discard clearing the bitmap (which is not supported during
> iteration, and is caught by the assertion).  That bdrv_reset_dirty is
> not removed by the commit you pointed out.

You are right, I think the bug is the bdrv_reset_dirty in bdrv_co_discard. The
discard may zero out the src area (depending on the backend), we should
probably replicate this to dest (actual data or the discard operation), rather
than forgetting the change.

Fam

Re: [Qemu-devel] Fwd: qemu drive mirror assert fault

[Kevin Wolf]

Am 05.05.2015 um 12:27 hat Fam Zheng geschrieben:
> On Tue, 05/05 12:13, Paolo Bonzini wrote:
> > 
> > 
> > On 05/05/2015 09:36, Fam Zheng wrote:
> > I don't think this commit is relevant.  The bug is caused by
> > bdrv_co_discard clearing the bitmap (which is not supported during
> > iteration, and is caught by the assertion).  That bdrv_reset_dirty is
> > not removed by the commit you pointed out.
> 
> You are right, I think the bug is the bdrv_reset_dirty in bdrv_co_discard. The
> discard may zero out the src area (depending on the backend), we should
> probably replicate this to dest (actual data or the discard operation), rather
> than forgetting the change.

It depends. In its basic form, bdrv_discard() just means "I don't care
about the data any more". Then clearing the dirty bitmap is correct.

The content is only important if the caller used discard to write zeros
because can_write_zeroes_with_unmap = true. Do we have any such callers
apart from qemu-img convert?

Kevin

Re: [Qemu-devel] Fwd: qemu drive mirror assert fault

[Paolo Bonzini]

On 05/05/2015 13:48, Kevin Wolf wrote:
> It depends. In its basic form, bdrv_discard() just means "I don't care
> about the data any more". Then clearing the dirty bitmap is correct.
> 
> The content is only important if the caller used discard to write zeros
> because can_write_zeroes_with_unmap = true. Do we have any such callers
> apart from qemu-img convert?

Yes, the SCSI command WRITE SAME with UNMAP = 1 (not coincidentially :))
calls discard too.  Who knows what the guest used it for...

However, write zeroes doesn't go through bdrv_co_discard, does it?

Paolo

Re: [Qemu-devel] Fwd: qemu drive mirror assert fault

[Paolo Bonzini]

On 05/05/2015 13:49, Paolo Bonzini wrote:
> 
> 
> On 05/05/2015 13:48, Kevin Wolf wrote:
>> It depends. In its basic form, bdrv_discard() just means "I don't care
>> about the data any more". Then clearing the dirty bitmap is correct.
>>
>> The content is only important if the caller used discard to write zeros
>> because can_write_zeroes_with_unmap = true. Do we have any such callers
>> apart from qemu-img convert?
> 
> Yes, the SCSI command WRITE SAME with UNMAP = 1 (not coincidentially :))
> calls discard too.

s/discard/write_zeroes/ of course!

Paolo

> Who knows what the guest used it for...
> However, write zeroes doesn't go through bdrv_co_discard, does it?
> 
> Paolo
> 
> 

Re: [Qemu-devel] Fwd: qemu drive mirror assert fault

[Kevin Wolf]

Am 05.05.2015 um 13:49 hat Paolo Bonzini geschrieben:
> 
> 
> On 05/05/2015 13:48, Kevin Wolf wrote:
> > It depends. In its basic form, bdrv_discard() just means "I don't care
> > about the data any more". Then clearing the dirty bitmap is correct.
> > 
> > The content is only important if the caller used discard to write zeros
> > because can_write_zeroes_with_unmap = true. Do we have any such callers
> > apart from qemu-img convert?
> 
> Yes, the SCSI command WRITE SAME with UNMAP = 1 (not coincidentially :))
> calls discard too.  Who knows what the guest used it for...
> 
> However, write zeroes doesn't go through bdrv_co_discard, does it?

Initially I expected that it does, but when I checked, it turned out
that it uses a different path. The only thing I found that really uses
discard is the call in qemu-img that I mentioned. But that can't be the
cause of the corruption you're debugging.

Kevin

Re: [Qemu-devel] Fwd: qemu drive mirror assert fault

[Paolo Bonzini]

On 05/05/2015 15:03, Kevin Wolf wrote:
> > Yes, the SCSI command WRITE SAME with UNMAP = 1 (not coincidentially :))
> > calls discard too.  Who knows what the guest used it for...
> > 
> > However, write zeroes doesn't go through bdrv_co_discard, does it?
> 
> Initially I expected that it does, but when I checked, it turned out
> that it uses a different path. The only thing I found that really uses
> discard is the call in qemu-img that I mentioned. But that can't be the
> cause of the corruption you're debugging.

I'm not seeing corruption; it's a crash in drive-mirror.

The question is how to treat discard:

1) mark sectors as dirty, discard on the destination as well (similar to
Fam's just-posted patches, but they do not discard on the destination)

2) keep marking sectors as not dirty, change util/hbitmap.c to avoid the
assertion failure.  This loses the discard on the destination but no
need to change block/mirror.c

Both are valid approaches.

Paolo

Re: [Qemu-devel] Fwd: qemu drive mirror assert fault

[Kevin Wolf]

Am 05.05.2015 um 15:07 hat Paolo Bonzini geschrieben:
> 
> 
> On 05/05/2015 15:03, Kevin Wolf wrote:
> > > Yes, the SCSI command WRITE SAME with UNMAP = 1 (not coincidentially :))
> > > calls discard too.  Who knows what the guest used it for...
> > > 
> > > However, write zeroes doesn't go through bdrv_co_discard, does it?
> > 
> > Initially I expected that it does, but when I checked, it turned out
> > that it uses a different path. The only thing I found that really uses
> > discard is the call in qemu-img that I mentioned. But that can't be the
> > cause of the corruption you're debugging.
> 
> I'm not seeing corruption; it's a crash in drive-mirror.

Ah, yes, sorry. I was confused.

> The question is how to treat discard:
> 
> 1) mark sectors as dirty, discard on the destination as well (similar to
> Fam's just-posted patches, but they do not discard on the destination)
> 
> 2) keep marking sectors as not dirty, change util/hbitmap.c to avoid the
> assertion failure.  This loses the discard on the destination but no
> need to change block/mirror.c
> 
> Both are valid approaches.

Which kind of suggests that it should be an option. But if I had to
choose one, I think 1) is nicer because it results in an exact copy.

Kevin

Re: [Qemu-devel] Fwd: qemu drive mirror assert fault

[Fam Zheng]

On Tue, 05/05 13:48, Kevin Wolf wrote:
> Am 05.05.2015 um 12:27 hat Fam Zheng geschrieben:
> > On Tue, 05/05 12:13, Paolo Bonzini wrote:
> > > 
> > > 
> > > On 05/05/2015 09:36, Fam Zheng wrote:
> > > I don't think this commit is relevant.  The bug is caused by
> > > bdrv_co_discard clearing the bitmap (which is not supported during
> > > iteration, and is caught by the assertion).  That bdrv_reset_dirty is
> > > not removed by the commit you pointed out.
> > 
> > You are right, I think the bug is the bdrv_reset_dirty in bdrv_co_discard. The
> > discard may zero out the src area (depending on the backend), we should
> > probably replicate this to dest (actual data or the discard operation), rather
> > than forgetting the change.
> 
> It depends. In its basic form, bdrv_discard() just means "I don't care
> about the data any more". Then clearing the dirty bitmap is correct.
> 
> The content is only important if the caller used discard to write zeros
> because can_write_zeroes_with_unmap = true. Do we have any such callers
> apart from qemu-img convert?

The guest could be taking advantage of this "can_write_zeroes_with_unmap"..

Fam