| Message ID | 20120921133031.GA1682@darkstar.nay.redhat.com |
|---|---|
| State | New |
| Headers | show |
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--; > } > >
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 >
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
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
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
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 >
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
--- 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--; }
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(-)