| Message ID | CAP8dc_acqeqpio7fVK4XSAxoH8QaGp9L=S1h2OCCWgrj=Q7VCw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
As I know, this is not a good way to submit a patch. You need to read the guideline first: http://wiki.qemu.org/Contribute/SubmitAPatch
On Fri, Dec 30, 2016 at 10:41:35AM +0800, Junkang Fu wrote: > From 74e913fc41ea98d1dde692175f1e3fb6729342aa Mon Sep 17 00:00:00 2001 > From: "junkang.fjk" <junkang.fjk@alibaba-inc.com> > Date: Wed, 24 Aug 2016 19:36:53 +0800 > Subject: [PATCH] virtio-blk: add disk-name device property > > Current virtio-blk disk name(ex. /dev/vdb) has nothing to do with the > target dev > name specified in libvirt xml file. For example, we may get disk name > /dev/vdb in > VM while target dev specified in libvirt xml is vdc. This may lead to a > little trouble > to find out the relationship between the disk name in VM and somewhere out > of > VM, for example in the control board of Public cloud service providers. I > suggest > if Qemu could add a VIRTIO_BLK_F_DISK_NAME feature, with > VIRTIO_BLK_F_DISK_NAME > capable Qemu and virtio-blk frontend drivers, disk name in the vm can be > specified > as follows: > -device virtio-blk-pci,disk-name=vdabc Did you try -device virtio-blk-pci,serial=vdabc and /dev/disk/by-id/virtio-vdabc inside a Linux guest? > --- > hw/block/virtio-blk.c | 5 +++++ > include/hw/virtio/virtio-blk.h | 1 + > include/standard-headers/linux/virtio_blk.h | 6 ++++++ > 3 files changed, 12 insertions(+) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 331d766..4039fb9 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -716,6 +716,8 @@ static void virtio_blk_update_config(VirtIODevice > *vdev, uint8_t *config) > blkcfg.alignment_offset = 0; > blkcfg.wce = blk_enable_write_cache(s->blk); > virtio_stw_p(vdev, &blkcfg.num_queues, s->conf.num_queues); > + if (s->disk_name) > + strncpy((char *)blkcfg.disk_name, s->disk_name, DISK_NAME_LEN); > memcpy(config, &blkcfg, sizeof(struct virtio_blk_config)); > } > @@ -740,6 +742,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice > *vdev, uint64_t features, > virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); > virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); > virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); > + virtio_add_feature(&features, VIRTIO_BLK_F_DISK_NAME); > + > if (virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > if (s->conf.scsi) { > error_setg(errp, "Please set scsi=off for virtio-blk devices > in order to use virtio 1.0"); > @@ -970,6 +974,7 @@ static Property virtio_blk_properties[] = { > DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, > 0, > true), > DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1), > + DEFINE_PROP_STRING("disk-name", VirtIOBlock, disk_name), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h > index 180bd8d..003e810 100644 > --- a/include/hw/virtio/virtio-blk.h > +++ b/include/hw/virtio/virtio-blk.h > @@ -56,6 +56,7 @@ typedef struct VirtIOBlock { > bool dataplane_disabled; > bool dataplane_started; > struct VirtIOBlockDataPlane *dataplane; > + char *disk_name; > } VirtIOBlock; > > typedef struct VirtIOBlockReq { > diff --git a/include/standard-headers/linux/virtio_blk.h > b/include/standard-headers/linux/virtio_blk.h > index ab16ec5..1f5d89d 100644 > --- a/include/standard-headers/linux/virtio_blk.h > +++ b/include/standard-headers/linux/virtio_blk.h > @@ -38,6 +38,7 @@ > #define VIRTIO_BLK_F_BLK_SIZE 6 /* Block size of disk is available*/ > #define VIRTIO_BLK_F_TOPOLOGY 10 /* Topology information is available */ > #define VIRTIO_BLK_F_MQ 12 /* support more than one vq */ > +#define VIRTIO_BLK_F_DISK_NAME 13 /* specify /dev/xxx name */ These bits are defined in the VIRTIO specification. In addition to modifying QEMU and guest drivers you would also need to send a patch to the VIRTIO Technical Committee to update the specification: https://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-2050003 > > /* Legacy feature bits */ > #ifndef VIRTIO_BLK_NO_LEGACY > @@ -51,6 +52,9 @@ > > #define VIRTIO_BLK_ID_BYTES 20 /* ID string length */ > > +/* micro defined in kernel genhd.h */ > +#define DISK_NAME_LEN 32 > + > struct virtio_blk_config { > /* The capacity (in 512-byte sectors). */ > uint64_t capacity; > @@ -84,6 +88,8 @@ struct virtio_blk_config { > > /* number of vqs, only available when VIRTIO_BLK_F_MQ is set */ > uint16_t num_queues; > + > + uint8_t disk_name[DISK_NAME_LEN]; > } QEMU_PACKED; > > /* > -- > 1.9.4
On 12/29/2016 08:41 PM, Junkang Fu wrote: >>From 74e913fc41ea98d1dde692175f1e3fb6729342aa Mon Sep 17 00:00:00 2001 > From: "junkang.fjk" <junkang.fjk@alibaba-inc.com> > Date: Wed, 24 Aug 2016 19:36:53 +0800 > Subject: [PATCH] virtio-blk: add disk-name device property > > Current virtio-blk disk name(ex. /dev/vdb) has nothing to do with the > target dev > name specified in libvirt xml file. For example, we may get disk name > /dev/vdb in > VM while target dev specified in libvirt xml is vdc. It's not really libvirt's fault. The libvirt XML names are for convenience, but nothing on the host side requires the guest to pick the same naming scheme as the host. I guess your proposal is to enhance the virtio spec such that clients that are new enough to honor the new addition to the virtio spec will change their name-picking algorithm to use the name provided by the host, rather than their current approach of picking whatever name they feel like, and then enhance libvirt to pass the XML name on down to the guest? It might work, but as others have pointed out, it will require a virtio spec change first.
On Tue, Jan 03, 2017 at 10:53:06AM -0600, Eric Blake wrote: > On 12/29/2016 08:41 PM, Junkang Fu wrote: > >>From 74e913fc41ea98d1dde692175f1e3fb6729342aa Mon Sep 17 00:00:00 2001 > > From: "junkang.fjk" <junkang.fjk@alibaba-inc.com> > > Date: Wed, 24 Aug 2016 19:36:53 +0800 > > Subject: [PATCH] virtio-blk: add disk-name device property > > > > Current virtio-blk disk name(ex. /dev/vdb) has nothing to do with the > > target dev > > name specified in libvirt xml file. For example, we may get disk name > > /dev/vdb in > > VM while target dev specified in libvirt xml is vdc. > > It's not really libvirt's fault. The libvirt XML names are for > convenience, but nothing on the host side requires the guest to pick the > same naming scheme as the host. > > I guess your proposal is to enhance the virtio spec such that clients > that are new enough to honor the new addition to the virtio spec will > change their name-picking algorithm to use the name provided by the > host, rather than their current approach of picking whatever name they > feel like, and then enhance libvirt to pass the XML name on down to the > guest? It might work, but as others have pointed out, it will require a > virtio spec change first. This change is unnecessary. The -device virtio-blk-pci,serial= property already exists for this purpose. Stefan
You got it, that's exactly what I mean.Thank you for your advise. 2017-01-04 0:53 GMT+08:00 Eric Blake <eblake@redhat.com>: > On 12/29/2016 08:41 PM, Junkang Fu wrote: > >>From 74e913fc41ea98d1dde692175f1e3fb6729342aa Mon Sep 17 00:00:00 2001 > > From: "junkang.fjk" <junkang.fjk@alibaba-inc.com> > > Date: Wed, 24 Aug 2016 19:36:53 +0800 > > Subject: [PATCH] virtio-blk: add disk-name device property > > > > Current virtio-blk disk name(ex. /dev/vdb) has nothing to do with the > > target dev > > name specified in libvirt xml file. For example, we may get disk name > > /dev/vdb in > > VM while target dev specified in libvirt xml is vdc. > > It's not really libvirt's fault. The libvirt XML names are for > convenience, but nothing on the host side requires the guest to pick the > same naming scheme as the host. > > I guess your proposal is to enhance the virtio spec such that clients > that are new enough to honor the new addition to the virtio spec will > change their name-picking algorithm to use the name provided by the > host, rather than their current approach of picking whatever name they > feel like, and then enhance libvirt to pass the XML name on down to the > guest? It might work, but as others have pointed out, it will require a > virtio spec change first. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > >
Thank you. -device virtio-blk-pci,serial=vdabc will solve my problem to some extent. 2017-01-04 0:29 GMT+08:00 Stefan Hajnoczi <stefanha@gmail.com>: > On Fri, Dec 30, 2016 at 10:41:35AM +0800, Junkang Fu wrote: > > From 74e913fc41ea98d1dde692175f1e3fb6729342aa Mon Sep 17 00:00:00 2001 > > From: "junkang.fjk" <junkang.fjk@alibaba-inc.com> > > Date: Wed, 24 Aug 2016 19:36:53 +0800 > > Subject: [PATCH] virtio-blk: add disk-name device property > > > > Current virtio-blk disk name(ex. /dev/vdb) has nothing to do with the > > target dev > > name specified in libvirt xml file. For example, we may get disk name > > /dev/vdb in > > VM while target dev specified in libvirt xml is vdc. This may lead to a > > little trouble > > to find out the relationship between the disk name in VM and somewhere > out > > of > > VM, for example in the control board of Public cloud service providers. I > > suggest > > if Qemu could add a VIRTIO_BLK_F_DISK_NAME feature, with > > VIRTIO_BLK_F_DISK_NAME > > capable Qemu and virtio-blk frontend drivers, disk name in the vm can be > > specified > > as follows: > > -device virtio-blk-pci,disk-name=vdabc > > Did you try -device virtio-blk-pci,serial=vdabc and > /dev/disk/by-id/virtio-vdabc inside a Linux guest? > > > --- > > hw/block/virtio-blk.c | 5 +++++ > > include/hw/virtio/virtio-blk.h | 1 + > > include/standard-headers/linux/virtio_blk.h | 6 ++++++ > > 3 files changed, 12 insertions(+) > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > index 331d766..4039fb9 100644 > > --- a/hw/block/virtio-blk.c > > +++ b/hw/block/virtio-blk.c > > @@ -716,6 +716,8 @@ static void virtio_blk_update_config(VirtIODevice > > *vdev, uint8_t *config) > > blkcfg.alignment_offset = 0; > > blkcfg.wce = blk_enable_write_cache(s->blk); > > virtio_stw_p(vdev, &blkcfg.num_queues, s->conf.num_queues); > > + if (s->disk_name) > > + strncpy((char *)blkcfg.disk_name, s->disk_name, DISK_NAME_LEN); > > memcpy(config, &blkcfg, sizeof(struct virtio_blk_config)); > > } > > @@ -740,6 +742,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice > > *vdev, uint64_t features, > > virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); > > virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); > > virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); > > + virtio_add_feature(&features, VIRTIO_BLK_F_DISK_NAME); > > + > > if (virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > > if (s->conf.scsi) { > > error_setg(errp, "Please set scsi=off for virtio-blk devices > > in order to use virtio 1.0"); > > @@ -970,6 +974,7 @@ static Property virtio_blk_properties[] = { > > DEFINE_PROP_BIT("request-merging", VirtIOBlock, > conf.request_merging, > > 0, > > true), > > DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1), > > + DEFINE_PROP_STRING("disk-name", VirtIOBlock, disk_name), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio- > blk.h > > index 180bd8d..003e810 100644 > > --- a/include/hw/virtio/virtio-blk.h > > +++ b/include/hw/virtio/virtio-blk.h > > @@ -56,6 +56,7 @@ typedef struct VirtIOBlock { > > bool dataplane_disabled; > > bool dataplane_started; > > struct VirtIOBlockDataPlane *dataplane; > > + char *disk_name; > > } VirtIOBlock; > > > > typedef struct VirtIOBlockReq { > > diff --git a/include/standard-headers/linux/virtio_blk.h > > b/include/standard-headers/linux/virtio_blk.h > > index ab16ec5..1f5d89d 100644 > > --- a/include/standard-headers/linux/virtio_blk.h > > +++ b/include/standard-headers/linux/virtio_blk.h > > @@ -38,6 +38,7 @@ > > #define VIRTIO_BLK_F_BLK_SIZE 6 /* Block size of disk is available*/ > > #define VIRTIO_BLK_F_TOPOLOGY 10 /* Topology information is available > */ > > #define VIRTIO_BLK_F_MQ 12 /* support more than one vq */ > > +#define VIRTIO_BLK_F_DISK_NAME 13 /* specify /dev/xxx name */ > > These bits are defined in the VIRTIO specification. In addition to > modifying QEMU and guest drivers you would also need to send a patch to > the VIRTIO Technical Committee to update the specification: > https://docs.oasis-open.org/virtio/virtio/v1.0/cs04/ > virtio-v1.0-cs04.html#x1-2050003 > > > > > /* Legacy feature bits */ > > #ifndef VIRTIO_BLK_NO_LEGACY > > @@ -51,6 +52,9 @@ > > > > #define VIRTIO_BLK_ID_BYTES 20 /* ID string length */ > > > > +/* micro defined in kernel genhd.h */ > > +#define DISK_NAME_LEN 32 > > + > > struct virtio_blk_config { > > /* The capacity (in 512-byte sectors). */ > > uint64_t capacity; > > @@ -84,6 +88,8 @@ struct virtio_blk_config { > > > > /* number of vqs, only available when VIRTIO_BLK_F_MQ is set */ > > uint16_t num_queues; > > + > > + uint8_t disk_name[DISK_NAME_LEN]; > > } QEMU_PACKED; > > > > /* > > -- > > 1.9.4 > > >
Thank you for pointing out this, i'll resend the patch if my suggestion is acceptted. 2016-12-30 15:34 GMT+08:00 Cao jin <caoj.fnst@cn.fujitsu.com>: > As I know, this is not a good way to submit a patch. You need to read > the guideline first: http://wiki.qemu.org/Contribute/SubmitAPatch > > -- > Sincerely, > Cao jin > > On 12/30/2016 10:41 AM, Junkang Fu wrote: > >>From 74e913fc41ea98d1dde692175f1e3fb6729342aa Mon Sep 17 00:00:00 2001 > > From: "junkang.fjk" <junkang.fjk@alibaba-inc.com> > > Date: Wed, 24 Aug 2016 19:36:53 +0800 > > Subject: [PATCH] virtio-blk: add disk-name device property > > > > Current virtio-blk disk name(ex. /dev/vdb) has nothing to do with the > > target dev > > name specified in libvirt xml file. For example, we may get disk name > > /dev/vdb in > > VM while target dev specified in libvirt xml is vdc. This may lead to a > > little trouble > > to find out the relationship between the disk name in VM and somewhere > out > > of > > VM, for example in the control board of Public cloud service providers. I > > suggest > > if Qemu could add a VIRTIO_BLK_F_DISK_NAME feature, with > > VIRTIO_BLK_F_DISK_NAME > > capable Qemu and virtio-blk frontend drivers, disk name in the vm can be > > specified > > as follows: > > -device virtio-blk-pci,disk-name=vdabc > > > > --- > > hw/block/virtio-blk.c | 5 +++++ > > include/hw/virtio/virtio-blk.h | 1 + > > include/standard-headers/linux/virtio_blk.h | 6 ++++++ > > 3 files changed, 12 insertions(+) > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > index 331d766..4039fb9 100644 > > --- a/hw/block/virtio-blk.c > > +++ b/hw/block/virtio-blk.c > > @@ -716,6 +716,8 @@ static void virtio_blk_update_config(VirtIODevice > > *vdev, uint8_t *config) > > blkcfg.alignment_offset = 0; > > blkcfg.wce = blk_enable_write_cache(s->blk); > > virtio_stw_p(vdev, &blkcfg.num_queues, s->conf.num_queues); > > + if (s->disk_name) > > + strncpy((char *)blkcfg.disk_name, s->disk_name, DISK_NAME_LEN); > > memcpy(config, &blkcfg, sizeof(struct virtio_blk_config)); > > } > > @@ -740,6 +742,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice > > *vdev, uint64_t features, > > virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); > > virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); > > virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); > > + virtio_add_feature(&features, VIRTIO_BLK_F_DISK_NAME); > > + > > if (virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > > if (s->conf.scsi) { > > error_setg(errp, "Please set scsi=off for virtio-blk devices > > in order to use virtio 1.0"); > > @@ -970,6 +974,7 @@ static Property virtio_blk_properties[] = { > > DEFINE_PROP_BIT("request-merging", VirtIOBlock, > conf.request_merging, > > 0, > > true), > > DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1), > > + DEFINE_PROP_STRING("disk-name", VirtIOBlock, disk_name), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio- > blk.h > > index 180bd8d..003e810 100644 > > --- a/include/hw/virtio/virtio-blk.h > > +++ b/include/hw/virtio/virtio-blk.h > > @@ -56,6 +56,7 @@ typedef struct VirtIOBlock { > > bool dataplane_disabled; > > bool dataplane_started; > > struct VirtIOBlockDataPlane *dataplane; > > + char *disk_name; > > } VirtIOBlock; > > > > typedef struct VirtIOBlockReq { > > diff --git a/include/standard-headers/linux/virtio_blk.h > > b/include/standard-headers/linux/virtio_blk.h > > index ab16ec5..1f5d89d 100644 > > --- a/include/standard-headers/linux/virtio_blk.h > > +++ b/include/standard-headers/linux/virtio_blk.h > > @@ -38,6 +38,7 @@ > > #define VIRTIO_BLK_F_BLK_SIZE 6 /* Block size of disk is available*/ > > #define VIRTIO_BLK_F_TOPOLOGY 10 /* Topology information is available > */ > > #define VIRTIO_BLK_F_MQ 12 /* support more than one vq */ > > +#define VIRTIO_BLK_F_DISK_NAME 13 /* specify /dev/xxx name */ > > > > /* Legacy feature bits */ > > #ifndef VIRTIO_BLK_NO_LEGACY > > @@ -51,6 +52,9 @@ > > > > #define VIRTIO_BLK_ID_BYTES 20 /* ID string length */ > > > > +/* micro defined in kernel genhd.h */ > > +#define DISK_NAME_LEN 32 > > + > > struct virtio_blk_config { > > /* The capacity (in 512-byte sectors). */ > > uint64_t capacity; > > @@ -84,6 +88,8 @@ struct virtio_blk_config { > > > > /* number of vqs, only available when VIRTIO_BLK_F_MQ is set */ > > uint16_t num_queues; > > + > > + uint8_t disk_name[DISK_NAME_LEN]; > > } QEMU_PACKED; > > > > /* > > -- > > 1.9.4 > > > > > > > > >
On 2017/1/4 22:44, Stefan Hajnoczi wrote: > On Tue, Jan 03, 2017 at 10:53:06AM -0600, Eric Blake wrote: >> On 12/29/2016 08:41 PM, Junkang Fu wrote: >>> >From 74e913fc41ea98d1dde692175f1e3fb6729342aa Mon Sep 17 00:00:00 2001 >>> From: "junkang.fjk" <junkang.fjk@alibaba-inc.com> >>> Date: Wed, 24 Aug 2016 19:36:53 +0800 >>> Subject: [PATCH] virtio-blk: add disk-name device property >>> >>> Current virtio-blk disk name(ex. /dev/vdb) has nothing to do with the >>> target dev >>> name specified in libvirt xml file. For example, we may get disk name >>> /dev/vdb in >>> VM while target dev specified in libvirt xml is vdc. >> >> It's not really libvirt's fault. The libvirt XML names are for >> convenience, but nothing on the host side requires the guest to pick the >> same naming scheme as the host. >> >> I guess your proposal is to enhance the virtio spec such that clients >> that are new enough to honor the new addition to the virtio spec will >> change their name-picking algorithm to use the name provided by the >> host, rather than their current approach of picking whatever name they >> feel like, and then enhance libvirt to pass the XML name on down to the >> guest? It might work, but as others have pointed out, it will require a >> virtio spec change first. > > This change is unnecessary. The -device virtio-blk-pci,serial= property > already exists for this purpose. how about the /dev/vdabc? I guess lots of people prefer to use it instead of /dev/disk/by-id/xxx?
On Thu, 01/12 09:22, Yang Zhang wrote: > On 2017/1/4 22:44, Stefan Hajnoczi wrote: > > On Tue, Jan 03, 2017 at 10:53:06AM -0600, Eric Blake wrote: > > > On 12/29/2016 08:41 PM, Junkang Fu wrote: > > > > >From 74e913fc41ea98d1dde692175f1e3fb6729342aa Mon Sep 17 00:00:00 2001 > > > > From: "junkang.fjk" <junkang.fjk@alibaba-inc.com> > > > > Date: Wed, 24 Aug 2016 19:36:53 +0800 > > > > Subject: [PATCH] virtio-blk: add disk-name device property > > > > > > > > Current virtio-blk disk name(ex. /dev/vdb) has nothing to do with the > > > > target dev > > > > name specified in libvirt xml file. For example, we may get disk name > > > > /dev/vdb in > > > > VM while target dev specified in libvirt xml is vdc. > > > > > > It's not really libvirt's fault. The libvirt XML names are for > > > convenience, but nothing on the host side requires the guest to pick the > > > same naming scheme as the host. > > > > > > I guess your proposal is to enhance the virtio spec such that clients > > > that are new enough to honor the new addition to the virtio spec will > > > change their name-picking algorithm to use the name provided by the > > > host, rather than their current approach of picking whatever name they > > > feel like, and then enhance libvirt to pass the XML name on down to the > > > guest? It might work, but as others have pointed out, it will require a > > > virtio spec change first. > > > > This change is unnecessary. The -device virtio-blk-pci,serial= property > > already exists for this purpose. > > how about the /dev/vdabc? I guess lots of people prefer to use it instead of > /dev/disk/by-id/xxx? I disagree. Using /dev/sdX has exactly the same issue and that's why fstab and boot loader etc almost always use UUID or disk label by default because they are more stable. Fam
On 2017/1/12 10:22, Fam Zheng wrote: > On Thu, 01/12 09:22, Yang Zhang wrote: >> On 2017/1/4 22:44, Stefan Hajnoczi wrote: >>> On Tue, Jan 03, 2017 at 10:53:06AM -0600, Eric Blake wrote: >>>> On 12/29/2016 08:41 PM, Junkang Fu wrote: >>>>> >From 74e913fc41ea98d1dde692175f1e3fb6729342aa Mon Sep 17 00:00:00 2001 >>>>> From: "junkang.fjk" <junkang.fjk@alibaba-inc.com> >>>>> Date: Wed, 24 Aug 2016 19:36:53 +0800 >>>>> Subject: [PATCH] virtio-blk: add disk-name device property >>>>> >>>>> Current virtio-blk disk name(ex. /dev/vdb) has nothing to do with the >>>>> target dev >>>>> name specified in libvirt xml file. For example, we may get disk name >>>>> /dev/vdb in >>>>> VM while target dev specified in libvirt xml is vdc. >>>> >>>> It's not really libvirt's fault. The libvirt XML names are for >>>> convenience, but nothing on the host side requires the guest to pick the >>>> same naming scheme as the host. >>>> >>>> I guess your proposal is to enhance the virtio spec such that clients >>>> that are new enough to honor the new addition to the virtio spec will >>>> change their name-picking algorithm to use the name provided by the >>>> host, rather than their current approach of picking whatever name they >>>> feel like, and then enhance libvirt to pass the XML name on down to the >>>> guest? It might work, but as others have pointed out, it will require a >>>> virtio spec change first. >>> >>> This change is unnecessary. The -device virtio-blk-pci,serial= property >>> already exists for this purpose. >> >> how about the /dev/vdabc? I guess lots of people prefer to use it instead of >> /dev/disk/by-id/xxx? > > I disagree. Using /dev/sdX has exactly the same issue and that's why fstab and > boot loader etc almost always use UUID or disk label by default because they are > more stable. I mean does it also change the /dev/sdX to the name specified in serial=sdX or it just show the name under /dev/disk/by-id/
On Thu, 01/12 15:39, Yang Zhang wrote: > On 2017/1/12 10:22, Fam Zheng wrote: > > On Thu, 01/12 09:22, Yang Zhang wrote: > > > On 2017/1/4 22:44, Stefan Hajnoczi wrote: > > > > On Tue, Jan 03, 2017 at 10:53:06AM -0600, Eric Blake wrote: > > > > > On 12/29/2016 08:41 PM, Junkang Fu wrote: > > > > > > >From 74e913fc41ea98d1dde692175f1e3fb6729342aa Mon Sep 17 00:00:00 2001 > > > > > > From: "junkang.fjk" <junkang.fjk@alibaba-inc.com> > > > > > > Date: Wed, 24 Aug 2016 19:36:53 +0800 > > > > > > Subject: [PATCH] virtio-blk: add disk-name device property > > > > > > > > > > > > Current virtio-blk disk name(ex. /dev/vdb) has nothing to do with the > > > > > > target dev > > > > > > name specified in libvirt xml file. For example, we may get disk name > > > > > > /dev/vdb in > > > > > > VM while target dev specified in libvirt xml is vdc. > > > > > > > > > > It's not really libvirt's fault. The libvirt XML names are for > > > > > convenience, but nothing on the host side requires the guest to pick the > > > > > same naming scheme as the host. > > > > > > > > > > I guess your proposal is to enhance the virtio spec such that clients > > > > > that are new enough to honor the new addition to the virtio spec will > > > > > change their name-picking algorithm to use the name provided by the > > > > > host, rather than their current approach of picking whatever name they > > > > > feel like, and then enhance libvirt to pass the XML name on down to the > > > > > guest? It might work, but as others have pointed out, it will require a > > > > > virtio spec change first. > > > > > > > > This change is unnecessary. The -device virtio-blk-pci,serial= property > > > > already exists for this purpose. > > > > > > how about the /dev/vdabc? I guess lots of people prefer to use it instead of > > > /dev/disk/by-id/xxx? > > > > I disagree. Using /dev/sdX has exactly the same issue and that's why fstab and > > boot loader etc almost always use UUID or disk label by default because they are > > more stable. > > I mean does it also change the /dev/sdX to the name specified in serial=sdX > or it just show the name under /dev/disk/by-id/ I was saying on real hardware, sata disks can have unstable /dev/sdX naming across reboots if you add or remove disks on the controller, or add or remove HBA. Fam
On Thu, Jan 12, 2017 at 10:22:12AM +0800, Fam Zheng wrote: > On Thu, 01/12 09:22, Yang Zhang wrote: > > On 2017/1/4 22:44, Stefan Hajnoczi wrote: > > > On Tue, Jan 03, 2017 at 10:53:06AM -0600, Eric Blake wrote: > > > > On 12/29/2016 08:41 PM, Junkang Fu wrote: > > > > > >From 74e913fc41ea98d1dde692175f1e3fb6729342aa Mon Sep 17 00:00:00 2001 > > > > > From: "junkang.fjk" <junkang.fjk@alibaba-inc.com> > > > > > Date: Wed, 24 Aug 2016 19:36:53 +0800 > > > > > Subject: [PATCH] virtio-blk: add disk-name device property > > > > > > > > > > Current virtio-blk disk name(ex. /dev/vdb) has nothing to do with the > > > > > target dev > > > > > name specified in libvirt xml file. For example, we may get disk name > > > > > /dev/vdb in > > > > > VM while target dev specified in libvirt xml is vdc. > > > > > > > > It's not really libvirt's fault. The libvirt XML names are for > > > > convenience, but nothing on the host side requires the guest to pick the > > > > same naming scheme as the host. > > > > > > > > I guess your proposal is to enhance the virtio spec such that clients > > > > that are new enough to honor the new addition to the virtio spec will > > > > change their name-picking algorithm to use the name provided by the > > > > host, rather than their current approach of picking whatever name they > > > > feel like, and then enhance libvirt to pass the XML name on down to the > > > > guest? It might work, but as others have pointed out, it will require a > > > > virtio spec change first. > > > > > > This change is unnecessary. The -device virtio-blk-pci,serial= property > > > already exists for this purpose. > > > > how about the /dev/vdabc? I guess lots of people prefer to use it instead of > > /dev/disk/by-id/xxx? > > I disagree. Using /dev/sdX has exactly the same issue and that's why fstab and > boot loader etc almost always use UUID or disk label by default because they are > more stable. Right, /dev/sdX shouldn't be used in configuration files or other persistent state. Try cat /etc/fstab on your own system. On my system there are no /dev/sdX paths in /etc/fstab, only UUIDs and LVM logical volume paths. Stefan
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 331d766..4039fb9 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -716,6 +716,8 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) blkcfg.alignment_offset = 0; blkcfg.wce = blk_enable_write_cache(s->blk); virtio_stw_p(vdev, &blkcfg.num_queues, s->conf.num_queues); + if (s->disk_name) + strncpy((char *)blkcfg.disk_name, s->disk_name, DISK_NAME_LEN); memcpy(config, &blkcfg, sizeof(struct virtio_blk_config)); } @@ -740,6 +742,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); + virtio_add_feature(&features, VIRTIO_BLK_F_DISK_NAME); + if (virtio_has_feature(features, VIRTIO_F_VERSION_1)) { if (s->conf.scsi) { error_setg(errp, "Please set scsi=off for virtio-blk devices in order to use virtio 1.0"); @@ -970,6 +974,7 @@ static Property virtio_blk_properties[] = { DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0, true), DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1), + DEFINE_PROP_STRING("disk-name", VirtIOBlock, disk_name), DEFINE_PROP_END_OF_LIST(), }; diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h index 180bd8d..003e810 100644 --- a/include/hw/virtio/virtio-blk.h +++ b/include/hw/virtio/virtio-blk.h @@ -56,6 +56,7 @@ typedef struct VirtIOBlock { bool dataplane_disabled; bool dataplane_started; struct VirtIOBlockDataPlane *dataplane; + char *disk_name; } VirtIOBlock; typedef struct VirtIOBlockReq { diff --git a/include/standard-headers/linux/virtio_blk.h b/include/standard-headers/linux/virtio_blk.h index ab16ec5..1f5d89d 100644 --- a/include/standard-headers/linux/virtio_blk.h +++ b/include/standard-headers/linux/virtio_blk.h @@ -38,6 +38,7 @@ #define VIRTIO_BLK_F_BLK_SIZE 6 /* Block size of disk is available*/ #define VIRTIO_BLK_F_TOPOLOGY 10 /* Topology information is available */ #define VIRTIO_BLK_F_MQ 12 /* support more than one vq */ +#define VIRTIO_BLK_F_DISK_NAME 13 /* specify /dev/xxx name */ /* Legacy feature bits */ #ifndef VIRTIO_BLK_NO_LEGACY @@ -51,6 +52,9 @@ #define VIRTIO_BLK_ID_BYTES 20 /* ID string length */ +/* micro defined in kernel genhd.h */ +#define DISK_NAME_LEN 32 + struct virtio_blk_config { /* The capacity (in 512-byte sectors). */ uint64_t capacity; @@ -84,6 +88,8 @@ struct virtio_blk_config { /* number of vqs, only available when VIRTIO_BLK_F_MQ is set */ uint16_t num_queues; + + uint8_t disk_name[DISK_NAME_LEN]; } QEMU_PACKED; /*
From 74e913fc41ea98d1dde692175f1e3fb6729342aa Mon Sep 17 00:00:00 2001 From: "junkang.fjk" <junkang.fjk@alibaba-inc.com> Date: Wed, 24 Aug 2016 19:36:53 +0800 Subject: [PATCH] virtio-blk: add disk-name device property Current virtio-blk disk name(ex. /dev/vdb) has nothing to do with the target dev name specified in libvirt xml file. For example, we may get disk name /dev/vdb in VM while target dev specified in libvirt xml is vdc. This may lead to a little trouble to find out the relationship between the disk name in VM and somewhere out of VM, for example in the control board of Public cloud service providers. I suggest if Qemu could add a VIRTIO_BLK_F_DISK_NAME feature, with VIRTIO_BLK_F_DISK_NAME capable Qemu and virtio-blk frontend drivers, disk name in the vm can be specified as follows: -device virtio-blk-pci,disk-name=vdabc --- hw/block/virtio-blk.c | 5 +++++ include/hw/virtio/virtio-blk.h | 1 + include/standard-headers/linux/virtio_blk.h | 6 ++++++ 3 files changed, 12 insertions(+) -- 1.9.4