bsg: Error print if device is not bidi capable when refusing a bidi command

PATCH In review Boaz Harrosh 3 participants 9 messages Jan 20, 2009 – Jan 21, 2009

Subsystems (3)

SCSI Block layer BSG (block layer generic sg v4 driver)

lore

Share & tools

lore

Boaz Harrosh

author

2009-01-20 14:13 UTC #0 lore raw

PATCH 1/1 bsg: Error print if device is not bidi capable when refusing a bidi command

If a bidi command was issued to a request_queue not mark as QUEUE_FLAG_BIDI. Issue an error report. This is a misconfiguration an administrator would like to know about, which is otherwise hard to detect.

Signed-off-by Boaz Harrosh <[email protected]>

---
 block/bsg.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

block/bsg.c +2 -0

index 44a2a0f..977547a 100644
@@ -270,6 +270,8 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
 
 	if (rw == WRITE && hdr->din_xfer_len) {
 		if (!test_bit(QUEUE_FLAG_BIDI, &q->queue_flags)) {
+			printk(KERN_ERR "bsg: Attempt to send a bidi command "
+			       "to a none bidi device\n");
 			ret = -EOPNOTSUPP;
 			goto out;
 		}

Signature · 1 line

1.6.0.1

Jens Axboe

2009-01-20 14:16 UTC #1 lore raw

Quoted 5 lines · Boaz Harrosh

If a bidi command was issued to a request_queue not mark as QUEUE_FLAG_BIDI. Issue an error report. This is a misconfiguration an administrator would like to know about, which is otherwise hard to detect.

At the very least, put rate limit on that printk.

Quoted 22 lines

Signed-off-by Boaz Harrosh <[email protected]>

---
 block/bsg.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

block/bsg.c +2 -0

index 44a2a0f..977547a 100644
@@ -270,6 +270,8 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
 
 	if (rw == WRITE && hdr->din_xfer_len) {
 		if (!test_bit(QUEUE_FLAG_BIDI, &q->queue_flags)) {
+			printk(KERN_ERR "bsg: Attempt to send a bidi command "
+			       "to a none bidi device\n");
 			ret = -EOPNOTSUPP;
 			goto out;
 		}

Signature · 1 line

1.6.0.1

Signature · 1 line

Jens Axboe

Boaz Harrosh

author

2009-01-20 14:38 UTC #2 lore raw

PATCH 1/1 bsg: Error print if device is not bidi capable when refusing a bidi command

Signed-off-by Boaz Harrosh <[email protected]>

---
 block/bsg.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

block/bsg.c +5 -0

index 44a2a0f..1da14fe 100644
@@ -270,6 +270,11 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
 
 	if (rw == WRITE && hdr->din_xfer_len) {
 		if (!test_bit(QUEUE_FLAG_BIDI, &q->queue_flags)) {
+			if (printk_ratelimit()) {
+				printk(KERN_ERR
+					"bsg: Attempt to send a bidi command "
+					"to a none bidi device\n");
+			}
 			ret = -EOPNOTSUPP;
 			goto out;
 		}

Signature · 1 line

1.6.0.1

Jens Axboe

2009-01-20 14:47 UTC #3 lore raw

Quoted 29 lines · Boaz Harrosh

If a bidi command was issued to a request_queue not mark as QUEUE_FLAG_BIDI. Issue an error report. This is a misconfiguration an administrator would like to know about, which is otherwise hard to detect.
Signed-off-by Boaz Harrosh <[email protected]>
---
 block/bsg.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
block/bsg.c +5 -0
index 44a2a0f..1da14fe 100644
@@ -270,6 +270,11 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
 
 	if (rw == WRITE && hdr->din_xfer_len) {
 		if (!test_bit(QUEUE_FLAG_BIDI, &q->queue_flags)) {
+			if (printk_ratelimit()) {
+				printk(KERN_ERR
+					"bsg: Attempt to send a bidi command "
+					"to a none bidi device\n");
+			}
 			ret = -EOPNOTSUPP;
 			goto out;
 		}
Signature · 1 line
1.6.0.1

Thanks, that works. But why isn't the -EOPNOTSUPP error return (which the app can see and print info about) enough? Do we really need to put this in the kernel log?

Signature · 1 line

Jens Axboe

Boaz Harrosh

author

2009-01-20 15:01 UTC #4 lore raw

Quoted 17 lines · Jens Axboe

Quoted 16 lines · Boaz Harrosh
If a bidi command was issued to a request_queue not mark as QUEUE_FLAG_BIDI. Issue an error report. This is a misconfiguration an administrator would like to know about, which is otherwise hard to detect.
Signed-off-by Boaz Harrosh <[email protected]>
---
 block/bsg.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
block/bsg.c +0 -0
index 44a2a0f..1da14fe 100644
@@ -270,6 +270,11 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
 

Quoted 17 lines

Quoted 12 lines
block/bsg.c +5 -0
 	if (rw == WRITE && hdr->din_xfer_len) {
 		if (!test_bit(QUEUE_FLAG_BIDI, &q->queue_flags)) {
+			if (printk_ratelimit()) {
+				printk(KERN_ERR
+					"bsg: Attempt to send a bidi command "
+					"to a none bidi device\n");
+			}
 			ret = -EOPNOTSUPP;
 			goto out;
 		}
Signature · 1 line
1.6.0.1
Thanks, that works. But why isn't the -EOPNOTSUPP error return (which the app can see and print info about) enough? Do we really need to put this in the kernel log?

This is your call, I'm not very experienced with this things but ...

What I was thinking is in situations when things used to work fine, and then later could stop working because of seemingly unrelated changes. For example: The same iscsi target behind a new accelerated-iscsi-card like cxgb3i, qla4xxx or iSER would stop working, since bidi is not enabled for them.

But let me check I had problems with the return value. I'll try to detect this condition from the application and report in the proper manner. Just that I thought of the poor Admin that needs to fix that after the fact... (me ;))

Thanks
Boaz

Boaz Harrosh

author

2009-01-20 15:31 UTC #5 lore raw

Quoted 8 lines · Boaz Harrosh

Quoted 4 lines · Jens Axboe
Thanks, that works. But why isn't the -EOPNOTSUPP error return (which the app can see and print info about) enough? Do we really need to put this in the kernel log?
But let me check I had problems with the return value.

OK I found my problem, blush:

- ret = ioctl(fd, SG_IO, &sg);
+ ret = ioctl(fd, SG_IO, &sg) ? -errno : 0;

So if I do a print to stderr in the lowest-most library will that be visible enough to an Admin. I have utils like osd_format, mkfs.exofs, a FUSE filesystem ...
OK I can always redirect stderr to the dmsg log

Thanks
Boaz

Jens Axboe

2009-01-21 07:54 UTC #6 lore raw

Quoted 15 lines · Boaz Harrosh

Quoted 8 lines · Boaz Harrosh
Quoted 4 lines · Jens Axboe
Thanks, that works. But why isn't the -EOPNOTSUPP error return (which the app can see and print info about) enough? Do we really need to put this in the kernel log?
But let me check I had problems with the return value.
OK I found my problem, blush:
(quoted diff) +1 -1
-		ret = ioctl(fd, SG_IO, &sg);
+		ret = ioctl(fd, SG_IO, &sg) ? -errno : 0;

Quoted 4 lines

So if I do a print to stderr in the lowest-most library will that be visible enough to an Admin. I have utils like osd_format, mkfs.exofs, a FUSE filesystem ...
OK I can always redirect stderr to the dmsg log

Goodie, that saves us that printk() :-)

Signature · 1 line

Jens Axboe

FUJITA Tomonori

maintainer

2009-01-20 23:10 UTC #7 lore raw

In reply to Boaz Harrosh

Quoted 16 lines · On Tue, 20 Jan 2009 16:38:38 +0200 Boaz Harrosh <[email protected]> wrote:

If a bidi command was issued to a request_queue not mark as QUEUE_FLAG_BIDI. Issue an error report. This is a misconfiguration an administrator would like to know about, which is otherwise hard to detect.
Signed-off-by Boaz Harrosh <[email protected]>
---
 block/bsg.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
block/bsg.c +0 -0
index 44a2a0f..1da14fe 100644

block/bsg.c bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm, · line 279

Quoted 10 lines

block/bsg.c +5 -0

@@ -270,6 +270,11 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
 
 	if (rw == WRITE && hdr->din_xfer_len) {
 		if (!test_bit(QUEUE_FLAG_BIDI, &q->queue_flags)) {
+			if (printk_ratelimit()) {
+				printk(KERN_ERR
+					"bsg: Attempt to send a bidi command "
+					"to a none bidi device\n");
+			}
 			ret = -EOPNOTSUPP;
 			goto out;

I don't think that we need this patch. Returning -EOPNOTSUPP should be enough.

Boaz Harrosh

author

2009-01-20 14:41 UTC #8 lore raw

In reply to Jens Axboe

Quoted 8 lines · Jens Axboe

Quoted 4 lines · Boaz Harrosh
If a bidi command was issued to a request_queue not mark as QUEUE_FLAG_BIDI. Issue an error report. This is a misconfiguration an administrator would like to know about, which is otherwise hard to detect.
At the very least, put rate limit on that printk.

Thanks, good catch it could be a DOS other wise. I did not know of this facility. Please check if done right?

Quoted 5 lines

Quoted 5 lines
Signed-off-by Boaz Harrosh <[email protected]>
---
 block/bsg.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Boaz