[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] block: make bdrv_drop_intermediate() less wrong
From: |
Max Reitz |
Subject: |
Re: [PATCH 1/2] block: make bdrv_drop_intermediate() less wrong |
Date: |
Thu, 5 Nov 2020 14:22:15 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0 |
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>
- Re: [PATCH 1/2] block: make bdrv_drop_intermediate() less wrong,
Max Reitz <=