| Message ID | 1487689130-30373-19-git-send-email-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <kwolf@redhat.com> > --- > block.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > include/block/block_int.h | 8 ++++++++ > 2 files changed, 50 insertions(+) Reviewed-by: Max Reitz <mreitz@redhat.com>
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 <kwolf@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? Max > + } else { > + shared = 0; > + } > + > + shared |= BLK_PERM_CONSISTENT_READ | BLK_PERM_GRAPH_MOD | > + BLK_PERM_WRITE_UNCHANGED; > + } > + > + *nperm = perm; > + *nshared = shared; > +} > > static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs) > { > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 17f4c2d..eb0598e 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -880,6 +880,14 @@ void bdrv_filter_default_perms(BlockDriverState *bs, BdrvChild *c, > uint64_t perm, uint64_t shared, > uint64_t *nperm, uint64_t *nshared); > > +/* Default implementation for BlockDriver.bdrv_child_perm() that can be used by > + * (non-raw) image formats: Like above for bs->backing, but for bs->file it > + * requires WRITE | RESIZE for read-write images, always requires > + * CONSISTENT_READ and doesn't share WRITE. */ > +void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, > + const BdrvChildRole *role, > + uint64_t perm, uint64_t shared, > + uint64_t *nperm, uint64_t *nshared); > > const char *bdrv_get_parent_name(const BlockDriverState *bs); > void blk_dev_change_media_cb(BlockBackend *blk, bool load); >
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 <kwolf@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. Kevin
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 <kwolf@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. Max
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 <kwolf@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
On 27.02.2017 15:05, Kevin Wolf wrote: > 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 <kwolf@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. Agreed. > The question is if this is a useful way > of seeing it when the job is to prevent accidental data corruption. Agreed. But then I would infer that any write to a backing file breaks CONSISTENT_READ on the overlay. > 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. I agree that we need to be able to share WRITE for a backing file. But I think this should only be set if the overlay's parents do not require CONSISTENT_READ from the overlay. Max
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; + } else { + shared = 0; + } + + shared |= BLK_PERM_CONSISTENT_READ | BLK_PERM_GRAPH_MOD | + BLK_PERM_WRITE_UNCHANGED; + } + + *nperm = perm; + *nshared = shared; +} static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs) { diff --git a/include/block/block_int.h b/include/block/block_int.h index 17f4c2d..eb0598e 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -880,6 +880,14 @@ void bdrv_filter_default_perms(BlockDriverState *bs, BdrvChild *c, uint64_t perm, uint64_t shared, uint64_t *nperm, uint64_t *nshared); +/* Default implementation for BlockDriver.bdrv_child_perm() that can be used by + * (non-raw) image formats: Like above for bs->backing, but for bs->file it + * requires WRITE | RESIZE for read-write images, always requires + * CONSISTENT_READ and doesn't share WRITE. */ +void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, + const BdrvChildRole *role, + uint64_t perm, uint64_t shared, + uint64_t *nperm, uint64_t *nshared); const char *bdrv_get_parent_name(const BlockDriverState *bs); void blk_dev_change_media_cb(BlockBackend *blk, bool load);
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 <kwolf@redhat.com> --- block.c | 42 ++++++++++++++++++++++++++++++++++++++++++ include/block/block_int.h | 8 ++++++++ 2 files changed, 50 insertions(+)