Re: [Qemu-devel] [PATCH 29/35] scsi-disk: remove cluster_size

[Kevin Wolf]

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 <address@hidden>
> ---
>  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