Am 27.02.2017 um 13:34 hat Max Reitz geschrieben:
> On 27.02.2017 13:33, Kevin Wolf wrote:
> > Am 25.02.2017 um 12:57 hat Max Reitz geschrieben:
> >> On 21.02.2017 15:58, Kevin Wolf wrote:
> >>> Almost all format drivers have the same characteristics as far as
> >>> permissions are concerned: They have one or more children for storing
> >>> their own data and, more importantly, metadata (can be written to and
> >>> grow even without external write requests, must be protected against
> >>> other writers and present consistent data) and optionally a backing file
> >>> (this is just data, so like for a filter, it only depends on what the
> >>> parent nodes need).
> >>>
> >>> This provides a default implementation that can be shared by most of
> >>> our format drivers.
> >>>
> >>> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> >>> ---
> >>> block.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >>> include/block/block_int.h | 8 ++++++++
> >>> 2 files changed, 50 insertions(+)
> >>>
> >>> diff --git a/block.c b/block.c
> >>> index 523cbd3..f2e7178 100644
> >>> --- a/block.c
> >>> +++ b/block.c
> >>> @@ -1554,6 +1554,48 @@ void bdrv_filter_default_perms(BlockDriverState
> >>> *bs, BdrvChild *c,
> >>> (c->shared_perm & DEFAULT_PERM_UNCHANGED);
> >>> }
> >>>
> >>> +void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
> >>> + const BdrvChildRole *role,
> >>> + uint64_t perm, uint64_t shared,
> >>> + uint64_t *nperm, uint64_t *nshared)
> >>> +{
> >>> + bool backing = (role == &child_backing);
> >>> + assert(role == &child_backing || role == &child_file);
> >>> +
> >>> + if (!backing) {
> >>> + /* Apart from the modifications below, the same permissions are
> >>> + * forwarded and left alone as for filters */
> >>> + bdrv_filter_default_perms(bs, c, role, perm, shared, &perm,
> >>> &shared);
> >>> +
> >>> + /* Format drivers may touch metadata even if the guest doesn't
> >>> write */
> >>> + if (!bdrv_is_read_only(bs)) {
> >>> + perm |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
> >>> + }
> >>> +
> >>> + /* 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;
> >>> + shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> >>> + } else {
> >>> + /* We want consistent read from backing files if the parent
> >>> needs it.
> >>> + * No other operations are performed on backing files. */
> >>> + perm &= BLK_PERM_CONSISTENT_READ;
> >>> +
> >>> + /* If the parent can deal with changing data, we're okay with a
> >>> + * writable and resizable backing file. */
> >>> + if (shared & BLK_PERM_WRITE) {
> >>> + shared = BLK_PERM_WRITE | BLK_PERM_RESIZE;
> >>
> >> Wouldn't this break CONSISTENT_READ?
> >
> > WRITE (even for multiple users) and CONSISTENT_READ aren't mutually
> > exclusive. I was afraid that I didn't define CONSISTENT_READ right, but
> > it appears that the definition is fine:
> >
> > * A user that has the "permission" of consistent reads is guaranteed that
> > * their view of the contents of the block device is complete and
> > * self-consistent, representing the contents of a disk at a specific
> > * point.
>
> Right, but writes to the backing file at least to me appear to be a
> different matter. If those don't break CONSISTENT_READ, then I don't see
> how commit breaks CONSISTENT_READ for the intermediate nodes.There's probably multiple ways to interpret such actions. You could understand a commit job as writing the desired image to the base node and at the same time it's a shared writer for the intermediate nodes that happens to write garbage. The question is if this is a useful way of seeing it when the job is to prevent accidental data corruption. Note that we need writable backing files for commit, so taking away BLK_PERM_WRITE from shared wouldn't work. We could probably make it dependent on cleared CONSISTENT_READ (commit jobs don't require this anyway), if you think that the current version is too permissive. Kevin
pgpfD_FZt3WGZ.pgp
Description: PGP signature
