From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39058) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1THDTg-0005bH-C1 for qemu-devel@nongnu.org; Thu, 27 Sep 2012 08:46:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1THDTZ-0005UK-Pb for qemu-devel@nongnu.org; Thu, 27 Sep 2012 08:46:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:28898) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1THDTZ-0005UG-H0 for qemu-devel@nongnu.org; Thu, 27 Sep 2012 08:45:57 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q8RCjuvx023531 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 27 Sep 2012 08:45:56 -0400 Message-ID: <50644A82.3010007@redhat.com> Date: Thu, 27 Sep 2012 14:45:54 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1348675011-8794-1-git-send-email-pbonzini@redhat.com> <1348675011-8794-7-git-send-email-pbonzini@redhat.com> <50644417.9080408@redhat.com> <50644641.8020203@redhat.com> In-Reply-To: <50644641.8020203@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 06/45] block: add support for job pause/resume List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: jcody@redhat.com, qemu-devel@nongnu.org Am 27.09.2012 14:27, schrieb Paolo Bonzini: > Il 27/09/2012 14:18, Kevin Wolf ha scritto: >>>> >>>> Signed-off-by: Paolo Bonzini >> I think there's a problem with terminology at least. What does "paused" >> really mean? Is it that the job has been requested to pause, or that it >> has actually yielded and is inactive? >> >> The commit message seems to use the latter semantics (which I would >> consider the intuitive one), > > You mean this: "Paused jobs cannot be canceled without first resuming > them". I can add a specification, like "(even if the job actually has > not reached the sleeping point and thus is still running)". I actually meant "pause happens at the next sleeping point", which isn't unspecific at all. >> the QMP documentation leaves it unclear, >> but the code actually implements the former semantics. > > This code comment is clear: > > /** > * Set to true if the job is either paused, or will pause itself > * as soon as possible (if busy == true). > */ > bool paused; Yes, this one is a good and clear comment (and possibly I wouldn't even have noticed without this comment) > but this one can indeed use some improvement. > > /** > * block_job_is_paused: > * @job: The job being queried. > * > * Returns whether the job is currently paused. > */ > bool block_job_is_paused(BlockJob *job); > > > From the QMP client's point of view it doesn't really matter, does it? > > - even after a job that writes to disk X has "really" paused, you cannot > read or write disk X. It's still owned by QEMU, it hasn't been flushed, > it may play games like lazy refcounts. I'm not sure about this one. Consider things like a built-in NBD server. Probably we'll find more cases in the future, where some monitor command might seem to be safe while a job is paused. It makes me nervous that clients could make assumptions based on the paused state despite having no way to make sure that a job is actually stopped - the documentation doesn't even tell them about the fact that "paused" doesn't really mean what they think it means. > - what matters is that a resume undoes a pause, even if it is still > pending (which it does). Agreed, this part looks okay. Kevin