Re: [PATCH] add virtio disk geometry feature

[Anthony Liguori <aliguori@us.ibm.com>]

Rusty Russell wrote:
> On Thursday 17 April 2008 04:56:37 Ryan Harper wrote:
>   
>> From: Ryan Harper <ryanh@us.ibm.com>
>>
>> Rather than faking up some geometry, allow the backend to push the disk
>> geometry via virtio pci config option.  Keep the old geo code around for
>> compatibility.
>>     
>
> Hi Ryan,
>
>    Looks good! Some brief review below.  Mainly just "how I would have done
> things" stuff.  BTW, does this help in real life?  I assume something in
> userspace wants it?
>   

Boot loaders (like grub) query the geometry from the kernel to figure 
out how to setup the stage1/stage2.  We've seen strange issues with grub 
thinking it has crazy geometries when installed on a virtio disk (as 
opposed to booting from virtio with an existing disk).

Ryan: have you tested a hardy install with your patches?  Does it help 
when installing to virtio?  I could pretty reliably reproduce the 
strangeness with a 20GB disk image FWIW.

> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -157,10 +157,25 @@ static int virtblk_ioctl(struct inode *i
>  /* We provide getgeo only to please some old bootloader/partitioning tools */
>  static int virtblk_getgeo(struct block_device *bd, struct hd_geometry *geo)
>  {
> -	/* some standard values, similar to sd */
> -	geo->heads = 1 << 6;
> -	geo->sectors = 1 << 5;
> -	geo->cylinders = get_capacity(bd->bd_disk) >> 11;
> +	struct virtio_blk *vblk = bd->bd_disk->private_data;
> +	struct virtio_blk_geometry vgeo;
> +	int err;
> +
> +	/* see if the host passed in geometry config */
> +	err = virtio_config_val(vblk->vdev, VIRTIO_BLK_F_GEOMETRY,
> +				offsetof(struct virtio_blk_config, geometry),
> +				&vgeo);
> +
> +	if (!err) {
> +		geo->heads = vgeo.heads;
> +		geo->sectors = vgeo.sectors;
> +		geo->cylinders = vgeo.cylinders;
> +	} else {
> +		/* some standard values, similar to sd */
> +		geo->heads = 1 << 6;
> +		geo->sectors = 1 << 5;
> +		geo->cylinders = get_capacity(bd->bd_disk) >> 11;
> +	}
>  	return 0;
>  }
>   

You're probably breaking PPC since the values in the config space are in 
little endian format.  virtio_config_val does automagic endianness 
conversion if the size is 2, 4, or 8.  In this case, the structure size 
is 4 so the endianness conversion will do the wrong thing.

Magic endianness conversion based on read size is looking pretty evil to 
me... Perhaps we need explicit *_val[8,16,32,64]?

Regards,

Anthony Liguori