qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v8 2/7] copy-on-read: add filter append/drop functions


From: Andrey Shinkevich
Subject: Re: [PATCH v8 2/7] copy-on-read: add filter append/drop functions
Date: Thu, 24 Sep 2020 20:40:59 +0300
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:68.0) Gecko/20100101 Thunderbird/68.9.0

On 24.09.2020 20:29, Andrey Shinkevich wrote:
On 24.09.2020 18:00, Max Reitz wrote:
On 24.09.20 16:51, Vladimir Sementsov-Ogievskiy wrote:
24.09.2020 16:25, Max Reitz wrote:
On 23.09.20 16:38, Vladimir Sementsov-Ogievskiy wrote:
17.09.2020 19:09, Andrey Shinkevich wrote:
On 04.09.2020 14:22, Max Reitz wrote:
On 28.08.20 18:52, Andrey Shinkevich wrote:
Provide API for the COR-filter insertion/removal.
...
Also, drop the filter child permissions for an inactive state when
the
filter node is being removed.
Do we need .active for that?  Shouldn’t it be sufficient to just not require BLK_PERM_WRITE_UNCHANGED when no permissions are taken on the
node (i.e. perm == 0 in cor_child_perm())?

Of course, using an .active field works, too.  But Vladimir wanted a
similar field in the preallocate filter, and there already is in
backup-top.  I feel like there shouldn’t be a need for this.

.bdrv_child_perm() should generally be able to decide what permissions it needs based on the information it gets.  If every driver needs to
keep track of whether it has any parents and feed that information
into
.bdrv_child_perm() as external state, then something feels wrong.

If perm == 0 is not sufficient to rule out any parents, we should just
explicitly add a boolean that tells .bdrv_child_perm() whether
there are
any parents or not – shouldn’t be too difficult.

The issue is that we fail in the bdrv_check_update_perm() with
""Conflicts with use by..." if the *nperm = 0 and the *nshared =
BLK_PERM_ALL are not set before the call to the bdrv_replace_node().
The filter is still in the backing chain by that moment and has a
parent with child->perm != 0.

The implementation of  the .bdrv_child_perm()in the given patch is
similar to one in the block/mirror.c.

How could we resolve the issue at the generic layer?


The problem is that when we update permissions in bdrv_replace_node, we consider new placement for "to" node, but old placement for "from" node. So, during update, we may consider stricter permission requirements for
"from" than needed and they conflict with new "to" permissions.

We should consider all graph changes for permission update
simultaneously, in same transaction. For this, we need refactor
permission update system to work on queue of nodes instead of simple DFS
recursion. And in the queue all the nodes to update should  be
toplogically sorted. In this way, when we update node N, all its parents are already updated. And of course, we should make no-perm graph update
before permission update, and rollback no-perm movement if permission
update failed.
OK, you’ve convinced me, .active is good enough for now. O:)

And we need topological sort anyway. Consider the following example:

A -
|  \
|  v
|  B
|  |
v  /
C<-

A is parent for B and C, B is parent for C.

Obviously, to update permissions, we should go in order A B C, so, when
we update C, all it's parents permission already updated. But with
current approach (simple recursion) we can update in sequence A C B C (C is updated twice). On first update of C, we consider old B permissions,
so doing wrong thing. If it succeed, all is OK, on second C update we
will finish with correct graph. But if the wrong thing failed, we break the whole process for no reason (it's possible that updated B permission
will be less strict, but we will never check it).
True.

I'll work on a patch for it.
Sounds great, though I fear for the complexity.  I’ll just wait and see
for now.

If you are OK with .active for now, then I think, Andrey can resend with
.active and I'll dig into permissions in parallel. If Andrey's series
go first, I'll just drop .active later if my idea works.
Sure, that works for me.

Max


So, I am keeping the filter insert/remove functions in the COR-driver code for now rather than moving them to the block generic layer, aren't I?

Andrey


As a concession, we can invoke .bdrv_insert/remove driver functions within the generic API ones.

Andrey




reply via email to

[Prev in Thread] Current Thread [Next in Thread]