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

[Paolo Bonzini <pbonzini@redhat.com>]

On 1/24/22 18:37, Vladimir Sementsov-Ogievskiy wrote:
> 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().

Emanuele has a similar patch in his series to protect all graph
modifications with drains:

@@ -3456,6 +3478,11 @@ int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
  
      assert(qemu_in_main_thread());
  
+    bdrv_subtree_drained_begin_unlocked(bs);
+    if (backing_hd) {
+        bdrv_subtree_drained_begin_unlocked(backing_hd);
+    }
+
      ret = bdrv_set_backing_noperm(bs, backing_hd, tran, errp);
      if (ret < 0) {
          goto out;
@@ -3464,6 +3491,10 @@ int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
      ret = bdrv_refresh_perms(bs, errp);
  out:
      tran_finalize(tran, ret);
+    if (backing_hd) {
+        bdrv_subtree_drained_end_unlocked(backing_hd);
+    }
+    bdrv_subtree_drained_end_unlocked(bs);
  
      return ret;
  }

so the idea at least is correct.

I don't object to fixing this independently, but please check
1) if a subtree drain would be more appropriate, 2) whether
backing_hd should be drained as well, 3) whether we're guaranteed
to be holding the AioContext lock as required for
bdrv_drained_begin/end.

Paolo