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 <vsement...@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