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 <vsement...@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 <mre...@redhat.com>
