Patchwork [29/35] scsi-disk: remove cluster_size

login
register
mail settings
Submitter Paolo Bonzini
Date Oct. 13, 2011, 11:03 a.m.
Message ID <1318503845-11473-30-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/119464/
State New
Headers show

Comments

Paolo Bonzini - Oct. 13, 2011, 11:03 a.m.
This field is redundant, and its presence makes it more complicated
to share reqops between the upcoming scsi-block and scsi-generic.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c |   45 ++++++++++++++++++++++-----------------------
 1 files changed, 22 insertions(+), 23 deletions(-)
Kevin Wolf - Oct. 24, 2011, 3:10 p.m.
Am 13.10.2011 13:03, schrieb Paolo Bonzini:
> This field is redundant, and its presence makes it more complicated
> to share reqops between the upcoming scsi-block and scsi-generic.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/scsi-disk.c |   45 ++++++++++++++++++++++-----------------------
>  1 files changed, 22 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index 5e3ef51..7f2f67f 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -65,9 +65,6 @@ typedef struct SCSIDiskReq {
>  struct SCSIDiskState
>  {
>      SCSIDevice qdev;
> -    /* The qemu block layer uses a fixed 512 byte sector size.
> -       This is the number of 512 byte blocks in a single scsi sector.  */
> -    int cluster_size;
>      uint32_t removable;
>      uint64_t max_lba;
>      bool media_changed;
> @@ -862,7 +859,7 @@ static int mode_sense_page(SCSIDiskState *s, int page, uint8_t **p_outbuf,
>          bdrv_get_geometry_hint(bdrv, &cylinders, &heads, &secs);
>          p[4] = heads & 0xff;
>          p[5] = secs & 0xff;
> -        p[6] = s->cluster_size * 2;
> +        p[6] = s->qdev.blocksize >> 8;
>          p[8] = (cylinders >> 8) & 0xff;
>          p[9] = cylinders & 0xff;
>          /* Write precomp start cylinder, disabled */
> @@ -991,7 +988,7 @@ static int scsi_disk_emulate_mode_sense(SCSIDiskReq *r, uint8_t *outbuf)
>          } else { /* MODE_SENSE_10 */
>              outbuf[7] = 8; /* Block descriptor length  */
>          }
> -        nb_sectors /= s->cluster_size;
> +        nb_sectors /= (s->qdev.blocksize / 512);
>          if (nb_sectors > 0xffffff)
>              nb_sectors = 0;
>          p[0] = 0; /* media density code */
> @@ -1000,7 +997,7 @@ static int scsi_disk_emulate_mode_sense(SCSIDiskReq *r, uint8_t *outbuf)
>          p[3] = nb_sectors & 0xff;
>          p[4] = 0; /* reserved */
>          p[5] = 0; /* bytes 5-7 are the sector size in bytes */
> -        p[6] = s->cluster_size * 2;
> +        p[6] = s->qdev.blocksize >> 8;
>          p[7] = 0;
>          p += 8;
>      }
> @@ -1050,7 +1047,7 @@ static int scsi_disk_emulate_read_toc(SCSIRequest *req, uint8_t *outbuf)
>      start_track = req->cmd.buf[6];
>      bdrv_get_geometry(s->qdev.conf.bs, &nb_sectors);
>      DPRINTF("Read TOC (track %d format %d msf %d)\n", start_track, format, msf >> 1);
> -    nb_sectors /= s->cluster_size;
> +    nb_sectors /= s->qdev.blocksize / 512;
>      switch (format) {
>      case 0:
>          toclen = cdrom_read_toc(nb_sectors, outbuf, msf, start_track);
> @@ -1176,7 +1173,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r)
>          if ((req->cmd.buf[8] & 1) == 0 && req->cmd.lba) {
>              goto illegal_request;
>          }
> -        nb_sectors /= s->cluster_size;
> +        nb_sectors /= s->qdev.blocksize / 512;
>          /* Returned value is the address of the last sector.  */
>          nb_sectors--;
>          /* Remember the new size for read/write sanity checking. */
> @@ -1190,7 +1187,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r)
>          outbuf[3] = nb_sectors & 0xff;
>          outbuf[4] = 0;
>          outbuf[5] = 0;
> -        outbuf[6] = s->cluster_size * 2;
> +        outbuf[6] = s->qdev.blocksize >> 8;
>          outbuf[7] = 0;
>          buflen = 8;
>          break;
> @@ -1226,7 +1223,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r)
>              if ((req->cmd.buf[14] & 1) == 0 && req->cmd.lba) {
>                  goto illegal_request;
>              }
> -            nb_sectors /= s->cluster_size;
> +            nb_sectors /= s->qdev.blocksize / 512;
>              /* Returned value is the address of the last sector.  */
>              nb_sectors--;
>              /* Remember the new size for read/write sanity checking. */
> @@ -1241,7 +1238,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r)
>              outbuf[7] = nb_sectors & 0xff;
>              outbuf[8] = 0;
>              outbuf[9] = 0;
> -            outbuf[10] = s->cluster_size * 2;
> +            outbuf[10] = s->qdev.blocksize >> 8;
>              outbuf[11] = 0;
>              outbuf[12] = 0;
>              outbuf[13] = get_physical_block_exp(&s->qdev.conf);
> @@ -1349,8 +1346,8 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
>          DPRINTF("Read (sector %" PRId64 ", count %d)\n", r->req.cmd.lba, len);
>          if (r->req.cmd.lba > s->max_lba)
>              goto illegal_lba;
> -        r->sector = r->req.cmd.lba * s->cluster_size;
> -        r->sector_count = len * s->cluster_size;
> +        r->sector = r->req.cmd.lba * (s->qdev.blocksize / 512);
> +        r->sector_count = len * (s->qdev.blocksize / 512);
>          break;
>      case WRITE_6:
>      case WRITE_10:
> @@ -1365,8 +1362,8 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
>                  r->req.cmd.lba, len);
>          if (r->req.cmd.lba > s->max_lba)
>              goto illegal_lba;
> -        r->sector = r->req.cmd.lba * s->cluster_size;
> -        r->sector_count = len * s->cluster_size;
> +        r->sector = r->req.cmd.lba * (s->qdev.blocksize / 512);
> +        r->sector_count = len * (s->qdev.blocksize / 512);
>          break;
>      case MODE_SELECT:
>          DPRINTF("Mode Select(6) (len %lu)\n", (long)r->req.cmd.xfer);
> @@ -1409,8 +1406,9 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
>              goto fail;
>          }
>  
> -        rc = bdrv_discard(s->qdev.conf.bs, r->req.cmd.lba * s->cluster_size,
> -                          len * s->cluster_size);
> +        rc = bdrv_discard(s->qdev.conf.bs,
> +                          r->req.cmd.lba * (s->qdev.blocksize / 512),
> +                          len * (s->qdev.blocksize / 512));
>          if (rc < 0) {
>              /* XXX: better error code ?*/
>              goto fail;
> @@ -1450,12 +1448,14 @@ static void scsi_disk_reset(DeviceState *dev)
>  
>      scsi_device_purge_requests(&s->qdev, SENSE_CODE(RESET));
>  
> -    bdrv_get_geometry(s->qdev.conf.bs, &nb_sectors);
> -    nb_sectors /= s->cluster_size;
> -    if (nb_sectors) {
> -        nb_sectors--;
> +    if (s->qdev.blocksize) {

When would it be 0? And wouldn't we crash with a zero blocksize anyway?

Kevin
Paolo Bonzini - Oct. 24, 2011, 3:36 p.m.
On 10/24/2011 05:10 PM, Kevin Wolf wrote:
>> >  -    bdrv_get_geometry(s->qdev.conf.bs,&nb_sectors);
>> >  -    nb_sectors /= s->cluster_size;
>> >  -    if (nb_sectors) {
>> >  -        nb_sectors--;
>> >  +    if (s->qdev.blocksize) {
> When would it be 0? And wouldn't we crash with a zero blocksize anyway?

blocksize can be zero when passing through a removable medium and no 
medium has ever been inserted in the disk since the guest was started. 
In practice it won't crash because the guest will always send READ 
CAPACITY first, will see a unit attention condition, and will not 
attempt a read.

A more complete solution involves asking raw-posix for the logical block 
size (right now logical_block_size acts as both the emulated and host 
block size).  This would also be useful to make cache=none work with 
4k-sector disks without manually specifying logical_block_size. 
However, it's not 1.0 material.

Paolo

Patch

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 5e3ef51..7f2f67f 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -65,9 +65,6 @@  typedef struct SCSIDiskReq {
 struct SCSIDiskState
 {
     SCSIDevice qdev;
-    /* The qemu block layer uses a fixed 512 byte sector size.
-       This is the number of 512 byte blocks in a single scsi sector.  */
-    int cluster_size;
     uint32_t removable;
     uint64_t max_lba;
     bool media_changed;
@@ -862,7 +859,7 @@  static int mode_sense_page(SCSIDiskState *s, int page, uint8_t **p_outbuf,
         bdrv_get_geometry_hint(bdrv, &cylinders, &heads, &secs);
         p[4] = heads & 0xff;
         p[5] = secs & 0xff;
-        p[6] = s->cluster_size * 2;
+        p[6] = s->qdev.blocksize >> 8;
         p[8] = (cylinders >> 8) & 0xff;
         p[9] = cylinders & 0xff;
         /* Write precomp start cylinder, disabled */
@@ -991,7 +988,7 @@  static int scsi_disk_emulate_mode_sense(SCSIDiskReq *r, uint8_t *outbuf)
         } else { /* MODE_SENSE_10 */
             outbuf[7] = 8; /* Block descriptor length  */
         }
-        nb_sectors /= s->cluster_size;
+        nb_sectors /= (s->qdev.blocksize / 512);
         if (nb_sectors > 0xffffff)
             nb_sectors = 0;
         p[0] = 0; /* media density code */
@@ -1000,7 +997,7 @@  static int scsi_disk_emulate_mode_sense(SCSIDiskReq *r, uint8_t *outbuf)
         p[3] = nb_sectors & 0xff;
         p[4] = 0; /* reserved */
         p[5] = 0; /* bytes 5-7 are the sector size in bytes */
-        p[6] = s->cluster_size * 2;
+        p[6] = s->qdev.blocksize >> 8;
         p[7] = 0;
         p += 8;
     }
@@ -1050,7 +1047,7 @@  static int scsi_disk_emulate_read_toc(SCSIRequest *req, uint8_t *outbuf)
     start_track = req->cmd.buf[6];
     bdrv_get_geometry(s->qdev.conf.bs, &nb_sectors);
     DPRINTF("Read TOC (track %d format %d msf %d)\n", start_track, format, msf >> 1);
-    nb_sectors /= s->cluster_size;
+    nb_sectors /= s->qdev.blocksize / 512;
     switch (format) {
     case 0:
         toclen = cdrom_read_toc(nb_sectors, outbuf, msf, start_track);
@@ -1176,7 +1173,7 @@  static int scsi_disk_emulate_command(SCSIDiskReq *r)
         if ((req->cmd.buf[8] & 1) == 0 && req->cmd.lba) {
             goto illegal_request;
         }
-        nb_sectors /= s->cluster_size;
+        nb_sectors /= s->qdev.blocksize / 512;
         /* Returned value is the address of the last sector.  */
         nb_sectors--;
         /* Remember the new size for read/write sanity checking. */
@@ -1190,7 +1187,7 @@  static int scsi_disk_emulate_command(SCSIDiskReq *r)
         outbuf[3] = nb_sectors & 0xff;
         outbuf[4] = 0;
         outbuf[5] = 0;
-        outbuf[6] = s->cluster_size * 2;
+        outbuf[6] = s->qdev.blocksize >> 8;
         outbuf[7] = 0;
         buflen = 8;
         break;
@@ -1226,7 +1223,7 @@  static int scsi_disk_emulate_command(SCSIDiskReq *r)
             if ((req->cmd.buf[14] & 1) == 0 && req->cmd.lba) {
                 goto illegal_request;
             }
-            nb_sectors /= s->cluster_size;
+            nb_sectors /= s->qdev.blocksize / 512;
             /* Returned value is the address of the last sector.  */
             nb_sectors--;
             /* Remember the new size for read/write sanity checking. */
@@ -1241,7 +1238,7 @@  static int scsi_disk_emulate_command(SCSIDiskReq *r)
             outbuf[7] = nb_sectors & 0xff;
             outbuf[8] = 0;
             outbuf[9] = 0;
-            outbuf[10] = s->cluster_size * 2;
+            outbuf[10] = s->qdev.blocksize >> 8;
             outbuf[11] = 0;
             outbuf[12] = 0;
             outbuf[13] = get_physical_block_exp(&s->qdev.conf);
@@ -1349,8 +1346,8 @@  static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
         DPRINTF("Read (sector %" PRId64 ", count %d)\n", r->req.cmd.lba, len);
         if (r->req.cmd.lba > s->max_lba)
             goto illegal_lba;
-        r->sector = r->req.cmd.lba * s->cluster_size;
-        r->sector_count = len * s->cluster_size;
+        r->sector = r->req.cmd.lba * (s->qdev.blocksize / 512);
+        r->sector_count = len * (s->qdev.blocksize / 512);
         break;
     case WRITE_6:
     case WRITE_10:
@@ -1365,8 +1362,8 @@  static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
                 r->req.cmd.lba, len);
         if (r->req.cmd.lba > s->max_lba)
             goto illegal_lba;
-        r->sector = r->req.cmd.lba * s->cluster_size;
-        r->sector_count = len * s->cluster_size;
+        r->sector = r->req.cmd.lba * (s->qdev.blocksize / 512);
+        r->sector_count = len * (s->qdev.blocksize / 512);
         break;
     case MODE_SELECT:
         DPRINTF("Mode Select(6) (len %lu)\n", (long)r->req.cmd.xfer);
@@ -1409,8 +1406,9 @@  static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
             goto fail;
         }
 
-        rc = bdrv_discard(s->qdev.conf.bs, r->req.cmd.lba * s->cluster_size,
-                          len * s->cluster_size);
+        rc = bdrv_discard(s->qdev.conf.bs,
+                          r->req.cmd.lba * (s->qdev.blocksize / 512),
+                          len * (s->qdev.blocksize / 512));
         if (rc < 0) {
             /* XXX: better error code ?*/
             goto fail;
@@ -1450,12 +1448,14 @@  static void scsi_disk_reset(DeviceState *dev)
 
     scsi_device_purge_requests(&s->qdev, SENSE_CODE(RESET));
 
-    bdrv_get_geometry(s->qdev.conf.bs, &nb_sectors);
-    nb_sectors /= s->cluster_size;
-    if (nb_sectors) {
-        nb_sectors--;
+    if (s->qdev.blocksize) {
+        bdrv_get_geometry(s->qdev.conf.bs, &nb_sectors);
+        nb_sectors /= s->qdev.blocksize / 512;
+        if (nb_sectors) {
+            nb_sectors--;
+        }
+        s->max_lba = nb_sectors;
     }
-    s->max_lba = nb_sectors;
 }
 
 static void scsi_destroy(SCSIDevice *dev)
@@ -1552,7 +1552,6 @@  static int scsi_initfn(SCSIDevice *dev, uint8_t scsi_type)
         error_report("scsi-disk: Unhandled SCSI type %02x", scsi_type);
         return -1;
     }
-    s->cluster_size = s->qdev.blocksize / 512;
     bdrv_set_buffer_alignment(s->qdev.conf.bs, s->qdev.blocksize);
 
     s->qdev.type = scsi_type;