iscsi: Fix bs->request_alignment failure on LUN 0

PATCH In review Kevin Wolf 4 participants 10 messages Mar 5, 2014 Block layer

Share & tools

Kevin Wolf

author

2014-03-05 15:01 UTC #0 lore raw

cover iscsi: Fix bs->request_alignment failure on LUN 0

Kevin Wolf (2):
iscsi: Use bs->sg for everything else than disks
block: Fix bs->request_alignment assertion for bs->sg=1

 block.c       | 2 +-
 block/iscsi.c | 9 ++++-----
 2 files changed, 5 insertions(+), 6 deletions(-)

Signature · 1 line

1.8.1.4

Kevin Wolf

author

2014-03-05 15:01 UTC #1 lore raw

PATCH 1/2 iscsi: Use bs->sg for everything else than disks

The current iscsi block driver code makes the rather arbitrary decision that TYPE_MEDIUM_CHANGER and TYPE_TAPE devices have bs->sg = 1 and all other device types are disks.

Instead of this, check for TYPE_DISK to expose the disk interface and make everything else bs->sg = 1. In particular, this includes devices with TYPE_STORAGE_ARRAY, which is what LUN 0 of an iscsi target is. (See https://bugzilla.redhat.com/show_bug.cgi?id=1067784 for the exact scenario.)

Signed-off-by Kevin Wolf <[email protected]>

---
 block/iscsi.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

block/iscsi.c +4 -5

index 0a15f53..b490e98 100644
@@ -1231,12 +1231,11 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     bs->total_sectors = sector_lun2qemu(iscsilun->num_blocks, iscsilun);
     bs->request_alignment = iscsilun->block_size;
 
-    /* Medium changer or tape. We dont have any emulation for this so this must
-     * be sg ioctl compatible. We force it to be sg, otherwise qemu will try
-     * to read from the device to guess the image format.
+    /* We don't have any emulation for devices other than disks and CD-ROMs, so
+     * this must be sg ioctl compatible. We force it to be sg, otherwise qemu
+     * will try to read from the device to guess the image format.
      */
-    if (iscsilun->type == TYPE_MEDIUM_CHANGER ||
-        iscsilun->type == TYPE_TAPE) {
+    if (iscsilun->type != TYPE_DISK && iscsilun->type != TYPE_ROM) {
         bs->sg = 1;
     }

Signature · 1 line

1.8.1.4

Benoît Canet

2014-03-05 15:33 UTC #2 lore raw

Re: [Qemu-devel] [PATCH 1/2] iscsi: Use bs->sg for everything else than disks

Quoted 23 lines · Kevin Wolf

The current iscsi block driver code makes the rather arbitrary decision that TYPE_MEDIUM_CHANGER and TYPE_TAPE devices have bs->sg = 1 and all other device types are disks.
Instead of this, check for TYPE_DISK to expose the disk interface and make everything else bs->sg = 1. In particular, this includes devices with TYPE_STORAGE_ARRAY, which is what LUN 0 of an iscsi target is. (See https://bugzilla.redhat.com/show_bug.cgi?id=1067784 for the exact scenario.)
Signed-off-by Kevin Wolf <[email protected]>
---
 block/iscsi.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
block/iscsi.c +0 -0
index 0a15f53..b490e98 100644
@@ -1231,12 +1231,11 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     bs->total_sectors = sector_lun2qemu(iscsilun->num_blocks, iscsilun);
     bs->request_alignment = iscsilun->block_size;
 

block/iscsi.c static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, · line 1238

Quoted 10 lines

block/iscsi.c +4 -5

-    /* Medium changer or tape. We dont have any emulation for this so this must
-     * be sg ioctl compatible. We force it to be sg, otherwise qemu will try
-     * to read from the device to guess the image format.
+    /* We don't have any emulation for devices other than disks and CD-ROMs, so
+     * this must be sg ioctl compatible. We force it to be sg, otherwise qemu
+     * will try to read from the device to guess the image format.
      */
-    if (iscsilun->type == TYPE_MEDIUM_CHANGER ||
-        iscsilun->type == TYPE_TAPE) {
+    if (iscsilun->type != TYPE_DISK && iscsilun->type != TYPE_ROM) {

I don't understand you are talking about type TYPE_STORAGE_ARRAY in the commit message but testing against TYPE_ROM and they are not the same. Maybe a comment explaining the role of TYPE_ROM would give a clue to non scsi savy persons like me.

Best regards

Benoît

Quoted 7 lines

block/iscsi.c +0 -0
         bs->sg = 1;
     }
 
Signature · 1 line
1.8.1.4

Kevin Wolf

author

2014-03-05 15:41 UTC #3 lore raw

Re: [Qemu-devel] [PATCH 1/2] iscsi: Use bs->sg for everything else than disks

Am 05.03.2014 um 16:33 hat Benoît Canet geschrieben:

Quoted 24 lines

Quoted 23 lines · Kevin Wolf
The current iscsi block driver code makes the rather arbitrary decision that TYPE_MEDIUM_CHANGER and TYPE_TAPE devices have bs->sg = 1 and all other device types are disks.
Instead of this, check for TYPE_DISK to expose the disk interface and make everything else bs->sg = 1. In particular, this includes devices with TYPE_STORAGE_ARRAY, which is what LUN 0 of an iscsi target is. (See https://bugzilla.redhat.com/show_bug.cgi?id=1067784 for the exact scenario.)
Signed-off-by Kevin Wolf <[email protected]>
---
 block/iscsi.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
block/iscsi.c +0 -0
index 0a15f53..b490e98 100644
@@ -1231,12 +1231,11 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     bs->total_sectors = sector_lun2qemu(iscsilun->num_blocks, iscsilun);
     bs->request_alignment = iscsilun->block_size;
 

Quoted 14 lines

block/iscsi.c static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, · line 1238
Quoted 10 lines
block/iscsi.c +4 -5
-    /* Medium changer or tape. We dont have any emulation for this so this must
-     * be sg ioctl compatible. We force it to be sg, otherwise qemu will try
-     * to read from the device to guess the image format.
+    /* We don't have any emulation for devices other than disks and CD-ROMs, so
+     * this must be sg ioctl compatible. We force it to be sg, otherwise qemu
+     * will try to read from the device to guess the image format.
      */
-    if (iscsilun->type == TYPE_MEDIUM_CHANGER ||
-        iscsilun->type == TYPE_TAPE) {
+    if (iscsilun->type != TYPE_DISK && iscsilun->type != TYPE_ROM) {
I don't understand you are talking about type TYPE_STORAGE_ARRAY in the commit message but testing against TYPE_ROM and they are not the same. Maybe a comment explaining the role of TYPE_ROM would give a clue to non scsi savy persons like me.

TYPE_STORAGE_ARRAY is what exposed the bug (see the Bugzilla reference). It was previously bs->sg = 0, but becomes now bs->sg = 1. The reason for this is that the usual block layer operations only work on disks (i.e. hard disks = TYPE_DISK and CD-ROM drives = TYPE_ROM), so for all the rest we can only expose a generic SCSI device, but no real block device.

Kevin

Benoît Canet

Reviewed-by Benoit Canet

2014-03-05 15:47 UTC #4 lore raw

Re: [Qemu-devel] [PATCH 1/2] iscsi: Use bs->sg for everything else than disks

Quoted 40 lines · Kevin Wolf

Am 05.03.2014 um 16:33 hat Benoît Canet geschrieben:
Quoted 24 lines
Quoted 23 lines · Kevin Wolf
The current iscsi block driver code makes the rather arbitrary decision that TYPE_MEDIUM_CHANGER and TYPE_TAPE devices have bs->sg = 1 and all other device types are disks.
Instead of this, check for TYPE_DISK to expose the disk interface and make everything else bs->sg = 1. In particular, this includes devices with TYPE_STORAGE_ARRAY, which is what LUN 0 of an iscsi target is. (See https://bugzilla.redhat.com/show_bug.cgi?id=1067784 for the exact scenario.)
Signed-off-by Kevin Wolf <[email protected]>
---
 block/iscsi.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
block/iscsi.c +0 -0
index 0a15f53..b490e98 100644
@@ -1231,12 +1231,11 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     bs->total_sectors = sector_lun2qemu(iscsilun->num_blocks, iscsilun);
     bs->request_alignment = iscsilun->block_size;
 
Quoted 14 lines
block/iscsi.c static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, · line 1238
Quoted 10 lines
block/iscsi.c +4 -5
-    /* Medium changer or tape. We dont have any emulation for this so this must
-     * be sg ioctl compatible. We force it to be sg, otherwise qemu will try
-     * to read from the device to guess the image format.
+    /* We don't have any emulation for devices other than disks and CD-ROMs, so
+     * this must be sg ioctl compatible. We force it to be sg, otherwise qemu
+     * will try to read from the device to guess the image format.
      */
-    if (iscsilun->type == TYPE_MEDIUM_CHANGER ||
-        iscsilun->type == TYPE_TAPE) {
+    if (iscsilun->type != TYPE_DISK && iscsilun->type != TYPE_ROM) {
I don't understand you are talking about type TYPE_STORAGE_ARRAY in the commit message but testing against TYPE_ROM and they are not the same. Maybe a comment explaining the role of TYPE_ROM would give a clue to non scsi savy persons like me.

Quoted 5 lines

TYPE_STORAGE_ARRAY is what exposed the bug (see the Bugzilla reference). It was previously bs->sg = 0, but becomes now bs->sg = 1. The reason for this is that the usual block layer operations only work on disks (i.e. hard disks = TYPE_DISK and CD-ROM drives = TYPE_ROM), so for all the rest we can only expose a generic SCSI device, but no real block device.

Ok I didn't knew that TYPE_ROM == type of CDROM. It's simpler now.

Reviewed-by Benoit Canet <[email protected]>

Quoted 3 lines

Kevin

Peter Lieven

2014-03-05 16:02 UTC #5 lore raw

Re: [Qemu-devel] [PATCH 1/2] iscsi: Use bs->sg for everything else than disks

In reply to Kevin Wolf

Am 05.03.2014 16:00, schrieb Kevin Wolf:

Quoted 26 lines

The current iscsi block driver code makes the rather arbitrary decision that TYPE_MEDIUM_CHANGER and TYPE_TAPE devices have bs->sg = 1 and all other device types are disks.
Instead of this, check for TYPE_DISK to expose the disk interface and make everything else bs->sg = 1. In particular, this includes devices with TYPE_STORAGE_ARRAY, which is what LUN 0 of an iscsi target is. (See https://bugzilla.redhat.com/show_bug.cgi?id=1067784 for the exact scenario.)
Signed-off-by Kevin Wolf <[email protected]>
---
 block/iscsi.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
block/iscsi.c +0 -3
index 0a15f53..b490e98 100644
@@ -1231,12 +1231,11 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     bs->total_sectors = sector_lun2qemu(iscsilun->num_blocks, iscsilun);
     bs->request_alignment = iscsilun->block_size;
 
-    /* Medium changer or tape. We dont have any emulation for this so this must
-     * be sg ioctl compatible. We force it to be sg, otherwise qemu will try
-     * to read from the device to guess the image format.

block/iscsi.c static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, · line 1241

Quoted 10 lines

block/iscsi.c +4 -2

+    /* We don't have any emulation for devices other than disks and CD-ROMs, so
+     * this must be sg ioctl compatible. We force it to be sg, otherwise qemu
+     * will try to read from the device to guess the image format.
      */
-    if (iscsilun->type == TYPE_MEDIUM_CHANGER ||
-        iscsilun->type == TYPE_TAPE) {
+    if (iscsilun->type != TYPE_DISK && iscsilun->type != TYPE_ROM) {
         bs->sg = 1;
     }

We have ioctl support in the iscsi block driver only for Linux. Is this a problem?

Peter

Kevin Wolf

author

2014-03-05 16:22 UTC #6 lore raw

Re: [Qemu-devel] [PATCH 1/2] iscsi: Use bs->sg for everything else than disks

Am 05.03.2014 um 17:02 hat Peter Lieven geschrieben:

Quoted 24 lines

Am 05.03.2014 16:00, schrieb Kevin Wolf:
Quoted 23 lines
The current iscsi block driver code makes the rather arbitrary decision that TYPE_MEDIUM_CHANGER and TYPE_TAPE devices have bs->sg = 1 and all other device types are disks.
Instead of this, check for TYPE_DISK to expose the disk interface and make everything else bs->sg = 1. In particular, this includes devices with TYPE_STORAGE_ARRAY, which is what LUN 0 of an iscsi target is. (See https://bugzilla.redhat.com/show_bug.cgi?id=1067784 for the exact scenario.)
Signed-off-by Kevin Wolf <[email protected]>
---
 block/iscsi.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
block/iscsi.c +0 -0
index 0a15f53..b490e98 100644
@@ -1231,12 +1231,11 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     bs->total_sectors = sector_lun2qemu(iscsilun->num_blocks, iscsilun);
     bs->request_alignment = iscsilun->block_size;
 

Quoted 14 lines

block/iscsi.c static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, · line 1241

Quoted 13 lines

block/iscsi.c +4 -5

-    /* Medium changer or tape. We dont have any emulation for this so this must
-     * be sg ioctl compatible. We force it to be sg, otherwise qemu will try
-     * to read from the device to guess the image format.
+    /* We don't have any emulation for devices other than disks and CD-ROMs, so
+     * this must be sg ioctl compatible. We force it to be sg, otherwise qemu
+     * will try to read from the device to guess the image format.
      */
-    if (iscsilun->type == TYPE_MEDIUM_CHANGER ||
-        iscsilun->type == TYPE_TAPE) {
+    if (iscsilun->type != TYPE_DISK && iscsilun->type != TYPE_ROM) {
         bs->sg = 1;
     }

We have ioctl support in the iscsi block driver only for Linux. Is this a problem?

Should be okay. It will cause the requests to fail, obviously (bdrv_ioctl returns -ENOTSUP, and bdrv_aio_ioctl returns NULL), but callers seem to cope with it and if we don't know how to handle the request, that's the right thing to do.

Kevin

Kevin Wolf

author

2014-03-05 15:01 UTC #7 lore raw

PATCH 2/2 block: Fix bs->request_alignment assertion for bs->sg=1

In reply to Kevin Wolf

For sg backends, bs->request_alignment is meaningless and may be 0.

Signed-off-by Kevin Wolf <[email protected]>

---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

block.c +1 -1

index 38bbdf3..f01b91c 100644
@@ -935,7 +935,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
 
     bdrv_refresh_limits(bs);
     assert(bdrv_opt_mem_align(bs) != 0);
-    assert(bs->request_alignment != 0);
+    assert((bs->request_alignment != 0) || bs->sg);
 
 #ifndef _WIN32
     if (bs->is_temporary) {

Signature · 1 line

1.8.1.4

Benoît Canet

Reviewed-by Benoit Canet on v1 2/2

2014-03-05 15:34 UTC #8 lore raw

Re: [Qemu-devel] [PATCH 2/2] block: Fix bs->request_alignment assertion for bs->sg=1

Quoted 24 lines · Kevin Wolf

For sg backends, bs->request_alignment is meaningless and may be 0.

Signed-off-by Kevin Wolf <[email protected]>

---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

block.c +1 -1

index 38bbdf3..f01b91c 100644
@@ -935,7 +935,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
 
     bdrv_refresh_limits(bs);
     assert(bdrv_opt_mem_align(bs) != 0);
-    assert(bs->request_alignment != 0);
+    assert((bs->request_alignment != 0) || bs->sg);
 
 #ifndef _WIN32
     if (bs->is_temporary) {

Signature · 1 line

1.8.1.4

Reviewed-by Benoit Canet <[email protected]>

Paolo Bonzini

2014-03-05 15:31 UTC #9 lore raw

In reply to Kevin Wolf

Il 05/03/2014 16:00, Kevin Wolf ha scritto:

Quoted 8 lines

Kevin Wolf (2):
iscsi: Use bs->sg for everything else than disks
block: Fix bs->request_alignment assertion for bs->sg=1
 block.c       | 2 +-
 block/iscsi.c | 9 ++++-----
 2 files changed, 5 insertions(+), 6 deletions(-)

Looks good. Thanks!

Paolo