Re: [PATCH 1/2] block: make bdrv_drop_intermediate() less wrong

[Max Reitz]

On 31.10.20 13:35, Vladimir Sementsov-Ogievskiy wrote:
> First, permission update loop tries to do iterations transactionally,
> but the whole update is not transactional: nobody roll-back successful
> loop iterations when some iteration fails.

Indeed.  Another thing that should be noted:

bdrv_check_update_perm() doc:
> Needs to be followed by a call to either bdrv_set_perm()
> or bdrv_abort_perm_update().

And besides rolling back on error, we don’t commit on success either.

> Second, in the iteration we have nested permission update:
> c->klass->update_filename may point to bdrv_child_cb_update_filename()
> which calls bdrv_backing_update_filename(), which may do node reopen to
> RW.
>
> Permission update system is not prepared to nested updates, at least it
> has intermediate permission-update state stored in BdrvChild
> structures: has_backup_perm, backup_perm and backup_shared_perm.
>
> So, let's first do bdrv_replace_node() (which is more transactional
> than open-coded update in bdrv_drop_intermediate()) and then call
> update_filename() in separate. We still do not rollback changes in case
> of update_filename() failure but it's not much worse than pre-patch
> behavior.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block.c | 36 +++++++++++++++---------------------
>   1 file changed, 15 insertions(+), 21 deletions(-)

And it’s also much simpler and clearer now.

Reviewed-by: Max Reitz <mreitz@redhat.com>