Later, in the next commit, we are going to need backing chain for all the disks within the domain. Let's parse them at domain startup phase.

On Tue, Sep 08, 2015 at 05:59:45PM +0200, Peter Krempa wrote: > On Tue, Sep 08, 2015 at 17:17:19 +0200, Michal Privoznik wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1192399 >> >> It's known fact for a while now that we should not only lock the >> top level layers of backing chain but the rest of it too. And >> well known too that we are not doing that. Well, up until this >> commit. The reason is that while one guest can have for instance >> the following backing chain: A (top) -> B -> C (bottom), the >> other can be started just over B or C. But libvirt should prevent >> that as it will almost certainly mangle the backing chain for the >> former guest. On the other hand, we want to allow guest with the > > Well, the corruption may still happen since if you run the guest that > uses image 'B' for writing, without starting the one that uses 'A' the > image 'A' will be invalidated anyways. Yep, the lock manager won't protect against you doing stupid stuff to the base image, which invalidates children, but that's out of scope really. We only need to consider the concurrent running VM problem here.

>> following backing chain: D (top) -> B -> C (bottom), because in >> both cases B and C are there just for reading. In order to >> achieve that we must lock the rest of backing chain with >> VIR_LOCK_MANAGER_RESOURCE_SHARED flag. > > See below ... >> + >> + if (!src->path) >> + break; >> + >> + VIR_DEBUG("Add disk %s", src->path); >> + if (virLockManagerAddResource(lock, >> + VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK, >> + src->path, >> + 0, >> + NULL, >> + diskFlags) < 0) { >> + VIR_DEBUG("Failed add disk %s", src->path); >> + goto cleanup; >> + } >> + >> + /* Now that we've added the first disk (top layer of backing chain) >> + * into the lock manager, let's traverse the rest of the backing chain >> + * and lock each layer for RO. This should prevent accidentally >> + * starting a domain with RW disk from the middle of the chain. */ >> + diskFlags = VIR_LOCK_MANAGER_RESOURCE_SHARED; > > The VIR_LOCK_MANAGER_RESOURCE_SHARED flag means "shared, writable" mode > according to lock_driver.h, I think you want to lock it with > VIR_LOCK_MANAGER_RESOURCE_READONLY. Yep, backing files should be marked readonly - 'shared' will allow multiple VMs to write to the image at once. So you've only provided mutual exclusion between an exclusive writer, vs a shared writer.

On 08.09.2015 18:04, Daniel P. Berrange wrote: >>> following backing chain: D (top) -> B -> C (bottom), because in >>> both cases B and C are there just for reading. In order to >>> achieve that we must lock the rest of backing chain with >>> VIR_LOCK_MANAGER_RESOURCE_SHARED flag. >> >> See below ... > >>> + >>> + if (!src->path) >>> + break; >>> + >>> + VIR_DEBUG("Add disk %s", src->path); >>> + if (virLockManagerAddResource(lock, >>> + VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK, >>> + src->path, >>> + 0, >>> + NULL, >>> + diskFlags) < 0) { >>> + VIR_DEBUG("Failed add disk %s", src->path); >>> + goto cleanup; >>> + } >>> + >>> + /* Now that we've added the first disk (top layer of backing chain) >>> + * into the lock manager, let's traverse the rest of the backing chain >>> + * and lock each layer for RO. This should prevent accidentally >>> + * starting a domain with RW disk from the middle of the chain. */ >>> + diskFlags = VIR_LOCK_MANAGER_RESOURCE_SHARED; >> >> The VIR_LOCK_MANAGER_RESOURCE_SHARED flag means "shared, writable" mode >> according to lock_driver.h, I think you want to lock it with >> VIR_LOCK_MANAGER_RESOURCE_READONLY. > > Yep, backing files should be marked readonly - 'shared' will allow > multiple VMs to write to the image at once. So you've only provided > mutual exclusion between an exclusive writer, vs a shared writer. Except that both of our locking drivers threat VIR_LOCK_MANAGER_RESOURCE_READONLY as no-op. But I think that since shared and exclusive writer are mutually exclusive, it doesn't matter that the lower layers are locked as shared writer. As soon as somebody wants to lock it exclusively they will fail. Nor virtlockd nor sanlock is actually enforcing the RO vs RW policy (if a file is locked for RO no writes are allowed). It's merely for tracking shared and exclusive locks.

On Wed, Sep 09, 2015 at 09:49:16AM +0100, Daniel P. Berrange wrote: >On Wed, Sep 09, 2015 at 10:13:24AM +0200, Michal Privoznik wrote: >>On 08.09.2015 18:04, Daniel P. Berrange wrote: >>>>> following backing chain: D (top) -> B -> C (bottom), because in >>>>> both cases B and C are there just for reading. In order to >>>>> achieve that we must lock the rest of backing chain with >>>>> VIR_LOCK_MANAGER_RESOURCE_SHARED flag. >>>> >>>> See below ... >>> >>>>> + >>>>> + if (!src->path) >>>>> + break; >>>>> + >>>>> + VIR_DEBUG("Add disk %s", src->path); >>>>> + if (virLockManagerAddResource(lock, >>>>> + VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK, >>>>> + src->path, >>>>> + 0, >>>>> + NULL, >>>>> + diskFlags) < 0) { >>>>> + VIR_DEBUG("Failed add disk %s", src->path); >>>>> + goto cleanup; >>>>> + } >>>>> + >>>>> + /* Now that we've added the first disk (top layer of backing chain) >>>>> + * into the lock manager, let's traverse the rest of the backing chain >>>>> + * and lock each layer for RO. This should prevent accidentally >>>>> + * starting a domain with RW disk from the middle of the chain. */ >>>>> + diskFlags = VIR_LOCK_MANAGER_RESOURCE_SHARED; >>>> >>>> The VIR_LOCK_MANAGER_RESOURCE_SHARED flag means "shared, writable" mode >>>> according to lock_driver.h, I think you want to lock it with >>>> VIR_LOCK_MANAGER_RESOURCE_READONLY. >>> >>> Yep, backing files should be marked readonly - 'shared' will allow >>> multiple VMs to write to the image at once. So you've only provided >>> mutual exclusion between an exclusive writer, vs a shared writer. >> >>Except that both of our locking drivers threat >>VIR_LOCK_MANAGER_RESOURCE_READONLY as no-op. >> >>But I think that since shared and exclusive writer are mutually >>exclusive, it doesn't matter that the lower layers are locked as shared >>writer. As soon as somebody wants to lock it exclusively they will fail. >>Nor virtlockd nor sanlock is actually enforcing the RO vs RW policy (if >>a file is locked for RO no writes are allowed). It's merely for tracking >>shared and exclusive locks. > >Right, so I remember why we ignore READONLY locks now - we were only >trying to protect against 2 processes /writing/ to the same image at >once, which would cause unrecoverable data corruption. > >We were not trying to protect against one guest having a disk readonly >while another guest has the disk read-write. That is bad because the >guest holding the image readonly will see inconsistent filesystem >state which may result in it kernel panicing. But the underling disk >is still only written by one guest at a time, so there's no disk >image corruption. > >This is similar to the scenario you illustrated with backing files. >One guest had a backing file open for write, and another guest had >it open readonly by virtue of having an upper layer open for write. >We're not actually going to suffer data corription of the backing >file here since we're still protected to not have concurrent writes >to it. > >We are going to destroy the upper layer image, by invalidating the >backing file, but that is true regardless of whether another guest >has the upper layer open right now or not. > >So in fact I think there's a case to say that this entire patch >is out of scope for the lock managers, as their goal is to prevent >concurrent writes to an image, not to protect against administrator >screwups with backing file writing. > I'm just jumping into the discussion from the middle, so don't beat me for details I might've missed. I don't quite get these last two paragraphs. There will always be cases that can screw up the whole backing chain and there will always be users doing so. But for the cases libvirt is able to know about, we should utilize the locks if we have the option to do so. Whatever two domains are doing concurrently, we can make sure it's OK. If one domain is invalidating a backing file of a another domain just because it's not running... well, there's plenty of stuff you can break without utilizing backing chains. I think the idea of this patch is good, it just needs some polishing and after that, few follow-up patches that will utilize exclusive locks for all files used by block jobs.