Re: [Qemu-devel] [PATCH] block: add logical_block_size property

[Kevin Wolf]

Am 04.03.2010 14:20, schrieb Christoph Hellwig:
> 
> Add a logical block size attribute as various guest side tools only
> increase the filesystem sector size based on it, not the advisory
> physical block size.
> 
> For scsi we already have support for a different logical block size
> in place for CDROMs that we can built upon.  Only my recent block
> device characteristics VPD page needs some fixups.  Note that we
> leave the logial block size for CDROMs hardcoded as the 2k value
> is expected for it in general.
> 
> For virtio-blk we already have a feature flag claiming to support
> a variable logical block size that was added for the s390 kuli
> hypervisor.  Interestingly it does not actually change the units
> in which the protocol works, which is still fixed at 512 bytes,
> but only communicates a different minimum I/O granularity.  So
> all we need to do in virtio is to add a trap for unaligned I/O
> and round down the device size to the next multiple of the logical
> block size.
> 
> IDE does not support any other logical block size than 512 bytes.
> 
> Signed-off-by: Christoph Hellwig <address@hidden>
> 
> Index: qemu/block_int.h
> ===================================================================
> --- qemu.orig/block_int.h     2010-03-03 19:16:13.408253228 +0100
> +++ qemu/block_int.h  2010-03-03 19:16:43.030003751 +0100
> @@ -209,6 +209,7 @@ struct DriveInfo;
>  typedef struct BlockConf {
>      struct DriveInfo *dinfo;
>      uint16_t physical_block_size;
> +    uint16_t logical_block_size;
>      uint16_t min_io_size;
>      uint32_t opt_io_size;
>  } BlockConf;
> @@ -226,6 +227,8 @@ static inline unsigned int get_physical_
>  
>  #define DEFINE_BLOCK_PROPERTIES(_state, _conf)                          \
>      DEFINE_PROP_DRIVE("drive", _state, _conf.dinfo),                    \
> +    DEFINE_PROP_UINT16("logical_block_size", _state,                    \
> +                       _conf.logical_block_size, 512),                  \
>      DEFINE_PROP_UINT16("physical_block_size", _state,                   \
>                         _conf.physical_block_size, 512),                 \
>      DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 512),  \
> Index: qemu/hw/scsi-disk.c
> ===================================================================
> --- qemu.orig/hw/scsi-disk.c  2010-03-03 19:16:13.419254346 +0100
> +++ qemu/hw/scsi-disk.c       2010-03-03 19:16:43.031004240 +0100
> @@ -397,8 +397,10 @@ static int scsi_disk_emulate_inquiry(SCS
>          }
>          case 0xb0: /* block device characteristics */
>          {
> -            unsigned int min_io_size = s->qdev.conf.min_io_size >> 9;
> -            unsigned int opt_io_size = s->qdev.conf.opt_io_size >> 9;
> +            unsigned int min_io_size =
> +                    s->qdev.conf.min_io_size / s->qdev.blocksize;
> +            unsigned int opt_io_size =
> +                    s->qdev.conf.opt_io_size / s->qdev.blocksize;
>  
>              /* required VPD size with unmap support */
>              outbuf[3] = buflen = 0x3c;
> @@ -1028,11 +1030,12 @@ static int scsi_disk_initfn(SCSIDevice *
>      s->bs = s->qdev.conf.dinfo->bdrv;
>  
>      if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
> -        s->cluster_size = 4;
> +        s->qdev.blocksize = 2048;
>      } else {
> -        s->cluster_size = 1;
> +        s->qdev.blocksize = s->qdev.conf.logical_block_size;
>      }
> -    s->qdev.blocksize = 512 * s->cluster_size;
> +    s->cluster_size = s->qdev.blocksize / 512;
> +
>      s->qdev.type = TYPE_DISK;
>      bdrv_get_geometry(s->bs, &nb_sectors);
>      nb_sectors /= s->cluster_size;
> Index: qemu/hw/virtio-blk.c
> ===================================================================
> --- qemu.orig/hw/virtio-blk.c 2010-03-03 19:16:13.426273971 +0100
> +++ qemu/hw/virtio-blk.c      2010-03-03 19:35:16.636028605 +0100
> @@ -27,6 +27,7 @@ typedef struct VirtIOBlock
>      void *rq;
>      QEMUBH *bh;
>      BlockConf *conf;
> +    unsigned short sector_mask;
>  } VirtIOBlock;
>  
>  static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
> @@ -250,6 +251,11 @@ static void virtio_blk_handle_flush(Virt
>  static void virtio_blk_handle_write(BlockRequest *blkreq, int *num_writes,
>      VirtIOBlockReq *req, BlockDriverState **old_bs)
>  {
> +    if (req->out->sector & req->dev->sector_mask) {
> +        virtio_blk_rw_complete(req, -EIO);
> +        return;
> +    }
> +
>      if (req->dev->bs != *old_bs || *num_writes == 32) {
>          if (*old_bs != NULL) {
>              do_multiwrite(*old_bs, blkreq, *num_writes);
> @@ -272,6 +278,11 @@ static void virtio_blk_handle_read(VirtI
>  {
>      BlockDriverAIOCB *acb;
>  
> +    if (req->out->sector & req->dev->sector_mask) {
> +        virtio_blk_rw_complete(req, -EIO);
> +        return;
> +    }
> +
>      acb = bdrv_aio_readv(req->dev->bs, req->out->sector, &req->qiov,
>                           req->qiov.size / 512, virtio_blk_rw_complete, req);
>      if (!acb) {
> @@ -404,12 +415,13 @@ static void virtio_blk_update_config(Vir
>      stl_raw(&blkcfg.seg_max, 128 - 2);
>      stw_raw(&blkcfg.cylinders, cylinders);
>      blkcfg.heads = heads;
> -    blkcfg.sectors = secs;
> +    blkcfg.sectors = secs & ~s->sector_mask;
> +    blkcfg.blk_size = s->conf->logical_block_size;
>      blkcfg.size_max = 0;
>      blkcfg.physical_block_exp = get_physical_block_exp(s->conf);
>      blkcfg.alignment_offset = 0;
> -    blkcfg.min_io_size = s->conf->min_io_size / 512;
> -    blkcfg.opt_io_size = s->conf->opt_io_size / 512;
> +    blkcfg.min_io_size = s->conf->min_io_size / blkcfg.blk_size;
> +    blkcfg.opt_io_size = s->conf->opt_io_size / blkcfg.blk_size;
>      memcpy(config, &blkcfg, sizeof(struct virtio_blk_config));
>  }
>  
> @@ -420,6 +432,7 @@ static uint32_t virtio_blk_get_features(
>      features |= (1 << VIRTIO_BLK_F_SEG_MAX);
>      features |= (1 << VIRTIO_BLK_F_GEOMETRY);
>      features |= (1 << VIRTIO_BLK_F_TOPOLOGY);
> +    features |= (1 << VIRTIO_BLK_F_BLK_SIZE);
>  
>      if (bdrv_enable_write_cache(s->bs))
>          features |= (1 << VIRTIO_BLK_F_WCACHE);
> @@ -479,6 +492,7 @@ VirtIODevice *virtio_blk_init(DeviceStat
>      s->bs = conf->dinfo->bdrv;
>      s->conf = conf;
>      s->rq = NULL;
> +    s->sector_mask = (s->conf->logical_block_size / 512) - 1;

Is there a check anywhere that the user didn't give us an odd block
size? We could get an interesting bit mask otherwise.

Kevin