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
