Patchwork [v2] virtio-blk: add default serial id

login
register
mail settings
Submitter Dave Young
Date Sept. 21, 2012, 1:30 p.m.
Message ID <20120921133031.GA1682@darkstar.nay.redhat.com>
Download mbox | patch
Permalink /patch/185731/
State New
Headers show

Comments

Dave Young - Sept. 21, 2012, 1:30 p.m.
For virtio block device, if user does not specify the serial attribute,
There will be no serial availabe, this is not convenient for identifying
the disk.

Doing something similar to ide disks, add a "VD0000?" default serial
number if user does not specify it.

[v1->v2 address comments from Eric Blake]:
fix spell errors in patch description
decrease drive_serial in virtio_blk_exit as well

Signed-off-by: Dave Young <dyoung@redhat.com>
---
 hw/virtio-blk.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
Eric Blake - Sept. 21, 2012, 2:15 p.m.
On 09/21/2012 07:30 AM, Dave Young wrote:
> 
> For virtio block device, if user does not specify the serial attribute,
> There will be no serial availabe, this is not convenient for identifying
> the disk.
> 
> Doing something similar to ide disks, add a "VD0000?" default serial
> number if user does not specify it.
> 
> [v1->v2 address comments from Eric Blake]:
> fix spell errors in patch description
> decrease drive_serial in virtio_blk_exit as well

Typically, patch changelogs belong...

> 
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---

...after the --- line, so that 'git am' doesn't make them part of git
history.  Also, I'm not sure that decreasing the serial number is
correct - you've now made it much easier to get duplicate serial numbers
compared to my original complaint of 100000 hotplug cycles.  Now all I
have to do is:

create a guest with two disks
hot unplug disk one
hot plug a new disk

and voila, both disks will now have serial number 2.

> @@ -632,6 +638,7 @@ VirtIODevice *virtio_blk_init(DeviceStat
>                                            sizeof(struct virtio_blk_config),
>                                            sizeof(VirtIOBlock));
>  
> +    s->drive_serial = drive_serial++;
>      s->vdev.get_config = virtio_blk_update_config;
>      s->vdev.set_config = virtio_blk_set_config;
>      s->vdev.get_features = virtio_blk_get_features;
> @@ -664,4 +671,5 @@ void virtio_blk_exit(VirtIODevice *vdev)
>      unregister_savevm(s->qdev, "virtio-blk", s);
>      blockdev_mark_auto_del(s->bs);
>      virtio_cleanup(vdev);
> +    drive_serial--;
>  }
> 
>
Dave Young - Sept. 23, 2012, 2:37 a.m.
On Fri, Sep 21, 2012 at 08:15:38AM -0600, Eric Blake wrote:
> On 09/21/2012 07:30 AM, Dave Young wrote:
> > 
> > For virtio block device, if user does not specify the serial attribute,
> > There will be no serial availabe, this is not convenient for identifying
> > the disk.
> > 
> > Doing something similar to ide disks, add a "VD0000?" default serial
> > number if user does not specify it.
> > 
> > [v1->v2 address comments from Eric Blake]:
> > fix spell errors in patch description
> > decrease drive_serial in virtio_blk_exit as well
> 
> Typically, patch changelogs belong...
> 
> > 
> > Signed-off-by: Dave Young <dyoung@redhat.com>
> > ---
> 
> ...after the --- line, so that 'git am' doesn't make them part of git
> history.  Also, I'm not sure that decreasing the serial number is
> correct - you've now made it much easier to get duplicate serial numbers
> compared to my original complaint of 100000 hotplug cycles.  Now all I
> have to do is:
> 
> create a guest with two disks
> hot unplug disk one
> hot plug a new disk
> 
> and voila, both disks will now have serial number 2.

Thanks for comment.

Add changelogs to git history is not bad IMO, this can reflect the changes
between diffrent version of the patches, it's quite normal. 

For the serial number decreasing issue, I think there's only these two ways to
select, there's no ideal way to resolve this issue.
My use case for this is for the kdump kernel to find proper disks,
after 1st kernel crashing 2nd kernel need find right disk to dump vmcore.
In this case v1 and v2 aproaches are both find to me.

From my point of view, patch v1 is better though, I think unpluging 100000 is
not a sane use case. It's not likely to happen.

> 
> > @@ -632,6 +638,7 @@ VirtIODevice *virtio_blk_init(DeviceStat
> >                                            sizeof(struct virtio_blk_config),
> >                                            sizeof(VirtIOBlock));
> >  
> > +    s->drive_serial = drive_serial++;
> >      s->vdev.get_config = virtio_blk_update_config;
> >      s->vdev.set_config = virtio_blk_set_config;
> >      s->vdev.get_features = virtio_blk_get_features;
> > @@ -664,4 +671,5 @@ void virtio_blk_exit(VirtIODevice *vdev)
> >      unregister_savevm(s->qdev, "virtio-blk", s);
> >      blockdev_mark_auto_del(s->bs);
> >      virtio_cleanup(vdev);
> > +    drive_serial--;
> >  }
> > 
> > 
> 
> -- 
> Eric Blake   eblake@redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
Stefan Hajnoczi - Oct. 5, 2012, 8:14 a.m.
On Sun, Sep 23, 2012 at 10:37:09AM +0800, Dave Young wrote:
> For the serial number decreasing issue, I think there's only these two ways to
> select, there's no ideal way to resolve this issue.
> My use case for this is for the kdump kernel to find proper disks,
> after 1st kernel crashing 2nd kernel need find right disk to dump vmcore.
> In this case v1 and v2 aproaches are both find to me.
> 
> From my point of view, patch v1 is better though, I think unpluging 100000 is
> not a sane use case. It's not likely to happen.

I'm not sure auto-assigning serial numbers is a good idea.  The guest can use
the serial number in /etc/fstab or other places where it expects the serial
number to be persistent.

Your patch does not provide persistent serial numbers, so a change to the QEMU
invocation could result in different serial numbers.  The guest will get
confused or perhaps refuse to boot.

I'd prefer if we don't expose a temporary serial number at all in order to
avoid issues like this.

Stefan
Dave Young - Oct. 9, 2012, 2:27 a.m.
On 10/05/2012 04:14 PM, Stefan Hajnoczi wrote:

> On Sun, Sep 23, 2012 at 10:37:09AM +0800, Dave Young wrote:
>> For the serial number decreasing issue, I think there's only these two ways to
>> select, there's no ideal way to resolve this issue.
>> My use case for this is for the kdump kernel to find proper disks,
>> after 1st kernel crashing 2nd kernel need find right disk to dump vmcore.
>> In this case v1 and v2 aproaches are both find to me.
>>
>> From my point of view, patch v1 is better though, I think unpluging 100000 is
>> not a sane use case. It's not likely to happen.
> 
> I'm not sure auto-assigning serial numbers is a good idea.  The guest can use
> the serial number in /etc/fstab or other places where it expects the serial
> number to be persistent.
> 
> Your patch does not provide persistent serial numbers, so a change to the QEMU
> invocation could result in different serial numbers.  The guest will get
> confused or perhaps refuse to boot.


Yes, it introduce confusion, but in this way at least the serial number
can be persistent across guest reboot. Traditionally ide disks use this
way as well, such as QEMU_HARDISK_00001, I think guest should not use
this in /etc/fstab.

> 
> I'd prefer if we don't expose a temporary serial number at all in order to
> avoid issues like this.
> 
> Stefan
Stefan Hajnoczi - Oct. 9, 2012, 8:31 a.m.
On Tue, Oct 09, 2012 at 10:27:26AM +0800, Dave Young wrote:
> On 10/05/2012 04:14 PM, Stefan Hajnoczi wrote:
> 
> > On Sun, Sep 23, 2012 at 10:37:09AM +0800, Dave Young wrote:
> >> For the serial number decreasing issue, I think there's only these two ways to
> >> select, there's no ideal way to resolve this issue.
> >> My use case for this is for the kdump kernel to find proper disks,
> >> after 1st kernel crashing 2nd kernel need find right disk to dump vmcore.
> >> In this case v1 and v2 aproaches are both find to me.
> >>
> >> From my point of view, patch v1 is better though, I think unpluging 100000 is
> >> not a sane use case. It's not likely to happen.
> > 
> > I'm not sure auto-assigning serial numbers is a good idea.  The guest can use
> > the serial number in /etc/fstab or other places where it expects the serial
> > number to be persistent.
> > 
> > Your patch does not provide persistent serial numbers, so a change to the QEMU
> > invocation could result in different serial numbers.  The guest will get
> > confused or perhaps refuse to boot.
> 
> 
> Yes, it introduce confusion, but in this way at least the serial number
> can be persistent across guest reboot. Traditionally ide disks use this
> way as well, such as QEMU_HARDISK_00001, I think guest should not use
> this in /etc/fstab.

If you don't want to set a persistent serial number, use another mechanism to
identify the disk.  For example, Linux has /dev/disk/by-path/ which identifies
virtio-blk PCI adapters, IDE, SCSI disks, etc.

Does this work for your use case?

Stefan
Dave Young - Oct. 10, 2012, 2:07 a.m.
On 10/09/2012 04:31 PM, Stefan Hajnoczi wrote:

> On Tue, Oct 09, 2012 at 10:27:26AM +0800, Dave Young wrote:
>> On 10/05/2012 04:14 PM, Stefan Hajnoczi wrote:
>>
>>> On Sun, Sep 23, 2012 at 10:37:09AM +0800, Dave Young wrote:
>>>> For the serial number decreasing issue, I think there's only these two ways to
>>>> select, there's no ideal way to resolve this issue.
>>>> My use case for this is for the kdump kernel to find proper disks,
>>>> after 1st kernel crashing 2nd kernel need find right disk to dump vmcore.
>>>> In this case v1 and v2 aproaches are both find to me.
>>>>
>>>> From my point of view, patch v1 is better though, I think unpluging 100000 is
>>>> not a sane use case. It's not likely to happen.
>>>
>>> I'm not sure auto-assigning serial numbers is a good idea.  The guest can use
>>> the serial number in /etc/fstab or other places where it expects the serial
>>> number to be persistent.
>>>
>>> Your patch does not provide persistent serial numbers, so a change to the QEMU
>>> invocation could result in different serial numbers.  The guest will get
>>> confused or perhaps refuse to boot.
>>
>>
>> Yes, it introduce confusion, but in this way at least the serial number
>> can be persistent across guest reboot. Traditionally ide disks use this
>> way as well, such as QEMU_HARDISK_00001, I think guest should not use
>> this in /etc/fstab.
> 
> If you don't want to set a persistent serial number, use another mechanism to
> identify the disk.  For example, Linux has /dev/disk/by-path/ which identifies
> virtio-blk PCI adapters, IDE, SCSI disks, etc.

>

> Does this work for your use case?


I have tried this before, but after rebooting (kexec/kdump) the by-path
link was not created. It might be udev bug anyway, I'm not sure though.


> 
> Stefan
>
Stefan Hajnoczi - Oct. 11, 2012, 8:24 a.m.
On Wed, Oct 10, 2012 at 10:07:01AM +0800, Dave Young wrote:
> On 10/09/2012 04:31 PM, Stefan Hajnoczi wrote:
> 
> > On Tue, Oct 09, 2012 at 10:27:26AM +0800, Dave Young wrote:
> >> On 10/05/2012 04:14 PM, Stefan Hajnoczi wrote:
> >>
> >>> On Sun, Sep 23, 2012 at 10:37:09AM +0800, Dave Young wrote:
> >>>> For the serial number decreasing issue, I think there's only these two ways to
> >>>> select, there's no ideal way to resolve this issue.
> >>>> My use case for this is for the kdump kernel to find proper disks,
> >>>> after 1st kernel crashing 2nd kernel need find right disk to dump vmcore.
> >>>> In this case v1 and v2 aproaches are both find to me.
> >>>>
> >>>> From my point of view, patch v1 is better though, I think unpluging 100000 is
> >>>> not a sane use case. It's not likely to happen.
> >>>
> >>> I'm not sure auto-assigning serial numbers is a good idea.  The guest can use
> >>> the serial number in /etc/fstab or other places where it expects the serial
> >>> number to be persistent.
> >>>
> >>> Your patch does not provide persistent serial numbers, so a change to the QEMU
> >>> invocation could result in different serial numbers.  The guest will get
> >>> confused or perhaps refuse to boot.
> >>
> >>
> >> Yes, it introduce confusion, but in this way at least the serial number
> >> can be persistent across guest reboot. Traditionally ide disks use this
> >> way as well, such as QEMU_HARDISK_00001, I think guest should not use
> >> this in /etc/fstab.
> > 
> > If you don't want to set a persistent serial number, use another mechanism to
> > identify the disk.  For example, Linux has /dev/disk/by-path/ which identifies
> > virtio-blk PCI adapters, IDE, SCSI disks, etc.
> 
> >
> 
> > Does this work for your use case?
> 
> 
> I have tried this before, but after rebooting (kexec/kdump) the by-path
> link was not created. It might be udev bug anyway, I'm not sure though.

Yes, it's udev.  Check /lib/udev/rules.d/60-persistent-storage.rules.

Stefan

Patch

--- qemu.orig/hw/virtio-blk.c
+++ qemu/hw/virtio-blk.c
@@ -22,6 +22,8 @@ 
 # include <scsi/sg.h>
 #endif
 
+static int drive_serial = 1;
+#define DEFAULT_VIRTIO_BLK_SERIAL_LEN 8
 typedef struct VirtIOBlock
 {
     VirtIODevice vdev;
@@ -33,6 +35,7 @@  typedef struct VirtIOBlock
     VirtIOBlkConf *blk;
     unsigned short sector_mask;
     DeviceState *qdev;
+    int drive_serial;
 } VirtIOBlock;
 
 static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
@@ -364,6 +367,7 @@  static void virtio_blk_handle_request(Vi
     MultiReqBuffer *mrb)
 {
     uint32_t type;
+    char serial[DEFAULT_VIRTIO_BLK_SERIAL_LEN];
 
     if (req->elem.out_num < 1 || req->elem.in_num < 1) {
         error_report("virtio-blk missing headers");
@@ -388,12 +392,14 @@  static void virtio_blk_handle_request(Vi
     } else if (type & VIRTIO_BLK_T_GET_ID) {
         VirtIOBlock *s = req->dev;
 
+        snprintf(serial, DEFAULT_VIRTIO_BLK_SERIAL_LEN,
+                 "VD%05d", s->drive_serial);
         /*
          * NB: per existing s/n string convention the string is
          * terminated by '\0' only when shorter than buffer.
          */
         strncpy(req->elem.in_sg[0].iov_base,
-                s->blk->serial ? s->blk->serial : "",
+                s->blk->serial ? s->blk->serial : serial,
                 MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES));
         virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
         g_free(req);
@@ -632,6 +638,7 @@  VirtIODevice *virtio_blk_init(DeviceStat
                                           sizeof(struct virtio_blk_config),
                                           sizeof(VirtIOBlock));
 
+    s->drive_serial = drive_serial++;
     s->vdev.get_config = virtio_blk_update_config;
     s->vdev.set_config = virtio_blk_set_config;
     s->vdev.get_features = virtio_blk_get_features;
@@ -664,4 +671,5 @@  void virtio_blk_exit(VirtIODevice *vdev)
     unregister_savevm(s->qdev, "virtio-blk", s);
     blockdev_mark_auto_del(s->bs);
     virtio_cleanup(vdev);
+    drive_serial--;
 }