Re: [Qemu-devel] [PATCH for-2.7 00/15] block: Lock images when opening

From: Fam Zheng <famz@redhat.com>
To: "Daniel P. Berrange" <berrange@redhat.com>
Cc: qemu-devel@nongnu.org, Kevin Wolf <kwolf@redhat.com>,
	Max Reitz <mreitz@redhat.com>, Jeff Cody <jcody@redhat.com>,
	Markus Armbruster <armbru@redhat.com>,
	Eric Blake <eblake@redhat.com>, John Snow <jsnow@redhat.com>,
	qemu-block@nongnu.org, pbonzini@redhat.com, den@openvz.org
Subject: Re: [Qemu-devel] [PATCH for-2.7 00/15] block: Lock images when opening
Date: Thu, 14 Apr 2016 10:31:09 +0800	[thread overview]
Message-ID: <20160414023109.GC31467@ad.usersys.redhat.com> (raw)
In-Reply-To: <20160413091929.GC8847@redhat.com>

On Wed, 04/13 10:19, Daniel P. Berrange wrote:
> On Wed, Apr 13, 2016 at 05:09:49PM +0800, Fam Zheng wrote:
> > Too many troubles have been caused by two processes writing to the same image
> > unexpectedly. This series introduces automatical image locking into QEMU to
> > avoid such tragedy. With this, the user won't be able to open the image from
> > two processes (e.g. using qemu-img when the image is attached to the guest).
> > 
> > Underneath is the fcntl syscall that locks the local file, similar to what is
> > already used in libvirt virtlockd.  Also because of that, we cannot directly
> > apply fcntl lock on the image file itself, instead we open and lock
> > "/var/tmp/.qemu-$sha1.lock", where $sha1 is derived from the image's full path
> > as in realpath(3). This mechanism should be equally useful for the single host
> > case, and it doesn't conflict with virtlockd when managed by libvirt.
> > 
> > The alternative file locking API on Linux, flock(2), cannot protect host NFS
> > mount points, so it's not used.
> 
> Maybe I'm missing something, but since you are locking /var/tmp/.qemu-$sha1.lock
> the question of NFS support is irrelevant. All your locks are only ever going
> to apply within the local host, since /var/tmp is always a local filesystem,
> regardless of whether the actual image is on NFS. IOW, even using fcntl() you
> have no cross-host protection for NFS based images here.

You are right. I had a wrong impression in mind when writing the cover letter,
because in a draft version, instead of "lock-image = on | off", I once
implemented "lock-mode = off | image | lockfile", where the "image" mode will
lock the image itself like libvirt does. Later I realized it is cleaner to
stick to one locking scheme, considering we could have format locking in the
future, also considering the glusterfs patch, where the "lockfile" mode doesn't
make much sense.

I'll update the code to lock byte 1 then this point will become valid.

Fam