Skip to content
  • Hanna Reitz's avatar
    b1e1af39
    block/stream: Drain subtree around graph change · b1e1af39
    Hanna Reitz authored
    
    
    When the stream block job cuts out the nodes between top and base in
    stream_prepare(), it does not drain the subtree manually; it fetches the
    base node, and tries to insert it as the top node's backing node with
    bdrv_set_backing_hd().  bdrv_set_backing_hd() however will drain, and so
    the actual base node might change (because the base node is actually not
    part of the stream job) before the old base node passed to
    bdrv_set_backing_hd() is installed.
    
    This has two implications:
    
    First, the stream job does not keep a strong reference to the base node.
    Therefore, if it is deleted in bdrv_set_backing_hd()'s drain (e.g.
    because some other block job is drained to finish), we will get a
    use-after-free.  We should keep a strong reference to that node.
    
    Second, even with such a strong reference, the problem remains that the
    base node might change before bdrv_set_backing_hd() actually runs and as
    a result the wrong base node is installed.
    
    Both effects can be seen in 030's TestParallelOps.test_overlapping_5()
    case, which has five nodes, and simultaneously streams from the middle
    node to the top node, and commits the middle node down to the base node.
    As it is, this will sometimes crash, namely when we encounter the
    above-described use-after-free.
    
    Taking a strong reference to the base node, we no longer get a crash,
    but the resuling block graph is less than ideal: The expected result is
    obviously that all middle nodes are cut out and the base node is the
    immediate backing child of the top node.  However, if stream_prepare()
    takes a strong reference to its base node (the middle node), and then
    the commit job finishes in bdrv_set_backing_hd(), supposedly dropping
    that middle node, the stream job will just reinstall it again.
    
    Therefore, we need to keep the whole subtree drained in
    stream_prepare(), so that the graph modification it performs is
    effectively atomic, i.e. that the base node it fetches is still the base
    node when bdrv_set_backing_hd() sets it as the top node's backing node.
    
    Verify this by asserting in said 030's test case that the base node is
    always the top node's immediate backing child when both jobs are done.
    
    Signed-off-by: default avatarHanna Reitz <hreitz@redhat.com>
    Message-Id: <20220324140907.17192-1-hreitz@redhat.com>
    Reviewed-by: default avatarEric Blake <eblake@redhat.com>
    Acked-by: default avatarVladimir Sementsov-Ogievskiy <v.sementsov-og@mail.ru>
    b1e1af39
    block/stream: Drain subtree around graph change
    Hanna Reitz authored
    
    
    When the stream block job cuts out the nodes between top and base in
    stream_prepare(), it does not drain the subtree manually; it fetches the
    base node, and tries to insert it as the top node's backing node with
    bdrv_set_backing_hd().  bdrv_set_backing_hd() however will drain, and so
    the actual base node might change (because the base node is actually not
    part of the stream job) before the old base node passed to
    bdrv_set_backing_hd() is installed.
    
    This has two implications:
    
    First, the stream job does not keep a strong reference to the base node.
    Therefore, if it is deleted in bdrv_set_backing_hd()'s drain (e.g.
    because some other block job is drained to finish), we will get a
    use-after-free.  We should keep a strong reference to that node.
    
    Second, even with such a strong reference, the problem remains that the
    base node might change before bdrv_set_backing_hd() actually runs and as
    a result the wrong base node is installed.
    
    Both effects can be seen in 030's TestParallelOps.test_overlapping_5()
    case, which has five nodes, and simultaneously streams from the middle
    node to the top node, and commits the middle node down to the base node.
    As it is, this will sometimes crash, namely when we encounter the
    above-described use-after-free.
    
    Taking a strong reference to the base node, we no longer get a crash,
    but the resuling block graph is less than ideal: The expected result is
    obviously that all middle nodes are cut out and the base node is the
    immediate backing child of the top node.  However, if stream_prepare()
    takes a strong reference to its base node (the middle node), and then
    the commit job finishes in bdrv_set_backing_hd(), supposedly dropping
    that middle node, the stream job will just reinstall it again.
    
    Therefore, we need to keep the whole subtree drained in
    stream_prepare(), so that the graph modification it performs is
    effectively atomic, i.e. that the base node it fetches is still the base
    node when bdrv_set_backing_hd() sets it as the top node's backing node.
    
    Verify this by asserting in said 030's test case that the base node is
    always the top node's immediate backing child when both jobs are done.
    
    Signed-off-by: default avatarHanna Reitz <hreitz@redhat.com>
    Message-Id: <20220324140907.17192-1-hreitz@redhat.com>
    Reviewed-by: default avatarEric Blake <eblake@redhat.com>
    Acked-by: default avatarVladimir Sementsov-Ogievskiy <v.sementsov-og@mail.ru>
Loading