Am 27.05.25 um 17:29 schrieb Kevin Wolf: > Am 26.05.2025 um 15:21 hat Fiona Ebner geschrieben: >> @@ -3578,7 +3577,6 @@ int bdrv_set_backing_hd_drained(BlockDriverState *bs, >> ret = bdrv_refresh_perms(bs, tran, errp); >> out: >> tran_finalize(tran, ret); >> - bdrv_drain_all_end(); >> return ret; >> } > > Do we need to update the comment for bdrv_set_backing_hd_drained()? > > * If a backing child is already present (i.e. we're detaching a node), that > * child node must be drained. > > Same as in the previous patch, this is now probably all nodes.
Yes, and even in case no backing child is already present. >> @@ -3594,11 +3592,11 @@ int bdrv_set_backing_hd(BlockDriverState *bs, >> BlockDriverState *backing_hd, >> bdrv_graph_rdunlock_main_loop(); >> >> bdrv_ref(drain_bs); >> - bdrv_drained_begin(drain_bs); >> + bdrv_drain_all_begin(); >> bdrv_graph_wrlock(); >> ret = bdrv_set_backing_hd_drained(bs, backing_hd, errp); >> bdrv_graph_wrunlock(); >> - bdrv_drained_end(drain_bs); >> + bdrv_drain_all_end(); >> bdrv_unref(drain_bs); > > The only thing we do with drain_bs now is finding it, bdrv_ref() and > immediately bdrv_unref(). I don't think it should exist any more after > the change to drain_all. I'll drop it in v4. I now noticed that bdrv_set_backing_hd() is required to be GRAPH_UNLOCKED, because it calls bdrv_drain_all_begin(), but is not marked as such yet. Adding that annotation requires adapting some callers of bdrv_set_backing_hd() first. I'll try to add some more patches at the end of the series for this. At least the caller in block/mirror.c seems to be better of using bdrv_set_backing_hd_drained() and bdrv_graph_wrlock_drained() itself, so the section can cover more related calls like "unfiltered_target = bdrv_skip_filters(target_bs);" Best Regards, Fiona
