[Qemu-devel] [RFC 0/3] block: Inquire images’ rotational info

[Qemu-devel] [RFC 0/3] block: Inquire images’ rotational info

[Max Reitz]

Hi,

http://lists.nongnu.org/archive/html/qemu-block/2019-05/msg00569.html
shows that optimizations affect HDDs and SSDs differently.  It would be
nice if we could differentiate between the two and then choose to adjust
our behavior depending on whether a given image is stored on an HDD or
not.

Or maybe it isn’t so nice.  That’s one reason this is an RFC.

The other is that I implemented recognition of the rotational status by
querying sysfs.  That looks stupid, but I didn’t find a better way
(there is a BLKROTATIONAL ioctl, but that only works on device files).

But, hey, if you look through block/file-posix.c, you’ll find that I’m
not the first to query sysfs.  hdev_get_max_segments() does so, too.  So
maybe it isn’t that bad of an idea.


What do you think?  Is the whole idea stupid?  Just the implementation?
Or does it make sense?


Max Reitz (3):
  block: Add ImageRotationalInfo
  file-posix: Inquire rotational status
  qcow2: Evaluate rotational info

 qapi/block-core.json  | 19 ++++++++++-
 block/qcow2.h         |  3 ++
 include/block/block.h |  7 +++++
 block.c               | 20 +++++++++++-
 block/file-posix.c    | 73 +++++++++++++++++++++++++++++++++++++++++++
 block/qapi.c          |  3 ++
 block/qcow2.c         | 34 +++++++++++++++++---
 7 files changed, 153 insertions(+), 6 deletions(-)

-- 
2.21.0

[Qemu-devel] [RFC 1/3] block: Add ImageRotationalInfo

[Max Reitz]

This enum indicates whether a file is stored on a rotating disk or a
solid-state drive.  Drivers report it via the .bdrv_get_info() callback,
and if they do not, the global bdrv_get_info() implementation
automatically takes it from bs->file or bs->backing, if available.

On the QAPI side, we report the value as part of the ImageInfo
structure.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qapi/block-core.json  | 19 ++++++++++++++++++-
 include/block/block.h |  7 +++++++
 block.c               | 20 +++++++++++++++++++-
 block/qapi.c          |  3 +++
 4 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 3e4042be7f..80b9f2a69c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -130,6 +130,20 @@
       'luks': 'QCryptoBlockInfoLUKS'
   } }
 
+##
+# @ImageRotationalInfo:
+#
+# Indicates whether an image is stored on a rotating disk or not.
+#
+# @solid-state: Image is stored on a solid-state drive
+#
+# @rotating:    Image is stored on a rotating disk
+#
+# Since: 4.1
+##
+{ 'enum': 'ImageRotationalInfo',
+  'data': [ 'solid-state', 'rotating' ] }
+
 ##
 # @ImageInfo:
 #
@@ -161,6 +175,9 @@
 #
 # @backing-image: info of the backing image (since 1.6)
 #
+# @rotational: indicates whether the image is stored on a rotating
+#              disk or not (since 4.1)
+#
 # @format-specific: structure supplying additional format-specific
 # information (since 1.7)
 #
@@ -173,7 +190,7 @@
            '*cluster-size': 'int', '*encrypted': 'bool', '*compressed': 'bool',
            '*backing-filename': 'str', '*full-backing-filename': 'str',
            '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'],
-           '*backing-image': 'ImageInfo',
+           '*backing-image': 'ImageInfo', '*rotational': 'ImageRotationalInfo',
            '*format-specific': 'ImageInfoSpecific' } }
 
 ##
diff --git a/include/block/block.h b/include/block/block.h
index 531cf595cf..dc0f8a4671 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -31,6 +31,13 @@ typedef struct BlockDriverInfo {
      * True if this block driver only supports compressed writes
      */
     bool needs_compressed_writes;
+
+    /*
+     * Indicates whether this image file is stored on a rotating disk
+     * or a solid-state drive.
+     */
+    bool has_rotational_info;
+    ImageRotationalInfo rotational_info;
 } BlockDriverInfo;
 
 typedef struct BlockFragInfo {
diff --git a/block.c b/block.c
index 4c3902508d..e6b2d6d23b 100644
--- a/block.c
+++ b/block.c
@@ -4950,6 +4950,8 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
 int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
     BlockDriver *drv = bs->drv;
+    int ret;
+
     /* if bs->drv == NULL, bs is closed, so there's nothing to do here */
     if (!drv) {
         return -ENOMEDIUM;
@@ -4960,8 +4962,24 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
         }
         return -ENOTSUP;
     }
+
     memset(bdi, 0, sizeof(*bdi));
-    return drv->bdrv_get_info(bs, bdi);
+    ret = drv->bdrv_get_info(bs, bdi);
+    if (ret < 0) {
+        return ret;
+    }
+
+    if (!bdi->has_rotational_info && (bs->file || bs->backing)) {
+        BlockDriverState *child = bs->file ? bs->file->bs : bs->backing->bs;
+        BlockDriverInfo child_bdi;
+
+        if (bdrv_get_info(child, &child_bdi) >= 0) {
+            bdi->rotational_info = child_bdi.rotational_info;
+            bdi->has_rotational_info = child_bdi.has_rotational_info;
+        }
+    }
+
+    return 0;
 }
 
 ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs,
diff --git a/block/qapi.c b/block/qapi.c
index 0c13c86f4e..48ebfbdcc1 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -286,6 +286,9 @@ void bdrv_query_image_info(BlockDriverState *bs,
         }
         info->dirty_flag = bdi.is_dirty;
         info->has_dirty_flag = true;
+
+        info->rotational = bdi.rotational_info;
+        info->has_rotational = bdi.has_rotational_info;
     }
     info->format_specific = bdrv_get_specific_info(bs, &err);
     if (err) {
-- 
2.21.0

[Qemu-devel] [RFC 2/3] file-posix: Inquire rotational status

[Max Reitz]

On Linux, we can inquire whether the file is stored on a rotating disk
or on a solid-state drive through the sysfs.  (The BLKROTATIONAL ioctl
only works on device files themselves.)

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/file-posix.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index d018429672..f84179c0dc 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -161,6 +161,9 @@ typedef struct BDRVRawState {
     bool check_cache_dropped;
 
     PRManager *pr_mgr;
+
+    bool has_rotational_info;
+    ImageRotationalInfo rotational_info;
 } BDRVRawState;
 
 typedef struct BDRVRawReopenState {
@@ -378,6 +381,68 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
     }
 }
 
+/**
+ * Tries to inquire whether the file is stored on a rotating disk or a
+ * solid-state drive.
+ */
+static void raw_update_rotational_info(BDRVRawState *s)
+{
+#ifdef CONFIG_LINUX
+    struct stat st;
+    char *part_fname, *rot_fname;
+    char rot_info[3];
+    dev_t dev;
+    int rot_fd;
+    int ret;
+
+    s->has_rotational_info = false;
+
+    if (fstat(s->fd, &st) < 0) {
+        return;
+    }
+
+    if (st.st_rdev) {
+        dev = st.st_rdev;
+    } else {
+        dev = st.st_dev;
+    }
+
+    part_fname = g_strdup_printf("/sys/dev/block/%u:%u/partition",
+                                 major(dev), minor(dev));
+    if (access(part_fname, F_OK) == 0) {
+        rot_fname = g_strdup_printf("/sys/dev/block/%u:%u/../queue/rotational",
+                                    major(dev), minor(dev));
+    } else {
+        rot_fname = g_strdup_printf("/sys/dev/block/%u:%u/queue/rotational",
+                                    major(dev), minor(dev));
+    }
+    g_free(part_fname);
+
+    rot_fd = open(rot_fname, O_RDONLY);
+    g_free(rot_fname);
+    if (rot_fd < 0) {
+        return;
+    }
+
+    ret = read(rot_fd, rot_info, 2);
+    close(rot_fd);
+    if (ret < 2) {
+        return;
+    }
+
+    rot_info[2] = '\0';
+    if (!strcmp(rot_info, "0\n")) {
+        s->rotational_info = IMAGE_ROTATIONAL_INFO_SOLID_STATE;
+        s->has_rotational_info = true;
+    } else if (!strcmp(rot_info, "1\n")) {
+        s->rotational_info = IMAGE_ROTATIONAL_INFO_ROTATING;
+        s->has_rotational_info = true;
+    }
+#else /* CONFIG_LINUX */
+    s->has_rotational_info = false;
+#endif
+}
+
 static void raw_parse_flags(int bdrv_flags, int *open_flags, bool has_writers)
 {
     bool read_write = false;
@@ -652,6 +717,8 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     }
 #endif
 
+    raw_update_rotational_info(s);
+
     bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK;
     ret = 0;
 fail:
@@ -1007,6 +1074,8 @@ static void raw_reopen_commit(BDRVReopenState *state)
     qemu_close(s->fd);
     s->fd = rs->fd;
 
+    raw_update_rotational_info(s);
+
     g_free(state->opaque);
     state->opaque = NULL;
 
@@ -2731,6 +2800,10 @@ static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     BDRVRawState *s = bs->opaque;
 
     bdi->unallocated_blocks_are_zero = s->discard_zeroes;
+
+    bdi->rotational_info = s->rotational_info;
+    bdi->has_rotational_info = s->has_rotational_info;
+
     return 0;
 }
 
-- 
2.21.0

[Qemu-devel] [RFC 3/3] qcow2: Evaluate rotational info

[Max Reitz]

The new handle_alloc_space() function only speeds up the allocation of
new ranges on solid-state drives.  We should skip it if we know that the
file is stored on a rotating disk.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.h |  3 +++
 block/qcow2.c | 34 ++++++++++++++++++++++++++++++----
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index fc1b0d3c1e..5052ab187f 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -359,6 +359,9 @@ typedef struct BDRVQcow2State {
 
     bool metadata_preallocation_checked;
     bool metadata_preallocation;
+
+    /* True if the image is stored on a rotating disk */
+    bool optimize_for_rotating;
 } BDRVQcow2State;
 
 typedef struct Qcow2COWRegion {
diff --git a/block/qcow2.c b/block/qcow2.c
index 14f914117f..b4df6d5085 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1197,6 +1197,22 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
     return ret;
 }
 
+static void qcow2_update_rotational_info(BlockDriverState *bs)
+{
+    BDRVQcow2State *s = bs->opaque;
+    BlockDriverInfo file_bdi;
+
+    s->optimize_for_rotating = false;
+
+    if (bdrv_get_info(bs->file->bs, &file_bdi) < 0) {
+        return;
+    }
+
+    s->optimize_for_rotating =
+        file_bdi.has_rotational_info &&
+        file_bdi.rotational_info == IMAGE_ROTATIONAL_INFO_ROTATING;
+}
+
 /* Called with s->lock held.  */
 static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
                                       int flags, Error **errp)
@@ -1461,6 +1477,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
         }
     }
 
+    qcow2_update_rotational_info(bs);
+
     /* Parse driver-specific options */
     ret = qcow2_update_options(bs, options, flags, errp);
     if (ret < 0) {
@@ -1829,6 +1847,8 @@ static void qcow2_reopen_commit(BDRVReopenState *state)
 {
     qcow2_update_options_commit(state->bs, state->opaque);
     g_free(state->opaque);
+
+    qcow2_update_rotational_info(state->bs);
 }
 
 static void qcow2_reopen_abort(BDRVReopenState *state)
@@ -2297,10 +2317,16 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
             qemu_iovec_add(&hd_qiov, cluster_data, cur_bytes);
         }
 
-        /* Try to efficiently initialize the physical space with zeroes */
-        ret = handle_alloc_space(bs, l2meta);
-        if (ret < 0) {
-            goto out_unlocked;
+        /*
+         * Try to efficiently initialize the physical space with zeroes.
+         * This incurs a performance penalty on rotating disks, so
+         * avoid doing it there.
+         */
+        if (!s->optimize_for_rotating) {
+            ret = handle_alloc_space(bs, l2meta);
+            if (ret < 0) {
+                goto out_unlocked;
+            }
         }
 
         /* If we need to do COW, check if it's possible to merge the
-- 
2.21.0

Re: [Qemu-devel] [RFC 0/3] block: Inquire images’ rotational info

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2161 bytes --]

On 5/24/19 12:28 PM, Max Reitz wrote:
> Hi,
> 
> http://lists.nongnu.org/archive/html/qemu-block/2019-05/msg00569.html
> shows that optimizations affect HDDs and SSDs differently.  It would be
> nice if we could differentiate between the two and then choose to adjust
> our behavior depending on whether a given image is stored on an HDD or
> not.
> 
> Or maybe it isn’t so nice.  That’s one reason this is an RFC.

Not an entirely bad idea. The NBD spec advertises whether an image is
rotational, precisely because it CAN make a difference for optimizing
access patterns. Likewise, commit 3b19f450 added support for ide
emulation to report rotation or non-rotational through to the guest.  So
plumbing it up in more places makes sense.

> 
> The other is that I implemented recognition of the rotational status by
> querying sysfs.  That looks stupid, but I didn’t find a better way
> (there is a BLKROTATIONAL ioctl, but that only works on device files).
> 
> But, hey, if you look through block/file-posix.c, you’ll find that I’m
> not the first to query sysfs.  hdev_get_max_segments() does so, too.  So
> maybe it isn’t that bad of an idea.
> 
> 
> What do you think?  Is the whole idea stupid?  Just the implementation?
> Or does it make sense?
> 

I'm in favor of the idea, whether or not review of the patches points
out tweaks to make before dropping RFC.

> 
> Max Reitz (3):
>   block: Add ImageRotationalInfo
>   file-posix: Inquire rotational status
>   qcow2: Evaluate rotational info
> 
>  qapi/block-core.json  | 19 ++++++++++-
>  block/qcow2.h         |  3 ++
>  include/block/block.h |  7 +++++
>  block.c               | 20 +++++++++++-
>  block/file-posix.c    | 73 +++++++++++++++++++++++++++++++++++++++++++
>  block/qapi.c          |  3 ++
>  block/qcow2.c         | 34 +++++++++++++++++---
>  7 files changed, 153 insertions(+), 6 deletions(-)

Given my comments about NBD, I'd expect a patch to block/nbd{,-client}.c
to expose this as well.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [RFC 1/3] block: Add ImageRotationalInfo

[Alberto Garcia]

On Fri 24 May 2019 07:28:10 PM CEST, Max Reitz <mreitz@redhat.com> wrote:
> +##
> +# @ImageRotationalInfo:
> +#
> +# Indicates whether an image is stored on a rotating disk or not.
> +#
> +# @solid-state: Image is stored on a solid-state drive
> +#
> +# @rotating:    Image is stored on a rotating disk

What happens when you cannot tell? You assume it's solid-state?

Berto

Re: [Qemu-devel] [RFC 1/3] block: Add ImageRotationalInfo

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

On 26.05.19 17:08, Alberto Garcia wrote:
> On Fri 24 May 2019 07:28:10 PM CEST, Max Reitz <mreitz@redhat.com> wrote:
>> +##
>> +# @ImageRotationalInfo:
>> +#
>> +# Indicates whether an image is stored on a rotating disk or not.
>> +#
>> +# @solid-state: Image is stored on a solid-state drive
>> +#
>> +# @rotating:    Image is stored on a rotating disk
> 
> What happens when you cannot tell? You assume it's solid-state?

When *I* cannot tell?  This field is generally optional, so in that case
it just will not be present.

(When Linux cannot tell?  I don’t know :-))

Do you think there should be an explicit value for that?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [RFC 1/3] block: Add ImageRotationalInfo

[Alberto Garcia]

On Mon 27 May 2019 02:16:53 PM CEST, Max Reitz wrote:
> On 26.05.19 17:08, Alberto Garcia wrote:
>> On Fri 24 May 2019 07:28:10 PM CEST, Max Reitz <mreitz@redhat.com> wrote:
>>> +##
>>> +# @ImageRotationalInfo:
>>> +#
>>> +# Indicates whether an image is stored on a rotating disk or not.
>>> +#
>>> +# @solid-state: Image is stored on a solid-state drive
>>> +#
>>> +# @rotating:    Image is stored on a rotating disk
>> 
>> What happens when you cannot tell? You assume it's solid-state?
>
> When *I* cannot tell?  This field is generally optional, so in that case
> it just will not be present.
>
> (When Linux cannot tell?  I don’t know :-))
>
> Do you think there should be an explicit value for that?

I'll try to rephrase:

we have a new optimization that improves performance on SSDs but reduces
performance on HDDs, so this series would detect where an image is
stored in order to enable the faster code path for each case.

What happens if QEMU cannot detect if we have a solid drive or a
rotational drive? (e.g. a remote storage backend). Will it default to
enabling preallocation using write_zeroes()?

Berto

Re: [Qemu-devel] [RFC 1/3] block: Add ImageRotationalInfo

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2046 bytes --]

On 27.05.19 14:37, Alberto Garcia wrote:
> On Mon 27 May 2019 02:16:53 PM CEST, Max Reitz wrote:
>> On 26.05.19 17:08, Alberto Garcia wrote:
>>> On Fri 24 May 2019 07:28:10 PM CEST, Max Reitz <mreitz@redhat.com> wrote:
>>>> +##
>>>> +# @ImageRotationalInfo:
>>>> +#
>>>> +# Indicates whether an image is stored on a rotating disk or not.
>>>> +#
>>>> +# @solid-state: Image is stored on a solid-state drive
>>>> +#
>>>> +# @rotating:    Image is stored on a rotating disk
>>>
>>> What happens when you cannot tell? You assume it's solid-state?
>>
>> When *I* cannot tell?  This field is generally optional, so in that case
>> it just will not be present.
>>
>> (When Linux cannot tell?  I don’t know :-))
>>
>> Do you think there should be an explicit value for that?
> 
> I'll try to rephrase:
> 
> we have a new optimization that improves performance on SSDs but reduces
> performance on HDDs, so this series would detect where an image is
> stored in order to enable the faster code path for each case.
> 
> What happens if QEMU cannot detect if we have a solid drive or a
> rotational drive? (e.g. a remote storage backend). Will it default to
> enabling preallocation using write_zeroes()?

In this series, yes.  That is the default I chose.

We have to make a separate decision for each case.  In the case of
filling newly allocated areas with zeroes, I think the performance gain
for SSDs is more important than the performance loss for HDDs.  That is
what I wrote in my response to Anton’s series.  So I took the series
even without it being able to distinguish both cases at all.
Consequentially, I believe it is reasonable for that to be the default
behavior if we cannot tell.

I think in general optimizing for SSDs should probably be the default.
HDDs are slow anyway, so whoever uses them probably doesn’t care about
performance too much anyway...?  Whereas people using SSDs probably do.
 But as I said, we can and should always make a separate decision for
each case.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [RFC 1/3] block: Add ImageRotationalInfo

[Anton Nefedov]

On 27/5/2019 3:57 PM, Max Reitz wrote:
> On 27.05.19 14:37, Alberto Garcia wrote:
>> On Mon 27 May 2019 02:16:53 PM CEST, Max Reitz wrote:
>>> On 26.05.19 17:08, Alberto Garcia wrote:
>>>> On Fri 24 May 2019 07:28:10 PM CEST, Max Reitz <mreitz@redhat.com> wrote:
>>>>> +##
>>>>> +# @ImageRotationalInfo:
>>>>> +#
>>>>> +# Indicates whether an image is stored on a rotating disk or not.
>>>>> +#
>>>>> +# @solid-state: Image is stored on a solid-state drive
>>>>> +#
>>>>> +# @rotating:    Image is stored on a rotating disk
>>>>
>>>> What happens when you cannot tell? You assume it's solid-state?
>>>
>>> When *I* cannot tell?  This field is generally optional, so in that case
>>> it just will not be present.
>>>
>>> (When Linux cannot tell?  I don’t know :-))
>>>

Linux defaults to rotational == 1 unless the driver sets
QUEUE_FLAG_NONROT.

By the way as far as I can tell, qemu does not report this flag unless
explicitly set in a device property.

     DEFINE_PROP_UINT16("rotation_rate", IDEDrive, dev.rotation_rate, 0),
and
     DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),

>>> Do you think there should be an explicit value for that?
>>
>> I'll try to rephrase:
>>
>> we have a new optimization that improves performance on SSDs but reduces
>> performance on HDDs, so this series would detect where an image is
>> stored in order to enable the faster code path for each case.
>>
>> What happens if QEMU cannot detect if we have a solid drive or a
>> rotational drive? (e.g. a remote storage backend). Will it default to
>> enabling preallocation using write_zeroes()?
> 
> In this series, yes.  That is the default I chose.
> 
> We have to make a separate decision for each case.  In the case of
> filling newly allocated areas with zeroes, I think the performance gain
> for SSDs is more important than the performance loss for HDDs.  That is
> what I wrote in my response to Anton’s series.  So I took the series
> even without it being able to distinguish both cases at all.
> Consequentially, I believe it is reasonable for that to be the default
> behavior if we cannot tell.
> 
> I think in general optimizing for SSDs should probably be the default.
> HDDs are slow anyway, so whoever uses them probably doesn’t care about
> performance too much anyway...?  Whereas people using SSDs probably do.
>   But as I said, we can and should always make a separate decision for
> each case.
> 

Overall it looks good to me but I wonder how do we ensure both variants
are test covered? Need a blockdev option to enforce the mode?

/Anton

Re: [Qemu-devel] [RFC 1/3] block: Add ImageRotationalInfo

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3042 bytes --]

On 27.05.19 15:44, Anton Nefedov wrote:
> On 27/5/2019 3:57 PM, Max Reitz wrote:
>> On 27.05.19 14:37, Alberto Garcia wrote:
>>> On Mon 27 May 2019 02:16:53 PM CEST, Max Reitz wrote:
>>>> On 26.05.19 17:08, Alberto Garcia wrote:
>>>>> On Fri 24 May 2019 07:28:10 PM CEST, Max Reitz <mreitz@redhat.com> wrote:
>>>>>> +##
>>>>>> +# @ImageRotationalInfo:
>>>>>> +#
>>>>>> +# Indicates whether an image is stored on a rotating disk or not.
>>>>>> +#
>>>>>> +# @solid-state: Image is stored on a solid-state drive
>>>>>> +#
>>>>>> +# @rotating:    Image is stored on a rotating disk
>>>>>
>>>>> What happens when you cannot tell? You assume it's solid-state?
>>>>
>>>> When *I* cannot tell?  This field is generally optional, so in that case
>>>> it just will not be present.
>>>>
>>>> (When Linux cannot tell?  I don’t know :-))
>>>>
> 
> Linux defaults to rotational == 1 unless the driver sets
> QUEUE_FLAG_NONROT.
> 
> By the way as far as I can tell, qemu does not report this flag unless
> explicitly set in a device property.
> 
>      DEFINE_PROP_UINT16("rotation_rate", IDEDrive, dev.rotation_rate, 0),
> and
>      DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
> 
>>>> Do you think there should be an explicit value for that?
>>>
>>> I'll try to rephrase:
>>>
>>> we have a new optimization that improves performance on SSDs but reduces
>>> performance on HDDs, so this series would detect where an image is
>>> stored in order to enable the faster code path for each case.
>>>
>>> What happens if QEMU cannot detect if we have a solid drive or a
>>> rotational drive? (e.g. a remote storage backend). Will it default to
>>> enabling preallocation using write_zeroes()?
>>
>> In this series, yes.  That is the default I chose.
>>
>> We have to make a separate decision for each case.  In the case of
>> filling newly allocated areas with zeroes, I think the performance gain
>> for SSDs is more important than the performance loss for HDDs.  That is
>> what I wrote in my response to Anton’s series.  So I took the series
>> even without it being able to distinguish both cases at all.
>> Consequentially, I believe it is reasonable for that to be the default
>> behavior if we cannot tell.
>>
>> I think in general optimizing for SSDs should probably be the default.
>> HDDs are slow anyway, so whoever uses them probably doesn’t care about
>> performance too much anyway...?  Whereas people using SSDs probably do.
>>   But as I said, we can and should always make a separate decision for
>> each case.
>>
> 
> Overall it looks good to me but I wonder how do we ensure both variants
> are test covered? Need a blockdev option to enforce the mode?

That’s a good point.  Yes, file-posix should probably take an option to
override the mode.  Actually, that may be a useful option in general (if
the file is on some file system where we cannot query this information
(like glusterfs), the user may want to manually provide it).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [RFC 1/3] block: Add ImageRotationalInfo

[Alberto Garcia]

On Mon 27 May 2019 03:44:59 PM CEST, Anton Nefedov wrote:
>> I think in general optimizing for SSDs should probably be the
>> default.  HDDs are slow anyway, so whoever uses them probably doesn’t
>> care about performance too much anyway...?  Whereas people using SSDs
>> probably do.  But as I said, we can and should always make a separate
>> decision for each case.
>
> Overall it looks good to me but I wonder how do we ensure both
> variants are test covered? Need a blockdev option to enforce the mode?

Max talked about a 30% slowdown on HDDs so I suppose it makes sense to
have an option to change that even if we optimize for SSDs by default.

Berto

Re: [Qemu-devel] [RFC 1/3] block: Add ImageRotationalInfo

[Kevin Wolf]

Am 24.05.2019 um 19:28 hat Max Reitz geschrieben:
> This enum indicates whether a file is stored on a rotating disk or a
> solid-state drive.  Drivers report it via the .bdrv_get_info() callback,
> and if they do not, the global bdrv_get_info() implementation
> automatically takes it from bs->file or bs->backing, if available.

Good that you wrote "bs->file or bs->backing" explicitly. Otherwise, I
might have missed that it begs one big question: What is the correct
answer for a qcow2 file that has bs->file on an SSD, but bs->backing on
a rotating disk?

I don't think there is a correct answer for the whole device, so maybe
this information shouldn't be per device in BlockDriverInfo, but per
block in bdrv_co_block_status() (optionally determined if the caller
requests it)?

Kevin

Re: [Qemu-devel] [RFC 1/3] block: Add ImageRotationalInfo

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2486 bytes --]

On 30.05.19 00:10, Kevin Wolf wrote:
> Am 24.05.2019 um 19:28 hat Max Reitz geschrieben:
>> This enum indicates whether a file is stored on a rotating disk or a
>> solid-state drive.  Drivers report it via the .bdrv_get_info() callback,
>> and if they do not, the global bdrv_get_info() implementation
>> automatically takes it from bs->file or bs->backing, if available.
> 
> Good that you wrote "bs->file or bs->backing" explicitly. Otherwise, I
> might have missed that it begs one big question: What is the correct
> answer for a qcow2 file that has bs->file on an SSD, but bs->backing on
> a rotating disk?
> 
> I don't think there is a correct answer for the whole device, so maybe
> this information shouldn't be per device in BlockDriverInfo, but per
> block in bdrv_co_block_status() (optionally determined if the caller
> requests it)?

I think that’s taking it a bit too far.  There is no heavy implication
in making the wrong choice here, it’s just a performance problem.
Having to call block_status for every block where we want to know what
to do seems like the opposite of performance optimization to me.  (We
could add a flag to block_status to only query that status, but that
sounds plainly wrong.)

So, in this series I decided that since all writes go to bs->file, that
seemed like what mostly determines the behavior of @bs.  (After my “Deal
with filters” series, that would become a decision of bdrv_storage_bs()
vs. bdrv_filtered_cow_bs().)

(Note that it has to get even funnier with vmdk, if your extents are on
an HDD, but your descriptor file is on an SSD.  I don’t care too much
about vmdk’s performance, though.)

In my v1, I’ll add a per-node @rotational parameter with which the user
can override the status we guessed.  In fact, currently, my commit
message explicitly notes that case:

https://git.xanclic.moe/XanClic/qemu/commit/0834f1ce77b4c27f0c00f1e4fbee099278e530b2

(Point 4)

(from
https://git.xanclic.moe/XanClic/qemu/commits/branch/spinning-rust-next)


Alternatively to making bs->file take precedence over bs->backing, we
could also just set the status to unknown if bs->file and bs->backing
differ in their status.


I think it’s generally better to prefer what bs->file says.  This is an
optimization case, so I think it’s more important to get it right most
of the time (and guess wrong sometimes) than to stop guessing in all
cases where we could be wrong.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [RFC 1/3] block: Add ImageRotationalInfo

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 3077 bytes --]

Am 31.05.2019 um 13:51 hat Max Reitz geschrieben:
> On 30.05.19 00:10, Kevin Wolf wrote:
> > Am 24.05.2019 um 19:28 hat Max Reitz geschrieben:
> >> This enum indicates whether a file is stored on a rotating disk or a
> >> solid-state drive.  Drivers report it via the .bdrv_get_info() callback,
> >> and if they do not, the global bdrv_get_info() implementation
> >> automatically takes it from bs->file or bs->backing, if available.
> > 
> > Good that you wrote "bs->file or bs->backing" explicitly. Otherwise, I
> > might have missed that it begs one big question: What is the correct
> > answer for a qcow2 file that has bs->file on an SSD, but bs->backing on
> > a rotating disk?
> > 
> > I don't think there is a correct answer for the whole device, so maybe
> > this information shouldn't be per device in BlockDriverInfo, but per
> > block in bdrv_co_block_status() (optionally determined if the caller
> > requests it)?
> 
> I think that’s taking it a bit too far.  There is no heavy implication
> in making the wrong choice here, it’s just a performance problem.
> Having to call block_status for every block where we want to know what
> to do seems like the opposite of performance optimization to me.  (We
> could add a flag to block_status to only query that status, but that
> sounds plainly wrong.)
> 
> So, in this series I decided that since all writes go to bs->file, that
> seemed like what mostly determines the behavior of @bs.  (After my “Deal
> with filters” series, that would become a decision of bdrv_storage_bs()
> vs. bdrv_filtered_cow_bs().)

Okay, if we consider the existing qcow2 case as the only case, writes
are what matters. Then we can ignore backing files. (However, it
shouldn't check bs->file, but s->data_file, I think.)

> (Note that it has to get even funnier with vmdk, if your extents are on
> an HDD, but your descriptor file is on an SSD.  I don’t care too much
> about vmdk’s performance, though.)
> 
> In my v1, I’ll add a per-node @rotational parameter with which the user
> can override the status we guessed.  In fact, currently, my commit
> message explicitly notes that case:
> 
> https://git.xanclic.moe/XanClic/qemu/commit/0834f1ce77b4c27f0c00f1e4fbee099278e530b2
> 
> (Point 4)
> 
> (from
> https://git.xanclic.moe/XanClic/qemu/commits/branch/spinning-rust-next)
> 
> 
> Alternatively to making bs->file take precedence over bs->backing, we
> could also just set the status to unknown if bs->file and bs->backing
> differ in their status.
> 
> I think it’s generally better to prefer what bs->file says.  This is an
> optimization case, so I think it’s more important to get it right most
> of the time (and guess wrong sometimes) than to stop guessing in all
> cases where we could be wrong.

Fair enough, but let's improve the documentation (both QAPI schema and
comments in the code) to be explicit about these details, how the result
is determined for nodes with multiple children, and what it means and
doesn't mean therefore.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [RFC 0/3] block: Inquire images’ rotational info

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 667 bytes --]

On Fri, May 24, 2019 at 07:28:09PM +0200, Max Reitz wrote:
> Hi,
> 
> http://lists.nongnu.org/archive/html/qemu-block/2019-05/msg00569.html
> shows that optimizations affect HDDs and SSDs differently.  It would be
> nice if we could differentiate between the two and then choose to adjust
> our behavior depending on whether a given image is stored on an HDD or
> not.
> 
> Or maybe it isn’t so nice.  That’s one reason this is an RFC.

Seems useful.

As long as this isn't exposed to the guest automatically (e.g. via SCSI)
then it's fine since guest-visible values are not allowed to change
across live migration or storage migration.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [RFC 0/3] block: Inquire images’ rotational info

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 801 bytes --]

On 13.06.19 18:12, Stefan Hajnoczi wrote:
> On Fri, May 24, 2019 at 07:28:09PM +0200, Max Reitz wrote:
>> Hi,
>>
>> http://lists.nongnu.org/archive/html/qemu-block/2019-05/msg00569.html
>> shows that optimizations affect HDDs and SSDs differently.  It would be
>> nice if we could differentiate between the two and then choose to adjust
>> our behavior depending on whether a given image is stored on an HDD or
>> not.
>>
>> Or maybe it isn’t so nice.  That’s one reason this is an RFC.
> 
> Seems useful.
> 
> As long as this isn't exposed to the guest automatically (e.g. via SCSI)
> then it's fine since guest-visible values are not allowed to change
> across live migration or storage migration.

Urgh.  I wanted to do that in v1.  Then I guess I better won’t...

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]