Re: [PATCH] block/stream: Drain subtree around graph change

[Vladimir Sementsov-Ogievskiy <v.sementsov-og@mail.ru>]

05.04.2022 17:41, Kevin Wolf wrote:
> Am 05.04.2022 um 14:12 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Thanks Kevin! I have already run out of arguments in the battle
>> against using subtree-drains to isolate graph modification operations
>> from each other in different threads in the mailing list)
>>
>> (Note also, that the top-most version of this patch is "[PATCH v2]
>> block/stream: Drain subtree around graph change")
> 
> Oops, I completely missed the v2. Thanks!
> 
>> About avoiding polling during graph-modifying operations, there is a
>> problem: some IO operations are involved into block-graph modifying
>> operations. At least it's rewriting "backing_file_offset" and
>> "backing_file_size" fields in qcow2 header.
>>
>> We can't just separate rewriting metadata from graph modifying
>> operation: this way another graph-modifying operation may interleave
>> and we'll write outdated metadata.
> 
> Hm, generally we don't update image metadata when we reconfigure the
> graph. Most changes are temporary (like insertion of filter nodes) and
> the image header only contains a "default configuration" to be used on
> the next start.
> 
> There are only a few places that update the image header; I think it's
> generally block job completions. They obviously update the in-memory
> graph, too, but they don't write to the image file (and therefore
> potentially poll) in the middle of updating the in-memory graph, but
> they do both in separate steps.
> 
> I think this is okay. We must just avoid polling in the middle of graph
> updates because if something else changes the graph there, it's not
> clear any more that we're really doing what the caller had in mind.

Hmm, interesting where is polling in described case?

First possible place I can find is bdrv_parent_drained_begin_single() in bdrv_replace_child_noperm().

Another is bdrv_apply_subtree_drain() in bdrv_child_cb_attach().

No idea how to get rid of them. Hmm.

I think, the core problem here is that when we wait in drained_begin(), nobody protects us from attaching one more node to the drained subgraph. And we should handle this, that's the complexity.

> 
>> So I still think, we need a kind of global lock for graph modifying
>> operations. Or a kind per-BDS locks as you propose. But in this case
>> we need to be sure that taking all needed per-BDS locks we'll avoid
>> deadlocking.
> 
> I guess this depends on the exact granularity of the locks we're using.
> If you take the lock only while updating a single edge, I don't think
> you could easily deadlock. If you hold it for more complex operations,
> it becomes harder to tell without checking the code.
> 

I think, keeping the whole operation, like reopen_multiple, or some job's .prepare(), etc., under one critical section is simplest to analyze.

Could this be something like this?

   uint8_t graph_locked;

   void graph_lock(AioContext *ctx) {
     AIO_POLL_WHILE(ctx, qatomic_cmpxchg(&graph_locked, 0, 1) == 1);
   }

   void graph_unlock() {
     qatomic_set(&graph_locked, 0);
     aio_wait_kick();
   }

-- 
Best regards,
Vladimir