[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v6 28/42] stream: Deal with filters
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [PATCH v6 28/42] stream: Deal with filters |
Date: |
Mon, 16 Sep 2019 16:47:35 +0200 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
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