From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:38190) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RLdro-0007MT-Ik for qemu-devel@nongnu.org; Wed, 02 Nov 2011 12:40:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RLdrn-0003En-5j for qemu-devel@nongnu.org; Wed, 02 Nov 2011 12:40:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:12046) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RLdrm-0003EV-L7 for qemu-devel@nongnu.org; Wed, 02 Nov 2011 12:40:43 -0400 Message-ID: <4EB17342.4050008@redhat.com> Date: Wed, 02 Nov 2011 17:43:46 +0100 From: Kevin Wolf MIME-Version: 1.0 References: <1319728975-6069-1-git-send-email-stefanha@linux.vnet.ibm.com> <1319728975-6069-4-git-send-email-stefanha@linux.vnet.ibm.com> <20111101180612.GA13205@amt.cnet> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/8] block: add image streaming block job List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Anthony Liguori , Marcelo Tosatti , Stefan Hajnoczi , qemu-devel@nongnu.org Am 02.11.2011 16:43, schrieb Stefan Hajnoczi: > On Tue, Nov 1, 2011 at 6:06 PM, Marcelo Tosatti wrote: >> On Thu, Oct 27, 2011 at 04:22:50PM +0100, Stefan Hajnoczi wrote: >>> +static int stream_one_iteration(StreamBlockJob *s, int64_t sector_num, >>> + void *buf, int max_sectors, int *n) >>> +{ >>> + BlockDriverState *bs = s->common.bs; >>> + int ret; >>> + >>> + trace_stream_one_iteration(s, sector_num, max_sectors); >>> + >>> + ret = bdrv_is_allocated(bs, sector_num, max_sectors, n); >>> + if (ret < 0) { >>> + return ret; >>> + } >> >> bdrv_is_allocated is still synchronous? If so, there should be at least >> a plan to make it asynchronous. > > Yes, that's a good discussion to have. My thoughts are that > bdrv_is_allocated() should be executed in coroutine context. bdrv_is_allocated() isn't coroutine-safe. You need to introduce bdrv_co_is_allocated and convert all drivers before you can do this. You don't want to access the qcow2 metadata cache without holding the lock, for example. > The > semantics are a little tricky because of parallel requests: > > 1. If a write request is in progress when we do bdrv_is_allocated() we > might get back "unallocated" even though clusters are just being > allocated. > 2. If a TRIM request is in progress when we do bdrv_is_allocated() we > might get back "allocated" even though clusters are just being > deallocated. > > In order to be reliable the caller needs to be aware of parallel > requests. I think it's correct to defer this problem to the caller. I agree. > In the case of image streaming we're not TRIM-safe, I haven't really > thought about it yet. But we are safe against parallel write requests > because there is serialization to prevent copy-on-read requests from > racing with write requests. I don't think it matters. If you lose a bdrv_discard, nothing bad has happened. bdrv_discard means that you have undefined content afterwards. Kevin