From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Ncy8r-0001Nz-Tr for qemu-devel@nongnu.org; Thu, 04 Feb 2010 04:36:53 -0500 Received: from [199.232.76.173] (port=47197 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Ncy8q-0001Nr-Ib for qemu-devel@nongnu.org; Thu, 04 Feb 2010 04:36:52 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1Ncy8p-0008Vo-6v for qemu-devel@nongnu.org; Thu, 04 Feb 2010 04:36:52 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43493) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Ncy8o-0008Vf-MI for qemu-devel@nongnu.org; Thu, 04 Feb 2010 04:36:50 -0500 Message-ID: <4B6A9523.7050806@redhat.com> Date: Thu, 04 Feb 2010 11:36:35 +0200 From: Naphtali Sprei MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 3/4] Block: readonly changes References: <1265221948-12613-1-git-send-email-nsprei@redhat.com> <1265221948-12613-2-git-send-email-nsprei@redhat.com> <1265221948-12613-3-git-send-email-nsprei@redhat.com> <1265221948-12613-4-git-send-email-nsprei@redhat.com> <20100203210653.GE19037@shareable.org> In-Reply-To: <20100203210653.GE19037@shareable.org> Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jamie Lokier Cc: kevin Wolf , qemu-devel@nongnu.org Jamie Lokier wrote: > Naphtali Sprei wrote: >> Open backing file for read-only >> During commit upgrade to read-write and back at end to read-only > >> + if (ro) { /* re-open as RO */ >> + bs_ro = bdrv_new(""); >> + ret = bdrv_open2(bs_ro, bs->backing_hd->filename, bs->backing_hd->open_flags & ~BDRV_O_RDWR, NULL); >> + if (ret < 0) { >> + bdrv_delete(bs_ro); >> + return -EACCES; >> + } >> + bdrv_close(bs->backing_hd); >> + qemu_free(bs->backing_hd); >> + bs->backing_hd = bs_ro; >> + bs->backing_hd->keep_read_only = 0; >> + } > > I think the general idea is perfect. > > A couple of concerns come to mind. > > 1. When changing read-write to read-only, if the backing file is a complex > format like qcow2 (or any others), is it possible for this bdrv_open2() > to read metadata such as format indexes, and even data, _before_ > all changes maintained by bs->backing_hd have been written to the file? > > (If the complex formats were like real filesystems and had a "mounted" > flags as real filesystems tend to, then it would be an issue, but I'm > not aware of any of them doing that.) > > Are there any such issues when switching from read-only to read-write > earlier? (It seems unlikely). > Good question. I looked at some of the formats (qcow, qcow2, vmdk) and didn't see anything problematic, since in the close function I didn't see any changes to the real file, only in-memory data and memory free. But an answer from an expert would help. > 2. Secondly, what if the re-open gets a different file (testable with > fstat()). I know, you get what you deserve if you rename files, but > still, do any of the formats which use backing files have a UUID check > or something to confirm they are using the right backing file, which > might be subverted by this? I didn't see any such checking/validation. It seems that handling such cases will complicate things more than you gain. > > 3. What about the bdrv file/device-locking which was actively looking > like it might get in a couple of months ago. Did it get in, and if > so does it conflict with this upgrade pattern? AFAIK, the locks thread terminated, don't think anything committed. But surely, there's a tight relationship between read-only/locks and sharing. > > Thanks! > -- Jamie Thanks, Naphtali