[PATCH 0/2] virtio-scsi event changes for 3.6

[PATCH 0/2] virtio-scsi event changes for 3.6

[Paolo Bonzini]

Hi James,

here are two more small patches for virtio-scsi.  The first fixes a
problem with the recently introduced hotplug/hot-unplug support
(happens with LUNs whose number is greater than 255).  The second
introduces support for online resizing of disks.

The host here will also send the sense code via unit attention.
This will be discarded by the high-level driver for non-removable
units.  I sent a separate patch to deal with it for removable
units.

Paolo

Paolo Bonzini (2):
  virtio-scsi: fix parsing of hotplug/hot-unplug LUN number
  virtio-scsi: support online resizing of disks

 drivers/scsi/virtio_scsi.c  |   39 ++++++++-
 include/linux/virtio_scsi.h |    2 +
 2 files changed, 38 insertions(+), 2 deletions(-)

[PATCH 1/2] virtio-scsi: fix parsing of hotplug/hot-unplug LUN number

[Paolo Bonzini]

Hotplug/hot-unplug of a LUN whose number is greater than 255
uses the "flat" format for LUNs, which has bit 14 set.  Clear
the bit when parsing the event structs.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/scsi/virtio_scsi.c |    8 ++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index c7030fb..c937232 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -252,13 +252,19 @@ static void virtscsi_cancel_event_work(struct virtio_scsi *vscsi)
 		cancel_work_sync(&vscsi->event_list[i].work);
 }
 
+static unsigned int virtscsi_get_lun(u8 *lun_bytes)
+{
+	unsigned int lun = (lun_bytes[2] << 8) | lun_bytes[3];
+	return lun & 16383;
+}
+
 static void virtscsi_handle_transport_reset(struct virtio_scsi *vscsi,
 						struct virtio_scsi_event *event)
 {
 	struct scsi_device *sdev;
 	struct Scsi_Host *shost = virtio_scsi_host(vscsi->vdev);
 	unsigned int target = event->lun[1];
-	unsigned int lun = (event->lun[2] << 8) | event->lun[3];
+	unsigned int lun = virtscsi_get_lun(event->lun);
 
 	switch (event->reason) {
 	case VIRTIO_SCSI_EVT_RESET_RESCAN:
-- 
1.7.1

Re: [PATCH 1/2] virtio-scsi: fix parsing of hotplug/hot-unplug LUN number

[James Bottomley]

On Mon, 2012-07-16 at 18:05 +0200, Paolo Bonzini wrote:
> Hotplug/hot-unplug of a LUN whose number is greater than 255
> uses the "flat" format for LUNs, which has bit 14 set.  Clear
> the bit when parsing the event structs.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  drivers/scsi/virtio_scsi.c |    8 ++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index c7030fb..c937232 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -252,13 +252,19 @@ static void virtscsi_cancel_event_work(struct virtio_scsi *vscsi)
>  		cancel_work_sync(&vscsi->event_list[i].work);
>  }
>  
> +static unsigned int virtscsi_get_lun(u8 *lun_bytes)
> +{
> +	unsigned int lun = (lun_bytes[2] << 8) | lun_bytes[3];
> +	return lun & 16383;
> +}
> +

Why are you rolling your own incomplete version of scsilun_to_int here?

James

Re: [PATCH 1/2] virtio-scsi: fix parsing of hotplug/hot-unplug LUN number

[Paolo Bonzini]

Il 26/07/2012 10:52, James Bottomley ha scritto:
>> > +static unsigned int virtscsi_get_lun(u8 *lun_bytes)
>> > +{
>> > +	unsigned int lun = (lun_bytes[2] << 8) | lun_bytes[3];
>> > +	return lun & 16383;
>> > +}
>> > +
> Why are you rolling your own incomplete version of scsilun_to_int here?

Because scsilun_to_int does not do the AND, so it would have exactly the
same bug I'm fixing.

Paolo

Re: [PATCH 1/2] virtio-scsi: fix parsing of hotplug/hot-unplug LUN number

[James Bottomley]

On Thu, 2012-07-26 at 11:04 +0200, Paolo Bonzini wrote:
> Il 26/07/2012 10:52, James Bottomley ha scritto:
> >> > +static unsigned int virtscsi_get_lun(u8 *lun_bytes)
> >> > +{
> >> > +	unsigned int lun = (lun_bytes[2] << 8) | lun_bytes[3];
> >> > +	return lun & 16383;
> >> > +}
> >> > +
> > Why are you rolling your own incomplete version of scsilun_to_int here?
> 
> Because scsilun_to_int does not do the AND, so it would have exactly the
> same bug I'm fixing.

It's not a bug ... it's the encoding.  All the other devices use this
too.  Ideally we should have switched to 64 bit lun numbers for the
encoding to be exact, but nothing so far has gone over 32 bits.  If we
don't encode the Address method as part of the lun number, we don't get
the reverse transform right and the addressing often fails.

That does mean that arrays that use address method=1 in REPORT LUNS have
their lun numbers start at 16384.

James

Re: [PATCH 1/2] virtio-scsi: fix parsing of hotplug/hot-unplug LUN number

[Paolo Bonzini]

Il 26/07/2012 11:21, James Bottomley ha scritto:
>> > Because scsilun_to_int does not do the AND, so it would have exactly the
>> > same bug I'm fixing.
> It's not a bug ... it's the encoding.  All the other devices use this
> too.  Ideally we should have switched to 64 bit lun numbers for the
> encoding to be exact, but nothing so far has gone over 32 bits.  If we
> don't encode the Address method as part of the lun number, we don't get
> the reverse transform right and the addressing often fails.

But virtio-scsi gets it right even if you use method=0 and method=1
interchangeably.

ibmvscsi (see function lun_from_dev) does something similar to
virtio-scsi, except here I need both directions.

> That does mean that arrays that use address method=1 in REPORT LUNS have
> their lun numbers start at 16384.

That is ugly.  I can see how it may be needed on buggy hardware, but
here we know it's not.

Paolo

Re: [PATCH 1/2] virtio-scsi: fix parsing of hotplug/hot-unplug LUN number

[James Bottomley]

On Thu, 2012-07-26 at 11:27 +0200, Paolo Bonzini wrote:
> Il 26/07/2012 11:21, James Bottomley ha scritto:
> >> > Because scsilun_to_int does not do the AND, so it would have exactly the
> >> > same bug I'm fixing.
> > It's not a bug ... it's the encoding.  All the other devices use this
> > too.  Ideally we should have switched to 64 bit lun numbers for the
> > encoding to be exact, but nothing so far has gone over 32 bits.  If we
> > don't encode the Address method as part of the lun number, we don't get
> > the reverse transform right and the addressing often fails.
> 
> But virtio-scsi gets it right even if you use method=0 and method=1
> interchangeably.

I don't actually understand this statement.  LUNS < 256 may be encoded
either way (they should be encoded with address method=0 but they don't
have to be) if you address the array with the wrong method, it doesn't
have to give you your lun.  Therefore we have to map back to whatever
the array gave us.  Hence the 1:1 mapping.  You're proposing an
injective mapping, so you can't reliably reverse it; that's why we don't
do it that way in SCSI.

> ibmvscsi (see function lun_from_dev) does something similar to
> virtio-scsi, except here I need both directions.
> 
> > That does mean that arrays that use address method=1 in REPORT LUNS have
> > their lun numbers start at 16384.
> 
> That is ugly.  I can see how it may be needed on buggy hardware, but
> here we know it's not.

It's nothing to do with buggy hardware ... it's to do with having an
exact representation of the lun.

James

Re: [PATCH 1/2] virtio-scsi: fix parsing of hotplug/hot-unplug LUN number

[Paolo Bonzini]

Il 26/07/2012 11:41, James Bottomley ha scritto:
> On Thu, 2012-07-26 at 11:27 +0200, Paolo Bonzini wrote:
>> Il 26/07/2012 11:21, James Bottomley ha scritto:
>>>>> Because scsilun_to_int does not do the AND, so it would have exactly the
>>>>> same bug I'm fixing.
>>> It's not a bug ... it's the encoding.  All the other devices use this
>>> too.  Ideally we should have switched to 64 bit lun numbers for the
>>> encoding to be exact, but nothing so far has gone over 32 bits.  If we
>>> don't encode the Address method as part of the lun number, we don't get
>>> the reverse transform right and the addressing often fails.
>>
>> But virtio-scsi gets it right even if you use method=0 and method=1
>> interchangeably.
> 
> I don't actually understand this statement.  LUNS < 256 may be encoded
> either way (they should be encoded with address method=0 but they don't
> have to be) if you address the array with the wrong method, it doesn't
> have to give you your lun.

But virtio-scsi does, LUN "16384" and LUN 0 are the same.

If somebody wanted to add support for >16383 LUNs, we would do it with
the 4-byte encoding that is in SAM, but I don't see that happening.

> It's nothing to do with buggy hardware ...

Hardware that knows about format=1 LUNs, and yet treats LUN 0
differently depending on the encoding sounds buggy.

Of course some hardware may not know anything about format=1, so it is
wrong to pass format=1 unconditionally, but virtio-scsi does.

Paolo

Re: [PATCH 1/2] virtio-scsi: fix parsing of hotplug/hot-unplug LUN number

[Bart Van Assche]

On 07/26/12 09:21, James Bottomley wrote:
> On Thu, 2012-07-26 at 11:04 +0200, Paolo Bonzini wrote:
>> Il 26/07/2012 10:52, James Bottomley ha scritto:
>>>>> +static unsigned int virtscsi_get_lun(u8 *lun_bytes)
>>>>> +{
>>>>> +	unsigned int lun = (lun_bytes[2] << 8) | lun_bytes[3];
>>>>> +	return lun & 16383;
>>>>> +}
>>>>> +
>>> Why are you rolling your own incomplete version of scsilun_to_int here?
>>
>> Because scsilun_to_int does not do the AND, so it would have exactly the
>> same bug I'm fixing.
> 
> It's not a bug ... it's the encoding.  All the other devices use this
> too.  Ideally we should have switched to 64 bit lun numbers for the
> encoding to be exact, but nothing so far has gone over 32 bits.  If we
> don't encode the Address method as part of the lun number, we don't get
> the reverse transform right and the addressing often fails.
> 
> That does mean that arrays that use address method=1 in REPORT LUNS have
> their lun numbers start at 16384.

Has it already been considered to modify scsilun_to_int() such that LUN
numbers start at zero even for addressing method 1 ? This is what e.g.
the function scst_unpack_lun() already does. See also
http://scst.svn.sourceforge.net/viewvc/scst/trunk/scst/src/scst_lib.c?revision=HEAD&view=markup.

Bart.

Re: [PATCH 1/2] virtio-scsi: fix parsing of hotplug/hot-unplug LUN number

[James Bottomley]

On Thu, 2012-07-26 at 10:17 +0000, Bart Van Assche wrote:
> On 07/26/12 09:21, James Bottomley wrote:
> > On Thu, 2012-07-26 at 11:04 +0200, Paolo Bonzini wrote:
> >> Il 26/07/2012 10:52, James Bottomley ha scritto:
> >>>>> +static unsigned int virtscsi_get_lun(u8 *lun_bytes)
> >>>>> +{
> >>>>> +	unsigned int lun = (lun_bytes[2] << 8) | lun_bytes[3];
> >>>>> +	return lun & 16383;
> >>>>> +}
> >>>>> +
> >>> Why are you rolling your own incomplete version of scsilun_to_int here?
> >>
> >> Because scsilun_to_int does not do the AND, so it would have exactly the
> >> same bug I'm fixing.
> > 
> > It's not a bug ... it's the encoding.  All the other devices use this
> > too.  Ideally we should have switched to 64 bit lun numbers for the
> > encoding to be exact, but nothing so far has gone over 32 bits.  If we
> > don't encode the Address method as part of the lun number, we don't get
> > the reverse transform right and the addressing often fails.
> > 
> > That does mean that arrays that use address method=1 in REPORT LUNS have
> > their lun numbers start at 16384.
> 
> Has it already been considered to modify scsilun_to_int() such that LUN
> numbers start at zero even for addressing method 1 ? This is what e.g.
> the function scst_unpack_lun() already does. See also
> http://scst.svn.sourceforge.net/viewvc/scst/trunk/scst/src/scst_lib.c?revision=HEAD&view=markup.

Yes, as I said before, the problem is that the actual numbers are

1. not 1:1: there are several possible encodings of luns 0-255
2. hierarchical, so once you go beyond a single level you can't properly
use a numeric representation either.

the mid layer just uses the lun number as an encoding of the actual SAM
lun.  The key for us is that int_to_scsilun has to go back the other
way.

James

Re: [PATCH 1/2] virtio-scsi: fix parsing of hotplug/hot-unplug LUN number

[Paolo Bonzini]

Il 26/07/2012 12:33, James Bottomley ha scritto:
>> > Has it already been considered to modify scsilun_to_int() such that LUN
>> > numbers start at zero even for addressing method 1 ? This is what e.g.
>> > the function scst_unpack_lun() already does. See also
>> > http://scst.svn.sourceforge.net/viewvc/scst/trunk/scst/src/scst_lib.c?revision=HEAD&view=markup.
> Yes, as I said before, the problem is that the actual numbers are
> 
> 1. not 1:1: there are several possible encodings of luns 0-255
> 2. hierarchical, so once you go beyond a single level you can't properly
> use a numeric representation either.
> 
> the mid layer just uses the lun number as an encoding of the actual SAM
> lun.  The key for us is that int_to_scsilun has to go back the other
> way.

I still disagree, but I will modify QEMU so that this patch is not
necessary.  Later I can switch to int_to_scsilun.

Paolo

Re: [PATCH 1/2] virtio-scsi: fix parsing of hotplug/hot-unplug LUN number

[James Bottomley]

On Thu, 2012-07-26 at 12:43 +0200, Paolo Bonzini wrote:
> Il 26/07/2012 12:33, James Bottomley ha scritto:
> >> > Has it already been considered to modify scsilun_to_int() such that LUN
> >> > numbers start at zero even for addressing method 1 ? This is what e.g.
> >> > the function scst_unpack_lun() already does. See also
> >> > http://scst.svn.sourceforge.net/viewvc/scst/trunk/scst/src/scst_lib.c?revision=HEAD&view=markup.
> > Yes, as I said before, the problem is that the actual numbers are
> > 
> > 1. not 1:1: there are several possible encodings of luns 0-255
> > 2. hierarchical, so once you go beyond a single level you can't properly
> > use a numeric representation either.
> > 
> > the mid layer just uses the lun number as an encoding of the actual SAM
> > lun.  The key for us is that int_to_scsilun has to go back the other
> > way.
> 
> I still disagree, but I will modify QEMU so that this patch is not
> necessary.  Later I can switch to int_to_scsilun.

Thanks, on the principle of least surprise, you definitely don't want
the lun numbering to change depending on whether you directly attach to
the array or pass it through virtio-scsi.

James

[PATCH 2/2] virtio-scsi: support online resizing of disks

[Paolo Bonzini]

Support the LUN parameter change event.  Currently, the host fires this event
when the capacity of a disk is changed from the virtual machine monitor.
The resize then appears in the kernel log like this:

  sd 0:0:0:0: [sda] 46137344 512-byte logical blocks: (23.6 GB/22.0 GIb)
  sda: detected capacity change from 22548578304 to 23622320128

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/scsi/virtio_scsi.c  |   31 ++++++++++++++++++++++++++++++-
 include/linux/virtio_scsi.h |    2 ++
 2 files changed, 32 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index c937232..d8c6a57 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -285,6 +285,31 @@ static void virtscsi_handle_transport_reset(struct virtio_scsi *vscsi,
 	}
 }
 
+static void virtscsi_handle_param_change(struct virtio_scsi *vscsi,
+					 struct virtio_scsi_event *event)
+{
+	struct scsi_device *sdev;
+	struct Scsi_Host *shost = virtio_scsi_host(vscsi->vdev);
+	unsigned int target = event->lun[1];
+	unsigned int lun = virtscsi_get_lun(event->lun);
+	u8 asc = event->reason & 255;
+	u8 ascq = event->reason >> 8;
+
+	sdev = scsi_device_lookup(shost, 0, target, lun);
+	if (!sdev) {
+		pr_err("SCSI device %d 0 %d %d not found\n",
+			shost->host_no, target, lun);
+		return;
+	}
+
+	/* Handle "Parameters changed", "Mode parameters changed", and
+	   "Capacity data has changed".  */
+	if (asc == 0x2a && (ascq == 0x00 || ascq == 0x01 || ascq == 0x09))
+		scsi_rescan_device(&sdev->sdev_gendev);
+
+	scsi_device_put(sdev);
+}
+
 static void virtscsi_handle_event(struct work_struct *work)
 {
 	struct virtio_scsi_event_node *event_node =
@@ -303,6 +328,9 @@ static void virtscsi_handle_event(struct work_struct *work)
 	case VIRTIO_SCSI_T_TRANSPORT_RESET:
 		virtscsi_handle_transport_reset(vscsi, event);
 		break;
+	case VIRTIO_SCSI_T_PARAM_CHANGE:
+		virtscsi_handle_param_change(vscsi, event);
+		break;
 	default:
 		pr_err("Unsupport virtio scsi event %x\n", event->event);
 	}
@@ -739,7 +767,8 @@ static struct virtio_device_id id_table[] = {
 };
 
 static unsigned int features[] = {
-	VIRTIO_SCSI_F_HOTPLUG
+	VIRTIO_SCSI_F_HOTPLUG,
+	VIRTIO_SCSI_F_CHANGE,
 };
 
 static struct virtio_driver virtio_scsi_driver = {
diff --git a/include/linux/virtio_scsi.h b/include/linux/virtio_scsi.h
index dc8d305..d6b4440 100644
--- a/include/linux/virtio_scsi.h
+++ b/include/linux/virtio_scsi.h
@@ -72,6 +72,7 @@ struct virtio_scsi_config {
 /* Feature Bits */
 #define VIRTIO_SCSI_F_INOUT                    0
 #define VIRTIO_SCSI_F_HOTPLUG                  1
+#define VIRTIO_SCSI_F_CHANGE                   2
 
 /* Response codes */
 #define VIRTIO_SCSI_S_OK                       0
@@ -108,6 +109,7 @@ struct virtio_scsi_config {
 #define VIRTIO_SCSI_T_NO_EVENT                 0
 #define VIRTIO_SCSI_T_TRANSPORT_RESET          1
 #define VIRTIO_SCSI_T_ASYNC_NOTIFY             2
+#define VIRTIO_SCSI_T_PARAM_CHANGE             3
 
 /* Reasons of transport reset event */
 #define VIRTIO_SCSI_EVT_RESET_HARD             0
-- 
1.7.1