From: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> To: Kevin Wolf <kwolf@redhat.com> Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org, aliguori@us.ibm.com, mtosatti@redhat.com, ananth@in.ibm.com, mst@redhat.com, dlaor@redhat.com, vatsa@linux.vnet.ibm.com, blauwirbel@gmail.com, ohmura.kei@lab.ntt.co.jp, avi@redhat.com, psuriset@linux.vnet.ibm.com, stefanha@linux.vnet.ibm.com Subject: Re: [Qemu-devel] [PATCH 14/19] block: insert event-tap to bdrv_aio_writev() and bdrv_aio_flush(). Date: Thu, 20 Jan 2011 14:01:05 +0900 [thread overview] Message-ID: <AANLkTinC0BJbhDrRvMdu_1d1-U8+iy1_zvRMgW+6B1BZ@mail.gmail.com> (raw) In-Reply-To: <4D36F046.60008@redhat.com> 2011/1/19 Kevin Wolf <kwolf@redhat.com>: > Am 19.01.2011 14:16, schrieb Yoshiaki Tamura: >> 2011/1/19 Kevin Wolf <kwolf@redhat.com>: >>> Am 19.01.2011 06:44, schrieb Yoshiaki Tamura: >>>> event-tap function is called only when it is on, and requests sent >>>> from device emulators. >>>> >>>> Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> >>>> --- >>>> block.c | 11 +++++++++++ >>>> 1 files changed, 11 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/block.c b/block.c >>>> index ff2795b..85bd8b8 100644 >>>> --- a/block.c >>>> +++ b/block.c >>>> @@ -28,6 +28,7 @@ >>>> #include "block_int.h" >>>> #include "module.h" >>>> #include "qemu-objects.h" >>>> +#include "event-tap.h" >>>> >>>> #ifdef CONFIG_BSD >>>> #include <sys/types.h> >>>> @@ -2111,6 +2112,11 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num, >>>> if (bdrv_check_request(bs, sector_num, nb_sectors)) >>>> return NULL; >>>> >>>> + if (bs->device_name && event_tap_is_on()) { >>>> + return event_tap_bdrv_aio_writev(bs, sector_num, qiov, nb_sectors, >>>> + cb, opaque); >>>> + } >>>> + >>>> if (bs->dirty_bitmap) { >>>> blk_cb_data = blk_dirty_cb_alloc(bs, sector_num, nb_sectors, cb, >>>> opaque); >>> >>> Just noticed the context here... Does this patch break block migration >>> when event-tap is on? >> >> I don't think so. event-tap will call bdrv_aio_writev() upon >> flushing requests and it shouldn't affect block-migration. The >> block written after the synchronization should be marked as dirty >> and should be sent in the next round. Am I missing the point? > > No, that makes sense. I don't have a complete understanding of the whole > series yet, so there may be well more misunderstandings on my side. It's OK. I'm glad that you're reviewing. >>> Another question that came to my mind is if we really hook everything we >>> need. I think we'll need to have a hook in bdrv_flush as well. I don't >>> know if you do hook qemu_aio_flush and friends - does a call cause >>> event-tap to flush its queue? If not, a call to qemu_aio_flush might >>> hang qemu because it's waiting for requests to complete which are >>> actually stuck in the event-tap queue. >> >> No it doesn't queue at event-tap. Marcelo pointed that we should >> hook bdrv_aio_flush to avoid requests inversion, that made sense >> to me. Do we need to hook bdrv_flush for that same reason? If > > bdrv_flush() is the synchronous version of bdrv_aio_flush(), so in > general it seems likely that we need to do the same. Hmm. Because it's synchronous, we need to start synchronization right away, and once done, flush requests queued in event-tap then return. >> we hook bdrv_flush and qemu_aio_flush, we're going loop forever >> because the synchronization code is calling vm_stop that call >> bdrv_flush_all and qemu_aio_flush. > > qemu_aio_flush doesn't invoke any bdrv_* functions, so I don't see why > we would loop forever. It just waits for AIO requests to complete. > > I just looked up the code and I think the situation is a bit different > than I thought originally: qemu_aio_flush waits only for completion of > requests which belong to a driver that has registered using > qemu_aio_set_fd_handler. So this means that AIO requests queued in > event-tap are not considered in-flight requests and we won't get stuck > in a loop. Maybe we have no problem in fact. :-) I had the same thoughts. We don't have to hook qemu_aio_flush. > On the other hand, e.g. migration relies on the fact that after a > qemu_aio_flush, all AIO requests that the device model has submitted are > completed. I think event-tap must take care that the requests which are > queued are not forgotten to be migrated. (Maybe the code already > considers this, I'm just writing down what comes to my mind...) That's where event-tap is calling qemu_aio_flush. It should be almost same as for live migration. Requests queued in event-tap are replayed on the secondary side, that is the core design of Kemari. Thanks, Yoshi > > Kevin > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
WARNING: multiple messages have this Message-ID (diff)
From: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> To: Kevin Wolf <kwolf@redhat.com> Cc: aliguori@us.ibm.com, dlaor@redhat.com, ananth@in.ibm.com, kvm@vger.kernel.org, mst@redhat.com, mtosatti@redhat.com, qemu-devel@nongnu.org, vatsa@linux.vnet.ibm.com, blauwirbel@gmail.com, ohmura.kei@lab.ntt.co.jp, avi@redhat.com, psuriset@linux.vnet.ibm.com, stefanha@linux.vnet.ibm.com Subject: Re: [Qemu-devel] [PATCH 14/19] block: insert event-tap to bdrv_aio_writev() and bdrv_aio_flush(). Date: Thu, 20 Jan 2011 14:01:05 +0900 [thread overview] Message-ID: <AANLkTinC0BJbhDrRvMdu_1d1-U8+iy1_zvRMgW+6B1BZ@mail.gmail.com> (raw) In-Reply-To: <4D36F046.60008@redhat.com> 2011/1/19 Kevin Wolf <kwolf@redhat.com>: > Am 19.01.2011 14:16, schrieb Yoshiaki Tamura: >> 2011/1/19 Kevin Wolf <kwolf@redhat.com>: >>> Am 19.01.2011 06:44, schrieb Yoshiaki Tamura: >>>> event-tap function is called only when it is on, and requests sent >>>> from device emulators. >>>> >>>> Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> >>>> --- >>>> block.c | 11 +++++++++++ >>>> 1 files changed, 11 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/block.c b/block.c >>>> index ff2795b..85bd8b8 100644 >>>> --- a/block.c >>>> +++ b/block.c >>>> @@ -28,6 +28,7 @@ >>>> #include "block_int.h" >>>> #include "module.h" >>>> #include "qemu-objects.h" >>>> +#include "event-tap.h" >>>> >>>> #ifdef CONFIG_BSD >>>> #include <sys/types.h> >>>> @@ -2111,6 +2112,11 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num, >>>> if (bdrv_check_request(bs, sector_num, nb_sectors)) >>>> return NULL; >>>> >>>> + if (bs->device_name && event_tap_is_on()) { >>>> + return event_tap_bdrv_aio_writev(bs, sector_num, qiov, nb_sectors, >>>> + cb, opaque); >>>> + } >>>> + >>>> if (bs->dirty_bitmap) { >>>> blk_cb_data = blk_dirty_cb_alloc(bs, sector_num, nb_sectors, cb, >>>> opaque); >>> >>> Just noticed the context here... Does this patch break block migration >>> when event-tap is on? >> >> I don't think so. event-tap will call bdrv_aio_writev() upon >> flushing requests and it shouldn't affect block-migration. The >> block written after the synchronization should be marked as dirty >> and should be sent in the next round. Am I missing the point? > > No, that makes sense. I don't have a complete understanding of the whole > series yet, so there may be well more misunderstandings on my side. It's OK. I'm glad that you're reviewing. >>> Another question that came to my mind is if we really hook everything we >>> need. I think we'll need to have a hook in bdrv_flush as well. I don't >>> know if you do hook qemu_aio_flush and friends - does a call cause >>> event-tap to flush its queue? If not, a call to qemu_aio_flush might >>> hang qemu because it's waiting for requests to complete which are >>> actually stuck in the event-tap queue. >> >> No it doesn't queue at event-tap. Marcelo pointed that we should >> hook bdrv_aio_flush to avoid requests inversion, that made sense >> to me. Do we need to hook bdrv_flush for that same reason? If > > bdrv_flush() is the synchronous version of bdrv_aio_flush(), so in > general it seems likely that we need to do the same. Hmm. Because it's synchronous, we need to start synchronization right away, and once done, flush requests queued in event-tap then return. >> we hook bdrv_flush and qemu_aio_flush, we're going loop forever >> because the synchronization code is calling vm_stop that call >> bdrv_flush_all and qemu_aio_flush. > > qemu_aio_flush doesn't invoke any bdrv_* functions, so I don't see why > we would loop forever. It just waits for AIO requests to complete. > > I just looked up the code and I think the situation is a bit different > than I thought originally: qemu_aio_flush waits only for completion of > requests which belong to a driver that has registered using > qemu_aio_set_fd_handler. So this means that AIO requests queued in > event-tap are not considered in-flight requests and we won't get stuck > in a loop. Maybe we have no problem in fact. :-) I had the same thoughts. We don't have to hook qemu_aio_flush. > On the other hand, e.g. migration relies on the fact that after a > qemu_aio_flush, all AIO requests that the device model has submitted are > completed. I think event-tap must take care that the requests which are > queued are not forgotten to be migrated. (Maybe the code already > considers this, I'm just writing down what comes to my mind...) That's where event-tap is calling qemu_aio_flush. It should be almost same as for live migration. Requests queued in event-tap are replayed on the secondary side, that is the core design of Kemari. Thanks, Yoshi > > Kevin > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
next prev parent reply other threads:[~2011-01-20 5:01 UTC|newest] Thread overview: 71+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-01-19 5:44 [PATCH 00/19] Kemari for KVM v0.2.6 Yoshiaki Tamura 2011-01-19 5:44 ` [Qemu-devel] " Yoshiaki Tamura 2011-01-19 5:44 ` [PATCH 01/19] Make QEMUFile buf expandable, and introduce qemu_realloc_buffer() and qemu_clear_buffer() Yoshiaki Tamura 2011-01-19 5:44 ` [Qemu-devel] " Yoshiaki Tamura 2011-01-19 5:44 ` [PATCH 02/19] Introduce read() to FdMigrationState Yoshiaki Tamura 2011-01-19 5:44 ` [Qemu-devel] " Yoshiaki Tamura 2011-01-19 5:44 ` [PATCH 03/19] Introduce skip_header parameter to qemu_loadvm_state() Yoshiaki Tamura 2011-01-19 5:44 ` [Qemu-devel] " Yoshiaki Tamura 2011-01-19 5:44 ` [PATCH 04/19] qemu-char: export socket_set_nodelay() Yoshiaki Tamura 2011-01-19 5:44 ` [Qemu-devel] " Yoshiaki Tamura 2011-01-19 5:44 ` [PATCH 05/19] vl.c: add deleted flag for deleting the handler Yoshiaki Tamura 2011-01-19 5:44 ` [Qemu-devel] " Yoshiaki Tamura 2011-01-19 5:44 ` [PATCH 06/19] virtio: decrement last_avail_idx with inuse before saving Yoshiaki Tamura 2011-01-19 5:44 ` [Qemu-devel] " Yoshiaki Tamura 2011-01-19 5:44 ` [PATCH 07/19] Introduce fault tolerant VM transaction QEMUFile and ft_mode Yoshiaki Tamura 2011-01-19 5:44 ` [Qemu-devel] " Yoshiaki Tamura 2011-01-19 5:44 ` [PATCH 08/19] savevm: introduce util functions to control ft_trans_file from savevm layer Yoshiaki Tamura 2011-01-19 5:44 ` [Qemu-devel] " Yoshiaki Tamura 2011-01-19 5:44 ` [PATCH 09/19] Introduce event-tap Yoshiaki Tamura 2011-01-19 5:44 ` [Qemu-devel] " Yoshiaki Tamura 2011-01-19 9:38 ` Kevin Wolf 2011-01-19 9:38 ` Kevin Wolf 2011-01-19 13:04 ` Yoshiaki Tamura 2011-01-19 13:04 ` Yoshiaki Tamura 2011-01-19 13:50 ` Kevin Wolf 2011-01-19 13:50 ` Kevin Wolf 2011-01-20 5:19 ` Yoshiaki Tamura 2011-01-20 9:15 ` Kevin Wolf 2011-01-20 10:39 ` Yoshiaki Tamura 2011-01-20 11:46 ` Kevin Wolf 2011-01-20 13:50 ` Yoshiaki Tamura 2011-01-20 14:21 ` Kevin Wolf 2011-01-20 15:48 ` Yoshiaki Tamura 2011-01-19 5:44 ` [PATCH 10/19] Call init handler of event-tap at main() in vl.c Yoshiaki Tamura 2011-01-19 5:44 ` [Qemu-devel] " Yoshiaki Tamura 2011-01-19 5:44 ` [PATCH 11/19] ioport: insert event_tap_ioport() to ioport_write() Yoshiaki Tamura 2011-01-19 5:44 ` [Qemu-devel] " Yoshiaki Tamura 2011-01-19 5:44 ` [PATCH 12/19] Insert event_tap_mmio() to cpu_physical_memory_rw() in exec.c Yoshiaki Tamura 2011-01-19 5:44 ` [Qemu-devel] " Yoshiaki Tamura 2011-01-19 5:44 ` [PATCH 13/19] net: insert event-tap to qemu_send_packet() and qemu_sendv_packet_async() Yoshiaki Tamura 2011-01-19 5:44 ` [Qemu-devel] " Yoshiaki Tamura 2011-01-19 5:44 ` [PATCH 14/19] block: insert event-tap to bdrv_aio_writev() and bdrv_aio_flush() Yoshiaki Tamura 2011-01-19 5:44 ` [Qemu-devel] " Yoshiaki Tamura 2011-01-19 9:05 ` Kevin Wolf 2011-01-19 9:05 ` Kevin Wolf 2011-01-19 12:06 ` Yoshiaki Tamura 2011-01-19 12:06 ` Yoshiaki Tamura 2011-01-19 9:47 ` Kevin Wolf 2011-01-19 9:47 ` Kevin Wolf 2011-01-19 13:16 ` Yoshiaki Tamura 2011-01-19 13:16 ` Yoshiaki Tamura 2011-01-19 14:08 ` Kevin Wolf 2011-01-19 14:08 ` Kevin Wolf 2011-01-20 5:01 ` Yoshiaki Tamura [this message] 2011-01-20 5:01 ` Yoshiaki Tamura 2011-01-19 5:45 ` [PATCH 15/19] savevm: introduce qemu_savevm_trans_{begin,commit} Yoshiaki Tamura 2011-01-19 5:45 ` [Qemu-devel] [PATCH 15/19] savevm: introduce qemu_savevm_trans_{begin, commit} Yoshiaki Tamura 2011-01-19 5:45 ` [PATCH 16/19] migration: introduce migrate_ft_trans_{put,get}_ready(), and modify migrate_fd_put_ready() when ft_mode is on Yoshiaki Tamura 2011-01-19 5:45 ` [Qemu-devel] [PATCH 16/19] migration: introduce migrate_ft_trans_{put, get}_ready(), " Yoshiaki Tamura 2011-01-19 5:45 ` [PATCH 17/19] migration-tcp: modify tcp_accept_incoming_migration() to handle ft_mode, and add a hack not to close fd when ft_mode is enabled Yoshiaki Tamura 2011-01-19 5:45 ` [Qemu-devel] " Yoshiaki Tamura 2011-01-19 5:45 ` [PATCH 18/19] Introduce -k option to enable FT migration mode (Kemari) Yoshiaki Tamura 2011-01-19 5:45 ` [Qemu-devel] " Yoshiaki Tamura 2011-01-19 5:45 ` [PATCH 19/19] migration: add a parser to accept FT migration incoming mode Yoshiaki Tamura 2011-01-19 5:45 ` [Qemu-devel] " Yoshiaki Tamura -- strict thread matches above, loose matches on Subject: below -- 2011-01-26 9:41 [PATCH 00/19] Kemari for KVM v0.2.7 Yoshiaki Tamura 2011-01-26 9:42 ` [Qemu-devel] [PATCH 14/19] block: insert event-tap to bdrv_aio_writev() and bdrv_aio_flush() Yoshiaki Tamura 2011-01-14 17:33 [PATCH 00/19] Kemari for KVM v0.2.5 Yoshiaki Tamura 2011-01-14 17:33 ` [Qemu-devel] [PATCH 14/19] block: insert event-tap to bdrv_aio_writev() and bdrv_aio_flush() Yoshiaki Tamura 2011-01-13 17:15 [PATCH 00/19] Kemari for KVM v0.2.4 Yoshiaki Tamura 2011-01-13 17:15 ` [Qemu-devel] [PATCH 14/19] block: insert event-tap to bdrv_aio_writev() and bdrv_aio_flush() Yoshiaki Tamura 2011-01-11 10:59 [PATCH 00/19] Kemari for KVM v0.2.3 Yoshiaki Tamura 2011-01-11 10:59 ` [Qemu-devel] [PATCH 14/19] block: insert event-tap to bdrv_aio_writev() and bdrv_aio_flush() Yoshiaki Tamura 2010-12-27 8:25 [PATCH 00/19] Kemari for KVM v0.2.2 Yoshiaki Tamura 2010-12-27 8:25 ` [Qemu-devel] [PATCH 14/19] block: insert event-tap to bdrv_aio_writev() and bdrv_aio_flush() Yoshiaki Tamura 2010-12-24 3:18 [PATCH 00/19] Kemari for KVM v0.2.1 Yoshiaki Tamura 2010-12-24 3:18 ` [Qemu-devel] [PATCH 14/19] block: insert event-tap to bdrv_aio_writev() and bdrv_aio_flush() Yoshiaki Tamura
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=AANLkTinC0BJbhDrRvMdu_1d1-U8+iy1_zvRMgW+6B1BZ@mail.gmail.com \ --to=tamura.yoshiaki@lab.ntt.co.jp \ --cc=aliguori@us.ibm.com \ --cc=ananth@in.ibm.com \ --cc=avi@redhat.com \ --cc=blauwirbel@gmail.com \ --cc=dlaor@redhat.com \ --cc=kvm@vger.kernel.org \ --cc=kwolf@redhat.com \ --cc=mst@redhat.com \ --cc=mtosatti@redhat.com \ --cc=ohmura.kei@lab.ntt.co.jp \ --cc=psuriset@linux.vnet.ibm.com \ --cc=qemu-devel@nongnu.org \ --cc=stefanha@linux.vnet.ibm.com \ --cc=vatsa@linux.vnet.ibm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.