Re: [PATCH] block: bdrv_set_backing_hd(): use drained section

[Kevin Wolf]

Am 24.01.2022 um 18:37 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Graph modifications should be done in drained section. stream_prepare()
> handler of block stream job call bdrv_set_backing_hd() without using
> drained section and it's theoretically possible that some IO request
> will interleave with graph modification and will use outdated pointers
> to removed block nodes.
> 
> Some other callers use bdrv_set_backing_hd() not caring about drained
> sections too. So it seems good to make a drained section exactly in
> bdrv_set_backing_hd().
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Thanks, applied to the block branch.

> Hi all!
> 
> We faced the following bug in our Rhel7-based downstream:
> read request crashes because backing bs is NULL unexpectedly (honestly,
> it crashes inside bdrv_is_allocated(), which is called during read and
> it's a downstream-only code, but that doesn't make real sense).
> 
> In gdb I also see block-stream job in state
> "refcnt = 0, status = JOB_STATUS_NULL", but it's still in jobs list.
> 
> So, I assume that backing file was disappeared exactly as final step of
> block-stream job. And the problem is that this step should be done in
> drained section, but seems that it isn't.
> 
> If we have a drained section, we'd wait for finish of read request
> before removing the backing node.
> 
> I don't have a reproducer. I spent some time to write a test, but there
> are problems that makes hard to use blkdebug's break-points: we have
> drained section at block-stream start, and we do have drained section at
> block-stream finish: bdrv_cor_filter_drop() called from stream_prepare()
> does drained section (unlike bdrv_set_backing_hd()).

Maybe a unit test would be easier to write for this kind of thing than
an iotest?

> So, the fix is intuitive. I think, it's correct)
> 
> Note also, that alternative would be to make a drained section in
> stream_prepare() and don't touch bdrv_set_backing_hd() function. But it
> seems good to make a public graph-modification function more safe.

Yes, makes sense to me.

Kevin