| Message ID | 20230817125020.208339-18-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Graph locking part 4 (node management) | expand |
Am 17/08/2023 um 14:50 schrieb Kevin Wolf: > The function reads the parents list, so it needs to hold the graph lock. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
On Thu, Aug 17, 2023 at 02:50:16PM +0200, Kevin Wolf wrote: > The function reads the parents list, so it needs to hold the graph lock. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/block.c b/block.c > index 7df8780d6e..a82389f742 100644 > --- a/block.c > +++ b/block.c > @@ -5934,9 +5934,11 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, > backing_file_str = base->filename; > } > > + bdrv_graph_rdlock_main_loop(); > QLIST_FOREACH(c, &top->parents, next_parent) { > updated_children = g_slist_prepend(updated_children, c); > } > + bdrv_graph_rdunlock_main_loop(); This is GLOBAL_STATE_CODE, so why take the read lock? I thought nothing can modify the graph here. If it could, then stashing the parents in the updated_children probably wouldn't be safe anyway. > > /* > * It seems correct to pass detach_subchain=true here, but it triggers > -- > 2.41.0 >
Am 22.08.2023 um 21:35 hat Stefan Hajnoczi geschrieben: > On Thu, Aug 17, 2023 at 02:50:16PM +0200, Kevin Wolf wrote: > > The function reads the parents list, so it needs to hold the graph lock. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > block.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/block.c b/block.c > > index 7df8780d6e..a82389f742 100644 > > --- a/block.c > > +++ b/block.c > > @@ -5934,9 +5934,11 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, > > backing_file_str = base->filename; > > } > > > > + bdrv_graph_rdlock_main_loop(); > > QLIST_FOREACH(c, &top->parents, next_parent) { > > updated_children = g_slist_prepend(updated_children, c); > > } > > + bdrv_graph_rdunlock_main_loop(); > > This is GLOBAL_STATE_CODE, so why take the read lock? I thought nothing > can modify the graph here. If it could, then stashing the parents in > the updated_children probably wouldn't be safe anyway. The only thing bdrv_graph_rdlock_main_loop() does is asserting that the conditions for doing nothing are met (GLOBAL_STATE_CODE + non-coroutine context) and providing the right TSA attributes to make the compiler happy. Kevin
diff --git a/block.c b/block.c index 7df8780d6e..a82389f742 100644 --- a/block.c +++ b/block.c @@ -5934,9 +5934,11 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, backing_file_str = base->filename; } + bdrv_graph_rdlock_main_loop(); QLIST_FOREACH(c, &top->parents, next_parent) { updated_children = g_slist_prepend(updated_children, c); } + bdrv_graph_rdunlock_main_loop(); /* * It seems correct to pass detach_subchain=true here, but it triggers
The function reads the parents list, so it needs to hold the graph lock. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 2 ++ 1 file changed, 2 insertions(+)