[v6,11/42] block: Add bdrv_supports_compressed_writes()

Commit Message

Max Reitz Aug. 9, 2019, 4:13 p.m. UTC

Filters cannot compress data themselves but they have to implement
.bdrv_co_pwritev_compressed() still (or they cannot forward compressed
writes).  Therefore, checking whether
bs->drv->bdrv_co_pwritev_compressed is non-NULL is not sufficient to
know whether the node can actually handle compressed writes.  This
function looks down the filter chain to see whether there is a
non-filter that can actually convert the compressed writes into
compressed data (and thus normal writes).

Signed-off-by: Max Reitz <[e-mail]>
Reviewed-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
---
 include/block/block.h |  1 +
 block.c               | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+)

Comments

Kevin Wolf Sept. 5, 2019, 1:11 p.m. UTC | #1

Am 09.08.2019 um 18:13 hat Max Reitz geschrieben:
> Filters cannot compress data themselves but they have to implement
> .bdrv_co_pwritev_compressed() still (or they cannot forward compressed
> writes).  Therefore, checking whether
> bs->drv->bdrv_co_pwritev_compressed is non-NULL is not sufficient to
> know whether the node can actually handle compressed writes.  This
> function looks down the filter chain to see whether there is a
> non-filter that can actually convert the compressed writes into
> compressed data (and thus normal writes).
> 
> Signed-off-by: Max Reitz <[e-mail]>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <[e-mail]>

Should patches 2 and 3 that add the .bdrv_co_pwritev_compressed()
callback to filter drivers come only after this one? And should we also
support it in the mirror filter?

Kevin

Max Reitz Sept. 9, 2019, 8:09 a.m. UTC | #2

On 05.09.19 15:11, Kevin Wolf wrote:
> Am 09.08.2019 um 18:13 hat Max Reitz geschrieben:
>> Filters cannot compress data themselves but they have to implement
>> .bdrv_co_pwritev_compressed() still (or they cannot forward compressed
>> writes).  Therefore, checking whether
>> bs->drv->bdrv_co_pwritev_compressed is non-NULL is not sufficient to
>> know whether the node can actually handle compressed writes.  This
>> function looks down the filter chain to see whether there is a
>> non-filter that can actually convert the compressed writes into
>> compressed data (and thus normal writes).
>>
>> Signed-off-by: Max Reitz <[e-mail]>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> 
> Should patches 2 and 3 that add the .bdrv_co_pwritev_compressed()
> callback to filter drivers come only after this one?

Why not.

> And should we also
> support it in the mirror filter?

Hm.  AFAIU, compressed writes have very limited use.  You can basically
only use them when writing to a new image (where you’d never write
anywhere you’ve already written something to), i.e. for qemu-img convert
or the backup target.  It makes sense to blockdev-backup to throttle, so
that’s why it should be implemented there.  I don’t really see how it
would make sense for mirror.

OTOH, it doesn’t make sense for COR either.  And it isn’t that hard.
Now I don’t have a strong preference for either dropping the COR patch
or adding it to mirror as well...

Max

Patch

diff --git a/include/block/block.h b/include/block/block.h
index 9cfe77abaf..6ba853fb90 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -487,6 +487,7 @@  BlockDriverState *bdrv_next(BdrvNextIterator *it);
 void bdrv_next_cleanup(BdrvNextIterator *it);
 
 BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs);
+bool bdrv_supports_compressed_writes(BlockDriverState *bs);
 void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
                          void *opaque, bool read_only);
 const char *bdrv_get_node_name(const BlockDriverState *bs);
diff --git a/block.c b/block.c
index 415c555bf5..029c809a8e 100644
--- a/block.c
+++ b/block.c
@@ -4696,6 +4696,28 @@  bool bdrv_is_sg(BlockDriverState *bs)
     return bs->sg;
 }
 
+/**
+ * Return whether the given node supports compressed writes.
+ */
+bool bdrv_supports_compressed_writes(BlockDriverState *bs)
+{
+    BlockDriverState *filtered = bdrv_filtered_rw_bs(bs);
+
+    if (!bs->drv || !bs->drv->bdrv_co_pwritev_compressed) {
+        return false;
+    }
+
+    if (filtered) {
+        /*
+         * Filters can only forward compressed writes, so we have to
+         * check the child.
+         */
+        return bdrv_supports_compressed_writes(filtered);
+    }
+
+    return true;
+}
+
 const char *bdrv_get_format_name(BlockDriverState *bs)
 {
     return bs->drv ? bs->drv->format_name : NULL;