From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50573) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YROiq-0003Ds-Qk for qemu-devel@nongnu.org; Fri, 27 Feb 2015 12:29:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YROip-0006ap-OX for qemu-devel@nongnu.org; Fri, 27 Feb 2015 12:29:08 -0500 Message-ID: <54F0A957.5090906@redhat.com> Date: Fri, 27 Feb 2015 12:28:55 -0500 From: Max Reitz MIME-Version: 1.0 References: <1425045947-9271-1-git-send-email-mreitz@redhat.com> <20150227165723.GB32542@stefanha-thinkpad.redhat.com> <54F0A86E.7000608@weilnetz.de> In-Reply-To: <54F0A86E.7000608@weilnetz.de> Content-Type: multipart/mixed; boundary="------------080501070900000601060808" 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 Weil , Stefan Hajnoczi Cc: Kevin Wolf , Paolo Bonzini , qemu-devel@nongnu.org, qemu-block@nongnu.org, qemu-stable@nongnu.org This is a multi-part message in MIME format. --------------080501070900000601060808 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit On 2015-02-27 at 12:25, Stefan Weil wrote: > 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. I have one for you. See the attached ruby script. (If there are no "Pattern verification failed" messages, everything is good) > 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 :-)). Will a macro defined in vdi.c be enough? Max --------------080501070900000601060808 Content-Type: application/x-ruby; name="test.rb" Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="test.rb" IyEvdXNyL2Jpbi9lbnYgcnVieQoKZm10ID0gJ3ZkaScKc2l6ZSA9IDUxMgoKc3lzdGVtKCIu L3FlbXUtaW1nIGNyZWF0ZSAtZiAje2ZtdH0gdGVzdC4je2ZtdH0gI3tzaXplfU0iKQpzeXN0 ZW0oIi4vcWVtdS1pbWcgY3JlYXRlIC1mIHJhdyB0ZXN0LnJhdyAje3NpemV9TSIpCgpjbWQg PSAnLi9xZW11LWlvIC1mIHJhdycKCjAudXB0byhzaXplIC0gMSkuZWFjaCBkbyB8aXwKICAg IGNtZCArPSAiIC1jICdhaW9fd3JpdGUgLVAgI3tpICUgMjU2fSAje2l9TSAxTSciCmVuZAoK Y21kICs9ICIgYmxrdmVyaWZ5OnRlc3QucmF3OnRlc3QuI3tmbXR9IgoKc3lzdGVtKGNtZCkK CgpwdXRzCnB1dHMgJy0tLSByZWFkIHJhdyAtLS0nCnB1dHMKCgpjbWQgPSAnLi9xZW11LWlv IC1mIHJhdycKCjAudXB0byg1MTEpLmVhY2ggZG8gfGl8CiAgICBjbWQgKz0gIiAtYyAncmVh ZCAtUCAje2kgJSAyNTZ9ICN7aX1NIDFNJyIKZW5kCgpjbWQgKz0gJyB0ZXN0LnJhdycKCnN5 c3RlbShjbWQpCgoKcHV0cwpwdXRzICItLS0gcmVhZCAje2ZtdH0gLS0tIgpwdXRzCgoKY21k ID0gIi4vcWVtdS1pbyAtZiAje2ZtdH0iCgowLnVwdG8oNTExKS5lYWNoIGRvIHxpfAogICAg Y21kICs9ICIgLWMgJ3JlYWQgLVAgI3tpICUgMjU2fSAje2l9TSAxTSciCmVuZAoKY21kICs9 ICIgdGVzdC4je2ZtdH0iCgpzeXN0ZW0oY21kKQo= --------------080501070900000601060808--