From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49122) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YROf4-00015i-Qf for qemu-devel@nongnu.org; Fri, 27 Feb 2015 12:25:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YROf3-0005B1-I2 for qemu-devel@nongnu.org; Fri, 27 Feb 2015 12:25:14 -0500 Message-ID: <54F0A86E.7000608@weilnetz.de> Date: Fri, 27 Feb 2015 18:25:02 +0100 From: Stefan Weil MIME-Version: 1.0 References: <1425045947-9271-1-git-send-email-mreitz@redhat.com> <20150227165723.GB32542@stefanha-thinkpad.redhat.com> In-Reply-To: <20150227165723.GB32542@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] block/vdi: Add locking for parallel requests List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi , Max Reitz Cc: Kevin Wolf , Paolo Bonzini , qemu-devel@nongnu.org, qemu-block@nongnu.org, qemu-stable@nongnu.org Am 27.02.2015 um 17:57 schrieb Stefan Hajnoczi: > On Fri, Feb 27, 2015 at 09:05:47AM -0500, Max Reitz wrote: >> Concurrently modifying the bmap does not seem to be a good idea; this patch adds >> a lock for it. See https://bugs.launchpad.net/qemu/+bug/1422307 for what >> can go wrong without. >> >> Cc: qemu-stable >> Signed-off-by: Max Reitz >> --- >> v2: >> - Make the mutex cover vdi_co_write() completely [Kevin] >> - Add a TODO comment [Kevin] [...] >> If we don't know why bmap_lock works, it would be more approprate to >> take the same approach as VMDK and VHDX where there is a simply s->lock >> that protects all reads and writes. That way we know for sure there is >> no parallel I/O going on. >> >> (Since the problem is not understood, maybe reads in parallel with >> writes could also cause problems. Better to really do a coarse lock >> instead of just bmap_lock in write.) >> >> Stefan block/vdi.c was never written for multi-threaded access, see my comment in the header of block/vdi.c: * The code is not thread safe (missing locks for changes in header and * block table, no problem with current QEMU). This was true in the past, but obviously later multi-threaded access was introduced for QEMU. Locking was added for qcow2 and other drivers in 2012 and 2013, but never for vdi. I must admit that I don't know which parts of the block filesystem drivers potentially run in parallel threads. Ideally there would be one or more test cases which test multi-threaded operations and which trigger a failure with the current vdi code. If I had a simple test scenario, I could have a look on the problem. The VMDK approach is fine as an intermediate work around, but please use conditional compilation to allow easy tests without coarse locks (and update the comments :-)). Regards Stefan (Weil)