| Message ID | 20230425173158.574203-14-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Graph locking, part 3 (more block drivers) | expand |
On Tue, Apr 25, 2023 at 07:31:51PM +0200, Kevin Wolf wrote: > From: Emanuele Giuseppe Esposito <eesposit@redhat.com> > > This adds GRAPH_RDLOCK annotations to declare that callers of > bdrv_co_get_allocated_file_size() need to hold a reader lock for the > graph. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > include/block/block-io.h | 7 +++++-- > include/block/block_int-common.h | 2 +- > block.c | 4 +++- > block/vmdk.c | 2 +- > 4 files changed, 10 insertions(+), 5 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
On Tue, Apr 25, 2023 at 07:31:51PM +0200, Kevin Wolf wrote: > @@ -5778,6 +5779,7 @@ int64_t coroutine_fn bdrv_co_get_allocated_file_size(BlockDriverState *bs) > { > BlockDriver *drv = bs->drv; > IO_CODE(); > + assert_bdrv_graph_readable(); Is there a need for runtime assertions in functions already checked by TSA? I guess not. Otherwise runtime assertions should have been added in many of the other functions marked GRAPH_RDLOCK in this series.
Am 01.05.2023 um 21:03 hat Stefan Hajnoczi geschrieben: > On Tue, Apr 25, 2023 at 07:31:51PM +0200, Kevin Wolf wrote: > > @@ -5778,6 +5779,7 @@ int64_t coroutine_fn bdrv_co_get_allocated_file_size(BlockDriverState *bs) > > { > > BlockDriver *drv = bs->drv; > > IO_CODE(); > > + assert_bdrv_graph_readable(); > > Is there a need for runtime assertions in functions already checked by > TSA? > > I guess not. Otherwise runtime assertions should have been added in many > of the other functions marked GRAPH_RDLOCK in this series. I guess we're a bit inconsistent with this. Emanuele started adding the assertions everywhere before I added the TSA annotations. Since then, I've tended to leave the assertions from Emanuele's patches (such as this one) around, but didn't add assertions in new patches. The point in favour for still having assertions is that people using gcc won't see TSA failures, but runtime assertions will still work for them. I don't think we should have them in every GRAPH_RDLOCK function, but having them in one central place in each call chain (i.e. the block.c wrappers for BlockDriver callbacks) does make sense to me. So if we stick to this standard, we'd keep this assertion. But if you prefer, I can drop it. I assume that enough developers run with clang that the additional assertion doesn't buy us that much. And I compile with clang anyway when applying patches. Kevin
diff --git a/include/block/block-io.h b/include/block/block-io.h index 5dab88521d..fb2adb31c7 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -84,8 +84,11 @@ int64_t coroutine_mixed_fn bdrv_nb_sectors(BlockDriverState *bs); int64_t coroutine_fn GRAPH_RDLOCK bdrv_co_getlength(BlockDriverState *bs); int64_t co_wrapper_mixed_bdrv_rdlock bdrv_getlength(BlockDriverState *bs); -int64_t coroutine_fn bdrv_co_get_allocated_file_size(BlockDriverState *bs); -int64_t co_wrapper bdrv_get_allocated_file_size(BlockDriverState *bs); +int64_t coroutine_fn GRAPH_RDLOCK +bdrv_co_get_allocated_file_size(BlockDriverState *bs); + +int64_t co_wrapper_bdrv_rdlock +bdrv_get_allocated_file_size(BlockDriverState *bs); BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts, BlockDriverState *in_bs, Error **errp); diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index 6fb28cd8fa..6e0365d8f2 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -685,7 +685,7 @@ struct BlockDriver { int64_t coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_getlength)( BlockDriverState *bs); - int64_t coroutine_fn (*bdrv_co_get_allocated_file_size)( + int64_t coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_get_allocated_file_size)( BlockDriverState *bs); BlockMeasureInfo *(*bdrv_measure)(QemuOpts *opts, BlockDriverState *in_bs, diff --git a/block.c b/block.c index abec940867..3ccb935950 100644 --- a/block.c +++ b/block.c @@ -5750,7 +5750,8 @@ exit: * sums the size of all data-bearing children. (This excludes backing * children.) */ -static int64_t coroutine_fn bdrv_sum_allocated_file_size(BlockDriverState *bs) +static int64_t coroutine_fn GRAPH_RDLOCK +bdrv_sum_allocated_file_size(BlockDriverState *bs) { BdrvChild *child; int64_t child_size, sum = 0; @@ -5778,6 +5779,7 @@ int64_t coroutine_fn bdrv_co_get_allocated_file_size(BlockDriverState *bs) { BlockDriver *drv = bs->drv; IO_CODE(); + assert_bdrv_graph_readable(); if (!drv) { return -ENOMEDIUM; diff --git a/block/vmdk.c b/block/vmdk.c index 11b553ef25..fddbd1c86c 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -2845,7 +2845,7 @@ static void vmdk_close(BlockDriverState *bs) error_free(s->migration_blocker); } -static int64_t coroutine_fn +static int64_t coroutine_fn GRAPH_RDLOCK vmdk_co_get_allocated_file_size(BlockDriverState *bs) { int i;