From mboxrd@z Thu Jan 1 00:00:00 1970 From: Juan Quintela Subject: Re: virtio_blk_load() question Date: Thu, 18 Mar 2010 13:07:41 +0100 Message-ID: References: <4BA1AC4A.2070402@lab.ntt.co.jp> <4BA1F578.4040500@lab.ntt.co.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "kvm\@vger.kernel.org" , "qemu-devel\@nongnu.org" , Avi Kivity , Yoshiaki Tamura To: OHMURA Kei Return-path: Received: from mx1.redhat.com ([209.132.183.28]:23500 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751332Ab0CRMHr (ORCPT ); Thu, 18 Mar 2010 08:07:47 -0400 In-Reply-To: <4BA1F578.4040500@lab.ntt.co.jp> (OHMURA Kei's message of "Thu, 18 Mar 2010 18:42:16 +0900") Sender: kvm-owner@vger.kernel.org List-ID: OHMURA Kei wrote: > Thanks for your reply. > >> When I ported virtio to vmstate, I was unable to get that list not empty >> for more than I tried. It should be not empty in the case of one error >> or similar, but I was not able to reproduce it. > > Actually, I wasn't able to get that condition either. > We're having problem in loading continuously sent VM image, and were > looking deeper into the device models. We were doubting the > virtio_blk_load() first, but seems to be different. > >> I agree this change is ok/needed. Notice that my series ( [PATCH 0/9] >> Virtio cleanups) that changes it to a QLIST and fixes it. > > I guess you're mentioning the following patch, and it's good to know > that. > > http://www.mail-archive.com/qemu-devel@nongnu.org/msg27324.html > > However, although QLIST_INSERT_HEAD is used, virtio_blk_save() is > adding requests to the tail of the list, and if we need to keep the > order of outstading requests, shouldn't we put incoming requests to > the tail in virtio_blk_load()? Really, ordering doesn't matter (in this case): see virtio-blk.c:virtio_blk_dma_restart_bh() QLIST_FOREACH_SAFE(req, &rq_copy, next, next_req) { QLIST_REMOVE(req, next); virtio_blk_handle_request(req, &mrb); } This mean that we are just removing from the beggining and addin from the beginnig (i.e. reversing). Adding by the beggining made it easier, but I can change if you mean. Notice that except if there are any errors (I was not able to trigger it, but didnt' try too hard), that list is going to be syncked in the qemu_aio_flush(); bdrv_flush_all(); in migrate_fd_put_ready(), so it is not trivial to hit it and probably the difference is just theoretical. Later, Juan.