* [Qemu-devel] [PATCH v2] virtio-blk: add default serial id
@ 2012-09-21 13:30 Dave Young
2012-09-21 14:15 ` Eric Blake
0 siblings, 1 reply; 8+ messages in thread
From: Dave Young @ 2012-09-21 13:30 UTC (permalink / raw)
To: qemu-devel; +Cc: dyoung, eblake
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(-)
--- 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--;
}
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH v2] virtio-blk: add default serial id
2012-09-21 13:30 [Qemu-devel] [PATCH v2] virtio-blk: add default serial id Dave Young
@ 2012-09-21 14:15 ` Eric Blake
2012-09-23 2:37 ` Dave Young
0 siblings, 1 reply; 8+ messages in thread
From: Eric Blake @ 2012-09-21 14:15 UTC (permalink / raw)
To: Dave Young; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1764 bytes --]
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--;
> }
>
>
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] virtio-blk: add default serial id
2012-09-21 14:15 ` Eric Blake
@ 2012-09-23 2:37 ` Dave Young
2012-10-05 8:14 ` Stefan Hajnoczi
0 siblings, 1 reply; 8+ messages in thread
From: Dave Young @ 2012-09-23 2:37 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] virtio-blk: add default serial id
2012-09-23 2:37 ` Dave Young
@ 2012-10-05 8:14 ` Stefan Hajnoczi
2012-10-09 2:27 ` Dave Young
0 siblings, 1 reply; 8+ messages in thread
From: Stefan Hajnoczi @ 2012-10-05 8:14 UTC (permalink / raw)
To: Dave Young; +Cc: Eric Blake, qemu-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] virtio-blk: add default serial id
2012-10-05 8:14 ` Stefan Hajnoczi
@ 2012-10-09 2:27 ` Dave Young
2012-10-09 8:31 ` Stefan Hajnoczi
0 siblings, 1 reply; 8+ messages in thread
From: Dave Young @ 2012-10-09 2:27 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel
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
--
Thanks
Dave
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] virtio-blk: add default serial id
2012-10-09 2:27 ` Dave Young
@ 2012-10-09 8:31 ` Stefan Hajnoczi
2012-10-10 2:07 ` Dave Young
0 siblings, 1 reply; 8+ messages in thread
From: Stefan Hajnoczi @ 2012-10-09 8:31 UTC (permalink / raw)
To: Dave Young; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] virtio-blk: add default serial id
2012-10-09 8:31 ` Stefan Hajnoczi
@ 2012-10-10 2:07 ` Dave Young
2012-10-11 8:24 ` Stefan Hajnoczi
0 siblings, 1 reply; 8+ messages in thread
From: Dave Young @ 2012-10-10 2:07 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel
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
>
--
Thanks
Dave
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] virtio-blk: add default serial id
2012-10-10 2:07 ` Dave Young
@ 2012-10-11 8:24 ` Stefan Hajnoczi
0 siblings, 0 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2012-10-11 8:24 UTC (permalink / raw)
To: Dave Young; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-10-11 8:24 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-21 13:30 [Qemu-devel] [PATCH v2] virtio-blk: add default serial id Dave Young
2012-09-21 14:15 ` Eric Blake
2012-09-23 2:37 ` Dave Young
2012-10-05 8:14 ` Stefan Hajnoczi
2012-10-09 2:27 ` Dave Young
2012-10-09 8:31 ` Stefan Hajnoczi
2012-10-10 2:07 ` Dave Young
2012-10-11 8:24 ` Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.