| Message ID | 20230915184130.403366-21-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
| Series | implement discard operation for Parallels images | expand |
I got a warning after this patch:
../block/parallels.c:541:25: warning: 'guarded_by' attribute only
applies to non-static data members and global variables
[-Wignored-attributes]
static int coroutine_fn GRAPH_RDLOCK_PTR
^
/Users/mg/sources/qemu/include/block/graph-lock.h:85:26: note:
expanded from macro 'GRAPH_RDLOCK_PTR'
#define GRAPH_RDLOCK_PTR TSA_GUARDED_BY(graph_lock)
^
/Users/mg/sources/qemu/include/qemu/clang-tsa.h:48:31: note: expanded
from macro 'TSA_GUARDED_BY'
#define TSA_GUARDED_BY(x) TSA(guarded_by(x))
Regards,
Mike.
On Fri, Sep 15, 2023 at 9:42 PM Denis V. Lunev <den@openvz.org> wrote:
>
> * Discarding with backing stores is not supported by the format.
> * There is no buffering/queueing of the discard operation.
> * Only operations aligned to the cluster are supported.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
> block/parallels.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 76aedfd7c4..83cb8d6722 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -537,6 +537,52 @@ parallels_co_readv(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
> return ret;
> }
>
> +
> +static int coroutine_fn GRAPH_RDLOCK_PTR
> +parallels_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
> +{
> + int ret = 0;
> + uint32_t cluster, count;
> + BDRVParallelsState *s = bs->opaque;
> +
> + /*
> + * The image does not support ZERO mark inside the BAT, which means that
> + * stale data could be exposed from the backing file.
> + */
> + if (bs->backing) {
> + return -ENOTSUP;
> + }
> +
> + if (!QEMU_IS_ALIGNED(offset, s->cluster_size)) {
> + return -ENOTSUP;
> + } else if (!QEMU_IS_ALIGNED(bytes, s->cluster_size)) {
> + return -ENOTSUP;
> + }
> +
> + cluster = offset / s->cluster_size;
> + count = bytes / s->cluster_size;
> +
> + qemu_co_mutex_lock(&s->lock);
> + for (; count > 0; cluster++, count--) {
> + int64_t host_off = bat2sect(s, cluster) << BDRV_SECTOR_BITS;
> + if (host_off == 0) {
> + continue;
> + }
> +
> + ret = bdrv_co_pdiscard(bs->file, cluster * s->cluster_size,
> + s->cluster_size);
> + if (ret < 0) {
> + goto done;
> + }
> +
> + parallels_set_bat_entry(s, cluster, 0);
> + bitmap_clear(s->used_bmap, host_cluster_index(s, host_off), 1);
> + }
> +done:
> + qemu_co_mutex_unlock(&s->lock);
> + return ret;
> +}
> +
> static void parallels_check_unclean(BlockDriverState *bs,
> BdrvCheckResult *res,
> BdrvCheckMode fix)
> @@ -1409,6 +1455,7 @@ static BlockDriver bdrv_parallels = {
> .bdrv_co_create = parallels_co_create,
> .bdrv_co_create_opts = parallels_co_create_opts,
> .bdrv_co_check = parallels_co_check,
> + .bdrv_co_pdiscard = parallels_co_pdiscard,
> };
>
> static void bdrv_parallels_init(void)
> --
> 2.34.1
>
On 9/15/23 20:41, Denis V. Lunev wrote: > * Discarding with backing stores is not supported by the format. > * There is no buffering/queueing of the discard operation. > * Only operations aligned to the cluster are supported. > > Signed-off-by: Denis V. Lunev <den@openvz.org> > --- > block/parallels.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > > diff --git a/block/parallels.c b/block/parallels.c > index 76aedfd7c4..83cb8d6722 100644 > --- a/block/parallels.c > +++ b/block/parallels.c > @@ -537,6 +537,52 @@ parallels_co_readv(BlockDriverState *bs, int64_t sector_num, int nb_sectors, > return ret; > } > > + > +static int coroutine_fn GRAPH_RDLOCK_PTR > +parallels_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes) > +{ > + int ret = 0; > + uint32_t cluster, count; > + BDRVParallelsState *s = bs->opaque; > + > + /* > + * The image does not support ZERO mark inside the BAT, which means that > + * stale data could be exposed from the backing file. > + */ > + if (bs->backing) { > + return -ENOTSUP; > + } > + > + if (!QEMU_IS_ALIGNED(offset, s->cluster_size)) { > + return -ENOTSUP; > + } else if (!QEMU_IS_ALIGNED(bytes, s->cluster_size)) { > + return -ENOTSUP; > + } > + > + cluster = offset / s->cluster_size; > + count = bytes / s->cluster_size; > + > + qemu_co_mutex_lock(&s->lock); > + for (; count > 0; cluster++, count--) { > + int64_t host_off = bat2sect(s, cluster) << BDRV_SECTOR_BITS; > + if (host_off == 0) { > + continue; > + } > + > + ret = bdrv_co_pdiscard(bs->file, cluster * s->cluster_size, > + s->cluster_size); It seems, bdrv_co_pdiscard() should be called with a host offset, but there is a guest one. > + if (ret < 0) { > + goto done; > + } > + > + parallels_set_bat_entry(s, cluster, 0); > + bitmap_clear(s->used_bmap, host_cluster_index(s, host_off), 1); > + } > +done: > + qemu_co_mutex_unlock(&s->lock); > + return ret; > +} > + > static void parallels_check_unclean(BlockDriverState *bs, > BdrvCheckResult *res, > BdrvCheckMode fix) > @@ -1409,6 +1455,7 @@ static BlockDriver bdrv_parallels = { > .bdrv_co_create = parallels_co_create, > .bdrv_co_create_opts = parallels_co_create_opts, > .bdrv_co_check = parallels_co_check, > + .bdrv_co_pdiscard = parallels_co_pdiscard, > }; > > static void bdrv_parallels_init(void)
On 9/18/23 15:57, Alexander Ivanov wrote: > On 9/15/23 20:41, Denis V. Lunev wrote: >> * Discarding with backing stores is not supported by the format. >> * There is no buffering/queueing of the discard operation. >> * Only operations aligned to the cluster are supported. >> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> --- >> block/parallels.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 47 insertions(+) >> >> diff --git a/block/parallels.c b/block/parallels.c >> index 76aedfd7c4..83cb8d6722 100644 >> --- a/block/parallels.c >> +++ b/block/parallels.c >> @@ -537,6 +537,52 @@ parallels_co_readv(BlockDriverState *bs, int64_t >> sector_num, int nb_sectors, >> return ret; >> } >> + >> +static int coroutine_fn GRAPH_RDLOCK_PTR >> +parallels_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t >> bytes) >> +{ >> + int ret = 0; >> + uint32_t cluster, count; >> + BDRVParallelsState *s = bs->opaque; >> + >> + /* >> + * The image does not support ZERO mark inside the BAT, which >> means that >> + * stale data could be exposed from the backing file. >> + */ >> + if (bs->backing) { >> + return -ENOTSUP; >> + } >> + >> + if (!QEMU_IS_ALIGNED(offset, s->cluster_size)) { >> + return -ENOTSUP; >> + } else if (!QEMU_IS_ALIGNED(bytes, s->cluster_size)) { >> + return -ENOTSUP; >> + } >> + >> + cluster = offset / s->cluster_size; >> + count = bytes / s->cluster_size; >> + >> + qemu_co_mutex_lock(&s->lock); >> + for (; count > 0; cluster++, count--) { >> + int64_t host_off = bat2sect(s, cluster) << BDRV_SECTOR_BITS; >> + if (host_off == 0) { >> + continue; >> + } >> + >> + ret = bdrv_co_pdiscard(bs->file, cluster * s->cluster_size, >> + s->cluster_size); > It seems, bdrv_co_pdiscard() should be called with a host offset, but > there is a guest one. correct. On top of that unit test should be modified to catch this problem. Den
diff --git a/block/parallels.c b/block/parallels.c index 76aedfd7c4..83cb8d6722 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -537,6 +537,52 @@ parallels_co_readv(BlockDriverState *bs, int64_t sector_num, int nb_sectors, return ret; } + +static int coroutine_fn GRAPH_RDLOCK_PTR +parallels_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes) +{ + int ret = 0; + uint32_t cluster, count; + BDRVParallelsState *s = bs->opaque; + + /* + * The image does not support ZERO mark inside the BAT, which means that + * stale data could be exposed from the backing file. + */ + if (bs->backing) { + return -ENOTSUP; + } + + if (!QEMU_IS_ALIGNED(offset, s->cluster_size)) { + return -ENOTSUP; + } else if (!QEMU_IS_ALIGNED(bytes, s->cluster_size)) { + return -ENOTSUP; + } + + cluster = offset / s->cluster_size; + count = bytes / s->cluster_size; + + qemu_co_mutex_lock(&s->lock); + for (; count > 0; cluster++, count--) { + int64_t host_off = bat2sect(s, cluster) << BDRV_SECTOR_BITS; + if (host_off == 0) { + continue; + } + + ret = bdrv_co_pdiscard(bs->file, cluster * s->cluster_size, + s->cluster_size); + if (ret < 0) { + goto done; + } + + parallels_set_bat_entry(s, cluster, 0); + bitmap_clear(s->used_bmap, host_cluster_index(s, host_off), 1); + } +done: + qemu_co_mutex_unlock(&s->lock); + return ret; +} + static void parallels_check_unclean(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix) @@ -1409,6 +1455,7 @@ static BlockDriver bdrv_parallels = { .bdrv_co_create = parallels_co_create, .bdrv_co_create_opts = parallels_co_create_opts, .bdrv_co_check = parallels_co_check, + .bdrv_co_pdiscard = parallels_co_pdiscard, }; static void bdrv_parallels_init(void)
* Discarding with backing stores is not supported by the format. * There is no buffering/queueing of the discard operation. * Only operations aligned to the cluster are supported. Signed-off-by: Denis V. Lunev <den@openvz.org> --- block/parallels.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)