diff mbox series

[18/21] parallels: naive implementation of parallels_co_pdiscard

Message ID 20230915184130.403366-21-den@openvz.org (mailing list archive)
State New, archived
Headers show
Series implement discard operation for Parallels images | expand

Commit Message

Denis V. Lunev Sept. 15, 2023, 6:41 p.m. UTC
* 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(+)

Comments

Mike Maslenkin Sept. 17, 2023, 11:40 a.m. UTC | #1
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
>
Alexander Ivanov Sept. 18, 2023, 1:57 p.m. UTC | #2
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)
Denis V. Lunev Sept. 18, 2023, 2:05 p.m. UTC | #3
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 mbox series

Patch

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)