From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57744) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bD7DU-0007L6-Pv for qemu-devel@nongnu.org; Wed, 15 Jun 2016 05:34:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bD7DT-0007IN-Dm for qemu-devel@nongnu.org; Wed, 15 Jun 2016 05:34:32 -0400 References: <1465917916-22348-1-git-send-email-den@openvz.org> <20160615090647.GE4566@noname.redhat.com> From: "Denis V. Lunev" Message-ID: <57612115.4040202@openvz.org> Date: Wed, 15 Jun 2016 12:34:13 +0300 MIME-Version: 1.0 In-Reply-To: <20160615090647.GE4566@noname.redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 0/9] major rework of drive-mirror List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, vsementsov@virtuozzo.com, Stefan Hajnoczi , Fam Zheng , Max Reitz , Jeff Cody , Eric Blake , pbonzini@redhat.com On 06/15/2016 12:06 PM, Kevin Wolf wrote: > Am 14.06.2016 um 17:25 hat Denis V. Lunev geschrieben: >> Block commit of the active image to the backing store on a slow disk >> could never end. For example with the guest with the following loop >> inside >> while true; do >> dd bs=1k count=1 if=/dev/zero of=x >> done >> running above slow storage could not complete the operation with a >> resonable amount of time: >> virsh blockcommit rhel7 sda --active --shallow >> virsh qemu-monitor-event >> virsh qemu-monitor-command rhel7 \ >> '{"execute":"block-job-complete",\ >> "arguments":{"device":"drive-scsi0-0-0-0"} }' >> virsh qemu-monitor-event >> Completion event is never received. >> >> This problem could not be fixed easily with the current architecture. We >> should either prohibit guest writes (making dirty bitmap dirty) or switch >> to the sycnchronous scheme. >> >> This series switches driver mirror to synch scheme. Actually we can have >> something more intelligent and switch to sync mirroring just after >> the first pass over the bitmap. Though this could be done relatively >> easily during discussion. The most difficult things are here. >> >> The set also adds some performance improvements dealing with >> known-to-be-zero areas. > I only read the cover letter and had a quick look at the patch doing the > actual switch, so this is by far not a real review, but I have a few > general comments anway: > > > First of all, let's make sure we're all using the same terminology. In > past discussions about mirror modes, we distinguished active/passive and > synchronous/asynchronous. > > * An active mirror mirrors requests immediately when they are made by > the guest. A passive mirror just remembers that it needs to mirror > something and does it whenever it wants. > > * A synchronous mirror completes the guest request only after the data > has successfully been written to both the live imaeg and the target. > An asynchronous one can complete the guest request before the mirror > I/O has completed. > > In these terms, the currently implemented mirror is a passive > asynchronous one. If I understand correctly, what you are doing in this > series is to convert it unconditionally to an active asynchronous one. > > > The "unconditionally" part is my first complaint: The active mirror does > potentially a lot more I/O, so it's not clear that you want to use it. > This should be the user's choice. (We always intended to add an active > mirror sooner or later, but so far nobody needed it desperately enough.) this sounds reasonable > > The second big thing is that I don't want to see new users of the > notifiers in I/O functions. Let's try if we can't add a filter > BlockDriver instead. Then we'd add an option to set the filter node-name > in the mirror QMP command so that the management tool is aware of the > node and can refer to it. this will be much more difficult to implement at my experience. Can you share more details why filters are bad? > If we don't do this now, we'll have to introduce it later and can't be > sure that the management tool knows about it. This would complicate > things quite a bit because we would have to make sure that the added > node stays invisible to the management tool. > > > I think these two things are the big architectural questions. The rest > is hopefully more or less implementation details. I completely agree with you. We have the following choices: 1. implement parameter to use 'active'/'passive' mode from the very beginning 2. switch to 'active' mode upon receiving "block-job-complete" command unconditionally 3. switch to 'active' mode upon receiving "block-job-complete" command with proper parameter 4. switch to 'active' mode after timeout (I personally do not like this option) I think that choices 1 and 3 do not contradict each other and could be implemented to gather. Den