Re: [Qemu-devel] [PATCH 2/7] scsi-disk: support WRITE SAME (16) with unmap bit

[Kevin Wolf]

Am 10.12.2010 16:00, schrieb Christoph Hellwig:
> Support discards via the WRITE SAME command with the unmap bit set, and
> tell the initiator about the support for it via the block limit and the
> new thin provisioning EVPD pages.  Also fix the comment which incorrectly
> describedthe block limits EVPD page.
> 
> Signed-off-by: Christoph Hellwig <address@hidden>
> 
> Index: qemu/hw/scsi-disk.c
> ===================================================================
> --- qemu.orig/hw/scsi-disk.c  2010-12-07 09:35:44.203009851 +0100
> +++ qemu/hw/scsi-disk.c       2010-12-07 09:42:07.984255141 +0100
> @@ -424,7 +424,8 @@ static int scsi_disk_emulate_inquiry(SCS
>              outbuf[buflen++] = 0x80; // unit serial number
>              outbuf[buflen++] = 0x83; // device identification
>              if (bdrv_get_type_hint(s->bs) != BDRV_TYPE_CDROM) {
> -                outbuf[buflen++] = 0xb0; // block device characteristics
> +                outbuf[buflen++] = 0xb0; // block limits
> +                outbuf[buflen++] = 0xb2; // thin provisioning
>              }
>              outbuf[pages] = buflen - pages - 1; // number of pages
>              break;
> @@ -466,8 +467,10 @@ static int scsi_disk_emulate_inquiry(SCS
>              buflen += id_len;
>              break;
>          }
> -        case 0xb0: /* block device characteristics */
> +        case 0xb0: /* block limits */
>          {
> +            unsigned int unmap_sectors =
> +                    s->qdev.conf.discard_granularity / s->qdev.blocksize;
>              unsigned int min_io_size =
>                      s->qdev.conf.min_io_size / s->qdev.blocksize;
>              unsigned int opt_io_size =
> @@ -492,6 +495,21 @@ static int scsi_disk_emulate_inquiry(SCS
>              outbuf[13] = (opt_io_size >> 16) & 0xff;
>              outbuf[14] = (opt_io_size >> 8) & 0xff;
>              outbuf[15] = opt_io_size & 0xff;
> +
> +            /* optimal unmap granularity */
> +            outbuf[28] = (unmap_sectors >> 24) & 0xff;
> +            outbuf[29] = (unmap_sectors >> 16) & 0xff;
> +            outbuf[30] = (unmap_sectors >> 8) & 0xff;
> +            outbuf[31] = unmap_sectors & 0xff;
> +            break;
> +        }
> +        case 0xb2: /* thin provisioning */
> +        {
> +            outbuf[3] = buflen = 8;
> +            outbuf[4] = 0;
> +            outbuf[5] = 0x40; /* write same with unmap supported */
> +            outbuf[6] = 0;
> +            outbuf[7] = 0;
>              break;
>          }
>          default:
> @@ -959,6 +977,11 @@ static int scsi_disk_emulate_command(SCS
>              outbuf[11] = 0;
>              outbuf[12] = 0;
>              outbuf[13] = get_physical_block_exp(&s->qdev.conf);
> +
> +            /* set TPE bit if the format supports discard */
> +            if (s->qdev.conf.discard_granularity)
> +                outbuf[14] = 0x80;

Missing braces.

> +
>              /* Protection, exponent and lowest lba field left blank. */
>              buflen = req->cmd.xfer;
>              break;
> @@ -1123,6 +1146,34 @@ static int32_t scsi_send_command(SCSIDev
>              goto illegal_lba;
>          }
>          break;
> +    case WRITE_SAME_16:
> +        len = r->req.cmd.xfer / d->blocksize;
> +
> +        DPRINTF("WRITE SAME(16) (sector %" PRId64 ", count %d)\n",
> +                r->req.cmd.lba, len);
> +
> +        if (r->req.cmd.lba > s->max_lba) {
> +            goto illegal_lba;
> +        }
> +
> +        /*
> +         * We only support WRITE SAME with the unmap bit set for now.
> +         */
> +        if (!(buf[1] & 0x8)) {
> +            goto fail;
> +        }
> +
> +        rc = bdrv_discard(s->bs, r->req.cmd.lba * s->cluster_size,
> +                          len * s->cluster_size);
> +        if (rc < 0) {
> +            /* XXX: better error code ?*/
> +            goto fail;
> +        }
> +
> +        scsi_req_set_status(r, GOOD, NO_SENSE);
> +        scsi_req_complete(&r->req);
> +        scsi_remove_request(r);

Isn't this the same as scsi_command_complete()?

> +        return 0;

And if you break; here instead of returning (like all other commands)
and remove the three lines above completely, I think it would just do
the right thing.

Kevin