[Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Alexey Kardashevskiy]

Hi!

I am hitting an racing issue with migration.

I migrate a guest from one machine to another using libvirt:

virsh migrate --live --persistent --undefinesource --copy-storage-all
--verbose --desturi qemu+ssh://legkvm/system --domain chig1

I.e. it copied the full disk which is qcow2, 20G virtual, 10GB of real disk
space.

When migration finished, process_incoming_migration_co() calls
bdrv_invalidate_cache_all() which calls qcow2_invalidate_cache() which does
qcow2_close() and the latter destroys l2_table_cache and
refcount_block_cache. It also calls qcow2_cache_flush(). All good.

However somehow after migration is completed as described above,
qcow2_co_flush_to_os() is called again and I either get crash in
qcow2_cache_flush (as @c==NULL) or I get assert like below as s->lock is
not set, the backtrace is below.

In qcow2_co_flush_to_os(), @bs points to valid data and @s is empty.

The xml I used is at the end of this email. But it does not seem essential
for the problem - it only happens on two selected machines and I cannot
reproduce it on the machines I got locally.

It sounds to me like qcow2_close() is called when there is still a
qcow2_co_flush_to_os() "coroutine" in flight (there is always one, I believe).

Why is this happening and how to fix it properly? Thanks.




Program received signal SIGSEGV, Segmentation fault.
0x0000000010504204 in qcow2_cache_flush (bs=0x10019aab420, c=0x0) at
/home/alexey/p/qemu/block/qcow2-cache.c:174
174	    for (i = 0; i < c->size; i++) {
(gdb) bt
#0  0x0000000010504204 in qcow2_cache_flush (bs=0x10019aab420, c=0x0) at
/home/alexey/p/qemu/block/qcow2-cache.c:174
#1  0x00000000104f557c in qcow2_co_flush_to_os (bs=0x10019aab420) at
/home/alexey/p/qemu/block/qcow2.c:2162
#2  0x00000000104c126c in bdrv_co_flush (bs=0x10019aab420) at
/home/alexey/p/qemu/block.c:4971
#3  0x00000000104b2000 in nbd_trip (opaque=0x10019cf75c0) at
/home/alexey/p/qemu/nbd.c:1259
#4  0x00000000104d17d4 in coroutine_trampoline (i0=0x100, i1=0x19cd5c00) at
/home/alexey/p/qemu/coroutine-ucontext.c:118
#5  0x00003fff94ff099c in .__makecontext () from /usr/lib64/libc.so.6
#6  0x0eeabf8ea4edc5a2 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
(gdb) up
#1  0x00000000104f557c in qcow2_co_flush_to_os (bs=0x10019aab420) at
/home/alexey/p/qemu/block/qcow2.c:2162
2162	    ret = qcow2_cache_flush(bs, s->l2_table_cache);
(gdb) p s
$1 = (BDRVQcowState *) 0x10019aaf300
(gdb) p s->l2_table_cache
$2 = (Qcow2Cache *) 0x0
(gdb) p *s
$3 = {
  cluster_bits = 0x0,
  cluster_size = 0x0,
  cluster_sectors = 0x0,
  l2_bits = 0x0,
  l2_size = 0x0,
  l1_size = 0x0,
  l1_vm_state_index = 0x0,
  csize_shift = 0x0,
  csize_mask = 0x0,
  cluster_offset_mask = 0x0,
  l1_table_offset = 0x0,
  l1_table = 0x0,
  l2_table_cache = 0x0,
  refcount_block_cache = 0x0,
  cluster_cache = 0x0,
  cluster_data = 0x0,
  cluster_cache_offset = 0x0,
  cluster_allocs = {
    lh_first = 0x0
  },
  refcount_table = 0x0,
  refcount_table_offset = 0x0,
  refcount_table_size = 0x0,
  free_cluster_index = 0x0,
  free_byte_offset = 0x0,
  lock = {
    locked = 0x1,
    queue = {
      entries = {
        tqh_first = 0x0,
        tqh_last = 0x0
      }
    }
  },
  crypt_method = 0x0,
  crypt_method_header = 0x0,
  aes_encrypt_key = {
    rd_key =       {0x0 <repeats 60 times>},
    rounds = 0x0
  },
  aes_decrypt_key = {
    rd_key =       {0x0 <repeats 60 times>},
    rounds = 0x0
  },
  snapshots_offset = 0x0,
  snapshots_size = 0x0,
  nb_snapshots = 0x0,
  snapshots = 0x0,
  flags = 0x0,
  qcow_version = 0x0,
  use_lazy_refcounts = 0x0,
  refcount_order = 0x0,
  discard_passthrough =     {0x0,
    0x0,
    0x0,
    0x0,
    0x0},
  overlap_check = 0x0,
  incompatible_features = 0x0,
  compatible_features = 0x0,
  autoclear_features = 0x0,
  unknown_header_fields_size = 0x0,
  unknown_header_fields = 0x0,
  unknown_header_ext = {
    lh_first = 0x0
  },
  discards = {
    tqh_first = 0x0,
    tqh_last = 0x0
  },
  cache_discards = 0x0
}
(gdb) p *bs
$4 = {
  total_sectors = 0x2800000,
  read_only = 0x0,
  open_flags = 0x2062,
  encrypted = 0x0,
  valid_key = 0x0,
  sg = 0x0,
  copy_on_read = 0x0,
  drv = 0x1078d440 <bdrv_qcow2>,
  opaque = 0x10019aaf300,
  dev = 0x10019a90b38,
  dev_ops = 0x105b49b0 <virtio_block_ops>,
  dev_opaque = 0x10019a90b38,
  aio_context = 0x10019a7f270,
  aio_notifiers = {
    lh_first = 0x10019cf74f0
  },
  filename =     "/var/lib/libvirt/images/chig1.qcow2",
  backing_file =     "",
  backing_format =     "",
  full_open_options = 0x10019c2a030,
  exact_filename =     "/var/lib/libvirt/images/chig1.qcow2",
  backing_hd = 0x0,
  file = 0x10019aae3b0,
  close_notifiers = {
    notifiers = {
      lh_first = 0x10019cf6460
    }
  },
  before_write_notifiers = {
    notifiers = {
      lh_first = 0x0
    }
  },
  serialising_in_flight = 0x0,
  throttle_state = {
    cfg = {
      buckets =         {{
          avg = 0,
          max = 0,
          level = 0
        },
        {
          avg = 0,
          max = 0,
          level = 0
        },
        {
          avg = 0,
          max = 0,
          level = 0
        },
        {
          avg = 0,
          max = 0,
          level = 0
        },
        {
          avg = 0,
          max = 0,
          level = 0
        },
        {
          avg = 0,
          max = 0,
          level = 0
        }},
      op_size = 0x0
    },
    previous_leak = 0x0,
    timers =       {0x0,
      0x0},
    clock_type = QEMU_CLOCK_REALTIME,
    read_timer_cb = 0x0,
    write_timer_cb = 0x0,
    timer_opaque = 0x0
  },
  throttled_reqs =     {{
      entries = {
        tqh_first = 0x0,
        tqh_last = 0x10019aac188
      }
    },
    {
      entries = {
        tqh_first = 0x0,
        tqh_last = 0x10019aac198
      }
    }},
  io_limits_enabled = 0x0,
  nr_bytes =     {0x0,
    0x0,
    0x0},
  nr_ops =     {0x0,
    0x0,
    0x0},
  total_time_ns =     {0x0,
    0x0,
    0x0},
  wr_highest_sector = 0x27fffff,
  bl = {
    max_discard = 0x0,
    discard_alignment = 0x0,
    max_write_zeroes = 0x0,
    write_zeroes_alignment = 0x80,
    opt_transfer_length = 0x0,
    opt_mem_alignment = 0x1000
  },
  growable = 0x0,
  zero_beyond_eof = 0x1,
  request_alignment = 0x200,
  guest_block_size = 0x200,
  enable_write_cache = 0x1,
  on_read_error = BLOCKDEV_ON_ERROR_REPORT,
  on_write_error = BLOCKDEV_ON_ERROR_ENOSPC,
  iostatus_enabled = 0x1,
  iostatus = BLOCK_DEVICE_IO_STATUS_OK,
  node_name =     "",
  node_list = {
    tqe_next = 0x0,
    tqe_prev = 0x0
  },
  device_name =     "drive-virtio-disk0",
  device_list = {
    tqe_next = 0x0,
    tqe_prev = 0x1078b328 <bdrv_states>
  },
  dirty_bitmaps = {
    lh_first = 0x0
  },
  refcnt = 0x2,
  tracked_requests = {
    lh_first = 0x0
  },
  op_blockers =     {{
      lh_first = 0x0
    } <repeats 14 times>},
  job = 0x0,
  options = 0x10019aa9db0,
  detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
  backing_blocker = 0x0
}





[root@chikvm ~]# cat chig1-aik.xml
<domain type='kvm'>
  <name>chig1</name>
  <uuid>bbf91237-3c78-489e-b426-ab593806c78b</uuid>
  <memory unit='KiB'>4194304</memory>
  <currentMemory unit='KiB'>4194304</currentMemory>
  <vcpu placement='static'>1</vcpu>
  <resource>
    <partition>/machine</partition>
  </resource>
  <os>
    <type arch='ppc64' machine='pseries'>hvm</type>
    <boot dev='hd'/>
    <boot dev='network'/>
    <bootmenu enable='yes'/>
  </os>
  <features>
    <acpi/>
    <apic/>
  </features>
  <cpu>
  </cpu>
  <clock offset='utc'/>
  <on_poweroff>destroy</on_poweroff>
  <on_reboot>restart</on_reboot>
  <on_crash>restart</on_crash>
  <devices>
    <emulator>/usr/bin/qemu-system-ppc64.aik</emulator>
    <disk type='file' device='disk'>
      <driver name='qemu' type='qcow2' cache='none'/>
      <source file='/var/lib/libvirt/images/chig1.qcow2'/>
      <target dev='vda' bus='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
function='0x0'/>
    </disk>
    <controller type='pci' index='0' model='pci-root'/>
    <controller type='usb' index='0'>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
function='0x0'/>
    </controller>
    <interface type='bridge'>
      <mac address='52:54:00:27:70:6f'/>
      <source bridge='brenP1p9s0f0'/>
      <driver name='qemu'/>
      <model type='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x02'
function='0x0'/>
    </interface>
    <serial type='pty'>
      <target port='0'/>
      <address type='spapr-vio' reg='0x30000000'/>
    </serial>
    <console type='pty'>
      <target type='serial' port='0'/>
      <address type='spapr-vio' reg='0x30000000'/>
    </console>
    <video>
      <model type='vga' vram='9216' heads='1'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x01'
function='0x0'/>
    </video>
    <memballoon model='virtio'>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x05'
function='0x0'/>
    </memballoon>
  </devices>
  <seclabel type='dynamic' model='selinux' relabel='yes'/>
</domain>






-- 
Alexey

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Alexey Kardashevskiy]

Hi!

I am having problems when migrate a guest via libvirt like this:

virsh migrate --live --persistent --undefinesource --copy-storage-all
--verbose --desturi qemu+ssh://legkvm/system --domain chig1

The XML used to create the guest is at the end of this mail.

I see NBD FLUSH command after the destination QEMU received EOF for
migration stream and this produces a crash in qcow2_co_flush_to_os() as
s->lock is false or s->l2_table_cache is NULL.


There are 2 scenarios I observe. First one is assert when
qemu_co_mutex_unlock(&s->lock) right before "return 0":

process_incoming_migration_co()
qcow2_invalidate_cache()
qcow2_close()
qcow2_cache_flush()
bdrv_flush()
bdrv_co_flush()
qemu_coroutine_yield()
nbd_trip() NBD_CMD_FLUSH
bdrv_co_flush()
qcow2_co_flush_to_os()

Second one is EOF is completely handled and next FLUSH crashes as
s->l2_table_cache == NULL.


Please help to understand what is going on here. Thanks!



Probably this is enough :) If it is not, I pushed debug branch to
git@github.com:aik/qemu.git
 * [new branch]      mig-dbg-legkvm -> mig-dbg


I added some traces and here is the log from the destination:

/home/alexey/p/qemu/nbd.c:nbd_trip():L1293: Request/Reply complete
+++Q+++ spapr_tce_pre_load 107
liobn 80000000 nb=262144 bus_off=0, shift=12, table=0x3fff915d0010
+++Q+++ (44846) qemu_loadvm_state 1000 EOF Received! section_type=0 (EOF=0)
+++Q+++ (44846) qemu_loadvm_state 1007
+++Q+++ (44846) qemu_loadvm_state 1016
+++Q+++ (44846) qcow2_close 1399 START
+++Q+++ (44846) qcow2_close 1404
_qcow2_cache_flush 0x10002b24030 0x100028e1390 - 0x100028df300 0x100028e1390
+++Q+++ (44846) bdrv_flush 5099
+++Q+++ (44846) bdrv_co_flush 4978
+++Q+++ (44846) bdrv_co_flush 4995
+++Q+++ (44846) bdrv_co_flush 4997
+++Q+++ (44846) bdrv_co_flush 5001
+++Q+++ (44846) bdrv_co_flush 5003
+++Q+++ (44846) bdrv_co_flush 5028
+++Q+++ (44846) bdrv_co_flush 4965
+++Q+++ (44846) bdrv_flush 5101
_qcow2_cache_flush 0x10002b24030 0x100028e13b0 - 0x100028df300 0x100028e1390
+++Q+++ (44846) bdrv_flush 5099
+++Q+++ (44846) bdrv_co_flush 4978
+++Q+++ (44846) bdrv_co_flush 4995
+++Q+++ (44846) bdrv_co_flush 4997
+++Q+++ (44846) bdrv_co_flush 5001
+++Q+++ (44846) bdrv_co_flush 5003
+++Q+++ (44846) bdrv_co_flush 5028
+++Q+++ (44846) bdrv_co_flush 4965
+++Q+++ (44846) bdrv_flush 5101
+++Q+++ (44846) qcow2_close 1409
+++Q+++ (44846) qcow2_close 1414
+++Q+++ (44846) qcow2_close 1422
+++Q+++ (44846) qcow2_close 1425 DONE!
+++Q+++ qcow2_invalidate_cache 1459
/home/alexey/p/qemu/nbd.c:nbd_trip():L1164: Reading request.
/home/alexey/p/qemu/nbd.c:nbd_receive_request():L785: Got request: { magic
= 0x25609513, .type = 65539, from = 0 , len = 0 }
/home/alexey/p/qemu/nbd.c:nbd_co_receive_request():L1130: Decoding type
/home/alexey/p/qemu/nbd.c:nbd_trip():L1257: Request type is FLUSH
+++Q+++ (44846) nbd_trip 1258 bs=0x100028db420 START
+++Q+++ (44846) qcow2_co_flush_to_os 2171
_qcow2_cache_flush 0x10002e5f030 (nil) - 0x100028df300 (nil)
2014-09-16 11:34:36.731+0000: shutting down

<here it crashed> as first (nil) is referenced by "c->size".




This is the sender:


+++Q+++ (67154) qemu_savevm_state_complete 747
+++Q+++ (67154) qemu_savevm_state_complete 749
+++Q+++ (67154) qemu_savevm_state_complete 751
+++Q+++ (67154) migration_thread 617
+++Q+++ (67154) migration_thread 628
+++Q+++ (67154) migration_thread 667
+++Q+++ (67154) migration_thread 684
+++Q+++ (67154) migration_thread 686
+++Q+++ (67154) migration_thread 688
+++Q+++ (67154) bdrv_flush 5099
+++Q+++ (67154) bdrv_co_flush 4978
+++Q+++ (67154) bdrv_co_flush 5028
+++Q+++ (67154) nbd_client_session_co_flush 305
/home/alexey/p/qemu/nbd.c:nbd_send_request():L739: Sending request to
client: { .from = 0, .le
n = 0, .handle = 1099744563680, .type=65539}
/home/alexey/p/qemu/nbd.c:nbd_receive_reply():L806: read failed



This is the XML:


[root@chikvm ~]# cat chig1-aik.xml
<domain type='kvm'>
  <name>chig1</name>
  <uuid>bbf91237-3c78-489e-b426-ab593806c78b</uuid>
  <memory unit='KiB'>4194304</memory>
  <currentMemory unit='KiB'>4194304</currentMemory>
  <vcpu placement='static'>1</vcpu>
  <resource>
    <partition>/machine</partition>
  </resource>
  <os>
    <type arch='ppc64' machine='pseries'>hvm</type>
    <boot dev='hd'/>
    <boot dev='network'/>
    <bootmenu enable='yes'/>
  </os>
  <features>
    <acpi/>
    <apic/>
  </features>
  <cpu>
  </cpu>
  <clock offset='utc'/>
  <on_poweroff>destroy</on_poweroff>
  <on_reboot>restart</on_reboot>
  <on_crash>restart</on_crash>
  <devices>
    <emulator>/usr/bin/qemu-system-ppc64.aik</emulator>
    <disk type='file' device='disk'>
      <driver name='qemu' type='qcow2' cache='none'/>
      <source file='/var/lib/libvirt/images/chig1.qcow2'/>
      <target dev='vda' bus='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
function='0x0'/>
    </disk>
    <controller type='pci' index='0' model='pci-root'/>
    <controller type='usb' index='0'>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
function='0x0'/>
    </controller>
    <interface type='bridge'>
      <mac address='52:54:00:27:70:6f'/>
      <source bridge='brenP1p9s0f0'/>
      <driver name='qemu'/>
      <model type='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x02'
function='0x0'/>
    </interface>
    <serial type='pty'>
      <target port='0'/>
      <address type='spapr-vio' reg='0x30000000'/>
    </serial>
    <console type='pty'>
      <target type='serial' port='0'/>
      <address type='spapr-vio' reg='0x30000000'/>
    </console>
    <video>
      <model type='vga' vram='9216' heads='1'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x01'
function='0x0'/>
    </video>
    <memballoon model='virtio'>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x05'
function='0x0'/>
    </memballoon>
  </devices>
  <seclabel type='dynamic' model='selinux' relabel='yes'/>
</domain>



-- 
Alexey

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Paolo Bonzini]

Il 16/09/2014 14:02, Alexey Kardashevskiy ha scritto:
> I am having problems when migrate a guest via libvirt like this:
> 
> virsh migrate --live --persistent --undefinesource --copy-storage-all
> --verbose --desturi qemu+ssh://legkvm/system --domain chig1
> 
> The XML used to create the guest is at the end of this mail.
> 
> I see NBD FLUSH command after the destination QEMU received EOF for
> migration stream and this produces a crash in qcow2_co_flush_to_os() as
> s->lock is false or s->l2_table_cache is NULL.
> 

Max, Kevin, could the fix be something like this?

diff --git a/block/qcow2.c b/block/qcow2.c
index 0daf25c..e7459ea 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1442,6 +1442,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
         memcpy(&aes_decrypt_key, &s->aes_decrypt_key, sizeof(aes_decrypt_key));
     }
 
+    qemu_co_mutex_lock(&s->lock);
     qcow2_close(bs);
 
     bdrv_invalidate_cache(bs->file, &local_err);
@@ -1455,6 +1456,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
 
     ret = qcow2_open(bs, options, flags, &local_err);
     QDECREF(options);
+    qemu_co_mutex_unlock(&s->lock);
     if (local_err) {
         error_setg(errp, "Could not reopen qcow2 layer: %s",
                    error_get_pretty(local_err));

On top of this, *_invalidate_cache needs to be marked as coroutine_fn.

Paolo

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Kevin Wolf]

Am 16.09.2014 um 14:10 hat Paolo Bonzini geschrieben:
> Il 16/09/2014 14:02, Alexey Kardashevskiy ha scritto:
> > I am having problems when migrate a guest via libvirt like this:
> > 
> > virsh migrate --live --persistent --undefinesource --copy-storage-all
> > --verbose --desturi qemu+ssh://legkvm/system --domain chig1
> > 
> > The XML used to create the guest is at the end of this mail.
> > 
> > I see NBD FLUSH command after the destination QEMU received EOF for
> > migration stream and this produces a crash in qcow2_co_flush_to_os() as
> > s->lock is false or s->l2_table_cache is NULL.
> > 
> 
> Max, Kevin, could the fix be something like this?
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 0daf25c..e7459ea 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1442,6 +1442,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
>          memcpy(&aes_decrypt_key, &s->aes_decrypt_key, sizeof(aes_decrypt_key));
>      }
>  
> +    qemu_co_mutex_lock(&s->lock);
>      qcow2_close(bs);
>  
>      bdrv_invalidate_cache(bs->file, &local_err);
> @@ -1455,6 +1456,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
>  
>      ret = qcow2_open(bs, options, flags, &local_err);
>      QDECREF(options);
> +    qemu_co_mutex_unlock(&s->lock);
>      if (local_err) {
>          error_setg(errp, "Could not reopen qcow2 layer: %s",
>                     error_get_pretty(local_err));
> 
> On top of this, *_invalidate_cache needs to be marked as coroutine_fn.

I think bdrv_invalidate_cache() really needs to call bdrv_drain_all()
before starting to reopen stuff. There could be requests in flight
without holding the lock and if you can indeed reopen their BDS under
their feet without breaking things (I doubt it), that would be pure
luck.

Kevin

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Paolo Bonzini]

Il 16/09/2014 14:34, Kevin Wolf ha scritto:
> I think bdrv_invalidate_cache() really needs to call bdrv_drain_all()
> before starting to reopen stuff. There could be requests in flight
> without holding the lock and if you can indeed reopen their BDS under
> their feet without breaking things (I doubt it), that would be pure
> luck.

But even that's not enough without a lock if .bdrv_invalidate_cache (the
callback) is called from a coroutine.  As soon as it yields, another
request can come in, for example from the NBD server.

Paolo

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Kevin Wolf]

Am 16.09.2014 um 14:35 hat Paolo Bonzini geschrieben:
> Il 16/09/2014 14:34, Kevin Wolf ha scritto:
> > I think bdrv_invalidate_cache() really needs to call bdrv_drain_all()
> > before starting to reopen stuff. There could be requests in flight
> > without holding the lock and if you can indeed reopen their BDS under
> > their feet without breaking things (I doubt it), that would be pure
> > luck.
> 
> But even that's not enough without a lock if .bdrv_invalidate_cache (the
> callback) is called from a coroutine.  As soon as it yields, another
> request can come in, for example from the NBD server.

Yes, that's true. We can't fix this problem in qcow2, though, because
it's a more general one.  I think we must make sure that
bdrv_invalidate_cache() doesn't yield.

Either by forbidding to run bdrv_invalidate_cache() in a coroutine and
moving the problem to the caller (where and why is it even called from a
coroutine?), or possibly by creating a new coroutine for the driver
callback and running that in a nested event loop that only handles
bdrv_invalidate_cache() callbacks, so that the NBD server doesn't get a
chance to process new requests in this thread.

Forbidding to run in a coroutine sounds easier, but I don't see yet
which caller would have to be fixed.

Kevin

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Paolo Bonzini]

Il 16/09/2014 14:52, Kevin Wolf ha scritto:
> Yes, that's true. We can't fix this problem in qcow2, though, because
> it's a more general one.  I think we must make sure that
> bdrv_invalidate_cache() doesn't yield.
> 
> Either by forbidding to run bdrv_invalidate_cache() in a coroutine and
> moving the problem to the caller (where and why is it even called from a
> coroutine?), or possibly by creating a new coroutine for the driver
> callback and running that in a nested event loop that only handles
> bdrv_invalidate_cache() callbacks, so that the NBD server doesn't get a
> chance to process new requests in this thread.

Incoming migration runs in a coroutine (the coroutine entry point is
process_incoming_migration_co).  But everything after qemu_fclose() can
probably be moved into a separate bottom half, so that it gets out of
coroutine context.

Paolo

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Alexey Kardashevskiy]

On 09/16/2014 10:10 PM, Paolo Bonzini wrote:
> Il 16/09/2014 14:02, Alexey Kardashevskiy ha scritto:
>> I am having problems when migrate a guest via libvirt like this:
>>
>> virsh migrate --live --persistent --undefinesource --copy-storage-all
>> --verbose --desturi qemu+ssh://legkvm/system --domain chig1
>>
>> The XML used to create the guest is at the end of this mail.
>>
>> I see NBD FLUSH command after the destination QEMU received EOF for
>> migration stream and this produces a crash in qcow2_co_flush_to_os() as
>> s->lock is false or s->l2_table_cache is NULL.
>>
> 
> Max, Kevin, could the fix be something like this?


btw this one did not help.


> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 0daf25c..e7459ea 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1442,6 +1442,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
>          memcpy(&aes_decrypt_key, &s->aes_decrypt_key, sizeof(aes_decrypt_key));
>      }
>  
> +    qemu_co_mutex_lock(&s->lock);
>      qcow2_close(bs);
>  
>      bdrv_invalidate_cache(bs->file, &local_err);
> @@ -1455,6 +1456,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
>  
>      ret = qcow2_open(bs, options, flags, &local_err);
>      QDECREF(options);
> +    qemu_co_mutex_unlock(&s->lock);
>      if (local_err) {
>          error_setg(errp, "Could not reopen qcow2 layer: %s",
>                     error_get_pretty(local_err));
> 
> On top of this, *_invalidate_cache needs to be marked as coroutine_fn.
> 
> Paolo
> 


-- 
Alexey

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Alexey Kardashevskiy]

On 09/16/2014 10:34 PM, Kevin Wolf wrote:
> Am 16.09.2014 um 14:10 hat Paolo Bonzini geschrieben:
>> Il 16/09/2014 14:02, Alexey Kardashevskiy ha scritto:
>>> I am having problems when migrate a guest via libvirt like this:
>>>
>>> virsh migrate --live --persistent --undefinesource --copy-storage-all
>>> --verbose --desturi qemu+ssh://legkvm/system --domain chig1
>>>
>>> The XML used to create the guest is at the end of this mail.
>>>
>>> I see NBD FLUSH command after the destination QEMU received EOF for
>>> migration stream and this produces a crash in qcow2_co_flush_to_os() as
>>> s->lock is false or s->l2_table_cache is NULL.
>>>
>>
>> Max, Kevin, could the fix be something like this?
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 0daf25c..e7459ea 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1442,6 +1442,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
>>          memcpy(&aes_decrypt_key, &s->aes_decrypt_key, sizeof(aes_decrypt_key));
>>      }
>>  
>> +    qemu_co_mutex_lock(&s->lock);
>>      qcow2_close(bs);
>>  
>>      bdrv_invalidate_cache(bs->file, &local_err);
>> @@ -1455,6 +1456,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
>>  
>>      ret = qcow2_open(bs, options, flags, &local_err);
>>      QDECREF(options);
>> +    qemu_co_mutex_unlock(&s->lock);
>>      if (local_err) {
>>          error_setg(errp, "Could not reopen qcow2 layer: %s",
>>                     error_get_pretty(local_err));
>>
>> On top of this, *_invalidate_cache needs to be marked as coroutine_fn.
> 
> I think bdrv_invalidate_cache() really needs to call bdrv_drain_all()
> before starting to reopen stuff. There could be requests in flight
> without holding the lock and if you can indeed reopen their BDS under
> their feet without breaking things (I doubt it), that would be pure
> luck.


I tried the patch below and it did not help. So I assume I did it wrong,
could you please explain more? Thanks!


diff --git a/block.c b/block.c
index 2df600e..ecc876d 100644
--- a/block.c
+++ b/block.c
@@ -5038,11 +5038,16 @@ void bdrv_invalidate_cache(BlockDriverState *bs,
Error **errp)
         return;
     }

+    bdrv_drain_all();
+
     if (bs->drv->bdrv_invalidate_cache) {
         bs->drv->bdrv_invalidate_cache(bs, &local_err);
     } else if (bs->file) {
         bdrv_invalidate_cache(bs->file, &local_err);
     }
+
+    bdrv_drain_all();
+
     if (local_err) {
         error_propagate(errp, local_err);
         return;



-- 
Alexey

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Stefan Hajnoczi]

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

On Tue, Sep 16, 2014 at 02:10:39PM +0200, Paolo Bonzini wrote:
> Il 16/09/2014 14:02, Alexey Kardashevskiy ha scritto:
> > I am having problems when migrate a guest via libvirt like this:
> > 
> > virsh migrate --live --persistent --undefinesource --copy-storage-all
> > --verbose --desturi qemu+ssh://legkvm/system --domain chig1
> > 
> > The XML used to create the guest is at the end of this mail.
> > 
> > I see NBD FLUSH command after the destination QEMU received EOF for
> > migration stream and this produces a crash in qcow2_co_flush_to_os() as
> > s->lock is false or s->l2_table_cache is NULL.
> > 
> 
> Max, Kevin, could the fix be something like this?
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 0daf25c..e7459ea 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1442,6 +1442,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
>          memcpy(&aes_decrypt_key, &s->aes_decrypt_key, sizeof(aes_decrypt_key));
>      }
>  
> +    qemu_co_mutex_lock(&s->lock);
>      qcow2_close(bs);
>  
>      bdrv_invalidate_cache(bs->file, &local_err);
> @@ -1455,6 +1456,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
>  
>      ret = qcow2_open(bs, options, flags, &local_err);
>      QDECREF(options);
> +    qemu_co_mutex_unlock(&s->lock);
>      if (local_err) {
>          error_setg(errp, "Could not reopen qcow2 layer: %s",
>                     error_get_pretty(local_err));
> 
> On top of this, *_invalidate_cache needs to be marked as coroutine_fn.

I think the fundamental problem here is that the mirror block job on the
source host does not synchronize with live migration.

Remember the mirror block job iterates on the dirty bitmap whenever it
feels like.

There is no guarantee that the mirror block job has quiesced before
migration handover takes place, right?

IMO that's the fundamental problem and trying to protect
qcow2_invalidate_cache() seems wrong.  We must make sure that the mirror
block job on the source has quiesced before we hand over, otherwise the
destination could see an outdated copy of the disk!

Please let me know if I missed something which makes the operation safe
on the source, but I didn't spot any guards.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Paolo Bonzini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 17/09/2014 11:06, Stefan Hajnoczi ha scritto:
> I think the fundamental problem here is that the mirror block job 
> on the source host does not synchronize with live migration.
> 
> Remember the mirror block job iterates on the dirty bitmap
> whenever it feels like.
> 
> There is no guarantee that the mirror block job has quiesced before
> migration handover takes place, right?

Libvirt does that.  Migration is started only once storage mirroring
is out of the bulk phase, and the handover looks like:

1) migration completes

2) because the source VM is stopped, the disk has quiesced on the source

3) libvirt sends block-job-complete

4) libvirt receives BLOCK_JOB_COMPLETED.  The disk has now quiesced on
the destination as well.

5) the VM is started on the destination

6) the NBD server is stopped on the destination and the source VM is quit.

It is actually a feature that storage migration is completed
asynchronously with respect to RAM migration.  The problem is that
qcow2_invalidate_cache happens between (3) and (5), and it doesn't
like the concurrent I/O received by the NBD server.

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJUGVOVAAoJEBvWZb6bTYby8gYP/irgMGZGWUrntNH6OrBYMcxn
isEqxq8waFDe5i08OycUKniVMRlWvvCPRAHmOccDzEJkB/hTBGh+M8RpBgFVfG1+
vzzdmid6wWpmSWdlOI/9niA9hNQy8idjn3nP8B0YmCjd1FOCTicDfiXVnTny6+HW
hhPoqfO84iIFHYOTtvZ4/MAWBwUSSDbSNWRkYFS/0eYeGNdqBclvglLjgrfgGIfX
n15QuqB5FpccB3Tq43UaCbR+hqytmoOd59zG30YoDNd2yEOzeuvI9fq90f+/GzXh
U/toL2RGM+CTNwoEeMmDwRBSmK8dNSEKnOxnXaxkzhNNcDU02qIwI29yYlzw2y+C
R3H1jMc4O/O53vWJqYVCR/5Wmhu8hi8MK+sDYnsKgq9QJTumy0z21qJX5KR/X0bJ
0gC0hOy+7bm5bkFJZ5NCNLnPnntfQhLWZJjuzxMDiI3I14gF4QysOfRWkWhsGIAp
3FCnG8ox0t5wbGxDCltyZMcIyNkVfcyxILr3HhXTt1vOdiesESI4BwM+4yUXOv5b
JBKHFB0Mdyksjq0ORA18OOiqEyMESEvGxcG6Lw92cLqh/TSbczBbW82DQVpuBqRV
gDeGHC5BiLaUG9TOdz5LiWJw9ZHxl6bcRaUpFbft29cZY8l8nUFNSsAQxy4XalNc
vPr8Qz7i9YA4hQY3Z2fe
=xLkq
-----END PGP SIGNATURE-----

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Alexey Kardashevskiy]

On 09/17/2014 07:25 PM, Paolo Bonzini wrote:
> Il 17/09/2014 11:06, Stefan Hajnoczi ha scritto:
>> I think the fundamental problem here is that the mirror block job 
>> on the source host does not synchronize with live migration.
> 
>> Remember the mirror block job iterates on the dirty bitmap
>> whenever it feels like.
> 
>> There is no guarantee that the mirror block job has quiesced before
>> migration handover takes place, right?
> 
> Libvirt does that.  Migration is started only once storage mirroring
> is out of the bulk phase, and the handover looks like:
> 
> 1) migration completes
> 
> 2) because the source VM is stopped, the disk has quiesced on the source
> 
> 3) libvirt sends block-job-complete
> 
> 4) libvirt receives BLOCK_JOB_COMPLETED.  The disk has now quiesced on
> the destination as well.
> 
> 5) the VM is started on the destination
> 
> 6) the NBD server is stopped on the destination and the source VM is quit.
> 
> It is actually a feature that storage migration is completed
> asynchronously with respect to RAM migration.  The problem is that
> qcow2_invalidate_cache happens between (3) and (5), and it doesn't
> like the concurrent I/O received by the NBD server.

How can it happen at all? I thought there are 2 channels/sockets - one for
live migration, one for NBD and they concur, nope?

btw any better idea of a hack to try? Testers are pushing me - they want to
upgrade the broken setup and I am blocking them :) Thanks!


-- 
Alexey

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Stefan Hajnoczi]

On Wed, Sep 17, 2014 at 10:25 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Il 17/09/2014 11:06, Stefan Hajnoczi ha scritto:
>> I think the fundamental problem here is that the mirror block job
>> on the source host does not synchronize with live migration.
>>
>> Remember the mirror block job iterates on the dirty bitmap
>> whenever it feels like.
>>
>> There is no guarantee that the mirror block job has quiesced before
>> migration handover takes place, right?
>
> Libvirt does that.  Migration is started only once storage mirroring
> is out of the bulk phase, and the handover looks like:
>
> 1) migration completes
>
> 2) because the source VM is stopped, the disk has quiesced on the source

But the mirror block job might still be writing out dirty blocks.

> 3) libvirt sends block-job-complete

No, it sends block-job-cancel after the source QEMU's migration has
completed.  See the qemuMigrationCancelDriveMirror() call in
src/qemu/qemu_migration.c:qemuMigrationRun().

> 4) libvirt receives BLOCK_JOB_COMPLETED.  The disk has now quiesced on
> the destination as well.

I don't see where this happens in the libvirt source code.  Libvirt
doesn't care about block job events for drive-mirror during migration.

And that's why there could still be I/O going on (since
block-job-cancel is asynchronous).

> 5) the VM is started on the destination
>
> 6) the NBD server is stopped on the destination and the source VM is quit.
>
> It is actually a feature that storage migration is completed
> asynchronously with respect to RAM migration.  The problem is that
> qcow2_invalidate_cache happens between (3) and (5), and it doesn't
> like the concurrent I/O received by the NBD server.

I agree that qcow2_invalidate_cache() (and any other invalidate cache
implementations) need to allow concurrent I/O requests.

Either I'm misreading the libvirt code or libvirt is not actually
ensuring that the block job on the source has cancelled/completed
before the guest is resumed on the destination.  So I think there is
still a bug, maybe Eric can verify this?

Stefan

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Stefan Hajnoczi]

On Wed, Sep 17, 2014 at 2:44 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> On 09/17/2014 07:25 PM, Paolo Bonzini wrote:
> btw any better idea of a hack to try? Testers are pushing me - they want to
> upgrade the broken setup and I am blocking them :) Thanks!

Paolo's qemu_co_mutex_lock(&s->lock) idea in qcow2_invalidate_cache()
is good.  Have you tried that patch?

I haven't checked the qcow2 code whether that works properly across
bdrv_close() (is the lock freed?) but in principle that's how you
protect against concurrent I/O.

Stefan

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Eric Blake]

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

[adding libvirt list]

On 09/17/2014 09:04 AM, Stefan Hajnoczi wrote:
> On Wed, Sep 17, 2014 at 10:25 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Il 17/09/2014 11:06, Stefan Hajnoczi ha scritto:
>>> I think the fundamental problem here is that the mirror block job
>>> on the source host does not synchronize with live migration.
>>>
>>> Remember the mirror block job iterates on the dirty bitmap
>>> whenever it feels like.
>>>
>>> There is no guarantee that the mirror block job has quiesced before
>>> migration handover takes place, right?
>>
>> Libvirt does that.  Migration is started only once storage mirroring
>> is out of the bulk phase, and the handover looks like:
>>
>> 1) migration completes
>>
>> 2) because the source VM is stopped, the disk has quiesced on the source
> 
> But the mirror block job might still be writing out dirty blocks.
> 
>> 3) libvirt sends block-job-complete
> 
> No, it sends block-job-cancel after the source QEMU's migration has
> completed.  See the qemuMigrationCancelDriveMirror() call in
> src/qemu/qemu_migration.c:qemuMigrationRun().
> 
>> 4) libvirt receives BLOCK_JOB_COMPLETED.  The disk has now quiesced on
>> the destination as well.
> 
> I don't see where this happens in the libvirt source code.  Libvirt
> doesn't care about block job events for drive-mirror during migration.
> 
> And that's why there could still be I/O going on (since
> block-job-cancel is asynchronous).
> 
>> 5) the VM is started on the destination
>>
>> 6) the NBD server is stopped on the destination and the source VM is quit.
>>
>> It is actually a feature that storage migration is completed
>> asynchronously with respect to RAM migration.  The problem is that
>> qcow2_invalidate_cache happens between (3) and (5), and it doesn't
>> like the concurrent I/O received by the NBD server.
> 
> I agree that qcow2_invalidate_cache() (and any other invalidate cache
> implementations) need to allow concurrent I/O requests.
> 
> Either I'm misreading the libvirt code or libvirt is not actually
> ensuring that the block job on the source has cancelled/completed
> before the guest is resumed on the destination.  So I think there is
> still a bug, maybe Eric can verify this?

You may indeed be correct that libvirt is not waiting long enough for
the block job to be gone on the source before resuming on the
destination.  I didn't write that particular code, so I'm cc'ing the
libvirt list, but I can try and take a look into it, since it's related
to code I've recently touched in getting libvirt to support active layer
block commit.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Paolo Bonzini]

Il 17/09/2014 17:04, Stefan Hajnoczi ha scritto:
> On Wed, Sep 17, 2014 at 10:25 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Il 17/09/2014 11:06, Stefan Hajnoczi ha scritto:
>>> I think the fundamental problem here is that the mirror block job
>>> on the source host does not synchronize with live migration.
>>>
>>> Remember the mirror block job iterates on the dirty bitmap
>>> whenever it feels like.
>>>
>>> There is no guarantee that the mirror block job has quiesced before
>>> migration handover takes place, right?
>>
>> Libvirt does that.  Migration is started only once storage mirroring
>> is out of the bulk phase, and the handover looks like:
>>
>> 1) migration completes
>>
>> 2) because the source VM is stopped, the disk has quiesced on the source
> 
> But the mirror block job might still be writing out dirty blocks.

Right, but it quiesces after (3).

>> 3) libvirt sends block-job-complete
> 
> No, it sends block-job-cancel after the source QEMU's migration has
> completed.  See the qemuMigrationCancelDriveMirror() call in
> src/qemu/qemu_migration.c:qemuMigrationRun().

No problem, block-job-cancel and block-job-complete are the same except
for pivoting to the destination.

>> 4) libvirt receives BLOCK_JOB_COMPLETED.  The disk has now quiesced on
>> the destination as well.
> 
> I don't see where this happens in the libvirt source code.  Libvirt
> doesn't care about block job events for drive-mirror during migration.
> 
> And that's why there could still be I/O going on (since
> block-job-cancel is asynchronous).

Oops, this would be a bug!  block-job-complete and block-job-cancel are
asynchronous.  CCing Michal Privoznik who wrote the libvirt code.

Paolo

>> 5) the VM is started on the destination
>>
>> 6) the NBD server is stopped on the destination and the source VM is quit.
>>
>> It is actually a feature that storage migration is completed
>> asynchronously with respect to RAM migration.  The problem is that
>> qcow2_invalidate_cache happens between (3) and (5), and it doesn't
>> like the concurrent I/O received by the NBD server.
> 
> I agree that qcow2_invalidate_cache() (and any other invalidate cache
> implementations) need to allow concurrent I/O requests.
> 
> Either I'm misreading the libvirt code or libvirt is not actually
> ensuring that the block job on the source has cancelled/completed
> before the guest is resumed on the destination.  So I think there is
> still a bug, maybe Eric can verify this?
> 
> Stefan
> 

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Alexey Kardashevskiy]

On 09/18/2014 01:07 AM, Stefan Hajnoczi wrote:
> On Wed, Sep 17, 2014 at 2:44 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>> On 09/17/2014 07:25 PM, Paolo Bonzini wrote:
>> btw any better idea of a hack to try? Testers are pushing me - they want to
>> upgrade the broken setup and I am blocking them :) Thanks!
> 
> Paolo's qemu_co_mutex_lock(&s->lock) idea in qcow2_invalidate_cache()
> is good.  Have you tried that patch?


Yes, did not help.

> 
> I haven't checked the qcow2 code whether that works properly across
> bdrv_close() (is the lock freed?) but in principle that's how you
> protect against concurrent I/O.

I thought we have to avoid qemu_coroutine_yield() in this particular case.
I fail to see how the locks may help if we still do yeild. But the whole
thing is already way behind of my understanding :) For example - how many
BlockDriverState things are layered here? NBD -> QCOW2 -> RAW?



-- 
Alexey

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Paolo Bonzini]

Il 18/09/2014 05:26, Alexey Kardashevskiy ha scritto:
> On 09/18/2014 01:07 AM, Stefan Hajnoczi wrote:
>> On Wed, Sep 17, 2014 at 2:44 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>> On 09/17/2014 07:25 PM, Paolo Bonzini wrote:
>>> btw any better idea of a hack to try? Testers are pushing me - they want to
>>> upgrade the broken setup and I am blocking them :) Thanks!
>>
>> Paolo's qemu_co_mutex_lock(&s->lock) idea in qcow2_invalidate_cache()
>> is good.  Have you tried that patch?
> 
> 
> Yes, did not help.
> 
>>
>> I haven't checked the qcow2 code whether that works properly across
>> bdrv_close() (is the lock freed?) but in principle that's how you
>> protect against concurrent I/O.
> 
> I thought we have to avoid qemu_coroutine_yield() in this particular case.
> I fail to see how the locks may help if we still do yeild. But the whole
> thing is already way behind of my understanding :) For example - how many
> BlockDriverState things are layered here? NBD -> QCOW2 -> RAW?

No, this is an NBD server.  So we have three users of the same QCOW2
image: migration, NBD server and virtio disk (not active while the bug
happens, and thus not depicted):


              NBD server   ->    QCOW2     <-     migration
                                   |
                                   v
                                 File

The problem is that the NBD server accesses the QCOW2 image while
migration does qcow2_invalidate_cache.

Paolo

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Alexey Kardashevskiy]

On 09/18/2014 07:56 PM, Paolo Bonzini wrote:
> Il 18/09/2014 05:26, Alexey Kardashevskiy ha scritto:
>> On 09/18/2014 01:07 AM, Stefan Hajnoczi wrote:
>>> On Wed, Sep 17, 2014 at 2:44 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>> On 09/17/2014 07:25 PM, Paolo Bonzini wrote:
>>>> btw any better idea of a hack to try? Testers are pushing me - they want to
>>>> upgrade the broken setup and I am blocking them :) Thanks!
>>>
>>> Paolo's qemu_co_mutex_lock(&s->lock) idea in qcow2_invalidate_cache()
>>> is good.  Have you tried that patch?
>>
>>
>> Yes, did not help.
>>
>>>
>>> I haven't checked the qcow2 code whether that works properly across
>>> bdrv_close() (is the lock freed?) but in principle that's how you
>>> protect against concurrent I/O.
>>
>> I thought we have to avoid qemu_coroutine_yield() in this particular case.
>> I fail to see how the locks may help if we still do yeild. But the whole
>> thing is already way behind of my understanding :) For example - how many
>> BlockDriverState things are layered here? NBD -> QCOW2 -> RAW?
> 
> No, this is an NBD server.  So we have three users of the same QCOW2
> image: migration, NBD server and virtio disk (not active while the bug
> happens, and thus not depicted):
> 
> 
>               NBD server   ->    QCOW2     <-     migration
>                                    |
>                                    v
>                                  File
> 
> The problem is that the NBD server accesses the QCOW2 image while
> migration does qcow2_invalidate_cache.


Ufff. Cool. Anyway, the qemu_co_mutex_lock(&s->lock) hack does not work as
after qcow2_close() the lock is cleared and qemu_co_mutex_unlock(&s->lock)
fails. Moving the lock to BlockDriverState caused weird side effects,
debugging...



-- 
Alexey

Re: [Qemu-devel] migration: qemu-coroutine-lock.c:141: qemu_co_mutex_unlock: Assertion `mutex->locked == 1' failed

[Kevin Wolf]

Am 16.09.2014 um 14:59 hat Paolo Bonzini geschrieben:
> Il 16/09/2014 14:52, Kevin Wolf ha scritto:
> > Yes, that's true. We can't fix this problem in qcow2, though, because
> > it's a more general one.  I think we must make sure that
> > bdrv_invalidate_cache() doesn't yield.
> > 
> > Either by forbidding to run bdrv_invalidate_cache() in a coroutine and
> > moving the problem to the caller (where and why is it even called from a
> > coroutine?), or possibly by creating a new coroutine for the driver
> > callback and running that in a nested event loop that only handles
> > bdrv_invalidate_cache() callbacks, so that the NBD server doesn't get a
> > chance to process new requests in this thread.
> 
> Incoming migration runs in a coroutine (the coroutine entry point is
> process_incoming_migration_co).  But everything after qemu_fclose() can
> probably be moved into a separate bottom half, so that it gets out of
> coroutine context.

Alexey, you should probably rather try this (and add a bdrv_drain_all()
in bdrv_invalidate_cache) than messing around with qcow2 locks. This
isn't a problem that can be completely fixed in qcow2.

Kevin

[Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation

[Alexey Kardashevskiy]

On 09/19/2014 06:47 PM, Kevin Wolf wrote:> Am 16.09.2014 um 14:59 hat Paolo Bonzini geschrieben:
>> Il 16/09/2014 14:52, Kevin Wolf ha scritto:
>>> Yes, that's true. We can't fix this problem in qcow2, though, because
>>> it's a more general one.  I think we must make sure that
>>> bdrv_invalidate_cache() doesn't yield.
>>>
>>> Either by forbidding to run bdrv_invalidate_cache() in a coroutine and
>>> moving the problem to the caller (where and why is it even called from a
>>> coroutine?), or possibly by creating a new coroutine for the driver
>>> callback and running that in a nested event loop that only handles
>>> bdrv_invalidate_cache() callbacks, so that the NBD server doesn't get a
>>> chance to process new requests in this thread.
>>
>> Incoming migration runs in a coroutine (the coroutine entry point is
>> process_incoming_migration_co).  But everything after qemu_fclose() can
>> probably be moved into a separate bottom half, so that it gets out of
>> coroutine context.
> 
> Alexey, you should probably rather try this (and add a bdrv_drain_all()
> in bdrv_invalidate_cache) than messing around with qcow2 locks. This
> isn't a problem that can be completely fixed in qcow2.


Ok. Tried :) Not very successful though. The patch is below.

Is that the correct bottom half? When I did it, I started getting crashes
in various sport on accesses to s->l1_cache which is NULL after qcow2_close.
Normally the code would check s->l1_size and then use but they are out of sync.

So I clear it in qcow2_close(). This allowed migrated guest to work and not
to crash until I shut it down when it aborted at "HERE IT FAILS ON SHUTDOWN".

Here I realized I am missing something in this picture again, what is it?
Thanks!


---
 block.c             |  2 ++
 block/qcow2-cache.c |  2 +-
 block/qcow2.c       | 50 ++++++++++++++++++++++++++++++++++++--------------
 block/qcow2.h       |  4 ++++
 4 files changed, 43 insertions(+), 15 deletions(-)

diff --git a/block.c b/block.c
index d06dd51..1e6dfd1 100644
--- a/block.c
+++ b/block.c
@@ -5044,6 +5044,8 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
         error_setg_errno(errp, -ret, "Could not refresh total sector count");
         return;
     }
+
+    bdrv_drain_all();
 }
 
 void bdrv_invalidate_cache_all(Error **errp)
diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index 904f6b1..59ff48c 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -259,7 +259,7 @@ static int qcow2_cache_find_entry_to_replace(Qcow2Cache *c)
     if (min_index == -1) {
         /* This can't happen in current synchronous code, but leave the check
          * here as a reminder for whoever starts using AIO with the cache */
-        abort();
+        abort(); // <==== HERE IT FAILS ON SHUTDOWN
     }
     return min_index;
 }
diff --git a/block/qcow2.c b/block/qcow2.c
index f9e045f..2b84562 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1399,6 +1399,7 @@ static void qcow2_close(BlockDriverState *bs)
     qemu_vfree(s->l1_table);
     /* else pre-write overlap checks in cache_destroy may crash */
     s->l1_table = NULL;
+    s->l1_size = 0;
 
     if (!(bs->open_flags & BDRV_O_INCOMING)) {
         qcow2_cache_flush(bs, s->l2_table_cache);
@@ -1419,16 +1420,11 @@ static void qcow2_close(BlockDriverState *bs)
     qcow2_free_snapshots(bs);
 }
 
+static void qcow2_invalidate_cache_bh_cb(void *opaque);
+
 static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
-    int flags = s->flags;
-    AES_KEY aes_encrypt_key;
-    AES_KEY aes_decrypt_key;
-    uint32_t crypt_method = 0;
-    QDict *options;
-    Error *local_err = NULL;
-    int ret;
 
     /*
      * Backing files are read-only which makes all of their metadata immutable,
@@ -1436,13 +1432,28 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
      */
 
     if (s->crypt_method) {
-        crypt_method = s->crypt_method;
-        memcpy(&aes_encrypt_key, &s->aes_encrypt_key, sizeof(aes_encrypt_key));
-        memcpy(&aes_decrypt_key, &s->aes_decrypt_key, sizeof(aes_decrypt_key));
+        s->bh_crypt_method = s->crypt_method;
+        memcpy(&s->bh_aes_encrypt_key, &s->aes_encrypt_key, sizeof(s->bh_aes_encrypt_key));
+        memcpy(&s->bh_aes_decrypt_key, &s->aes_decrypt_key, sizeof(s->bh_aes_decrypt_key));
+    } else {
+        s->bh_crypt_method = 0;
     }
 
     qcow2_close(bs);
 
+    s->cache_inv_bh = aio_bh_new(bdrv_get_aio_context(bs),
+                                 qcow2_invalidate_cache_bh_cb,
+                                 bs);
+}
+
+static void qcow2_invalidate_cache_bh(BlockDriverState *bs, Error **errp)
+{
+    BDRVQcowState *s = bs->opaque;
+    int flags = s->flags;
+    QDict *options;
+    Error *local_err = NULL;
+    int ret;
+
     bdrv_invalidate_cache(bs->file, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -1464,11 +1475,22 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
         return;
     }
 
-    if (crypt_method) {
-        s->crypt_method = crypt_method;
-        memcpy(&s->aes_encrypt_key, &aes_encrypt_key, sizeof(aes_encrypt_key));
-        memcpy(&s->aes_decrypt_key, &aes_decrypt_key, sizeof(aes_decrypt_key));
+    if (s->bh_crypt_method) {
+        s->crypt_method = s->bh_crypt_method;
+        memcpy(&s->aes_encrypt_key, &s->bh_aes_encrypt_key, sizeof(s->bh_aes_encrypt_key));
+        memcpy(&s->aes_decrypt_key, &s->bh_aes_decrypt_key, sizeof(s->bh_aes_decrypt_key));
     }
+
+    qemu_bh_delete(s->cache_inv_bh);
+    s->cache_inv_bh = NULL;
+}
+
+static void qcow2_invalidate_cache_bh_cb(void *opaque)
+{
+    BlockDriverState *bs = opaque;
+    Error *local_err = NULL;
+
+    qcow2_invalidate_cache_bh(bs, &local_err);
 }
 
 static size_t header_ext_add(char *buf, uint32_t magic, const void *s,
diff --git a/block/qcow2.h b/block/qcow2.h
index 6aeb7ea..58d1859 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -271,6 +271,10 @@ typedef struct BDRVQcowState {
     QLIST_HEAD(, Qcow2UnknownHeaderExtension) unknown_header_ext;
     QTAILQ_HEAD (, Qcow2DiscardRegion) discards;
     bool cache_discards;
+    QEMUBH *cache_inv_bh;
+    AES_KEY bh_aes_encrypt_key;
+    AES_KEY bh_aes_decrypt_key;
+    uint32_t bh_crypt_method;
 } BDRVQcowState;
 
 /* XXX: use std qcow open function ? */
-- 
2.0.0

Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation

[Alexey Kardashevskiy]

On 09/23/2014 06:47 PM, Alexey Kardashevskiy wrote:
> On 09/19/2014 06:47 PM, Kevin Wolf wrote:> Am 16.09.2014 um 14:59 hat Paolo Bonzini geschrieben:
>>> Il 16/09/2014 14:52, Kevin Wolf ha scritto:
>>>> Yes, that's true. We can't fix this problem in qcow2, though, because
>>>> it's a more general one.  I think we must make sure that
>>>> bdrv_invalidate_cache() doesn't yield.
>>>>
>>>> Either by forbidding to run bdrv_invalidate_cache() in a coroutine and
>>>> moving the problem to the caller (where and why is it even called from a
>>>> coroutine?), or possibly by creating a new coroutine for the driver
>>>> callback and running that in a nested event loop that only handles
>>>> bdrv_invalidate_cache() callbacks, so that the NBD server doesn't get a
>>>> chance to process new requests in this thread.
>>>
>>> Incoming migration runs in a coroutine (the coroutine entry point is
>>> process_incoming_migration_co).  But everything after qemu_fclose() can
>>> probably be moved into a separate bottom half, so that it gets out of
>>> coroutine context.
>>
>> Alexey, you should probably rather try this (and add a bdrv_drain_all()
>> in bdrv_invalidate_cache) than messing around with qcow2 locks. This
>> isn't a problem that can be completely fixed in qcow2.
> 
> 
> Ok. Tried :) Not very successful though. The patch is below.
> 
> Is that the correct bottom half? When I did it, I started getting crashes
> in various sport on accesses to s->l1_cache which is NULL after qcow2_close.
> Normally the code would check s->l1_size and then use but they are out of sync.
> 
> So I clear it in qcow2_close(). This allowed migrated guest to work and not
> to crash until I shut it down when it aborted at "HERE IT FAILS ON SHUTDOWN".
> 
> Here I realized I am missing something in this picture again, what is it?
> Thanks!

To be more precise, I can remove that abort() and it seems working for a
while but when shutting migrated guest down, the disk fails:

Will now unmount local filesystems:sd 0:0:0:0: [sda]
Result: hostbyte=0x00 driverbyte=0x08
sd 0:0:0:0: [sda]
Sense Key : 0xb [current]
sd 0:0:0:0: [sda]
ASC=0x0 ASCQ=0x6
sd 0:0:0:0: [sda] CDB:
cdb[0]=0x2a: 2a 00 00 3c 10 10 00 00 08 00
end_request: I/O error, dev sda, sector 3936272
end_request: I/O error, dev sda, sector 3936272
Buffer I/O error on device sda, logical block 492034
lost page write due to I/O error on sda
JBD2: Error -5 detected when updating journal superblock for sda-8.
[...]

spapr-vscsi or virtio-scsi - does not matter. Or crash:

Program received signal SIGSEGV, Segmentation fault.
0x000000001050a69c in qcow2_cache_find_entry_to_replace (c=0x10038317bb0)
at /home/alexey/p/qemu/block/qcow2-cache.c:256
(gdb) l
251                 min_count = c->entries[i].cache_hits;
(gdb) p i
$2 = 0xfd6
(gdb) p c->size
$3 = 0x3ffe
(gdb) p c->entries[i]
$5 = {
  table = 0x804dd70210,
  offset = 0x40,
  dirty = 0x0,
  cache_hits = 0xee498,
  ref = 0x0
}

Weird things are happening, that's my point :)


> 
> 
> ---
>  block.c             |  2 ++
>  block/qcow2-cache.c |  2 +-
>  block/qcow2.c       | 50 ++++++++++++++++++++++++++++++++++++--------------
>  block/qcow2.h       |  4 ++++
>  4 files changed, 43 insertions(+), 15 deletions(-)
> 
> diff --git a/block.c b/block.c
> index d06dd51..1e6dfd1 100644
> --- a/block.c
> +++ b/block.c
> @@ -5044,6 +5044,8 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
>          error_setg_errno(errp, -ret, "Could not refresh total sector count");
>          return;
>      }
> +
> +    bdrv_drain_all();
>  }
>  
>  void bdrv_invalidate_cache_all(Error **errp)
> diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
> index 904f6b1..59ff48c 100644
> --- a/block/qcow2-cache.c
> +++ b/block/qcow2-cache.c
> @@ -259,7 +259,7 @@ static int qcow2_cache_find_entry_to_replace(Qcow2Cache *c)
>      if (min_index == -1) {
>          /* This can't happen in current synchronous code, but leave the check
>           * here as a reminder for whoever starts using AIO with the cache */
> -        abort();
> +        abort(); // <==== HERE IT FAILS ON SHUTDOWN
>      }
>      return min_index;
>  }
> diff --git a/block/qcow2.c b/block/qcow2.c
> index f9e045f..2b84562 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1399,6 +1399,7 @@ static void qcow2_close(BlockDriverState *bs)
>      qemu_vfree(s->l1_table);
>      /* else pre-write overlap checks in cache_destroy may crash */
>      s->l1_table = NULL;
> +    s->l1_size = 0;
>  
>      if (!(bs->open_flags & BDRV_O_INCOMING)) {
>          qcow2_cache_flush(bs, s->l2_table_cache);
> @@ -1419,16 +1420,11 @@ static void qcow2_close(BlockDriverState *bs)
>      qcow2_free_snapshots(bs);
>  }
>  
> +static void qcow2_invalidate_cache_bh_cb(void *opaque);
> +
>  static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
>  {
>      BDRVQcowState *s = bs->opaque;
> -    int flags = s->flags;
> -    AES_KEY aes_encrypt_key;
> -    AES_KEY aes_decrypt_key;
> -    uint32_t crypt_method = 0;
> -    QDict *options;
> -    Error *local_err = NULL;
> -    int ret;
>  
>      /*
>       * Backing files are read-only which makes all of their metadata immutable,
> @@ -1436,13 +1432,28 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
>       */
>  
>      if (s->crypt_method) {
> -        crypt_method = s->crypt_method;
> -        memcpy(&aes_encrypt_key, &s->aes_encrypt_key, sizeof(aes_encrypt_key));
> -        memcpy(&aes_decrypt_key, &s->aes_decrypt_key, sizeof(aes_decrypt_key));
> +        s->bh_crypt_method = s->crypt_method;
> +        memcpy(&s->bh_aes_encrypt_key, &s->aes_encrypt_key, sizeof(s->bh_aes_encrypt_key));
> +        memcpy(&s->bh_aes_decrypt_key, &s->aes_decrypt_key, sizeof(s->bh_aes_decrypt_key));
> +    } else {
> +        s->bh_crypt_method = 0;
>      }
>  
>      qcow2_close(bs);
>  
> +    s->cache_inv_bh = aio_bh_new(bdrv_get_aio_context(bs),
> +                                 qcow2_invalidate_cache_bh_cb,
> +                                 bs);
> +}
> +
> +static void qcow2_invalidate_cache_bh(BlockDriverState *bs, Error **errp)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    int flags = s->flags;
> +    QDict *options;
> +    Error *local_err = NULL;
> +    int ret;
> +
>      bdrv_invalidate_cache(bs->file, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> @@ -1464,11 +1475,22 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
>          return;
>      }
>  
> -    if (crypt_method) {
> -        s->crypt_method = crypt_method;
> -        memcpy(&s->aes_encrypt_key, &aes_encrypt_key, sizeof(aes_encrypt_key));
> -        memcpy(&s->aes_decrypt_key, &aes_decrypt_key, sizeof(aes_decrypt_key));
> +    if (s->bh_crypt_method) {
> +        s->crypt_method = s->bh_crypt_method;
> +        memcpy(&s->aes_encrypt_key, &s->bh_aes_encrypt_key, sizeof(s->bh_aes_encrypt_key));
> +        memcpy(&s->aes_decrypt_key, &s->bh_aes_decrypt_key, sizeof(s->bh_aes_decrypt_key));
>      }
> +
> +    qemu_bh_delete(s->cache_inv_bh);
> +    s->cache_inv_bh = NULL;
> +}
> +
> +static void qcow2_invalidate_cache_bh_cb(void *opaque)
> +{
> +    BlockDriverState *bs = opaque;
> +    Error *local_err = NULL;
> +
> +    qcow2_invalidate_cache_bh(bs, &local_err);
>  }
>  
>  static size_t header_ext_add(char *buf, uint32_t magic, const void *s,
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 6aeb7ea..58d1859 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -271,6 +271,10 @@ typedef struct BDRVQcowState {
>      QLIST_HEAD(, Qcow2UnknownHeaderExtension) unknown_header_ext;
>      QTAILQ_HEAD (, Qcow2DiscardRegion) discards;
>      bool cache_discards;
> +    QEMUBH *cache_inv_bh;
> +    AES_KEY bh_aes_encrypt_key;
> +    AES_KEY bh_aes_decrypt_key;
> +    uint32_t bh_crypt_method;
>  } BDRVQcowState;
>  
>  /* XXX: use std qcow open function ? */
> 


-- 
Alexey

Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation

[Kevin Wolf]

Am 23.09.2014 um 10:47 hat Alexey Kardashevskiy geschrieben:
> On 09/19/2014 06:47 PM, Kevin Wolf wrote:> Am 16.09.2014 um 14:59 hat Paolo Bonzini geschrieben:
> >> Il 16/09/2014 14:52, Kevin Wolf ha scritto:
> >>> Yes, that's true. We can't fix this problem in qcow2, though, because
> >>> it's a more general one.  I think we must make sure that
> >>> bdrv_invalidate_cache() doesn't yield.
> >>>
> >>> Either by forbidding to run bdrv_invalidate_cache() in a coroutine and
> >>> moving the problem to the caller (where and why is it even called from a
> >>> coroutine?), or possibly by creating a new coroutine for the driver
> >>> callback and running that in a nested event loop that only handles
> >>> bdrv_invalidate_cache() callbacks, so that the NBD server doesn't get a
> >>> chance to process new requests in this thread.
> >>
> >> Incoming migration runs in a coroutine (the coroutine entry point is
> >> process_incoming_migration_co).  But everything after qemu_fclose() can
> >> probably be moved into a separate bottom half, so that it gets out of
> >> coroutine context.
> > 
> > Alexey, you should probably rather try this (and add a bdrv_drain_all()
> > in bdrv_invalidate_cache) than messing around with qcow2 locks. This
> > isn't a problem that can be completely fixed in qcow2.
> 
> 
> Ok. Tried :) Not very successful though. The patch is below.
> 
> Is that the correct bottom half? When I did it, I started getting crashes
> in various sport on accesses to s->l1_cache which is NULL after qcow2_close.
> Normally the code would check s->l1_size and then use but they are out of sync.

No, that's not the place we were talking about.

What Paolo meant is that in process_incoming_migration_co(), you can
split out the final part that calls bdrv_invalidate_cache_all() into a
BH (you need to move everything until the end of the function into the
BH then). His suggestion was to move everything below the qemu_fclose().

> So I clear it in qcow2_close(). This allowed migrated guest to work and not
> to crash until I shut it down when it aborted at "HERE IT FAILS ON SHUTDOWN".
> 
> Here I realized I am missing something in this picture again, what is it?

The problem with your patch seems to be that you close the image and
then let the VM access the image before it is reopened in the BH. That
can't work out well. This is why it's important that the vm_start() call
is in the BH, too.

>  void bdrv_invalidate_cache_all(Error **errp)
> diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
> index 904f6b1..59ff48c 100644
> --- a/block/qcow2-cache.c
> +++ b/block/qcow2-cache.c
> @@ -259,7 +259,7 @@ static int qcow2_cache_find_entry_to_replace(Qcow2Cache *c)
>      if (min_index == -1) {
>          /* This can't happen in current synchronous code, but leave the check
>           * here as a reminder for whoever starts using AIO with the cache */
> -        abort();
> +        abort(); // <==== HERE IT FAILS ON SHUTDOWN
>      }
>      return min_index;
>  }

It's a weird failure mode anyway...

Kevin

Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation

[Alexey Kardashevskiy]

On 09/24/2014 07:48 PM, Kevin Wolf wrote:
> Am 23.09.2014 um 10:47 hat Alexey Kardashevskiy geschrieben:
>> On 09/19/2014 06:47 PM, Kevin Wolf wrote:> Am 16.09.2014 um 14:59 hat Paolo Bonzini geschrieben:
>>>> Il 16/09/2014 14:52, Kevin Wolf ha scritto:
>>>>> Yes, that's true. We can't fix this problem in qcow2, though, because
>>>>> it's a more general one.  I think we must make sure that
>>>>> bdrv_invalidate_cache() doesn't yield.
>>>>>
>>>>> Either by forbidding to run bdrv_invalidate_cache() in a coroutine and
>>>>> moving the problem to the caller (where and why is it even called from a
>>>>> coroutine?), or possibly by creating a new coroutine for the driver
>>>>> callback and running that in a nested event loop that only handles
>>>>> bdrv_invalidate_cache() callbacks, so that the NBD server doesn't get a
>>>>> chance to process new requests in this thread.
>>>>
>>>> Incoming migration runs in a coroutine (the coroutine entry point is
>>>> process_incoming_migration_co).  But everything after qemu_fclose() can
>>>> probably be moved into a separate bottom half, so that it gets out of
>>>> coroutine context.
>>>
>>> Alexey, you should probably rather try this (and add a bdrv_drain_all()
>>> in bdrv_invalidate_cache) than messing around with qcow2 locks. This
>>> isn't a problem that can be completely fixed in qcow2.
>>
>>
>> Ok. Tried :) Not very successful though. The patch is below.
>>
>> Is that the correct bottom half? When I did it, I started getting crashes
>> in various sport on accesses to s->l1_cache which is NULL after qcow2_close.
>> Normally the code would check s->l1_size and then use but they are out of sync.
> 
> No, that's not the place we were talking about.
> 
> What Paolo meant is that in process_incoming_migration_co(), you can
> split out the final part that calls bdrv_invalidate_cache_all() into a
> BH (you need to move everything until the end of the function into the
> BH then). His suggestion was to move everything below the qemu_fclose().

Ufff. I took it very literally. Ok. Let it be
process_incoming_migration_co(). But there is something I am missing about
BHs. Here is a patch:


diff --git a/migration.c b/migration.c
index 6db04a6..101043e 100644
--- a/migration.c
+++ b/migration.c
@@ -88,6 +88,9 @@ void qemu_start_incoming_migration(const char *uri, Error
**errp)
     }
 }

+static QEMUBH *migration_complete_bh;
+static void process_incoming_migration_complete(void *opaque);
+
 static void process_incoming_migration_co(void *opaque)
 {
     QEMUFile *f = opaque;
@@ -117,6 +120,16 @@ static void process_incoming_migration_co(void *opaque)
     } else {
         runstate_set(RUN_STATE_PAUSED);
     }
+
+    migration_complete_bh = aio_bh_new(qemu_get_aio_context(),
+                                       process_incoming_migration_complete,
+                                       NULL);
+}
+
+static void process_incoming_migration_complete(void *opaque)
+{
+    qemu_bh_delete(migration_complete_bh);
+    migration_complete_bh = NULL;
 }

 void process_incoming_migration(QEMUFile *f)



Then I run it under gdb and set breakpoint in
process_incoming_migration_complete - and it never hits. Why is that? Thanks.




-- 
Alexey

Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation

[Kevin Wolf]

Am 25.09.2014 um 10:41 hat Alexey Kardashevskiy geschrieben:
> On 09/24/2014 07:48 PM, Kevin Wolf wrote:
> > Am 23.09.2014 um 10:47 hat Alexey Kardashevskiy geschrieben:
> >> On 09/19/2014 06:47 PM, Kevin Wolf wrote:> Am 16.09.2014 um 14:59 hat Paolo Bonzini geschrieben:
> >>>> Il 16/09/2014 14:52, Kevin Wolf ha scritto:
> >>>>> Yes, that's true. We can't fix this problem in qcow2, though, because
> >>>>> it's a more general one.  I think we must make sure that
> >>>>> bdrv_invalidate_cache() doesn't yield.
> >>>>>
> >>>>> Either by forbidding to run bdrv_invalidate_cache() in a coroutine and
> >>>>> moving the problem to the caller (where and why is it even called from a
> >>>>> coroutine?), or possibly by creating a new coroutine for the driver
> >>>>> callback and running that in a nested event loop that only handles
> >>>>> bdrv_invalidate_cache() callbacks, so that the NBD server doesn't get a
> >>>>> chance to process new requests in this thread.
> >>>>
> >>>> Incoming migration runs in a coroutine (the coroutine entry point is
> >>>> process_incoming_migration_co).  But everything after qemu_fclose() can
> >>>> probably be moved into a separate bottom half, so that it gets out of
> >>>> coroutine context.
> >>>
> >>> Alexey, you should probably rather try this (and add a bdrv_drain_all()
> >>> in bdrv_invalidate_cache) than messing around with qcow2 locks. This
> >>> isn't a problem that can be completely fixed in qcow2.
> >>
> >>
> >> Ok. Tried :) Not very successful though. The patch is below.
> >>
> >> Is that the correct bottom half? When I did it, I started getting crashes
> >> in various sport on accesses to s->l1_cache which is NULL after qcow2_close.
> >> Normally the code would check s->l1_size and then use but they are out of sync.
> > 
> > No, that's not the place we were talking about.
> > 
> > What Paolo meant is that in process_incoming_migration_co(), you can
> > split out the final part that calls bdrv_invalidate_cache_all() into a
> > BH (you need to move everything until the end of the function into the
> > BH then). His suggestion was to move everything below the qemu_fclose().
> 
> Ufff. I took it very literally. Ok. Let it be
> process_incoming_migration_co(). But there is something I am missing about
> BHs. Here is a patch:
> 
> 
> diff --git a/migration.c b/migration.c
> index 6db04a6..101043e 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -88,6 +88,9 @@ void qemu_start_incoming_migration(const char *uri, Error
> **errp)
>      }
>  }
> 
> +static QEMUBH *migration_complete_bh;
> +static void process_incoming_migration_complete(void *opaque);
> +
>  static void process_incoming_migration_co(void *opaque)
>  {
>      QEMUFile *f = opaque;
> @@ -117,6 +120,16 @@ static void process_incoming_migration_co(void *opaque)
>      } else {
>          runstate_set(RUN_STATE_PAUSED);
>      }
> +
> +    migration_complete_bh = aio_bh_new(qemu_get_aio_context(),
> +                                       process_incoming_migration_complete,
> +                                       NULL);
> +}
> +
> +static void process_incoming_migration_complete(void *opaque)
> +{
> +    qemu_bh_delete(migration_complete_bh);
> +    migration_complete_bh = NULL;
>  }
> 
>  void process_incoming_migration(QEMUFile *f)
> 
> 
> 
> Then I run it under gdb and set breakpoint in
> process_incoming_migration_complete - and it never hits. Why is that? Thanks.

You need to call qemu_bh_schedule().

Kevin

Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation

[Alexey Kardashevskiy]

On 09/25/2014 06:57 PM, Kevin Wolf wrote:
> Am 25.09.2014 um 10:41 hat Alexey Kardashevskiy geschrieben:
>> On 09/24/2014 07:48 PM, Kevin Wolf wrote:
>>> Am 23.09.2014 um 10:47 hat Alexey Kardashevskiy geschrieben:
>>>> On 09/19/2014 06:47 PM, Kevin Wolf wrote:> Am 16.09.2014 um 14:59 hat Paolo Bonzini geschrieben:
>>>>>> Il 16/09/2014 14:52, Kevin Wolf ha scritto:
>>>>>>> Yes, that's true. We can't fix this problem in qcow2, though, because
>>>>>>> it's a more general one.  I think we must make sure that
>>>>>>> bdrv_invalidate_cache() doesn't yield.
>>>>>>>
>>>>>>> Either by forbidding to run bdrv_invalidate_cache() in a coroutine and
>>>>>>> moving the problem to the caller (where and why is it even called from a
>>>>>>> coroutine?), or possibly by creating a new coroutine for the driver
>>>>>>> callback and running that in a nested event loop that only handles
>>>>>>> bdrv_invalidate_cache() callbacks, so that the NBD server doesn't get a
>>>>>>> chance to process new requests in this thread.
>>>>>>
>>>>>> Incoming migration runs in a coroutine (the coroutine entry point is
>>>>>> process_incoming_migration_co).  But everything after qemu_fclose() can
>>>>>> probably be moved into a separate bottom half, so that it gets out of
>>>>>> coroutine context.
>>>>>
>>>>> Alexey, you should probably rather try this (and add a bdrv_drain_all()
>>>>> in bdrv_invalidate_cache) than messing around with qcow2 locks. This
>>>>> isn't a problem that can be completely fixed in qcow2.
>>>>
>>>>
>>>> Ok. Tried :) Not very successful though. The patch is below.
>>>>
>>>> Is that the correct bottom half? When I did it, I started getting crashes
>>>> in various sport on accesses to s->l1_cache which is NULL after qcow2_close.
>>>> Normally the code would check s->l1_size and then use but they are out of sync.
>>>
>>> No, that's not the place we were talking about.
>>>
>>> What Paolo meant is that in process_incoming_migration_co(), you can
>>> split out the final part that calls bdrv_invalidate_cache_all() into a
>>> BH (you need to move everything until the end of the function into the
>>> BH then). His suggestion was to move everything below the qemu_fclose().
>>
>> Ufff. I took it very literally. Ok. Let it be
>> process_incoming_migration_co(). But there is something I am missing about
>> BHs. Here is a patch:
>>
> You need to call qemu_bh_schedule().

Right. Cool. So is below what was suggested? I am doublechecking as it does
not solve the original issue - the bottomhalf is called first and then
nbd_trip() crashes in qcow2_co_flush_to_os().



diff --git a/block.c b/block.c
index d06dd51..1e6dfd1 100644
--- a/block.c
+++ b/block.c
@@ -5037,20 +5037,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs,
Error **errp)
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }

     ret = refresh_total_sectors(bs, bs->total_sectors);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not refresh total sector count");
         return;
     }
+
+    bdrv_drain_all();
 }

 void bdrv_invalidate_cache_all(Error **errp)
 {
     BlockDriverState *bs;
     Error *local_err = NULL;

     QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
         AioContext *aio_context = bdrv_get_aio_context(bs);

diff --git a/migration.c b/migration.c
index 6db04a6..dc026a9 100644
--- a/migration.c
+++ b/migration.c
@@ -81,49 +81,64 @@ void qemu_start_incoming_migration(const char *uri,
Error **errp)
     else if (strstart(uri, "unix:", &p))
         unix_start_incoming_migration(p, errp);
     else if (strstart(uri, "fd:", &p))
         fd_start_incoming_migration(p, errp);
 #endif
     else {
         error_setg(errp, "unknown migration protocol: %s", uri);
     }
 }

+static QEMUBH *migration_complete_bh;
+static void process_incoming_migration_complete(void *opaque);
+
 static void process_incoming_migration_co(void *opaque)
 {
     QEMUFile *f = opaque;
-    Error *local_err = NULL;
     int ret;

     ret = qemu_loadvm_state(f);
     qemu_fclose(f);
     free_xbzrle_decoded_buf();
     if (ret < 0) {
         error_report("load of migration failed: %s", strerror(-ret));
         exit(EXIT_FAILURE);
     }
     qemu_announce_self();

     bdrv_clear_incoming_migration_all();
+
+    migration_complete_bh = aio_bh_new(qemu_get_aio_context(),
+                                       process_incoming_migration_complete,
+                                       NULL);
+    qemu_bh_schedule(migration_complete_bh);
+}
+
+static void process_incoming_migration_complete(void *opaque)
+{
+    Error *local_err = NULL;
+
     /* Make sure all file formats flush their mutable metadata */
     bdrv_invalidate_cache_all(&local_err);
     if (local_err) {
         qerror_report_err(local_err);
         error_free(local_err);
         exit(EXIT_FAILURE);
     }

     if (autostart) {
         vm_start();
     } else {
         runstate_set(RUN_STATE_PAUSED);
     }
+    qemu_bh_delete(migration_complete_bh);
+    migration_complete_bh = NULL;
 }




-- 
Alexey

Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation

[Kevin Wolf]

Am 25.09.2014 um 11:55 hat Alexey Kardashevskiy geschrieben:
> Right. Cool. So is below what was suggested? I am doublechecking as it does
> not solve the original issue - the bottomhalf is called first and then
> nbd_trip() crashes in qcow2_co_flush_to_os().
> 
> diff --git a/block.c b/block.c
> index d06dd51..1e6dfd1 100644
> --- a/block.c
> +++ b/block.c
> @@ -5037,20 +5037,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs,
> Error **errp)
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
> 
>      ret = refresh_total_sectors(bs, bs->total_sectors);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "Could not refresh total sector count");
>          return;
>      }
> +
> +    bdrv_drain_all();
>  }

Try moving the bdrv_drain_all() call to the top of the function (at
least it must be called before bs->drv->bdrv_invalidate_cache).

> +static QEMUBH *migration_complete_bh;
> +static void process_incoming_migration_complete(void *opaque);
> +
>  static void process_incoming_migration_co(void *opaque)
>  {
>      QEMUFile *f = opaque;
> -    Error *local_err = NULL;
>      int ret;
> 
>      ret = qemu_loadvm_state(f);
>      qemu_fclose(f);

Paolo suggested to move eveything starting from here, but as far as I
can tell, leaving the next few lines here shouldn't hurt.

>      free_xbzrle_decoded_buf();
>      if (ret < 0) {
>          error_report("load of migration failed: %s", strerror(-ret));
>          exit(EXIT_FAILURE);
>      }
>      qemu_announce_self();
> 
>      bdrv_clear_incoming_migration_all();
> +
> +    migration_complete_bh = aio_bh_new(qemu_get_aio_context(),
> +                                       process_incoming_migration_complete,
> +                                       NULL);
> +    qemu_bh_schedule(migration_complete_bh);
> +}
> +
> +static void process_incoming_migration_complete(void *opaque)
> +{
> +    Error *local_err = NULL;
> +
>      /* Make sure all file formats flush their mutable metadata */
>      bdrv_invalidate_cache_all(&local_err);
>      if (local_err) {
>          qerror_report_err(local_err);
>          error_free(local_err);
>          exit(EXIT_FAILURE);
>      }
> 
>      if (autostart) {
>          vm_start();
>      } else {
>          runstate_set(RUN_STATE_PAUSED);
>      }
> +    qemu_bh_delete(migration_complete_bh);
> +    migration_complete_bh = NULL;
>  }

That part looks good to me. I hope moving bdrv_drain_all() does the
trick, otherwise there's somthing wrong with our reasoning.

Kevin

Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation

[Alexey Kardashevskiy]

On 09/25/2014 08:20 PM, Kevin Wolf wrote:
> Am 25.09.2014 um 11:55 hat Alexey Kardashevskiy geschrieben:
>> Right. Cool. So is below what was suggested? I am doublechecking as it does
>> not solve the original issue - the bottomhalf is called first and then
>> nbd_trip() crashes in qcow2_co_flush_to_os().
>>
>> diff --git a/block.c b/block.c
>> index d06dd51..1e6dfd1 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -5037,20 +5037,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs,
>> Error **errp)
>>      if (local_err) {
>>          error_propagate(errp, local_err);
>>          return;
>>      }
>>
>>      ret = refresh_total_sectors(bs, bs->total_sectors);
>>      if (ret < 0) {
>>          error_setg_errno(errp, -ret, "Could not refresh total sector count");
>>          return;
>>      }
>> +
>> +    bdrv_drain_all();
>>  }
> 
> Try moving the bdrv_drain_all() call to the top of the function (at
> least it must be called before bs->drv->bdrv_invalidate_cache).


Ok, I did. Did not help.


> 
>> +static QEMUBH *migration_complete_bh;
>> +static void process_incoming_migration_complete(void *opaque);
>> +
>>  static void process_incoming_migration_co(void *opaque)
>>  {
>>      QEMUFile *f = opaque;
>> -    Error *local_err = NULL;
>>      int ret;
>>
>>      ret = qemu_loadvm_state(f);
>>      qemu_fclose(f);
> 
> Paolo suggested to move eveything starting from here, but as far as I
> can tell, leaving the next few lines here shouldn't hurt.


Ouch. I was looking at wrong qcow2_fclose() all this time :)
Aaaany what you suggested did not help -
bdrv_co_flush() calls qemu_coroutine_yield() while this BH is being
executed and the situation is still the same.


> 
>>      free_xbzrle_decoded_buf();
>>      if (ret < 0) {
>>          error_report("load of migration failed: %s", strerror(-ret));
>>          exit(EXIT_FAILURE);
>>      }
>>      qemu_announce_self();
>>
>>      bdrv_clear_incoming_migration_all();
>> +
>> +    migration_complete_bh = aio_bh_new(qemu_get_aio_context(),
>> +                                       process_incoming_migration_complete,
>> +                                       NULL);
>> +    qemu_bh_schedule(migration_complete_bh);
>> +}
>> +
>> +static void process_incoming_migration_complete(void *opaque)
>> +{
>> +    Error *local_err = NULL;
>> +
>>      /* Make sure all file formats flush their mutable metadata */
>>      bdrv_invalidate_cache_all(&local_err);
>>      if (local_err) {
>>          qerror_report_err(local_err);
>>          error_free(local_err);
>>          exit(EXIT_FAILURE);
>>      }
>>
>>      if (autostart) {
>>          vm_start();
>>      } else {
>>          runstate_set(RUN_STATE_PAUSED);
>>      }
>> +    qemu_bh_delete(migration_complete_bh);
>> +    migration_complete_bh = NULL;
>>  }
> 
> That part looks good to me. I hope moving bdrv_drain_all() does the
> trick, otherwise there's somthing wrong with our reasoning.
> 
> Kevin
> 


-- 
Alexey

Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation

[Kevin Wolf]

Am 25.09.2014 um 14:29 hat Alexey Kardashevskiy geschrieben:
> On 09/25/2014 08:20 PM, Kevin Wolf wrote:
> > Am 25.09.2014 um 11:55 hat Alexey Kardashevskiy geschrieben:
> >> Right. Cool. So is below what was suggested? I am doublechecking as it does
> >> not solve the original issue - the bottomhalf is called first and then
> >> nbd_trip() crashes in qcow2_co_flush_to_os().
> >>
> >> diff --git a/block.c b/block.c
> >> index d06dd51..1e6dfd1 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -5037,20 +5037,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs,
> >> Error **errp)
> >>      if (local_err) {
> >>          error_propagate(errp, local_err);
> >>          return;
> >>      }
> >>
> >>      ret = refresh_total_sectors(bs, bs->total_sectors);
> >>      if (ret < 0) {
> >>          error_setg_errno(errp, -ret, "Could not refresh total sector count");
> >>          return;
> >>      }
> >> +
> >> +    bdrv_drain_all();
> >>  }
> > 
> > Try moving the bdrv_drain_all() call to the top of the function (at
> > least it must be called before bs->drv->bdrv_invalidate_cache).
> 
> 
> Ok, I did. Did not help.
> 
> 
> > 
> >> +static QEMUBH *migration_complete_bh;
> >> +static void process_incoming_migration_complete(void *opaque);
> >> +
> >>  static void process_incoming_migration_co(void *opaque)
> >>  {
> >>      QEMUFile *f = opaque;
> >> -    Error *local_err = NULL;
> >>      int ret;
> >>
> >>      ret = qemu_loadvm_state(f);
> >>      qemu_fclose(f);
> > 
> > Paolo suggested to move eveything starting from here, but as far as I
> > can tell, leaving the next few lines here shouldn't hurt.
> 
> 
> Ouch. I was looking at wrong qcow2_fclose() all this time :)
> Aaaany what you suggested did not help -
> bdrv_co_flush() calls qemu_coroutine_yield() while this BH is being
> executed and the situation is still the same.

Hm, do you have a backtrace? The idea with the BH was that it would be
executed _outside_ coroutine context and therefore wouldn't be able to
yield. If it's still executed in coroutine context, it would be
interesting to see who that caller is.

Kevin

Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation

[Alexey Kardashevskiy]

On 09/25/2014 10:39 PM, Kevin Wolf wrote:
> Am 25.09.2014 um 14:29 hat Alexey Kardashevskiy geschrieben:
>> On 09/25/2014 08:20 PM, Kevin Wolf wrote:
>>> Am 25.09.2014 um 11:55 hat Alexey Kardashevskiy geschrieben:
>>>> Right. Cool. So is below what was suggested? I am doublechecking as it does
>>>> not solve the original issue - the bottomhalf is called first and then
>>>> nbd_trip() crashes in qcow2_co_flush_to_os().
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index d06dd51..1e6dfd1 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -5037,20 +5037,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs,
>>>> Error **errp)
>>>>      if (local_err) {
>>>>          error_propagate(errp, local_err);
>>>>          return;
>>>>      }
>>>>
>>>>      ret = refresh_total_sectors(bs, bs->total_sectors);
>>>>      if (ret < 0) {
>>>>          error_setg_errno(errp, -ret, "Could not refresh total sector count");
>>>>          return;
>>>>      }
>>>> +
>>>> +    bdrv_drain_all();
>>>>  }
>>>
>>> Try moving the bdrv_drain_all() call to the top of the function (at
>>> least it must be called before bs->drv->bdrv_invalidate_cache).
>>
>>
>> Ok, I did. Did not help.
>>
>>
>>>
>>>> +static QEMUBH *migration_complete_bh;
>>>> +static void process_incoming_migration_complete(void *opaque);
>>>> +
>>>>  static void process_incoming_migration_co(void *opaque)
>>>>  {
>>>>      QEMUFile *f = opaque;
>>>> -    Error *local_err = NULL;
>>>>      int ret;
>>>>
>>>>      ret = qemu_loadvm_state(f);
>>>>      qemu_fclose(f);
>>>
>>> Paolo suggested to move eveything starting from here, but as far as I
>>> can tell, leaving the next few lines here shouldn't hurt.
>>
>>
>> Ouch. I was looking at wrong qcow2_fclose() all this time :)
>> Aaaany what you suggested did not help -
>> bdrv_co_flush() calls qemu_coroutine_yield() while this BH is being
>> executed and the situation is still the same.
> 
> Hm, do you have a backtrace? The idea with the BH was that it would be
> executed _outside_ coroutine context and therefore wouldn't be able to
> yield. If it's still executed in coroutine context, it would be
> interesting to see who that caller is.

Like this?
process_incoming_migration_complete
bdrv_invalidate_cache_all
bdrv_drain_all
aio_dispatch
node->io_read (which is nbd_read)
nbd_trip
bdrv_co_flush
[...]



-- 
Alexey

Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation

[Alexey Kardashevskiy]

On 09/26/2014 12:05 AM, Alexey Kardashevskiy wrote:
> On 09/25/2014 10:39 PM, Kevin Wolf wrote:
>> Am 25.09.2014 um 14:29 hat Alexey Kardashevskiy geschrieben:
>>> On 09/25/2014 08:20 PM, Kevin Wolf wrote:
>>>> Am 25.09.2014 um 11:55 hat Alexey Kardashevskiy geschrieben:
>>>>> Right. Cool. So is below what was suggested? I am doublechecking as it does
>>>>> not solve the original issue - the bottomhalf is called first and then
>>>>> nbd_trip() crashes in qcow2_co_flush_to_os().
>>>>>
>>>>> diff --git a/block.c b/block.c
>>>>> index d06dd51..1e6dfd1 100644
>>>>> --- a/block.c
>>>>> +++ b/block.c
>>>>> @@ -5037,20 +5037,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs,
>>>>> Error **errp)
>>>>>      if (local_err) {
>>>>>          error_propagate(errp, local_err);
>>>>>          return;
>>>>>      }
>>>>>
>>>>>      ret = refresh_total_sectors(bs, bs->total_sectors);
>>>>>      if (ret < 0) {
>>>>>          error_setg_errno(errp, -ret, "Could not refresh total sector count");
>>>>>          return;
>>>>>      }
>>>>> +
>>>>> +    bdrv_drain_all();
>>>>>  }
>>>>
>>>> Try moving the bdrv_drain_all() call to the top of the function (at
>>>> least it must be called before bs->drv->bdrv_invalidate_cache).
>>>
>>>
>>> Ok, I did. Did not help.
>>>
>>>
>>>>
>>>>> +static QEMUBH *migration_complete_bh;
>>>>> +static void process_incoming_migration_complete(void *opaque);
>>>>> +
>>>>>  static void process_incoming_migration_co(void *opaque)
>>>>>  {
>>>>>      QEMUFile *f = opaque;
>>>>> -    Error *local_err = NULL;
>>>>>      int ret;
>>>>>
>>>>>      ret = qemu_loadvm_state(f);
>>>>>      qemu_fclose(f);
>>>>
>>>> Paolo suggested to move eveything starting from here, but as far as I
>>>> can tell, leaving the next few lines here shouldn't hurt.
>>>
>>>
>>> Ouch. I was looking at wrong qcow2_fclose() all this time :)
>>> Aaaany what you suggested did not help -
>>> bdrv_co_flush() calls qemu_coroutine_yield() while this BH is being
>>> executed and the situation is still the same.
>>
>> Hm, do you have a backtrace? The idea with the BH was that it would be
>> executed _outside_ coroutine context and therefore wouldn't be able to
>> yield. If it's still executed in coroutine context, it would be
>> interesting to see who that caller is.
> 
> Like this?
> process_incoming_migration_complete
> bdrv_invalidate_cache_all
> bdrv_drain_all
> aio_dispatch
> node->io_read (which is nbd_read)
> nbd_trip
> bdrv_co_flush
> [...]


Ping? I do not know how to understand this backtrace - in fact, in gdb at
the moment of crash I only see traces up to nbd_trip and
coroutine_trampoline (below). What is the context here then?...


Program received signal SIGSEGV, Segmentation fault.
0x000000001050a8d4 in qcow2_cache_flush (bs=0x100363531a0, c=0x0) at
/home/alexey/p/qemu/block/qcow2-cache.c:174
(gdb) bt
#0  0x000000001050a8d4 in qcow2_cache_flush (bs=0x100363531a0, c=0x0) at
/home/alexey/p/qemu/block/qcow2-cache.c:174
#1  0x00000000104fbc4c in qcow2_co_flush_to_os (bs=0x100363531a0) at
/home/alexey/p/qemu/block/qcow2.c:2162
#2  0x00000000104c7234 in bdrv_co_flush (bs=0x100363531a0) at
/home/alexey/p/qemu/block.c:4978
#3  0x00000000104b7e68 in nbd_trip (opaque=0x1003653e530) at
/home/alexey/p/qemu/nbd.c:1260
#4  0x00000000104d7d84 in coroutine_trampoline (i0=0x100, i1=0x36549850) at
/home/alexey/p/qemu/coroutine-ucontext.c:118
#5  0x000000804db01a9c in .__makecontext () from /lib64/libc.so.6
#6  0x0000000000000000 in ?? ()



-- 
Alexey