| Message ID | 20230425173158.574203-2-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:39PM +0200, Kevin Wolf wrote: > There is a bdrv_co_getlength() now, which should be used in coroutine > context. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/qcow2.h | 4 +++- > block/qcow2-refcount.c | 2 +- > block/qcow2.c | 19 +++++++++---------- > 3 files changed, 13 insertions(+), 12 deletions(-) > > diff --git a/block/qcow2.h b/block/qcow2.h > index c75decc38a..4f67eb912a 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -895,7 +895,9 @@ int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order, > void *cb_opaque, Error **errp); > int coroutine_fn GRAPH_RDLOCK qcow2_shrink_reftable(BlockDriverState *bs); > int64_t coroutine_fn qcow2_get_last_cluster(BlockDriverState *bs, int64_t size); > -int coroutine_fn qcow2_detect_metadata_preallocation(BlockDriverState *bs); > + > +int coroutine_fn GRAPH_RDLOCK > +qcow2_detect_metadata_preallocation(BlockDriverState *bs); > +++ b/block/qcow2.c > @@ -2089,11 +2089,10 @@ static void qcow2_join_options(QDict *options, QDict *old_options) > } > } > > -static int coroutine_fn qcow2_co_block_status(BlockDriverState *bs, > - bool want_zero, > - int64_t offset, int64_t count, > - int64_t *pnum, int64_t *map, > - BlockDriverState **file) > +static int coroutine_fn GRAPH_RDLOCK > +qcow2_co_block_status(BlockDriverState *bs, bool want_zero, int64_t offset, > + int64_t count, int64_t *pnum, int64_t *map, > + BlockDriverState **file) > { Should the commit message also call out that you are using this opportunity to add GRAPH_RDLOCK annotations on affected functions? Overall, all changes in this patch make sense, but I'm reluctant to add R-b unless you confirm whether this was a rebase mistake (where the annotations were intended to be added in a later patch).
Am 25.04.2023 um 20:37 hat Eric Blake geschrieben: > On Tue, Apr 25, 2023 at 07:31:39PM +0200, Kevin Wolf wrote: > > There is a bdrv_co_getlength() now, which should be used in coroutine > > context. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > block/qcow2.h | 4 +++- > > block/qcow2-refcount.c | 2 +- > > block/qcow2.c | 19 +++++++++---------- > > 3 files changed, 13 insertions(+), 12 deletions(-) > > > > > diff --git a/block/qcow2.h b/block/qcow2.h > > index c75decc38a..4f67eb912a 100644 > > --- a/block/qcow2.h > > +++ b/block/qcow2.h > > @@ -895,7 +895,9 @@ int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order, > > void *cb_opaque, Error **errp); > > int coroutine_fn GRAPH_RDLOCK qcow2_shrink_reftable(BlockDriverState *bs); > > int64_t coroutine_fn qcow2_get_last_cluster(BlockDriverState *bs, int64_t size); > > -int coroutine_fn qcow2_detect_metadata_preallocation(BlockDriverState *bs); > > + > > +int coroutine_fn GRAPH_RDLOCK > > +qcow2_detect_metadata_preallocation(BlockDriverState *bs); > > > +++ b/block/qcow2.c > > @@ -2089,11 +2089,10 @@ static void qcow2_join_options(QDict *options, QDict *old_options) > > } > > } > > > > -static int coroutine_fn qcow2_co_block_status(BlockDriverState *bs, > > - bool want_zero, > > - int64_t offset, int64_t count, > > - int64_t *pnum, int64_t *map, > > - BlockDriverState **file) > > +static int coroutine_fn GRAPH_RDLOCK > > +qcow2_co_block_status(BlockDriverState *bs, bool want_zero, int64_t offset, > > + int64_t count, int64_t *pnum, int64_t *map, > > + BlockDriverState **file) > > { > > Should the commit message also call out that you are using this > opportunity to add GRAPH_RDLOCK annotations on affected functions? This is not just some additional change I did on the side, but the patch doesn't compile (on clang with TSA enabled) without it because bdrv_co_getlength() is GRAPH_RDLOCK, so its callers already must hold the lock. I can be more explicit about it in this patch, though I expect that the same situation might happen more often in the future, and I'm not sure if we want to mention that in the commit message any more than why we're passing through an Error pointer. > Overall, all changes in this patch make sense, but I'm reluctant to > add R-b unless you confirm whether this was a rebase mistake (where > the annotations were intended to be added in a later patch). No, it's intentional. Kevin
On Thu, Apr 27, 2023 at 01:12:43PM +0200, Kevin Wolf wrote: > Am 25.04.2023 um 20:37 hat Eric Blake geschrieben: > > On Tue, Apr 25, 2023 at 07:31:39PM +0200, Kevin Wolf wrote: > > > There is a bdrv_co_getlength() now, which should be used in coroutine > > > context. > > > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > > --- > > > > > > -static int coroutine_fn qcow2_co_block_status(BlockDriverState *bs, > > > - bool want_zero, > > > - int64_t offset, int64_t count, > > > - int64_t *pnum, int64_t *map, > > > - BlockDriverState **file) > > > +static int coroutine_fn GRAPH_RDLOCK > > > +qcow2_co_block_status(BlockDriverState *bs, bool want_zero, int64_t offset, > > > + int64_t count, int64_t *pnum, int64_t *map, > > > + BlockDriverState **file) > > > { > > > > Should the commit message also call out that you are using this > > opportunity to add GRAPH_RDLOCK annotations on affected functions? > > This is not just some additional change I did on the side, but the patch > doesn't compile (on clang with TSA enabled) without it because > bdrv_co_getlength() is GRAPH_RDLOCK, so its callers already must hold > the lock. Okay, that makes sense. > > I can be more explicit about it in this patch, though I expect that the > same situation might happen more often in the future, and I'm not sure > if we want to mention that in the commit message any more than why we're > passing through an Error pointer. Still, a commit message something like: There is a bdrv_co_getlength() now, which should be used in coroutine context, which in turn requires more accurate function annotations. would have helped me review faster. > > > Overall, all changes in this patch make sense, but I'm reluctant to > > add R-b unless you confirm whether this was a rebase mistake (where > > the annotations were intended to be added in a later patch). > > No, it's intentional. All right, you've answered my question. Reviewed-by: Eric Blake <eblake@redhat.com>
On Tue, Apr 25, 2023 at 07:31:39PM +0200, Kevin Wolf wrote: > There is a bdrv_co_getlength() now, which should be used in coroutine > context. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/qcow2.h | 4 +++- > block/qcow2-refcount.c | 2 +- > block/qcow2.c | 19 +++++++++---------- > 3 files changed, 13 insertions(+), 12 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff --git a/block/qcow2.h b/block/qcow2.h index c75decc38a..4f67eb912a 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -895,7 +895,9 @@ int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order, void *cb_opaque, Error **errp); int coroutine_fn GRAPH_RDLOCK qcow2_shrink_reftable(BlockDriverState *bs); int64_t coroutine_fn qcow2_get_last_cluster(BlockDriverState *bs, int64_t size); -int coroutine_fn qcow2_detect_metadata_preallocation(BlockDriverState *bs); + +int coroutine_fn GRAPH_RDLOCK +qcow2_detect_metadata_preallocation(BlockDriverState *bs); /* qcow2-cluster.c functions */ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index b2a81ff707..4cf91bd955 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -3715,7 +3715,7 @@ int coroutine_fn qcow2_detect_metadata_preallocation(BlockDriverState *bs) qemu_co_mutex_assert_locked(&s->lock); - file_length = bdrv_getlength(bs->file->bs); + file_length = bdrv_co_getlength(bs->file->bs); if (file_length < 0) { return file_length; } diff --git a/block/qcow2.c b/block/qcow2.c index fe5def438e..94cf59af8b 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2089,11 +2089,10 @@ static void qcow2_join_options(QDict *options, QDict *old_options) } } -static int coroutine_fn qcow2_co_block_status(BlockDriverState *bs, - bool want_zero, - int64_t offset, int64_t count, - int64_t *pnum, int64_t *map, - BlockDriverState **file) +static int coroutine_fn GRAPH_RDLOCK +qcow2_co_block_status(BlockDriverState *bs, bool want_zero, int64_t offset, + int64_t count, int64_t *pnum, int64_t *map, + BlockDriverState **file) { BDRVQcow2State *s = bs->opaque; uint64_t host_offset; @@ -3235,7 +3234,7 @@ preallocate_co(BlockDriverState *bs, uint64_t offset, uint64_t new_length, * all of the allocated clusters (otherwise we get failing reads after * EOF). Extend the image to the last allocated sector. */ - file_length = bdrv_getlength(s->data_file->bs); + file_length = bdrv_co_getlength(s->data_file->bs); if (file_length < 0) { error_setg_errno(errp, -file_length, "Could not get file size"); ret = file_length; @@ -4098,7 +4097,7 @@ qcow2_co_copy_range_from(BlockDriverState *bs, case QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN: case QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC: if (bs->backing && bs->backing->bs) { - int64_t backing_length = bdrv_getlength(bs->backing->bs); + int64_t backing_length = bdrv_co_getlength(bs->backing->bs); if (src_offset >= backing_length) { cur_write_flags |= BDRV_REQ_ZERO_WRITE; } else { @@ -4293,7 +4292,7 @@ qcow2_co_truncate(BlockDriverState *bs, int64_t offset, bool exact, goto fail; } - old_file_size = bdrv_getlength(bs->file->bs); + old_file_size = bdrv_co_getlength(bs->file->bs); if (old_file_size < 0) { error_setg_errno(errp, -old_file_size, "Failed to inquire current file length"); @@ -4386,7 +4385,7 @@ qcow2_co_truncate(BlockDriverState *bs, int64_t offset, bool exact, break; } - old_file_size = bdrv_getlength(bs->file->bs); + old_file_size = bdrv_co_getlength(bs->file->bs); if (old_file_size < 0) { error_setg_errno(errp, -old_file_size, "Failed to inquire current file length"); @@ -4694,7 +4693,7 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs, * align end of file to a sector boundary to ease reading with * sector based I/Os */ - int64_t len = bdrv_getlength(bs->file->bs); + int64_t len = bdrv_co_getlength(bs->file->bs); if (len < 0) { return len; }
There is a bdrv_co_getlength() now, which should be used in coroutine context. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2.h | 4 +++- block/qcow2-refcount.c | 2 +- block/qcow2.c | 19 +++++++++---------- 3 files changed, 13 insertions(+), 12 deletions(-)