From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41132) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YsVf7-0003ok-OD for qemu-devel@nongnu.org; Wed, 13 May 2015 08:21:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YsVf6-0001OV-QB for qemu-devel@nongnu.org; Wed, 13 May 2015 08:21:21 -0400 Sender: Paolo Bonzini Message-ID: <555341B8.3070604@redhat.com> Date: Wed, 13 May 2015 14:21:12 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1431538099-3286-1-git-send-email-famz@redhat.com> <1431538099-3286-11-git-send-email-famz@redhat.com> In-Reply-To: <1431538099-3286-11-git-send-email-famz@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 10/11] blockdev: Block device IO during blockdev-backup transaction List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Stefan Hajnoczi , armbru@redhat.com, qemu-block@nongnu.org, mreitz@redhat.com On 13/05/2015 19:28, Fam Zheng wrote: > + state->bs = bs; > + error_setg(&state->blocker, "blockdev-backup in progress"); > + bdrv_op_block(bs, BLOCK_OP_TYPE_DEVICE_IO, state->blocker); > + > qmp_blockdev_backup(backup->device, backup->target, > backup->sync, > backup->has_speed, backup->speed, > @@ -1696,7 +1701,6 @@ static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp) > return; > } > > - state->bs = bs; I don't understand this. Jobs could pause/resume themselves by adding a DEVICE_IO notifier on the targets. However, block backups is the one job that cannot do this, because I/O on the source triggers I/O on the target. So if we consider this idea worthwhile, and decide that pausing device I/O on the target should pause the block job, the backup job actually has to prevent *adding a DEVICE_IO blocker* on the target. This "meta-block" is not possible in your design, which is a pity because on the surface it looked nicer than mine. FWIW, my original idea was: - bdrv_pause checks if there is an operation blocker for PAUSE. if it is there, it fails - otherwise, bdrv_pause invokes a notifier list if this is the outermost call. if not the outermost call, it does nothing - bdrv_resume does the same, but does not need a blocker - drive-backup should block PAUSE on its target Also, should the blockers (either DEVICE_IO in your design, or PAUSE in mine) be included in bdrv_op_block_all. I would say no in your case; here is the proof: - block-backup doesn't like DEVICE_IO blockers on the target - block-backup calls bdrv_op_block_all on the target - hence, bdrv_op_block_all shouldn't block DEVICE_IO block_job_create is another suspicious caller of bdrv_op_block_all. It probably shouldn't block neither PAUSE nor DEVICE_IO. Paolo