* Re: [Qemu-devel] [PATCH for-2.1] scsi: fix scsi disk symbol confusion in guest os
2014-07-17 13:45 ` Paolo Bonzini
@ 2014-07-18 1:32 ` Gonglei (Arei)
2014-07-18 2:01 ` Gonglei (Arei)
1 sibling, 0 replies; 4+ messages in thread
From: Gonglei (Arei) @ 2014-07-18 1:32 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel@nongnu.org; +Cc: Luonengjun, Huangweidong (C)
Hi,
> -----Original Message-----
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Sent: Thursday, July 17, 2014 9:45 PM
> To: Gonglei (Arei); qemu-devel@nongnu.org
> Cc: Huangweidong (C); Luonengjun
> Subject: Re: [PATCH for-2.1] scsi: fix scsi disk symbol confusion in guest os
>
> Il 17/07/2014 14:55, arei.gonglei@huawei.com ha scritto:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > Assuming that we hotplug three virtio-scsi disk as follow steps:
> > 1. start vm with virtio-scsi as system disk (guest os: suse11 sp3 ).
> > 2. hotplug disk 1 (as lun2)
> > -drive
> file=/Images/TestImg/kvm-disk-scsi_001,if=none,id=drive-scsi0-0-0-2,format=r
> aw, \
> > cache=none,aio=native -device
> scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=2, \
> > drive=drive-scsi0-0-0-2,id=scsi0-0-0-2
> > 2. hotplug disk 2 (as lun3)
> > -drive
> file=/Images/TestImg/kvm-disk-scsi_002,if=none,id=drive-scsi0-0-0-3,format=r
> aw,\
> > cache=none,aio=native -device
> scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=3,\
> > drive=drive-scsi0-0-0-3,id=scsi0-0-0-3
> > 3. hotplug disk 3 (as lun4)
> > -drive
> file=/Images/TestImg/kvm-disk-scsi_003,if=none,id=drive-scsi0-0-0-4,format=r
> aw,\
> > cache=none,aio=native -device
> scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=4,\
> > drive=drive-scsi0-0-0-4,id=scsi0-0-0-4
> >
> > We can see lun2 as sdb, lun3 as sdc, lun4 as sdd in the guest os.
> > But after rebootint the guest, the scsi disk symbol will changed,
> > lun2 change to sdd, lun3 as sdc, lun4 as sdb.
> >
> > Lun2 -> sdb reboot lun2 -> sdd
> > Lun3 -> sdc ------> lun3 -> sdc
> > Lun4 -> sdd lun4 -> sdb
> >
> > In Linux os, the scsi_scan_host() will scan scsi host adapter's lun, firstly scan
> lun 0
> > and add into system, secondly send REPORT_LUNS command to qurey the
> other luns.
> >
> > In QEMU, the scsi_target_emulate_report_luns() emulate the REPORT_LUNS
> command.
> > The function will scan virtio-scsi-bus's children and report to guest os finally.
> > QTAILQ_FOREACH(kid, &r->req.bus->qbus.children, sibling) {
> >
> > The step of attaching device in QEMU:
> > qdev_device_add
> > qdev_set_parent_bus
> > bus_add_child
> > QTAILQ_INSERT_HEAD(&bus->children, kid, sibling); // insert list
> head
> >
> > The latest hotplugged disk is at the head of bus->children list.
> >
> > Finally those cause the disk symbol confusion in the guest os.
> >
> > Fix the issue by QTAILQ_FOREACH_REVERSE replace QTAILQ_FOREACH in
> > scsi_target_emulate_report_luns(), which follow the FIFO principle for
> > scsi disks hotplugging.
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> > hw/scsi/scsi-bus.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> > index 4341754..b6671ea 100644
> > --- a/hw/scsi/scsi-bus.c
> > +++ b/hw/scsi/scsi-bus.c
> > @@ -371,7 +371,8 @@ static bool
> scsi_target_emulate_report_luns(SCSITargetReq *r)
> > memset(r->buf, 0, len);
> > stl_be_p(&r->buf[0], n);
> > i = found_lun0 ? 8 : 16;
> > - QTAILQ_FOREACH(kid, &r->req.bus->qbus.children, sibling) {
> > + QTAILQ_FOREACH_REVERSE(kid, &r->req.bus->qbus.children,
> > + ChildrenHead, sibling) {
> > DeviceState *qdev = kid->child;
> > SCSIDevice *dev = SCSI_DEVICE(qdev);
> >
> >
>
> This is a change to the guest ABI, so you would have to make it only for
> the latest machine type;
Would you give me more details? Thanks.
> and this unfortunately disqualifies it already
> from 2.1 since it's very late.
>
OK.
> Does this happen for cold-plugged disks too? If so, I wonder if this
> would cause more pain than it fixes, because it could break machines
> when you upgrade QEMU.
No, it doesn't happen for cold-plugged disks.
> In the end, /dev/sd* is not stable and people
> should use /dev/disk/* and/or partition labels instead.
>
Agree.
But for general users, the /dev/sd* usually more commonly used. They
can be used easily through easy mount command "/dev/sd* /mnt/sd* ".
So, this changes make sense for people IMHO.
Best regards,
-Gonglei
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH for-2.1] scsi: fix scsi disk symbol confusion in guest os
2014-07-17 13:45 ` Paolo Bonzini
2014-07-18 1:32 ` Gonglei (Arei)
@ 2014-07-18 2:01 ` Gonglei (Arei)
1 sibling, 0 replies; 4+ messages in thread
From: Gonglei (Arei) @ 2014-07-18 2:01 UTC (permalink / raw)
To: Gonglei (Arei), Paolo Bonzini, qemu-devel@nongnu.org
Cc: Luonengjun, Huangweidong (C)
> -----Original Message-----
> From: Gonglei (Arei)
> Sent: Friday, July 18, 2014 9:32 AM
> To: 'Paolo Bonzini'; qemu-devel@nongnu.org
> Cc: Huangweidong (C); Luonengjun
> Subject: RE: [PATCH for-2.1] scsi: fix scsi disk symbol confusion in guest os
>
> Hi,
>
> > -----Original Message-----
> > From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> > Sent: Thursday, July 17, 2014 9:45 PM
> > To: Gonglei (Arei); qemu-devel@nongnu.org
> > Cc: Huangweidong (C); Luonengjun
> > Subject: Re: [PATCH for-2.1] scsi: fix scsi disk symbol confusion in guest os
> >
> > Il 17/07/2014 14:55, arei.gonglei@huawei.com ha scritto:
> > > From: Gonglei <arei.gonglei@huawei.com>
> > >
> > > Assuming that we hotplug three virtio-scsi disk as follow steps:
> > > 1. start vm with virtio-scsi as system disk (guest os: suse11 sp3 ).
> > > 2. hotplug disk 1 (as lun2)
> > > -drive
> >
> file=/Images/TestImg/kvm-disk-scsi_001,if=none,id=drive-scsi0-0-0-2,format=r
> > aw, \
> > > cache=none,aio=native -device
> > scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=2, \
> > > drive=drive-scsi0-0-0-2,id=scsi0-0-0-2
> > > 2. hotplug disk 2 (as lun3)
> > > -drive
> >
> file=/Images/TestImg/kvm-disk-scsi_002,if=none,id=drive-scsi0-0-0-3,format=r
> > aw,\
> > > cache=none,aio=native -device
> > scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=3,\
> > > drive=drive-scsi0-0-0-3,id=scsi0-0-0-3
> > > 3. hotplug disk 3 (as lun4)
> > > -drive
> >
> file=/Images/TestImg/kvm-disk-scsi_003,if=none,id=drive-scsi0-0-0-4,format=r
> > aw,\
> > > cache=none,aio=native -device
> > scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=4,\
> > > drive=drive-scsi0-0-0-4,id=scsi0-0-0-4
> > >
> > > We can see lun2 as sdb, lun3 as sdc, lun4 as sdd in the guest os.
> > > But after rebootint the guest, the scsi disk symbol will changed,
> > > lun2 change to sdd, lun3 as sdc, lun4 as sdb.
> > >
> > > Lun2 -> sdb reboot lun2 -> sdd
> > > Lun3 -> sdc ------> lun3 -> sdc
> > > Lun4 -> sdd lun4 -> sdb
> > >
> > > In Linux os, the scsi_scan_host() will scan scsi host adapter's lun, firstly
> scan
> > lun 0
> > > and add into system, secondly send REPORT_LUNS command to qurey the
> > other luns.
> > >
> > > In QEMU, the scsi_target_emulate_report_luns() emulate the
> REPORT_LUNS
> > command.
> > > The function will scan virtio-scsi-bus's children and report to guest os finally.
> > > QTAILQ_FOREACH(kid, &r->req.bus->qbus.children, sibling) {
> > >
> > > The step of attaching device in QEMU:
> > > qdev_device_add
> > > qdev_set_parent_bus
> > > bus_add_child
> > > QTAILQ_INSERT_HEAD(&bus->children, kid, sibling); // insert
> list
> > head
> > >
> > > The latest hotplugged disk is at the head of bus->children list.
> > >
> > > Finally those cause the disk symbol confusion in the guest os.
> > >
> > > Fix the issue by QTAILQ_FOREACH_REVERSE replace QTAILQ_FOREACH in
> > > scsi_target_emulate_report_luns(), which follow the FIFO principle for
> > > scsi disks hotplugging.
> > >
> > > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > > ---
> > > hw/scsi/scsi-bus.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> > > index 4341754..b6671ea 100644
> > > --- a/hw/scsi/scsi-bus.c
> > > +++ b/hw/scsi/scsi-bus.c
> > > @@ -371,7 +371,8 @@ static bool
> > scsi_target_emulate_report_luns(SCSITargetReq *r)
> > > memset(r->buf, 0, len);
> > > stl_be_p(&r->buf[0], n);
> > > i = found_lun0 ? 8 : 16;
> > > - QTAILQ_FOREACH(kid, &r->req.bus->qbus.children, sibling) {
> > > + QTAILQ_FOREACH_REVERSE(kid, &r->req.bus->qbus.children,
> > > + ChildrenHead, sibling) {
> > > DeviceState *qdev = kid->child;
> > > SCSIDevice *dev = SCSI_DEVICE(qdev);
> > >
> > >
> >
> > This is a change to the guest ABI, so you would have to make it only for
> > the latest machine type;
>
> Would you give me more details? Thanks.
>
> > and this unfortunately disqualifies it already
> > from 2.1 since it's very late.
> >
> OK.
>
> > Does this happen for cold-plugged disks too? If so, I wonder if this
> > would cause more pain than it fixes, because it could break machines
> > when you upgrade QEMU.
>
> No, it doesn't happen for cold-plugged disks.
>
Sorry, I may misunderstand your mean. This change will happen for
cold-plugged disks too when using the QEMU before committing this patch.
Best regards,
-Gonglei
> > In the end, /dev/sd* is not stable and people
> > should use /dev/disk/* and/or partition labels instead.
> >
> Agree.
>
> But for general users, the /dev/sd* usually more commonly used. They
> can be used easily through easy mount command "/dev/sd* /mnt/sd* ".
> So, this changes make sense for people IMHO.
>
> Best regards,
> -Gonglei
^ permalink raw reply [flat|nested] 4+ messages in thread