[Qemu-devel] Re: [PATCH 1/3] Add virtioblk_id tool to extract drive serial numbers

[john cooper]

Ryan Harper wrote:
> * Kay Sievers <address@hidden> [2010-06-03 15:06]:
>> On Thu, Jun 3, 2010 at 22:01, Ryan Harper <address@hidden> wrote:
>>> * Kay Sievers <address@hidden> [2010-06-03 14:53]:
>>>> On Thu, Jun 3, 2010 at 21:07, Ryan Harper <address@hidden> wrote:
>>>>> Use the 'VBID' virtio-blk ioctl to extract drive serial numbers
>>>>> to be used for building disk/by-id symlinks.  After extracting
>>>>> the serial number of the device it prints out the minimum info
>>>>> needed in a similar format to `scsi_id --export` so that the
>>>>> persistent-storage rules can process the serial information.
>>>>>
>>>>> This program depends on the virtio-blk serial device patches posted
>>>>> here[1] being applied to qemu and linux-kernel.
>>>>>
>>>>> Here is what the output looks like:
>>>>>
>>>>> % ./virtioblk_id /dev/vdb
>>>>> ID_VIRTIO=1
>>>>> ID_TYPE=disk
>>>>> ID_VIRTIO_SERIAL=QM00001
>>>> Yikes! An ioctl to copy a plain string, and an entire binary to call
>>>> that ioctl and print it. If we don't have enough problems we make new
>>>> ones? :)

[Jumping in the thread here as I hadn't seen the comments
until now.  This issue has been discussed ad nauseam in the
context of qemu so here is a summary]

Packaging this up this data within a mock ATA_IDENTIFY was
proposed.  However that is a large bag of largely archaic
bits most of which are dummied up normally, and would be even
more so in the case here of virtio.  Putting a proposed
identify struct mock-up in qemu made the most sense in this
scenario as that's where the disk geometry information exists,
unfortunately doing so didn't fly with the maintainer.  Locating
this mock-up in the virtio driver makes less sense as the
geometry data for mock-up would need to be packaged up in yet
another structure and passed to the driver for formatting onto
the identify data.  No one even seriously considered doing this
as it combines the worst of both worlds.

>>>> What's the reason to drop the ATA identify, that would work out-of-the
>>>> box without any of this stuff. It could also support WWN, which is
>>>> what people are looking for these days.

That is the singular, very strong argument for ATA_IDENTIFY
and one which I'd made several times however it isn't the only
consideration in this multi-way trade off.

>>> http://www.mail-archive.com/address@hidden/msg24321.html
>>>
>>> 1. Virtio-blk isn't an ATA device
>>> 2. The ATA identify page is too large to fit into the virtio config
>>> space

Passing this data in PCI config made sense in the original
version of this patch when I was just shuffling 20 bytes of
id to the driver.  But when the ATA_IDENTIFY argument arose
it extended the size of the passed data to 512 bytes which caused
breakage of PCI config space due to the size now consumed to
quietly exceed the PCI specification.  Moving the identify
scheme to pass data by virtio request was proposed but by this
time the maintainer was strongly objecting to mock-up of this data
within qemu.

>>> I'm not finding the older threads where this was discussed in detail.
>>> I'll keep looking if the above isn't a sufficient explaination.
>> Well, if ATA doesn't fit, then put that string in sysfs like mmc block
>> devices are doing it. It looks really awkward to require a new binary
>> and a new ioctl to get a single string out of something that was just
>> invented.

There are two issues:

- Getting the serial/id string exported from qemu to the driver.
  This is the much less contentious portion unless we're still
  bent on using the identify scheme which will surely torpedo
  the effort once again.  Assuming we're satisfied with qemu
  exporting the serial/id string alone we should be quite close
  here.

- Presentation of the serial/id to the application by the
  virtio driver.  ATA_IDENTIFY would be nice if the above restrictions
  didn't exist, but they do.  So let's just get the job done
  once and for all.  No one wants to invent yet another ioctl
  interface but it is the best compromise available as the code
  in both the driver and application is trivial.  The other
  alternatives at their respective extremes (/sys: new but pretty
  interface, over powered for the job, more cruft in the driver;
  ATA_IDENTIFY: existing interface, ugliest possible code, even
  more cruft in the driver) have been proposed and abandoned for
  reasons cited above.
 
> John,
> 
> you mentioned that you had a sys interface in-mind earlier.  I'm not
> sure how to proceed given the virtio-blk kernel side patches are
> already upstream[1][2].  Are we looking to revert and switch?

No, we can use the interface as-is, however we have an opportunity
to make it more usage friendly.

> 1.  
> http://repo.or.cz/w/linux-2.6.git/commitdiff/234f2725a5d03f78539f1d36cb32f2c4f9b1822c

My slight modification to the above commit makes it possible
for the application to determine the data size the driver intends
to copy out, and allows the user to indicate the destination data
size for the copy operation.  Otherwise the caller needs to have
the size assumption built-in.

Your immediate use case here I believe to be the needed motivation
to finally drive this to a conclusion.  So folks lets just tie this
one off. 

Thanks,

-john


-- 
address@hidden