Re: [Qemu-devel] [PATCH 6/6] block: Fix permissions after bdrv_reopen()

[Kevin Wolf]

Am 15.09.2017 um 21:06 hat Eric Blake geschrieben:
> On 09/15/2017 05:10 AM, Kevin Wolf wrote:
> > If we switch between read-only and read-write, the permissions that
> > image format drivers need on bs->file change, too. Make sure to update
> > the permissions during bdrv_reopen().
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  include/block/block.h |  1 +
> >  block.c   | 64 
> > +++
> >  2 files changed, 65 insertions(+)
> > 
> 
> > +static BlockReopenQueueEntry *find_parent_in_reopen_queue(BlockReopenQueue 
> > *q,
> > +  BdrvChild *c)
> > +{
> > +BlockReopenQueueEntry *entry;
> > +
> > +QSIMPLEQ_FOREACH(entry, q, entry) {
> > +BlockDriverState *bs = entry->state.bs;
> > +BdrvChild *child;
> > +
> > +QLIST_FOREACH(child, >children, next) {
> > +if (child == c) {
> > +return entry;
> 
> An O(n^2) loop. Is it going to bite us at any point in the future, or
> are we generally dealing with a small enough queue size and BDS graph to
> not worry about it?

The loops you're quoting aren't O(n^2), they don't loop over the same
thing. This part is O(n) in terms of BdrvChild elements looked at.

The thing that worried me a bit more is the caller:

+QLIST_FOREACH(c, >parents, next_parent) {
+parent = find_parent_in_reopen_queue(q, c);

This is indeed O(n^2) (again with n = number of BdrvChild elements) in
the pathological worst case of a quorum node where all children point to
the same node.

As soon as all parents of the node are distinct - and I don't see any
reason why they wouldn't in practice - we're back to O(n) because each
BdrvChild belongs to only one parent. Even if we ever introduce a driver
where having the same node as a child in a constant number of different
roles makes sense for a parent (i.e. anything that doesn't involve an
(unbounded) array of children), we would still be O(n) with an additional
small constant factor.

So I think in practice we should be okay.

Kevin


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 6/6] block: Fix permissions after bdrv_reopen()

[Kevin Wolf]

Am 18.09.2017 um 09:37 hat Fam Zheng geschrieben:
> On Fri, 09/15 12:10, Kevin Wolf wrote:
> > If we switch between read-only and read-write, the permissions that
> > image format drivers need on bs->file change, too. Make sure to update
> > the permissions during bdrv_reopen().
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  include/block/block.h |  1 +
> >  block.c   | 64 
> > +++
> >  2 files changed, 65 insertions(+)
> > 
> > diff --git a/include/block/block.h b/include/block/block.h
> > index 082eb2cd9c..3c3af462e4 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -166,6 +166,7 @@ typedef QSIMPLEQ_HEAD(BlockReopenQueue, 
> > BlockReopenQueueEntry) BlockReopenQueue;
> >  typedef struct BDRVReopenState {
> >  BlockDriverState *bs;
> >  int flags;
> > +uint64_t perm, shared_perm;
> >  QDict *options;
> >  QDict *explicit_options;
> >  void *opaque;
> > diff --git a/block.c b/block.c
> > index 204cbb46c7..5c65fac672 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2781,6 +2781,10 @@ static BlockReopenQueue 
> > *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
> >  bs_entry->state.explicit_options = explicit_options;
> >  bs_entry->state.flags = flags;
> >  
> > +/* This needs to be overwritten in bdrv_reopen_prepare() */
> > +bs_entry->state.perm = UINT64_MAX;
> 
> Probably doesn't matter because as the comment says it will be overwritten 
> soon,
> but is BLK_PERM_ALL more appropriate?

I had BLK_PERM_ALL at first, but after debugging some assertion failures
in gdb, I came to the conclusion that UINT64_MAX is easier to identify as
uninitialised than BLK_PERM_ALL, which could be a valid result of the
permission calculation.

Kevin

Re: [Qemu-devel] [PATCH 6/6] block: Fix permissions after bdrv_reopen()

[Fam Zheng]

On Fri, 09/15 12:10, Kevin Wolf wrote:
> If we switch between read-only and read-write, the permissions that
> image format drivers need on bs->file change, too. Make sure to update
> the permissions during bdrv_reopen().
> 
> Signed-off-by: Kevin Wolf 
> ---
>  include/block/block.h |  1 +
>  block.c   | 64 
> +++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 082eb2cd9c..3c3af462e4 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -166,6 +166,7 @@ typedef QSIMPLEQ_HEAD(BlockReopenQueue, 
> BlockReopenQueueEntry) BlockReopenQueue;
>  typedef struct BDRVReopenState {
>  BlockDriverState *bs;
>  int flags;
> +uint64_t perm, shared_perm;
>  QDict *options;
>  QDict *explicit_options;
>  void *opaque;
> diff --git a/block.c b/block.c
> index 204cbb46c7..5c65fac672 100644
> --- a/block.c
> +++ b/block.c
> @@ -2781,6 +2781,10 @@ static BlockReopenQueue 
> *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>  bs_entry->state.explicit_options = explicit_options;
>  bs_entry->state.flags = flags;
>  
> +/* This needs to be overwritten in bdrv_reopen_prepare() */
> +bs_entry->state.perm = UINT64_MAX;

Probably doesn't matter because as the comment says it will be overwritten soon,
but is BLK_PERM_ALL more appropriate?

> +bs_entry->state.shared_perm = 0;
> +
>  QLIST_FOREACH(child, >children, next) {
>  QDict *new_child_options;
>  char *child_key_dot;
> @@ -2887,6 +2891,52 @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, 
> Error **errp)
>  return ret;
>  }
>  
> +static BlockReopenQueueEntry *find_parent_in_reopen_queue(BlockReopenQueue 
> *q,
> +  BdrvChild *c)
> +{
> +BlockReopenQueueEntry *entry;
> +
> +QSIMPLEQ_FOREACH(entry, q, entry) {
> +BlockDriverState *bs = entry->state.bs;
> +BdrvChild *child;
> +
> +QLIST_FOREACH(child, >children, next) {
> +if (child == c) {
> +return entry;
> +}
> +}
> +}
> +
> +return NULL;
> +}
> +
> +static void bdrv_reopen_perm(BlockReopenQueue *q, BlockDriverState *bs,
> + uint64_t *perm, uint64_t *shared)
> +{
> +BdrvChild *c;
> +BlockReopenQueueEntry *parent;
> +uint64_t cumulative_perms = 0;
> +uint64_t cumulative_shared_perms = BLK_PERM_ALL;
> +
> +QLIST_FOREACH(c, >parents, next_parent) {
> +parent = find_parent_in_reopen_queue(q, c);
> +if (!parent) {
> +cumulative_perms |= c->perm;
> +cumulative_shared_perms &= c->shared_perm;
> +} else {
> +uint64_t nperm, nshared;
> +
> +bdrv_child_perm(parent->state.bs, bs, c, c->role, q,
> +parent->state.perm, parent->state.shared_perm,
> +, );
> +
> +cumulative_perms |= nperm;
> +cumulative_shared_perms &= nshared;
> +}
> +}
> +*perm = cumulative_perms;
> +*shared = cumulative_shared_perms;
> +}
>  
>  /*
>   * Prepares a BlockDriverState for reopen. All changes are staged in the
> @@ -2952,6 +3002,9 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
> BlockReopenQueue *queue,
>  goto error;
>  }
>  
> +/* Calculate required permissions after reopening */
> +bdrv_reopen_perm(queue, reopen_state->bs,
> + _state->perm, _state->shared_perm);
>  
>  ret = bdrv_flush(reopen_state->bs);
>  if (ret) {
> @@ -3007,6 +3060,12 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
> BlockReopenQueue *queue,
>  } while ((entry = qdict_next(reopen_state->options, entry)));
>  }
>  
> +ret = bdrv_check_perm(reopen_state->bs, queue, reopen_state->perm,
> +  reopen_state->shared_perm, NULL, errp);
> +if (ret < 0) {
> +goto error;
> +}
> +
>  ret = 0;
>  
>  error:
> @@ -3047,6 +3106,9 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>  
>  bdrv_refresh_limits(bs, NULL);
>  
> +bdrv_set_perm(reopen_state->bs, reopen_state->perm,
> +  reopen_state->shared_perm);
> +
>  new_can_write =
>  !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
>  if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) {
> @@ -3080,6 +3142,8 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
>  }
>  
>  QDECREF(reopen_state->explicit_options);
> +
> +bdrv_abort_perm_update(reopen_state->bs);
>  }
>  
>  
> -- 
> 2.13.5
> 

Re: [Qemu-devel] [PATCH 6/6] block: Fix permissions after bdrv_reopen()

[Eric Blake]

On 09/15/2017 05:10 AM, Kevin Wolf wrote:
> If we switch between read-only and read-write, the permissions that
> image format drivers need on bs->file change, too. Make sure to update
> the permissions during bdrv_reopen().
> 
> Signed-off-by: Kevin Wolf 
> ---
>  include/block/block.h |  1 +
>  block.c   | 64 
> +++
>  2 files changed, 65 insertions(+)
> 

> +static BlockReopenQueueEntry *find_parent_in_reopen_queue(BlockReopenQueue 
> *q,
> +  BdrvChild *c)
> +{
> +BlockReopenQueueEntry *entry;
> +
> +QSIMPLEQ_FOREACH(entry, q, entry) {
> +BlockDriverState *bs = entry->state.bs;
> +BdrvChild *child;
> +
> +QLIST_FOREACH(child, >children, next) {
> +if (child == c) {
> +return entry;

An O(n^2) loop. Is it going to bite us at any point in the future, or
are we generally dealing with a small enough queue size and BDS graph to
not worry about it?

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 6/6] block: Fix permissions after bdrv_reopen()

[Kevin Wolf]

If we switch between read-only and read-write, the permissions that
image format drivers need on bs->file change, too. Make sure to update
the permissions during bdrv_reopen().

Signed-off-by: Kevin Wolf 
---
 include/block/block.h |  1 +
 block.c   | 64 +++
 2 files changed, 65 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index 082eb2cd9c..3c3af462e4 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -166,6 +166,7 @@ typedef QSIMPLEQ_HEAD(BlockReopenQueue, 
BlockReopenQueueEntry) BlockReopenQueue;
 typedef struct BDRVReopenState {
 BlockDriverState *bs;
 int flags;
+uint64_t perm, shared_perm;
 QDict *options;
 QDict *explicit_options;
 void *opaque;
diff --git a/block.c b/block.c
index 204cbb46c7..5c65fac672 100644
--- a/block.c
+++ b/block.c
@@ -2781,6 +2781,10 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 bs_entry->state.explicit_options = explicit_options;
 bs_entry->state.flags = flags;
 
+/* This needs to be overwritten in bdrv_reopen_prepare() */
+bs_entry->state.perm = UINT64_MAX;
+bs_entry->state.shared_perm = 0;
+
 QLIST_FOREACH(child, >children, next) {
 QDict *new_child_options;
 char *child_key_dot;
@@ -2887,6 +2891,52 @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, 
Error **errp)
 return ret;
 }
 
+static BlockReopenQueueEntry *find_parent_in_reopen_queue(BlockReopenQueue *q,
+  BdrvChild *c)
+{
+BlockReopenQueueEntry *entry;
+
+QSIMPLEQ_FOREACH(entry, q, entry) {
+BlockDriverState *bs = entry->state.bs;
+BdrvChild *child;
+
+QLIST_FOREACH(child, >children, next) {
+if (child == c) {
+return entry;
+}
+}
+}
+
+return NULL;
+}
+
+static void bdrv_reopen_perm(BlockReopenQueue *q, BlockDriverState *bs,
+ uint64_t *perm, uint64_t *shared)
+{
+BdrvChild *c;
+BlockReopenQueueEntry *parent;
+uint64_t cumulative_perms = 0;
+uint64_t cumulative_shared_perms = BLK_PERM_ALL;
+
+QLIST_FOREACH(c, >parents, next_parent) {
+parent = find_parent_in_reopen_queue(q, c);
+if (!parent) {
+cumulative_perms |= c->perm;
+cumulative_shared_perms &= c->shared_perm;
+} else {
+uint64_t nperm, nshared;
+
+bdrv_child_perm(parent->state.bs, bs, c, c->role, q,
+parent->state.perm, parent->state.shared_perm,
+, );
+
+cumulative_perms |= nperm;
+cumulative_shared_perms &= nshared;
+}
+}
+*perm = cumulative_perms;
+*shared = cumulative_shared_perms;
+}
 
 /*
  * Prepares a BlockDriverState for reopen. All changes are staged in the
@@ -2952,6 +3002,9 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
 goto error;
 }
 
+/* Calculate required permissions after reopening */
+bdrv_reopen_perm(queue, reopen_state->bs,
+ _state->perm, _state->shared_perm);
 
 ret = bdrv_flush(reopen_state->bs);
 if (ret) {
@@ -3007,6 +3060,12 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
 } while ((entry = qdict_next(reopen_state->options, entry)));
 }
 
+ret = bdrv_check_perm(reopen_state->bs, queue, reopen_state->perm,
+  reopen_state->shared_perm, NULL, errp);
+if (ret < 0) {
+goto error;
+}
+
 ret = 0;
 
 error:
@@ -3047,6 +3106,9 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 
 bdrv_refresh_limits(bs, NULL);
 
+bdrv_set_perm(reopen_state->bs, reopen_state->perm,
+  reopen_state->shared_perm);
+
 new_can_write =
 !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
 if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) {
@@ -3080,6 +3142,8 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
 }
 
 QDECREF(reopen_state->explicit_options);
+
+bdrv_abort_perm_update(reopen_state->bs);
 }
 
 
-- 
2.13.5