Am 30.11.2017 um 20:05 hat Eric Blake geschrieben: > On 11/30/2017 12:27 PM, Kevin Wolf wrote: > > > > @@ -1936,7 +1938,9 @@ void bdrv_format_default_perms(BlockDriverState > > > > *bs, BdrvChild *c, > > > > /* bs->file always needs to be consistent because of the > > > > metadata. We > > > > * can never allow other users to resize or write to it. */ > > > > - perm |= BLK_PERM_CONSISTENT_READ; > > > > + if (!(flags & BDRV_O_NO_IO)) { > > > > + perm |= BLK_PERM_CONSISTENT_READ; > > > > > > I thought BDRV_O_NO_IO only means we aren't doing I/O on guest-visible > > > data, > > > but doesn't stop us from reading the metadata. The comment is telling: if > > > we can read metadata, then we depend on CONSISTENT_READ for the metadata > > > to > > > be stable (even if we don't care about guest data consistency). > > > > Yes and no. The trouble is that at the file system level we have only a > > single bit to describe the consistency of the whole image throughout the > > whole block driver tree. > > > > We forbid shared BLK_PERM_CONSISTENT_READ for mirror targets (which > > aren't fully populated yet) and intermediate nodes for commit (which > > expose corrupted data). Both scenarios are really about the data exposed > > at the format layer, the metadata stays completely consistent. > > I guess it is the block of writes/resizes that prevents metadata from > getting inconsistent; CONSISTENT_READ does indeed make more sense if > interpreted solely in light of will the guest read consistent data > (and not will the format layer see consistent contents from the > protocol layer).
Yes, that's what the write/resize locks are meant for. Consistent read is more about "this image may not contain the useful data you're expecting". > > The question is what we do with this as we propagate permissions down to > > the protocol layer. Strictly speaking, the file at the protocol layer is > > perfectly consistent, so we might not forbid BLK_PERM_CONSISTENT_READ > > there. But I think it's more useful to do it anyway so that image > > locking can prevent the typical case of another process that uses qcow2 > > over file-posix again, where the file-posix node could in theory be > > considered consistent, but the qcow2 one wouldn't. > > Does that mean we need two separate permission bits, one for whether > the guest-visible data is consistent, and one for whether the metadata > is consistent? But as I mentioned above, blocking writes should mean > that no one else is messing with metadata. Even this works only in the simple case of format over protocol. I think this becomes pretty clear when you think of the (admittedly unlikely) case of nested qcow2. Then you need at least a bit per layer, one for the guest-visible data, one for the top-level qcow2 metadata and another one for the second qcow2 metadata. > > In the end, this is just a pragmatic way to let 'qemu-img info' work > > while the image is a mirror target or intermediate node for commit, but > > to forbid cases where corrupted data is used. > > > > Or would you argue that either 'qemu-img info' shouldn't be working or > > reading corrupted data should be allowed in other processes? > > Having qemu-img info work on a mirror target makes sense; as you pointed > out, the metadata flushed to disk is consistent (may have leaked clusters, > but not broken image) at any given point by the writer (it has to be, since > the writer has to be able to recover the image if there is an abrupt > restart). Exactly, that's the reasoning. > And a second reader can still manage to see inconsistent data > when reading two separate clusters if the timing is just right, regardless > of whether the first writer is always able to see consistent data. So to > some extent, it's a measure of how risky is the action? qemu-img info is > read-only, and most of the time will just work; it is very hard to get to > the rare race condition where two consecutive reads raced with a parallel > writer combine to result in incorrect interpretation of the data by the > reader. This again is more about the write permission. You still need 'qemu-img info -U' because there is a concurrent writer, so you have to acknowledge the risk you're mentioning. > I'm not opposed to your patch, but am trying to make sure that I'm not > overlooking any problem before giving R-b. Maybe it's just that the > comment needs updating in v2. Do you have a suggestion for the comment? Kevin