Re: [Qemu-devel] [RFC PATCH 06/17] block: use bdrv_{co, aio}_discard for write_zeroes operations

[Richard Laager <rlaager@wiktel.com>]

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

On Wed, 2012-03-14 at 08:41 +0100, Paolo Bonzini wrote:
> Il 13/03/2012 20:13, Richard Laager ha scritto:
> >>> > >       * For SCSI, report an unmap_granularity to the guest as follows:
> >>> > >       max(logical_block_size, discard_granularity) / logical_block_size
> >> > 
> >> > This is more or less already in place later in the series.
> > I didn't see it. Which patch number?
> 
> Patch 11:

I was saying QEMU should pass the discard_granularity to the guest as
OPTIMAL UNMAP GRANULARITY. This would almost surely need to be done in
hw/scsi-disk.c, roughly around this change from your patch 10:

--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1295,8 +1295,11 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r)
             outbuf[13] = get_physical_block_exp(&s->qdev.conf);
 
             /* set TPE bit if the format supports discard */
-            if (s->qdev.conf.discard_granularity) {
+            if (s->qdev.type == TYPE_DISK && s->qdev.conf.discard_granularity) {
                 outbuf[14] = 0x80;
+                if (s->qdev.conf.discard_zeroes_data) {
+                    outbuf[14] |= 0x40;
+                }
             }
 
             /* Protection, exponent and lowest lba field left blank. */

The code from patch 11 is more along the lines of what I think QEMU
should have in the block layer:

> +    } else if (discard_granularity < s->qdev.conf.logical_block_size) {
> +        error_report("scsi-block: invalid discard_granularity");
> +        return -1;
>
> +    } else if (discard_granularity & (discard_granularity - 1)) {
> +        error_report("scsi-block: discard_granularity not a power of two");
> +        return -1;
> +    }

However, I think the first check is unnecessarily restrictive. As long
as discard_granularity is a power of two, if it's less than the block
size (which is also a power of two), the block size will always be a
multiple of discard_granularity, so there's no problem.

I'd also like to explicitly allow discard_granularity = 1, which is what
fallocate() provides.

> It is worse in that we do not want the hardware parameters exposed to the
> guest to change behind the scenes, except if you change the machine type
> or if you use the default unversioned type.

You're saying that discard_granularity and discard_zeros_data need to be
properties of the machine type? If you start with that as a requirement,
I can see why you want to always report discard_granularity=512 &
discard_zeros_data=1. But that design has many downsides. I'm not
convinced that discard_granularity and discard_zeros_data need to be
properties of the machine type. Why do you feel that's necessary? What's
the harm in those properties changing across QEMU startups (i.e. guest
boots)?

-- 
Richard

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]