Re: [Qemu-devel] [PATCH v6 28/42] stream: Deal with filters

[Kevin Wolf]

Am 16.09.2019 um 11:52 hat Max Reitz geschrieben:
> On 13.09.19 16:16, Kevin Wolf wrote:
> > Am 09.08.2019 um 18:13 hat Max Reitz geschrieben:
> >> @@ -261,16 +272,19 @@ void stream_start(const char *job_id, 
> >> BlockDriverState *bs,
> >>       * disappear from the chain after this operation. The streaming job 
> >> reads
> >>       * every block only once, assuming that it doesn't change, so forbid 
> >> writes
> >>       * and resizes. Reassign the base node pointer because the backing BS 
> >> of the
> >> -     * bottom node might change after the call to 
> >> bdrv_reopen_set_read_only()
> >> -     * due to parallel block jobs running.
> >> +     * above_base node might change after the call to
> >> +     * bdrv_reopen_set_read_only() due to parallel block jobs running.
> >>       */
> >> -    base = backing_bs(bottom);
> >> -    for (iter = backing_bs(bs); iter && iter != base; iter = 
> >> backing_bs(iter)) {
> >> +    base = bdrv_filtered_bs(above_base);
> > 
> > We just calculated above_base such that it's the parent of base. Why
> > would base not already have the value we're assigning it again here?
> 
> That’s no change to existing code, whose reasoning is explained in the
> comment above: bdrv_reopen_set_read_only() can yield, which might lead
> to children of the bottom node changing.
> 
> If you feel like either that’s superfluous, or that if something like
> that were to happen we’d have much bigger problems, be my guest to drop
> both.
> 
> But in this series I’d rather just not change it.

Ah, you mean comments are there to be read?

But actually, I think iterating down to base is too much anyway. The
reasoning in the comment for block_job_add_bdrv() is that the nodes will
be dropped at the end. But base with all of its filter will be kept
after this patch.

So I think the for loop should stop after bs->base_overlay. And then
concurrently changing links aren't even a problem any more because
that's exactly the place up to which we've frozen the chain.

Kevin

signature.asc
Description: PGP signature