Re: bidi bsg is non-blocking
From: [EMAIL PROTECTED]
Subject: bidi bsg is non-blocking
Date: Mon, 7 May 2007 10:21:18 -0500
I'm attempting to use the bidi variant of bsg to talk to an OSD target
device. I've run into an undesirable situation.
My application has a free-running receive loop (doing a read() on the bsg
device) waiting for responses to commands sent to bsg by another thread.
The problem is that the receive thread is getting ENODATA from the read()
when there are no commands pending. I have NOT set non-blocking.
Note that the old sg driver was quite willing to block until there was a
response available. I find this scenario greatly preferable.
Could bsg be fixed so that read() blocks when there is nothing to return?
I think that this is a bug.
This patch is against the bsg branch in the Jens' git tree.
I guess that it would be nice to do more cleanups on these parts.
From 52a9fcf0af72f212ddd93918b7f9f0f0e706c215 Mon Sep 17 00:00:00 2001
From: FUJITA Tomonori [EMAIL PROTECTED]
Date: Tue, 8 May 2007 15:57:43 +0900
Subject: [PATCH] fix read ENODATA bug
This patch fixes a bug that read() gives ENODATA even with a blocking
file descriptor when there are no commands pending.
This also includes some cleanups.
Signed-off-by: FUJITA Tomonori [EMAIL PROTECTED]
---
block/bsg.c | 85 ++
1 files changed, 21 insertions(+), 64 deletions(-)
diff --git a/block/bsg.c b/block/bsg.c
index a333c93..7fcc77a 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -115,9 +115,9 @@ static void bsg_free_command(struct bsg_
wake_up(bd-wq_free);
}
-static struct bsg_command *__bsg_alloc_command(struct bsg_device *bd)
+static struct bsg_command *bsg_alloc_command(struct bsg_device *bd)
{
- struct bsg_command *bc = NULL;
+ struct bsg_command *bc = ERR_PTR(-EINVAL);
spin_lock_irq(bd-lock);
@@ -131,6 +131,7 @@ static struct bsg_command *__bsg_alloc_c
if (unlikely(!bc)) {
spin_lock_irq(bd-lock);
bd-queued_cmds--;
+ bc = ERR_PTR(-ENOMEM);
goto out;
}
@@ -198,30 +199,6 @@ unlock:
return ret;
}
-/*
- * get a new free command, blocking if needed and specified
- */
-static struct bsg_command *bsg_get_command(struct bsg_device *bd)
-{
- struct bsg_command *bc;
- int ret;
-
- do {
- bc = __bsg_alloc_command(bd);
- if (bc)
- break;
-
- ret = bsg_io_schedule(bd, TASK_INTERRUPTIBLE);
- if (ret) {
- bc = ERR_PTR(ret);
- break;
- }
-
- } while (1);
-
- return bc;
-}
-
static int blk_fill_sgv4_hdr_rq(request_queue_t *q, struct request *rq,
struct sg_io_v4 *hdr, int has_write_perm)
{
@@ -397,7 +374,7 @@ static inline struct bsg_command *bsg_ne
/*
* Get a finished command from the done list
*/
-static struct bsg_command *__bsg_get_done_cmd(struct bsg_device *bd, int state)
+static struct bsg_command *bsg_get_done_cmd(struct bsg_device *bd)
{
struct bsg_command *bc;
int ret;
@@ -407,11 +384,17 @@ static struct bsg_command *__bsg_get_don
if (bc)
break;
- ret = bsg_io_schedule(bd, state);
- if (ret) {
- bc = ERR_PTR(ret);
+ if (!test_bit(BSG_F_BLOCK, bd-flags)) {
+ bc = ERR_PTR(-EAGAIN);
break;
}
+
+ ret = wait_event_interruptible(bd-wq_done, bd-done_cmds);
+ if (ret) {
+ bc = ERR_PTR(-ERESTARTSYS);
+ break;
+ } else
+ continue;
} while (1);
dprintk(%s: returning done %p\n, bd-name, bc);
@@ -419,18 +402,6 @@ static struct bsg_command *__bsg_get_don
return bc;
}
-static struct bsg_command *
-bsg_get_done_cmd(struct bsg_device *bd, const struct iovec *iov)
-{
- return __bsg_get_done_cmd(bd, TASK_INTERRUPTIBLE);
-}
-
-static struct bsg_command *
-bsg_get_done_cmd_nosignals(struct bsg_device *bd)
-{
- return __bsg_get_done_cmd(bd, TASK_UNINTERRUPTIBLE);
-}
-
static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
struct bio *bio)
{
@@ -492,22 +463,13 @@ static int bsg_complete_all_commands(str
} while (ret != -ENODATA);
/*
-* discard done commands
+* discard done commands.
*/
ret = 0;
do {
- bc = bsg_get_done_cmd_nosignals(bd);
-
- /*
-* we _must_ complete before restarting, because
-* bsg_release can't handle this failing.
-*/
- if (PTR_ERR(bc) == -ERESTARTSYS)
- continue;
- if (IS_ERR(bc)) {
-
Re: bidi bsg is non-blocking
From: FUJITA Tomonori [EMAIL PROTECTED]
Subject: Re: bidi bsg is non-blocking
Date: Tue, 08 May 2007 16:23:26 +0900
From: [EMAIL PROTECTED]
Subject: bidi bsg is non-blocking
Date: Mon, 7 May 2007 10:21:18 -0500
I'm attempting to use the bidi variant of bsg to talk to an OSD target
device. I've run into an undesirable situation.
My application has a free-running receive loop (doing a read() on the bsg
device) waiting for responses to commands sent to bsg by another thread.
The problem is that the receive thread is getting ENODATA from the read()
when there are no commands pending. I have NOT set non-blocking.
Note that the old sg driver was quite willing to block until there was a
response available. I find this scenario greatly preferable.
Could bsg be fixed so that read() blocks when there is nothing to return?
I think that this is a bug.
This patch is against the bsg branch in the Jens' git tree.
I guess that it would be nice to do more cleanups on these parts.
From 52a9fcf0af72f212ddd93918b7f9f0f0e706c215 Mon Sep 17 00:00:00 2001
From: FUJITA Tomonori [EMAIL PROTECTED]
Date: Tue, 8 May 2007 15:57:43 +0900
Subject: [PATCH] fix read ENODATA bug
This patch fixes a bug that read() gives ENODATA even with a blocking
file descriptor when there are no commands pending.
This also includes some cleanups.
Signed-off-by: FUJITA Tomonori [EMAIL PROTECTED]
Oops, I sent a wrong patch. This is a right one.
---
From 115a65feaadae0c100b5366670eb729e9e6fd243 Mon Sep 17 00:00:00 2001
From: FUJITA Tomonori [EMAIL PROTECTED]
Date: Tue, 8 May 2007 16:28:36 +0900
Subject: [PATCH] fix read ENODATA bug
This patch fixes a bug that read() gives ENODATA even with a blocking
file descriptor when there are no commands pending.
This also includes some cleanups.
Signed-off-by: FUJITA Tomonori [EMAIL PROTECTED]
---
block/bsg.c | 89 +-
1 files changed, 26 insertions(+), 63 deletions(-)
diff --git a/block/bsg.c b/block/bsg.c
index a333c93..a5a71fc 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -115,9 +115,9 @@ static void bsg_free_command(struct bsg_
wake_up(bd-wq_free);
}
-static struct bsg_command *__bsg_alloc_command(struct bsg_device *bd)
+static struct bsg_command *bsg_alloc_command(struct bsg_device *bd)
{
- struct bsg_command *bc = NULL;
+ struct bsg_command *bc = ERR_PTR(-EINVAL);
spin_lock_irq(bd-lock);
@@ -131,6 +131,7 @@ static struct bsg_command *__bsg_alloc_c
if (unlikely(!bc)) {
spin_lock_irq(bd-lock);
bd-queued_cmds--;
+ bc = ERR_PTR(-ENOMEM);
goto out;
}
@@ -198,30 +199,6 @@ unlock:
return ret;
}
-/*
- * get a new free command, blocking if needed and specified
- */
-static struct bsg_command *bsg_get_command(struct bsg_device *bd)
-{
- struct bsg_command *bc;
- int ret;
-
- do {
- bc = __bsg_alloc_command(bd);
- if (bc)
- break;
-
- ret = bsg_io_schedule(bd, TASK_INTERRUPTIBLE);
- if (ret) {
- bc = ERR_PTR(ret);
- break;
- }
-
- } while (1);
-
- return bc;
-}
-
static int blk_fill_sgv4_hdr_rq(request_queue_t *q, struct request *rq,
struct sg_io_v4 *hdr, int has_write_perm)
{
@@ -397,7 +374,7 @@ static inline struct bsg_command *bsg_ne
/*
* Get a finished command from the done list
*/
-static struct bsg_command *__bsg_get_done_cmd(struct bsg_device *bd, int state)
+static struct bsg_command *bsg_get_done_cmd(struct bsg_device *bd)
{
struct bsg_command *bc;
int ret;
@@ -407,11 +384,17 @@ static struct bsg_command *__bsg_get_don
if (bc)
break;
- ret = bsg_io_schedule(bd, state);
- if (ret) {
- bc = ERR_PTR(ret);
+ if (!test_bit(BSG_F_BLOCK, bd-flags)) {
+ bc = ERR_PTR(-EAGAIN);
break;
}
+
+ ret = wait_event_interruptible(bd-wq_done, bd-done_cmds);
+ if (ret) {
+ bc = ERR_PTR(-ERESTARTSYS);
+ break;
+ } else
+ continue;
} while (1);
dprintk(%s: returning done %p\n, bd-name, bc);
@@ -419,18 +402,6 @@ static struct bsg_command *__bsg_get_don
return bc;
}
-static struct bsg_command *
-bsg_get_done_cmd(struct bsg_device *bd, const struct iovec *iov)
-{
- return __bsg_get_done_cmd(bd, TASK_INTERRUPTIBLE);
-}
-
-static struct bsg_command *
-bsg_get_done_cmd_nosignals(struct bsg_device *bd)
-{
- return __bsg_get_done_cmd(bd, TASK_UNINTERRUPTIBLE);
-}
-
static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
[PATCH 0/5] zfcp: print S_ID and D_ID with 3 bytes
From: Christof Schmitt [EMAIL PROTECTED]
S_ID and D_ID are defined in the FCP spec as 3 byte fields.
Change the output in zfcp print statements accordingly to print
them with only 3 bytes.
Signed-off-by: Christof Schmitt [EMAIL PROTECTED]
Signed-off-by: Swen Schillig [EMAIL PROTECTED]
---
drivers/s390/scsi/zfcp_aux.c |6 +++---
drivers/s390/scsi/zfcp_erp.c | 20 ++--
drivers/s390/scsi/zfcp_fsf.c | 30 +++---
3 files changed, 28 insertions(+), 28 deletions(-)
--- linux-2.6.orig/drivers/s390/scsi/zfcp_aux.c 2007-04-26 14:53:13.0
+0200
+++ linux-2.6/drivers/s390/scsi/zfcp_aux.c 2007-05-07 13:29:48.0
+0200
@@ -1497,7 +1497,7 @@
if (!port || (port-wwpn != (*(wwn_t *) els_plogi-serv_param.wwpn))) {
ZFCP_LOG_DEBUG(ignored incoming PLOGI for nonexisting port
- with d_id 0x%08x on adapter %s\n,
+ with d_id 0x%06x on adapter %s\n,
status_buffer-d_id,
zfcp_get_busid_by_adapter(adapter));
} else {
@@ -1522,7 +1522,7 @@
if (!port || (port-wwpn != els_logo-nport_wwpn)) {
ZFCP_LOG_DEBUG(ignored incoming LOGO for nonexisting port
- with d_id 0x%08x on adapter %s\n,
+ with d_id 0x%06x on adapter %s\n,
status_buffer-d_id,
zfcp_get_busid_by_adapter(adapter));
} else {
@@ -1704,7 +1704,7 @@
/* looks like a valid d_id */
port-d_id = ct_iu_resp-d_id ZFCP_DID_MASK;
atomic_set_mask(ZFCP_STATUS_PORT_DID_DID, port-status);
- ZFCP_LOG_DEBUG(adapter %s: wwpn=0x%016Lx --- d_id=0x%08x\n,
+ ZFCP_LOG_DEBUG(adapter %s: wwpn=0x%016Lx --- d_id=0x%06x\n,
zfcp_get_busid_by_port(port), port-wwpn, port-d_id);
goto out;
--- linux-2.6.orig/drivers/s390/scsi/zfcp_erp.c 2007-05-07 12:37:01.0
+0200
+++ linux-2.6/drivers/s390/scsi/zfcp_erp.c 2007-05-07 13:29:48.0
+0200
@@ -342,9 +342,9 @@
adisc-wwpn = fc_host_port_name(adapter-scsi_host);
adisc-wwnn = fc_host_node_name(adapter-scsi_host);
adisc-nport_id = fc_host_port_id(adapter-scsi_host);
- ZFCP_LOG_INFO(ADISC request from s_id 0x%08x to d_id 0x%08x
+ ZFCP_LOG_INFO(ADISC request from s_id 0x%06x to d_id 0x%06x
(wwpn=0x%016Lx, wwnn=0x%016Lx,
- hard_nport_id=0x%08x, nport_id=0x%08x)\n,
+ hard_nport_id=0x%06x, nport_id=0x%06x)\n,
adisc-nport_id, send_els-d_id, (wwn_t) adisc-wwpn,
(wwn_t) adisc-wwnn, adisc-hard_nport_id,
adisc-nport_id);
@@ -352,7 +352,7 @@
retval = zfcp_fsf_send_els(send_els);
if (retval != 0) {
ZFCP_LOG_NORMAL(error: initiation of Send ELS failed for port
- 0x%08x on adapter %s\n, send_els-d_id,
+ 0x%06x on adapter %s\n, send_els-d_id,
zfcp_get_busid_by_adapter(adapter));
goto freemem;
}
@@ -398,7 +398,7 @@
if (send_els-status != 0) {
ZFCP_LOG_NORMAL(ELS request rejected/timed out,
force physical port reopen
- (adapter %s, port d_id=0x%08x)\n,
+ (adapter %s, port d_id=0x%06x)\n,
zfcp_get_busid_by_adapter(adapter), d_id);
debug_text_event(adapter-erp_dbf, 3, forcreop);
if (zfcp_erp_port_forced_reopen(port, 0))
@@ -411,9 +411,9 @@
adisc = zfcp_sg_to_address(send_els-resp);
- ZFCP_LOG_INFO(ADISC response from d_id 0x%08x to s_id
- 0x%08x (wwpn=0x%016Lx, wwnn=0x%016Lx,
- hard_nport_id=0x%08x, nport_id=0x%08x)\n,
+ ZFCP_LOG_INFO(ADISC response from d_id 0x%06x to s_id
+ 0x%06x (wwpn=0x%016Lx, wwnn=0x%016Lx,
+ hard_nport_id=0x%06x, nport_id=0x%06x)\n,
d_id, fc_host_port_id(adapter-scsi_host),
(wwn_t) adisc-wwpn, (wwn_t) adisc-wwnn,
adisc-hard_nport_id, adisc-nport_id);
@@ -1377,7 +1377,7 @@
if (atomic_test_mask(ZFCP_STATUS_PORT_WKA, port-status))
ZFCP_LOG_NORMAL(port erp failed (adapter %s,
- port d_id=0x%08x)\n,
+ port d_id=0x%06x)\n,
zfcp_get_busid_by_port(port), port-d_id);
else
ZFCP_LOG_NORMAL(port erp failed (adapter %s, wwpn=0x%016Lx)\n,
@@ -2401,7 +2401,7 @@
retval = ZFCP_ERP_FAILED;
}
} else {
- ZFCP_LOG_DEBUG(port
[PATCH 1/5] zfcp: Locking for req_no and req_seq_no
From: Christof Schmitt [EMAIL PROTECTED]
There is a possible race condition while generating the unique
request ids and sequence numbers. Both might be read at the
same time and have the same value. Fix this by serializing the
access through the queue lock of the adapter: First call
zfcp_fsf_req_sbal_get that acquires the lock, then read and
increment the unique ids.
Signed-off-by: Christof Schmitt [EMAIL PROTECTED]
Signed-off-by: Swen Schillig [EMAIL PROTECTED]
---
drivers/s390/scsi/zfcp_fsf.c | 17 -
1 file changed, 8 insertions(+), 9 deletions(-)
--- linux-2.6.orig/drivers/s390/scsi/zfcp_fsf.c 2007-05-07 13:56:57.0
+0200
+++ linux-2.6/drivers/s390/scsi/zfcp_fsf.c 2007-05-07 13:57:34.0
+0200
@@ -4645,23 +4645,22 @@
fsf_req-adapter = adapter;
fsf_req-fsf_command = fsf_cmd;
INIT_LIST_HEAD(fsf_req-list);
-
- /* this is serialized (we are holding req_queue-lock of adapter */
- if (adapter-req_no == 0)
- adapter-req_no++;
- fsf_req-req_id = adapter-req_no++;
-
init_timer(fsf_req-timer);
- zfcp_fsf_req_qtcb_init(fsf_req);
/* initialize waitqueue which may be used to wait on
this request completion */
init_waitqueue_head(fsf_req-completion_wq);
ret = zfcp_fsf_req_sbal_get(adapter, req_flags, lock_flags);
-if(ret 0) {
+if (ret 0)
goto failed_sbals;
- }
+
+ /* this is serialized (we are holding req_queue-lock of adapter) */
+ if (adapter-req_no == 0)
+ adapter-req_no++;
+ fsf_req-req_id = adapter-req_no++;
+
+ zfcp_fsf_req_qtcb_init(fsf_req);
/*
* We hold queue_lock here. Check if QDIOUP is set and let request fail
---
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/5] zfcp: Fix deadlock between zfcp ERP and SCSI
From: Christof Schmitt [EMAIL PROTECTED]
The SCSI stack requires low level drivers to register and
unregister devices. For zfcp this leads to the situation where
zfcp calls the SCSI stack, the SCSI tries to scan the new device
and the scan SCSI command fails. This would require the zfcp erp,
but the erp thread is already blocked in the register call.
The fix is to make sure that the calls from the ERP thread to
the SCSI stack do not block the ERP thread. In detail:
1) Use a workqueue to avoid blocking of the scsi_scan_target calls.
2) When removing a unit make sure that no scsi_scan_target call is
pending.
3) Replace scsi_flush_work with scsi_target_unblock. This avoids
blocking and has the same result.
Signed-off-by: Christof Schmitt [EMAIL PROTECTED]
Signed-off-by: Swen Schillig [EMAIL PROTECTED]
---
drivers/s390/scsi/zfcp_aux.c |2 +
drivers/s390/scsi/zfcp_def.h |5 +++
drivers/s390/scsi/zfcp_erp.c | 64 +++---
drivers/s390/scsi/zfcp_scsi.c |5 +++
4 files changed, 72 insertions(+), 4 deletions(-)
--- linux-2.6.orig/drivers/s390/scsi/zfcp_aux.c 2007-05-07 13:56:57.0
+0200
+++ linux-2.6/drivers/s390/scsi/zfcp_aux.c 2007-05-07 15:50:19.0
+0200
@@ -913,6 +913,8 @@
unit-sysfs_device.release = zfcp_sysfs_unit_release;
dev_set_drvdata(unit-sysfs_device, unit);
+ init_waitqueue_head(unit-scsi_scan_wq);
+
/* mark unit unusable as long as sysfs registration is not complete */
atomic_set_mask(ZFCP_STATUS_COMMON_REMOVE, unit-status);
--- linux-2.6.orig/drivers/s390/scsi/zfcp_def.h 2007-04-26 14:53:13.0
+0200
+++ linux-2.6/drivers/s390/scsi/zfcp_def.h 2007-05-07 15:54:46.0
+0200
@@ -637,6 +637,7 @@
#define ZFCP_STATUS_UNIT_SHARED0x0004
#define ZFCP_STATUS_UNIT_READONLY 0x0008
#define ZFCP_STATUS_UNIT_REGISTERED0x0010
+#define ZFCP_STATUS_UNIT_SCSI_WORK_PENDING 0x0020
/* FSF request status (this does not have a common part) */
#define ZFCP_STATUS_FSFREQ_NOT_INIT0x
@@ -980,6 +981,10 @@
struct scsi_device *device;/* scsi device struct pointer */
struct zfcp_erp_action erp_action; /* pending error recovery */
atomic_t erp_counter;
+ wait_queue_head_t scsi_scan_wq; /* can be used to wait until
+ all scsi_scan_target
+ requests have been
+ completed. */
};
/* FSF request */
--- linux-2.6.orig/drivers/s390/scsi/zfcp_erp.c 2007-05-07 13:56:57.0
+0200
+++ linux-2.6/drivers/s390/scsi/zfcp_erp.c 2007-05-07 15:50:19.0
+0200
@@ -1591,6 +1591,62 @@
return result;
}
+struct zfcp_erp_add_work {
+ struct zfcp_unit *unit;
+ struct work_struct work;
+};
+
+/**
+ * zfcp_erp_scsi_scan
+ * @data: pointer to a struct zfcp_erp_add_work
+ *
+ * Registers a logical unit with the SCSI stack.
+ */
+static void zfcp_erp_scsi_scan(struct work_struct *work)
+{
+ struct zfcp_erp_add_work *p =
+ container_of(work, struct zfcp_erp_add_work, work);
+ struct zfcp_unit *unit = p-unit;
+ struct fc_rport *rport = unit-port-rport;
+ scsi_scan_target(rport-dev, 0, rport-scsi_target_id,
+unit-scsi_lun, 0);
+ atomic_clear_mask(ZFCP_STATUS_UNIT_SCSI_WORK_PENDING, unit-status);
+ wake_up(unit-scsi_scan_wq);
+ zfcp_unit_put(unit);
+ kfree(p);
+}
+
+/**
+ * zfcp_erp_schedule_work
+ * @unit: pointer to unit which should be registered with SCSI stack
+ *
+ * Schedules work which registers a unit with the SCSI stack
+ */
+static void
+zfcp_erp_schedule_work(struct zfcp_unit *unit)
+{
+ struct zfcp_erp_add_work *p;
+
+ p = kmalloc(sizeof(*p), GFP_KERNEL);
+ if (!p) {
+ ZFCP_LOG_NORMAL(error: Out of resources. Could not register
+ the FCP-LUN 0x%Lx connected to
+ the port with WWPN 0x%Lx connected to
+ the adapter %s with the SCSI stack.\n,
+ unit-fcp_lun,
+ unit-port-wwpn,
+ zfcp_get_busid_by_unit(unit));
+ return;
+ }
+
+ zfcp_unit_get(unit);
+ memset(p, 0, sizeof(*p));
+ atomic_set_mask(ZFCP_STATUS_UNIT_SCSI_WORK_PENDING, unit-status);
+ INIT_WORK(p-work, zfcp_erp_scsi_scan);
+ p-unit = unit;
+ schedule_work(p-work);
+}
+
/*
* function:
*
@@ -3092,9 +3148,9 @@
port-rport) {
atomic_set_mask(ZFCP_STATUS_UNIT_REGISTERED,
unit-status);
- scsi_scan_target(port-rport-dev, 0,
-
[PATCH 4/5] zfcp: clear adapter failed flag if an fsf request times out.
From: Heiko Carstens [EMAIL PROTECTED]
Must clear adapter failed flag if an fsf request times out. This is
necessary because on link down situations the failed flags gets set
but the QDIO queues are still up. Since an adapter reopen will be
skipped if the failed flag is set an adapter_reopen that is issued
on fsf request timeout has no effect if the local link is down.
Might lead to locked up system if the SCSI stack is waiting for abort
completion.
Signed-off-by: Heiko Carstens [EMAIL PROTECTED]
Signed-off-by: Swen Schillig [EMAIL PROTECTED]
---
drivers/s390/scsi/zfcp_erp.c |2 +-
1 files changed, 1 insertion(+), 1 deletion(-)
--- linux-2.6.orig/drivers/s390/scsi/zfcp_erp.c 2007-05-07 16:26:34.0
+0200
+++ linux-2.6/drivers/s390/scsi/zfcp_erp.c 2007-05-07 16:33:54.0
+0200
@@ -179,7 +179,7 @@
static void zfcp_fsf_request_timeout_handler(unsigned long data)
{
struct zfcp_adapter *adapter = (struct zfcp_adapter *) data;
- zfcp_erp_adapter_reopen(adapter, 0);
+ zfcp_erp_adapter_reopen(adapter, ZFCP_STATUS_COMMON_ERP_FAILED);
}
void zfcp_fsf_start_timer(struct zfcp_fsf_req *fsf_req, unsigned long timeout)
---
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/5] zfcp: clear boxed flag on unit reopen.
From: Heiko Carstens [EMAIL PROTECTED] The boxed flag for units was never cleared. This doesn't hurt, but on ACL updates the error recovery could reopen more units than needed. Signed-off-by: Heiko Carstens [EMAIL PROTECTED] Signed-off-by: Swen Schillig [EMAIL PROTECTED] --- drivers/s390/scsi/zfcp_fsf.c |1 + 1 files changed, 1 insertion(+) --- linux-2.6.orig/drivers/s390/scsi/zfcp_fsf.c 2007-05-07 16:26:34.0 +0200 +++ linux-2.6/drivers/s390/scsi/zfcp_fsf.c 2007-05-07 16:34:16.0 +0200 @@ -3043,6 +3043,7 @@ queue_designator = header-fsf_status_qual.fsf_queue_designator; atomic_clear_mask(ZFCP_STATUS_COMMON_ACCESS_DENIED | + ZFCP_STATUS_COMMON_ACCESS_BOXED | ZFCP_STATUS_UNIT_SHARED | ZFCP_STATUS_UNIT_READONLY, unit-status); --- - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bidi bsg is non-blocking
On Tue, May 08 2007, FUJITA Tomonori wrote:
From: [EMAIL PROTECTED]
Subject: bidi bsg is non-blocking
Date: Mon, 7 May 2007 10:21:18 -0500
I'm attempting to use the bidi variant of bsg to talk to an OSD target
device. I've run into an undesirable situation.
My application has a free-running receive loop (doing a read() on the bsg
device) waiting for responses to commands sent to bsg by another thread.
The problem is that the receive thread is getting ENODATA from the read()
when there are no commands pending. I have NOT set non-blocking.
Note that the old sg driver was quite willing to block until there was a
response available. I find this scenario greatly preferable.
Could bsg be fixed so that read() blocks when there is nothing to return?
I think that this is a bug.
This patch is against the bsg branch in the Jens' git tree.
I guess that it would be nice to do more cleanups on these parts.
From 52a9fcf0af72f212ddd93918b7f9f0f0e706c215 Mon Sep 17 00:00:00 2001
From: FUJITA Tomonori [EMAIL PROTECTED]
Date: Tue, 8 May 2007 15:57:43 +0900
Subject: [PATCH] fix read ENODATA bug
This patch fixes a bug that read() gives ENODATA even with a blocking
file descriptor when there are no commands pending.
This also includes some cleanups.
Signed-off-by: FUJITA Tomonori [EMAIL PROTECTED]
Oops, I sent a wrong patch. This is a right one.
Patch looks good, I agree this is the way that bsg should act for a
blocking read on an empty queue.
+ ret = wait_event_interruptible(bd-wq_done, bd-done_cmds);
+ if (ret) {
+ bc = ERR_PTR(-ERESTARTSYS);
+ break;
+ } else
+ continue;
} while (1);
The else clause is pointless.
Apart from that, it looks sounds. Can you resend?
--
Jens Axboe
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bidi bsg is non-blocking
From: Jens Axboe [EMAIL PROTECTED]
Subject: Re: bidi bsg is non-blocking
Date: Tue, 8 May 2007 14:21:34 +0200
On Tue, May 08 2007, FUJITA Tomonori wrote:
From: [EMAIL PROTECTED]
Subject: bidi bsg is non-blocking
Date: Mon, 7 May 2007 10:21:18 -0500
I'm attempting to use the bidi variant of bsg to talk to an OSD target
device. I've run into an undesirable situation.
My application has a free-running receive loop (doing a read() on the
bsg
device) waiting for responses to commands sent to bsg by another thread.
The problem is that the receive thread is getting ENODATA from the
read()
when there are no commands pending. I have NOT set non-blocking.
Note that the old sg driver was quite willing to block until there was a
response available. I find this scenario greatly preferable.
Could bsg be fixed so that read() blocks when there is nothing to
return?
I think that this is a bug.
This patch is against the bsg branch in the Jens' git tree.
I guess that it would be nice to do more cleanups on these parts.
From 52a9fcf0af72f212ddd93918b7f9f0f0e706c215 Mon Sep 17 00:00:00 2001
From: FUJITA Tomonori [EMAIL PROTECTED]
Date: Tue, 8 May 2007 15:57:43 +0900
Subject: [PATCH] fix read ENODATA bug
This patch fixes a bug that read() gives ENODATA even with a blocking
file descriptor when there are no commands pending.
This also includes some cleanups.
Signed-off-by: FUJITA Tomonori [EMAIL PROTECTED]
Oops, I sent a wrong patch. This is a right one.
Patch looks good, I agree this is the way that bsg should act for a
blocking read on an empty queue.
+ ret = wait_event_interruptible(bd-wq_done, bd-done_cmds);
+ if (ret) {
+ bc = ERR_PTR(-ERESTARTSYS);
+ break;
+ } else
+ continue;
} while (1);
The else clause is pointless.
Oops.
Apart from that, it looks sounds. Can you resend?
Sure, I'll resend an updated version shortly.
BTW, any comments on the bidi patch for the block layer?
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] zfcp: Stop system after memory corruption
On Tue, May 08, 2007 at 01:56:07AM +1000, Julian Calaby wrote: From: Christof Schmitt [EMAIL PROTECTED] [snip] - ZFCP_LOG_NORMAL(bug: unexpected inbound - packet on adapter %s - (reqid=0x%lx, - first_element=%d, - elements_processed=%d)\n, [...] As a minor quibble, may I suggest that these lines belong just above the panic introduced above? I assume that they'd be useful for debugging should this situation occur. If this problem occurs, the administrator of the Linux system will be advised to dump the Linux system and to also capture the logs from the FCP adapter. The memory dump already contains the information about the unexpected packet and also information about the last requests sent to the FCP adapter. So, the messages do not provide additional debug data, and i removed them to simplify the code. Christof Schmitt - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] bsg: fix a blocking read bug
This patch fixes a bug that read() returns ENODATA even with a
blocking file descriptor when there are no commands pending.
This also includes some cleanups.
Signed-off-by: FUJITA Tomonori [EMAIL PROTECTED]
---
block/bsg.c | 84 --
1 files changed, 23 insertions(+), 61 deletions(-)
diff --git a/block/bsg.c b/block/bsg.c
index a333c93..2f78d7d 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -115,9 +115,9 @@ static void bsg_free_command(struct bsg_
wake_up(bd-wq_free);
}
-static struct bsg_command *__bsg_alloc_command(struct bsg_device *bd)
+static struct bsg_command *bsg_alloc_command(struct bsg_device *bd)
{
- struct bsg_command *bc = NULL;
+ struct bsg_command *bc = ERR_PTR(-EINVAL);
spin_lock_irq(bd-lock);
@@ -131,6 +131,7 @@ static struct bsg_command *__bsg_alloc_c
if (unlikely(!bc)) {
spin_lock_irq(bd-lock);
bd-queued_cmds--;
+ bc = ERR_PTR(-ENOMEM);
goto out;
}
@@ -198,30 +199,6 @@ unlock:
return ret;
}
-/*
- * get a new free command, blocking if needed and specified
- */
-static struct bsg_command *bsg_get_command(struct bsg_device *bd)
-{
- struct bsg_command *bc;
- int ret;
-
- do {
- bc = __bsg_alloc_command(bd);
- if (bc)
- break;
-
- ret = bsg_io_schedule(bd, TASK_INTERRUPTIBLE);
- if (ret) {
- bc = ERR_PTR(ret);
- break;
- }
-
- } while (1);
-
- return bc;
-}
-
static int blk_fill_sgv4_hdr_rq(request_queue_t *q, struct request *rq,
struct sg_io_v4 *hdr, int has_write_perm)
{
@@ -397,7 +374,7 @@ static inline struct bsg_command *bsg_ne
/*
* Get a finished command from the done list
*/
-static struct bsg_command *__bsg_get_done_cmd(struct bsg_device *bd, int state)
+static struct bsg_command *bsg_get_done_cmd(struct bsg_device *bd)
{
struct bsg_command *bc;
int ret;
@@ -407,9 +384,14 @@ static struct bsg_command *__bsg_get_don
if (bc)
break;
- ret = bsg_io_schedule(bd, state);
+ if (!test_bit(BSG_F_BLOCK, bd-flags)) {
+ bc = ERR_PTR(-EAGAIN);
+ break;
+ }
+
+ ret = wait_event_interruptible(bd-wq_done, bd-done_cmds);
if (ret) {
- bc = ERR_PTR(ret);
+ bc = ERR_PTR(-ERESTARTSYS);
break;
}
} while (1);
@@ -419,18 +401,6 @@ static struct bsg_command *__bsg_get_don
return bc;
}
-static struct bsg_command *
-bsg_get_done_cmd(struct bsg_device *bd, const struct iovec *iov)
-{
- return __bsg_get_done_cmd(bd, TASK_INTERRUPTIBLE);
-}
-
-static struct bsg_command *
-bsg_get_done_cmd_nosignals(struct bsg_device *bd)
-{
- return __bsg_get_done_cmd(bd, TASK_UNINTERRUPTIBLE);
-}
-
static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
struct bio *bio)
{
@@ -496,19 +466,16 @@ static int bsg_complete_all_commands(str
*/
ret = 0;
do {
- bc = bsg_get_done_cmd_nosignals(bd);
-
- /*
-* we _must_ complete before restarting, because
-* bsg_release can't handle this failing.
-*/
- if (PTR_ERR(bc) == -ERESTARTSYS)
- continue;
- if (IS_ERR(bc)) {
- ret = PTR_ERR(bc);
+ spin_lock_irq(bd-lock);
+ if (!bd-queued_cmds) {
+ spin_unlock_irq(bd-lock);
break;
}
+ bc = bsg_get_done_cmd(bd);
+ if (IS_ERR(bc))
+ break;
+
tret = blk_complete_sgv4_hdr_rq(bc-rq, bc-hdr, bc-bio);
if (!ret)
ret = tret;
@@ -519,11 +486,9 @@ static int bsg_complete_all_commands(str
return ret;
}
-typedef struct bsg_command *(*bsg_command_callback)(struct bsg_device *bd,
const struct iovec *iov);
-
static ssize_t
-__bsg_read(char __user *buf, size_t count, bsg_command_callback get_bc,
- struct bsg_device *bd, const struct iovec *iov, ssize_t *bytes_read)
+__bsg_read(char __user *buf, size_t count, struct bsg_device *bd,
+ const struct iovec *iov, ssize_t *bytes_read)
{
struct bsg_command *bc;
int nr_commands, ret;
@@ -534,7 +499,7 @@ __bsg_read(char __user *buf, size_t coun
ret = 0;
nr_commands = count / sizeof(struct sg_io_v4);
while (nr_commands) {
- bc = get_bc(bd, iov);
+ bc = bsg_get_done_cmd(bd);
if (IS_ERR(bc)) {
ret =
Re: [PATCH] zfcp: Stop system after memory corruption
On 5/8/07, Christof Schmitt [EMAIL PROTECTED] wrote: On Tue, May 08, 2007 at 01:56:07AM +1000, Julian Calaby wrote: From: Christof Schmitt [EMAIL PROTECTED] [snip] - ZFCP_LOG_NORMAL(bug: unexpected inbound - packet on adapter %s - (reqid=0x%lx, - first_element=%d, - elements_processed=%d)\n, [...] As a minor quibble, may I suggest that these lines belong just above the panic introduced above? I assume that they'd be useful for debugging should this situation occur. If this problem occurs, the administrator of the Linux system will be advised to dump the Linux system and to also capture the logs from the FCP adapter. The memory dump already contains the information about the unexpected packet and also information about the last requests sent to the FCP adapter. So, the messages do not provide additional debug data, and i removed them to simplify the code. Ah! Thanks, -- Julian Calaby Email: [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bidi bsg is non-blocking
From: Jens Axboe [EMAIL PROTECTED] Subject: Re: bidi bsg is non-blocking Date: Tue, 8 May 2007 15:00:32 +0200 On Tue, May 08 2007, FUJITA Tomonori wrote: BTW, any comments on the bidi patch for the block layer? Yeah it looks much better. I agree with your notion that bidi should be block layer agnostic at least for now (until a general use comes up), it's just a way to transport these commands. And I do still prefer -next_rq as the pointer name for partly those reasons. Thanks for the comments. Seems that the block layer part is done. And my patch uses -next_rq pointer name: http://marc.info/?l=linux-scsim=117835587615642w=2 - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] zfcp: print S_ID and D_ID with 3 bytes
On Tue, 2007-05-08 at 11:14 +0200, Swen Schillig wrote: From: Christof Schmitt [EMAIL PROTECTED] S_ID and D_ID are defined in the FCP spec as 3 byte fields. Change the output in zfcp print statements accordingly to print them with only 3 bytes. For future reference, if you send out a [PATCH 0/N] it typically contains a description of the patches that are coming, not a patch itself ... Having six patches numbered 0 to 5 out of 5 is somewhat confusing ... James - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] zfcp: Fix deadlock between zfcp ERP and SCSI
Curious why you are calling scsi_scan_target() or
scsi_target_block()/scsi_target_unblock() directly. I would have
thought the add/remove rport code would have done this for you,
and it deals with all the flush conditions, etc.
-- james
Swen Schillig wrote:
From: Christof Schmitt [EMAIL PROTECTED]
The SCSI stack requires low level drivers to register and
unregister devices. For zfcp this leads to the situation where
zfcp calls the SCSI stack, the SCSI tries to scan the new device
and the scan SCSI command fails. This would require the zfcp erp,
but the erp thread is already blocked in the register call.
The fix is to make sure that the calls from the ERP thread to
the SCSI stack do not block the ERP thread. In detail:
1) Use a workqueue to avoid blocking of the scsi_scan_target calls.
2) When removing a unit make sure that no scsi_scan_target call is
pending.
3) Replace scsi_flush_work with scsi_target_unblock. This avoids
blocking and has the same result.
Signed-off-by: Christof Schmitt [EMAIL PROTECTED]
Signed-off-by: Swen Schillig [EMAIL PROTECTED]
---
drivers/s390/scsi/zfcp_aux.c |2 +
drivers/s390/scsi/zfcp_def.h |5 +++
drivers/s390/scsi/zfcp_erp.c | 64 +++---
drivers/s390/scsi/zfcp_scsi.c |5 +++
4 files changed, 72 insertions(+), 4 deletions(-)
--- linux-2.6.orig/drivers/s390/scsi/zfcp_aux.c 2007-05-07 13:56:57.0
+0200
+++ linux-2.6/drivers/s390/scsi/zfcp_aux.c 2007-05-07 15:50:19.0
+0200
@@ -913,6 +913,8 @@
unit-sysfs_device.release = zfcp_sysfs_unit_release;
dev_set_drvdata(unit-sysfs_device, unit);
+ init_waitqueue_head(unit-scsi_scan_wq);
+
/* mark unit unusable as long as sysfs registration is not complete */
atomic_set_mask(ZFCP_STATUS_COMMON_REMOVE, unit-status);
--- linux-2.6.orig/drivers/s390/scsi/zfcp_def.h 2007-04-26 14:53:13.0
+0200
+++ linux-2.6/drivers/s390/scsi/zfcp_def.h 2007-05-07 15:54:46.0
+0200
@@ -637,6 +637,7 @@
#define ZFCP_STATUS_UNIT_SHARED0x0004
#define ZFCP_STATUS_UNIT_READONLY 0x0008
#define ZFCP_STATUS_UNIT_REGISTERED0x0010
+#define ZFCP_STATUS_UNIT_SCSI_WORK_PENDING 0x0020
/* FSF request status (this does not have a common part) */
#define ZFCP_STATUS_FSFREQ_NOT_INIT0x
@@ -980,6 +981,10 @@
struct scsi_device *device;/* scsi device struct pointer */
struct zfcp_erp_action erp_action; /* pending error recovery */
atomic_t erp_counter;
+ wait_queue_head_t scsi_scan_wq; /* can be used to wait until
+ all scsi_scan_target
+ requests have been
+ completed. */
};
/* FSF request */
--- linux-2.6.orig/drivers/s390/scsi/zfcp_erp.c 2007-05-07 13:56:57.0
+0200
+++ linux-2.6/drivers/s390/scsi/zfcp_erp.c 2007-05-07 15:50:19.0
+0200
@@ -1591,6 +1591,62 @@
return result;
}
+struct zfcp_erp_add_work {
+ struct zfcp_unit *unit;
+ struct work_struct work;
+};
+
+/**
+ * zfcp_erp_scsi_scan
+ * @data: pointer to a struct zfcp_erp_add_work
+ *
+ * Registers a logical unit with the SCSI stack.
+ */
+static void zfcp_erp_scsi_scan(struct work_struct *work)
+{
+ struct zfcp_erp_add_work *p =
+ container_of(work, struct zfcp_erp_add_work, work);
+ struct zfcp_unit *unit = p-unit;
+ struct fc_rport *rport = unit-port-rport;
+ scsi_scan_target(rport-dev, 0, rport-scsi_target_id,
+unit-scsi_lun, 0);
+ atomic_clear_mask(ZFCP_STATUS_UNIT_SCSI_WORK_PENDING, unit-status);
+ wake_up(unit-scsi_scan_wq);
+ zfcp_unit_put(unit);
+ kfree(p);
+}
+
+/**
+ * zfcp_erp_schedule_work
+ * @unit: pointer to unit which should be registered with SCSI stack
+ *
+ * Schedules work which registers a unit with the SCSI stack
+ */
+static void
+zfcp_erp_schedule_work(struct zfcp_unit *unit)
+{
+ struct zfcp_erp_add_work *p;
+
+ p = kmalloc(sizeof(*p), GFP_KERNEL);
+ if (!p) {
+ ZFCP_LOG_NORMAL(error: Out of resources. Could not register
+ the FCP-LUN 0x%Lx connected to
+ the port with WWPN 0x%Lx connected to
+ the adapter %s with the SCSI stack.\n,
+ unit-fcp_lun,
+ unit-port-wwpn,
+ zfcp_get_busid_by_unit(unit));
+ return;
+ }
+
+ zfcp_unit_get(unit);
+ memset(p, 0, sizeof(*p));
+ atomic_set_mask(ZFCP_STATUS_UNIT_SCSI_WORK_PENDING, unit-status);
+ INIT_WORK(p-work, zfcp_erp_scsi_scan);
+ p-unit = unit;
+ schedule_work(p-work);
+}
+
/*
* function:
*
@@ -3092,9
Re: [patch 16/30] drivers/scsi/aic7xxx_old: Convert to generic boolean-values
On Mon, 2007-05-07 at 14:06 -0700, Andrew Morton wrote: On Mon, 07 May 2007 14:50:19 -0500 James Bottomley [EMAIL PROTECTED] wrote: On Thu, 2007-04-26 at 00:35 -0700, [EMAIL PROTECTED] wrote: From: Richard Knutsson [EMAIL PROTECTED] Convert: FALSE - false TRUE - true NAK'd by maintainer. I went back and reviewed the previous discussion. Maintaner's reasons for NAKing were, and remain wholly specious. Well, I agree with them. There are three primary reasons for this 1. In C logical values are arithmetic types by design. Trying to backfill C++ ideas of separation of logical and arithmetic types is asking for bugs. 2. If people have to use boolean values (which I don't like; I prefer simple assignment and checking), I prefer the upper case values because, effectively, they're simple arithmetical constants. 3. it's going to generate a huge amount of churn in all the subsystems and drivers for no appreciable benefit. Unless someone can actually articulate one? (Other than because it's there). The patch in question basically just downcased the TRUE/FALSE in the driver, which makes it less readable (mainly because I really don't like the way it uses truth values, but that's a maintainer's prerogative, and I recognise the upper case values as warning me to be careful). That being said, the patch is moderately wrong (or at least incomplete) because it does things like: -unsigned char done = FALSE; +unsigned char done = false; whereas it should have done -unsigned char done = FALSE; +bool done = false; And the value to the driver of this transformation? James - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] zfcp: Fix deadlock between zfcp ERP and SCSI
On Tue, May 08, 2007 at 11:00:11AM -0400, James Smart wrote: Curious why you are calling scsi_scan_target() or scsi_target_block()/scsi_target_unblock() directly. I would have thought the add/remove rport code would have done this for you, and it deals with all the flush conditions, etc. fc_remote_port_add issues a scsi_scan_target with SCAN_WILD_CARD. This tries to get all LUNs for the target via REPORT LUNS for LUN 0. The current design for zfcp is to prevent the wildcard scan from the SCSI layer by not allocating a device for LUN 0. When a FCP adapter does not support NPIV, the adapter can still be shared among multiple Linux systems. Each Linux system can access different LUNs, that are activated manually via sysfs. So, the wildcard scan from the FC transport class does nothing and zfcp activates the LUNs it wants to use via direct calls to scsi_scan_target. If every adapter would support NPIV, zfcp could rely on the scan by the FC transport class, unfortunately, this is not the case. Christof Schmitt - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH]: Fix old SCSI adapter crashes with CD-ROM
The CD-ROM layer doesn't bounce requests for old ISA controllers (and
nor should it). However they get injected into the SCSI layer via
sr_ioctl which also doesn't bounce them and SCSI then passes the buffer
along to a device with unchecked_isa_dma set which either panics or
truncates the buffer to 24bits.
According to Jens the right long term fix is for the CD layer to route
the requests differently but in the mean time this has been tested by a
victim and verified to sort the problem out. For the other 99.9% of users
it's a no-op and doesn't bounce data.
Signed-off-by: Alan Cox [EMAIL PROTECTED]
diff -u --new-file --recursive --exclude-from /usr/src/exclude
linux.vanilla-2.6.21-rc6-mm1/drivers/scsi/sr_ioctl.c
linux-2.6.21-rc6-mm1/drivers/scsi/sr_ioctl.c
--- linux.vanilla-2.6.21-rc6-mm1/drivers/scsi/sr_ioctl.c2007-04-12
14:14:45.0 +0100
+++ linux-2.6.21-rc6-mm1/drivers/scsi/sr_ioctl.c2007-05-01
14:23:40.0 +0100
@@ -187,9 +187,10 @@
struct scsi_sense_hdr sshdr;
int result, err = 0, retries = 0;
struct request_sense *sense = cgc-sense;
-
+ void *zebedee = cgc-buffer;
+
SDev = cd-device;
-
+
if (!sense) {
sense = kmalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
if (!sense) {
@@ -197,7 +198,22 @@
goto out;
}
}
-
+
+ if (cgc-buflen cd-device-host-unchecked_isa_dma) {
+ switch(cgc-data_direction) {
+ case DMA_NONE:
+ break;
+ case DMA_FROM_DEVICE:
+ case DMA_TO_DEVICE:
+ zebedee = kmalloc(cgc-buflen,
GFP_KERNEL|GFP_DMA);
+ if (zebedee ==NULL) {
+ err = -ENOMEM;
+ goto out;
+ }
+ }
+ if (cgc-data_direction == DMA_TO_DEVICE)
+ memcpy(zebedee, cgc-buffer, cgc-buflen);
+ }
retry:
if (!scsi_block_when_processing_errors(SDev)) {
err = -ENODEV;
@@ -206,10 +222,16 @@
memset(sense, 0, sizeof(*sense));
result = scsi_execute(SDev, cgc-cmd, cgc-data_direction,
- cgc-buffer, cgc-buflen, (char *)sense,
+ zebedee, cgc-buflen, (char *)sense,
cgc-timeout, IOCTL_RETRIES, 0);
scsi_normalize_sense((char *)sense, sizeof(*sense), sshdr);
+
+ if (zebedee != cgc-buffer) {
+ if (cgc-data_direction == DMA_FROM_DEVICE)
+ memcpy(cgc-buffer, zebedee, cgc-buflen);
+ kfree(zebedee); /* Time for bed */
+ }
/* Minimal error checking. Ignore cases we know about, and report the
rest. */
if (driver_byte(result) != 0) {
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM
On Tue, 8 May 2007 17:03:35 +0100 Alan Cox [EMAIL PROTECTED] wrote: The CD-ROM layer doesn't bounce requests for old ISA controllers (and nor should it). However they get injected into the SCSI layer via sr_ioctl which also doesn't bounce them and SCSI then passes the buffer along to a device with unchecked_isa_dma set which either panics or truncates the buffer to 24bits. Umm duh there's a memory leak in an error case there still. Please discard and I'll post a new one shortly - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
The CD-ROM layer doesn't bounce requests for old ISA controllers (and
nor should it). However they get injected into the SCSI layer via
sr_ioctl which also doesn't bounce them and SCSI then passes the buffer
along to a device with unchecked_isa_dma set which either panics or
truncates the buffer to 24bits.
According to Jens the right long term fix is for the CD layer to route
the requests differently but in the mean time this has been tested by a
victim and verified to sort the problem out. For the other 99.9% of users
it's a no-op and doesn't bounce data.
Signed-off-by: Alan Cox [EMAIL PROTECTED]
diff -u --new-file --recursive --exclude-from /usr/src/exclude
linux.vanilla-2.6.21-rc6-mm1/drivers/scsi/sr_ioctl.c
linux-2.6.21-rc6-mm1/drivers/scsi/sr_ioctl.c
--- linux.vanilla-2.6.21-rc6-mm1/drivers/scsi/sr_ioctl.c2007-04-12
14:14:45.0 +0100
+++ linux-2.6.21-rc6-mm1/drivers/scsi/sr_ioctl.c2007-05-08
16:55:01.446661608 +0100
@@ -187,9 +187,10 @@
struct scsi_sense_hdr sshdr;
int result, err = 0, retries = 0;
struct request_sense *sense = cgc-sense;
-
+ void *zebedee = cgc-buffer;
+
SDev = cd-device;
-
+
if (!sense) {
sense = kmalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
if (!sense) {
@@ -197,7 +198,22 @@
goto out;
}
}
-
+
+ if (cgc-buflen cd-device-host-unchecked_isa_dma) {
+ switch(cgc-data_direction) {
+ case DMA_NONE:
+ break;
+ case DMA_FROM_DEVICE:
+ case DMA_TO_DEVICE:
+ zebedee = kmalloc(cgc-buflen,
GFP_KERNEL|GFP_DMA);
+ if (zebedee ==NULL) {
+ err = -ENOMEM;
+ goto out;
+ }
+ }
+ if (cgc-data_direction == DMA_TO_DEVICE)
+ memcpy(zebedee, cgc-buffer, cgc-buflen);
+ }
retry:
if (!scsi_block_when_processing_errors(SDev)) {
err = -ENODEV;
@@ -206,10 +222,15 @@
memset(sense, 0, sizeof(*sense));
result = scsi_execute(SDev, cgc-cmd, cgc-data_direction,
- cgc-buffer, cgc-buflen, (char *)sense,
+ zebedee, cgc-buflen, (char *)sense,
cgc-timeout, IOCTL_RETRIES, 0);
scsi_normalize_sense((char *)sense, sizeof(*sense), sshdr);
+
+ if (zebedee != cgc-buffer) {
+ if (cgc-data_direction == DMA_FROM_DEVICE)
+ memcpy(cgc-buffer, zebedee, cgc-buflen);
+ }
/* Minimal error checking. Ignore cases we know about, and report the
rest. */
if (driver_byte(result) != 0) {
@@ -266,6 +287,8 @@
/* Wake up a process waiting for device */
out:
+ if (zebedee != cgc-buffer)
+ kfree(zebedee); /* Time for bed */
if (!cgc-sense)
kfree(sense);
cgc-stat = err;
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
On Tue, May 08 2007, Alan Cox wrote: The CD-ROM layer doesn't bounce requests for old ISA controllers (and nor should it). However they get injected into the SCSI layer via sr_ioctl which also doesn't bounce them and SCSI then passes the buffer along to a device with unchecked_isa_dma set which either panics or truncates the buffer to 24bits. According to Jens the right long term fix is for the CD layer to route the requests differently but in the mean time this has been tested by a victim and verified to sort the problem out. For the other 99.9% of users it's a no-op and doesn't bounce data. Signed-off-by: Alan Cox [EMAIL PROTECTED] Signed-off-by: Jens Axboe [EMAIL PROTECTED] Christoph passed me his patch to get rid of -generic_packet() in the cdrom layer, so the work is almost complete. This patch is fine as a work-around until that gets merged, though. -- Jens Axboe - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
On Tue, 2007-05-08 at 18:14 +0200, Jens Axboe wrote: On Tue, May 08 2007, Alan Cox wrote: The CD-ROM layer doesn't bounce requests for old ISA controllers (and nor should it). However they get injected into the SCSI layer via sr_ioctl which also doesn't bounce them and SCSI then passes the buffer along to a device with unchecked_isa_dma set which either panics or truncates the buffer to 24bits. According to Jens the right long term fix is for the CD layer to route the requests differently but in the mean time this has been tested by a victim and verified to sort the problem out. For the other 99.9% of users it's a no-op and doesn't bounce data. Signed-off-by: Alan Cox [EMAIL PROTECTED] Signed-off-by: Jens Axboe [EMAIL PROTECTED] Christoph passed me his patch to get rid of -generic_packet() in the cdrom layer, so the work is almost complete. This patch is fine as a work-around until that gets merged, though. Actually, I think the new scsi request infrastructure should be doing the bouncing (rather than have it done in each problem path we discover). Mike Christie tells me we're missing bouncing by accident in the scsi_execute path (but not the scsi_execute_async path). He says this is the fix he proposed: http://marc.info/?l=linux-scsim=115981479822790w=2 Can we just merge this instead? James - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
On Tue, May 08 2007, James Bottomley wrote: On Tue, 2007-05-08 at 18:14 +0200, Jens Axboe wrote: On Tue, May 08 2007, Alan Cox wrote: The CD-ROM layer doesn't bounce requests for old ISA controllers (and nor should it). However they get injected into the SCSI layer via sr_ioctl which also doesn't bounce them and SCSI then passes the buffer along to a device with unchecked_isa_dma set which either panics or truncates the buffer to 24bits. According to Jens the right long term fix is for the CD layer to route the requests differently but in the mean time this has been tested by a victim and verified to sort the problem out. For the other 99.9% of users it's a no-op and doesn't bounce data. Signed-off-by: Alan Cox [EMAIL PROTECTED] Signed-off-by: Jens Axboe [EMAIL PROTECTED] Christoph passed me his patch to get rid of -generic_packet() in the cdrom layer, so the work is almost complete. This patch is fine as a work-around until that gets merged, though. Actually, I think the new scsi request infrastructure should be doing the bouncing (rather than have it done in each problem path we discover). Of course, bouncing should only be done in one layer (the block layer). Mike Christie tells me we're missing bouncing by accident in the scsi_execute path (but not the scsi_execute_async path). He says this is the fix he proposed: http://marc.info/?l=linux-scsim=115981479822790w=2 Can we just merge this instead? That's another issue. The problem here are requests (cgc's) initiated by the cdrom.c layer. Those _should_ get mapped to a request and put on the queue for the device, and thus get bounced by the block layer if appropriate. Mike's fix looks legit and should be merged as well, but it wont fix this issue. -- Jens Axboe - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
James Bottomley wrote: On Tue, 2007-05-08 at 18:14 +0200, Jens Axboe wrote: On Tue, May 08 2007, Alan Cox wrote: The CD-ROM layer doesn't bounce requests for old ISA controllers (and nor should it). However they get injected into the SCSI layer via sr_ioctl which also doesn't bounce them and SCSI then passes the buffer along to a device with unchecked_isa_dma set which either panics or truncates the buffer to 24bits. According to Jens the right long term fix is for the CD layer to route the requests differently but in the mean time this has been tested by a victim and verified to sort the problem out. For the other 99.9% of users it's a no-op and doesn't bounce data. Signed-off-by: Alan Cox [EMAIL PROTECTED] Signed-off-by: Jens Axboe [EMAIL PROTECTED] Christoph passed me his patch to get rid of -generic_packet() in the cdrom layer, so the work is almost complete. This patch is fine as a work-around until that gets merged, though. Actually, I think the new scsi request infrastructure should be doing the bouncing (rather than have it done in each problem path we discover). Mike Christie tells me we're missing bouncing by accident in the scsi_execute path (but not the scsi_execute_async path). He says this is the fix he proposed: http://marc.info/?l=linux-scsim=115981479822790w=2 Hey Jens and James, one thing I forgot to mention is that I could not remember if I needed an extra bio_get in there. I thought I did not because the caller is not touching the bio after the bio_endio calls like is done with the blk/bio_map_user path. But I did that patch so long ago I do not remember now. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
On Tue, 2007-05-08 at 18:40 +0200, Jens Axboe wrote: On Tue, May 08 2007, James Bottomley wrote: On Tue, 2007-05-08 at 18:14 +0200, Jens Axboe wrote: On Tue, May 08 2007, Alan Cox wrote: The CD-ROM layer doesn't bounce requests for old ISA controllers (and nor should it). However they get injected into the SCSI layer via sr_ioctl which also doesn't bounce them and SCSI then passes the buffer along to a device with unchecked_isa_dma set which either panics or truncates the buffer to 24bits. According to Jens the right long term fix is for the CD layer to route the requests differently but in the mean time this has been tested by a victim and verified to sort the problem out. For the other 99.9% of users it's a no-op and doesn't bounce data. Signed-off-by: Alan Cox [EMAIL PROTECTED] Signed-off-by: Jens Axboe [EMAIL PROTECTED] Christoph passed me his patch to get rid of -generic_packet() in the cdrom layer, so the work is almost complete. This patch is fine as a work-around until that gets merged, though. Actually, I think the new scsi request infrastructure should be doing the bouncing (rather than have it done in each problem path we discover). Of course, bouncing should only be done in one layer (the block layer). Mike Christie tells me we're missing bouncing by accident in the scsi_execute path (but not the scsi_execute_async path). He says this is the fix he proposed: http://marc.info/?l=linux-scsim=115981479822790w=2 Can we just merge this instead? That's another issue. The problem here are requests (cgc's) initiated by the cdrom.c layer. Those _should_ get mapped to a request and put on the queue for the device, and thus get bounced by the block layer if appropriate. Mike's fix looks legit and should be merged as well, but it wont fix this issue. It won't? I thought the issue (from the fix) is that cgc-buffer is outside of the device accessibility mask. The scsi_execute path allocates a request and then calls blk_rq_map_kern on the buffer (in this case cgc-buffer) ... the problem is that blk_rq_map_kern() doesn't currently bounce the buffer, but if it did (which is the functionality Mike's patch adds), surely the need to bounce it in the ioctl path would go away? James - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
On Tue, May 08 2007, Mike Christie wrote: James Bottomley wrote: On Tue, 2007-05-08 at 18:14 +0200, Jens Axboe wrote: On Tue, May 08 2007, Alan Cox wrote: The CD-ROM layer doesn't bounce requests for old ISA controllers (and nor should it). However they get injected into the SCSI layer via sr_ioctl which also doesn't bounce them and SCSI then passes the buffer along to a device with unchecked_isa_dma set which either panics or truncates the buffer to 24bits. According to Jens the right long term fix is for the CD layer to route the requests differently but in the mean time this has been tested by a victim and verified to sort the problem out. For the other 99.9% of users it's a no-op and doesn't bounce data. Signed-off-by: Alan Cox [EMAIL PROTECTED] Signed-off-by: Jens Axboe [EMAIL PROTECTED] Christoph passed me his patch to get rid of -generic_packet() in the cdrom layer, so the work is almost complete. This patch is fine as a work-around until that gets merged, though. Actually, I think the new scsi request infrastructure should be doing the bouncing (rather than have it done in each problem path we discover). Mike Christie tells me we're missing bouncing by accident in the scsi_execute path (but not the scsi_execute_async path). He says this is the fix he proposed: http://marc.info/?l=linux-scsim=115981479822790w=2 Hey Jens and James, one thing I forgot to mention is that I could not remember if I needed an extra bio_get in there. I thought I did not because the caller is not touching the bio after the bio_endio calls like is done with the blk/bio_map_user path. But I did that patch so long ago I do not remember now. If you don't touch it after bio_endio(), then you don't need to hold an extra reference to it. I'll add your patch. -- Jens Axboe - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [patch 18/30] Fix |/|| confusion in fusion driver
On Thursday, April 26, 2007 1:35 AM, Dirk Mueller wrote:
This patch corrects a |/|| confusion in
mptscsih_copy_sense_data. Using ||
means that the data that ends up being written is (almost always) 1,
instead of being bit-wise or'ed.
Cc: Eric Moore [EMAIL PROTECTED]
Cc: James Bottomley [EMAIL PROTECTED]
Signed-off-by: Andrew Morton [EMAIL PROTECTED]
---
drivers/message/fusion/mptscsih.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff -puN
drivers/message/fusion/mptscsih.c~fix--confusion-in-fusion-dri
ver drivers/message/fusion/mptscsih.c
---
a/drivers/message/fusion/mptscsih.c~fix--confusion-in-fusion-driver
+++ a/drivers/message/fusion/mptscsih.c
@@ -2463,9 +2463,9 @@ mptscsih_copy_sense_data(struct scsi_cmn
ioc-events[idx].event =
MPI_EVENT_SCSI_DEVICE_STATUS_CHANGE;
ioc-events[idx].eventContext =
ioc-eventContext;
- ioc-events[idx].data[0] =
(pReq-LUN[1] 24) ||
-
(MPI_EVENT_SCSI_DEV_STAT_RC_SMART_DATA 16) ||
- (sc-device-channel
8) || sc-device-id;
+ ioc-events[idx].data[0] =
(pReq-LUN[1] 24) |
+
(MPI_EVENT_SCSI_DEV_STAT_RC_SMART_DATA 16) |
+ (sc-device-channel
8) | sc-device-id;
ioc-events[idx].data[1] =
(sense_data[13] 8) || sense_data[12];
Thanks, I agree with the change, however shouldn't we be changing:
ioc-events[idx].data[1] = (sense_data[13] 8) || sense_data[12];
to
ioc-events[idx].data[1] = (sense_data[13] 8) | sense_data[12];
And in the mptbase.h, the definition of the struct should be changed
from
typedef struct _mpt_ioctl_events {
u32 event; /* Specified by define above */
u32 eventContext; /* Index or counter */
int data[2];/* First 8 bytes of Event Data
*/
} MPT_IOCTL_EVENTS;
to
typedef struct _mpt_ioctl_events {
u32 event; /* Specified by define above */
u32 eventContext; /* Index or counter */
u32 data[2];/* First 8 bytes of Event Data
*/
} MPT_IOCTL_EVENTS;
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [patch 18/30] Fix |/|| confusion in fusion driver
On Tue, 2007-05-08 at 11:06 -0600, Moore, Eric wrote:
On Thursday, April 26, 2007 1:35 AM, Dirk Mueller wrote:
This patch corrects a |/|| confusion in
mptscsih_copy_sense_data. Using ||
means that the data that ends up being written is (almost always) 1,
instead of being bit-wise or'ed.
Cc: Eric Moore [EMAIL PROTECTED]
Cc: James Bottomley [EMAIL PROTECTED]
Signed-off-by: Andrew Morton [EMAIL PROTECTED]
---
drivers/message/fusion/mptscsih.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff -puN
drivers/message/fusion/mptscsih.c~fix--confusion-in-fusion-dri
ver drivers/message/fusion/mptscsih.c
---
a/drivers/message/fusion/mptscsih.c~fix--confusion-in-fusion-driver
+++ a/drivers/message/fusion/mptscsih.c
@@ -2463,9 +2463,9 @@ mptscsih_copy_sense_data(struct scsi_cmn
ioc-events[idx].event =
MPI_EVENT_SCSI_DEVICE_STATUS_CHANGE;
ioc-events[idx].eventContext =
ioc-eventContext;
- ioc-events[idx].data[0] =
(pReq-LUN[1] 24) ||
-
(MPI_EVENT_SCSI_DEV_STAT_RC_SMART_DATA 16) ||
- (sc-device-channel
8) || sc-device-id;
+ ioc-events[idx].data[0] =
(pReq-LUN[1] 24) |
+
(MPI_EVENT_SCSI_DEV_STAT_RC_SMART_DATA 16) |
+ (sc-device-channel
8) | sc-device-id;
ioc-events[idx].data[1] =
(sense_data[13] 8) || sense_data[12];
Thanks, I agree with the change, however shouldn't we be changing:
ioc-events[idx].data[1] = (sense_data[13] 8) || sense_data[12];
to
ioc-events[idx].data[1] = (sense_data[13] 8) | sense_data[12];
And in the mptbase.h, the definition of the struct should be changed
from
typedef struct _mpt_ioctl_events {
u32 event; /* Specified by define above */
u32 eventContext; /* Index or counter */
int data[2];/* First 8 bytes of Event Data
*/
} MPT_IOCTL_EVENTS;
to
typedef struct _mpt_ioctl_events {
u32 event; /* Specified by define above */
u32 eventContext; /* Index or counter */
u32 data[2];/* First 8 bytes of Event Data
*/
} MPT_IOCTL_EVENTS;
Probably ... could you update the patch accordingly and send it in?
James
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
On Tue, May 08 2007, James Bottomley wrote: On Tue, 2007-05-08 at 18:40 +0200, Jens Axboe wrote: On Tue, May 08 2007, James Bottomley wrote: On Tue, 2007-05-08 at 18:14 +0200, Jens Axboe wrote: On Tue, May 08 2007, Alan Cox wrote: The CD-ROM layer doesn't bounce requests for old ISA controllers (and nor should it). However they get injected into the SCSI layer via sr_ioctl which also doesn't bounce them and SCSI then passes the buffer along to a device with unchecked_isa_dma set which either panics or truncates the buffer to 24bits. According to Jens the right long term fix is for the CD layer to route the requests differently but in the mean time this has been tested by a victim and verified to sort the problem out. For the other 99.9% of users it's a no-op and doesn't bounce data. Signed-off-by: Alan Cox [EMAIL PROTECTED] Signed-off-by: Jens Axboe [EMAIL PROTECTED] Christoph passed me his patch to get rid of -generic_packet() in the cdrom layer, so the work is almost complete. This patch is fine as a work-around until that gets merged, though. Actually, I think the new scsi request infrastructure should be doing the bouncing (rather than have it done in each problem path we discover). Of course, bouncing should only be done in one layer (the block layer). Mike Christie tells me we're missing bouncing by accident in the scsi_execute path (but not the scsi_execute_async path). He says this is the fix he proposed: http://marc.info/?l=linux-scsim=115981479822790w=2 Can we just merge this instead? That's another issue. The problem here are requests (cgc's) initiated by the cdrom.c layer. Those _should_ get mapped to a request and put on the queue for the device, and thus get bounced by the block layer if appropriate. Mike's fix looks legit and should be merged as well, but it wont fix this issue. It won't? I thought the issue (from the fix) is that cgc-buffer is outside of the device accessibility mask. The scsi_execute path allocates a request and then calls blk_rq_map_kern on the buffer (in this case cgc-buffer) ... the problem is that blk_rq_map_kern() doesn't currently bounce the buffer, but if it did (which is the functionality Mike's patch adds), surely the need to bounce it in the ioctl path would go away? You are right, I missed that scsi_execute() actually builds a request in the proper manner. Mikes patch is in the for-2.6.22 block branch and I asked Linus to pull, so all should be well. -- Jens Axboe - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
Mike Christie tells me we're missing bouncing by accident in the scsi_execute path (but not the scsi_execute_async path). He says this is the fix he proposed: http://marc.info/?l=linux-scsim=115981479822790w=2 Can we just merge this instead? Short answer: No Long answer - it doesn't take this path. Different bug, both want fixing I suspect. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
On Tue, 2007-05-08 at 18:42 +0100, Alan Cox wrote: Mike Christie tells me we're missing bouncing by accident in the scsi_execute path (but not the scsi_execute_async path). He says this is the fix he proposed: http://marc.info/?l=linux-scsim=115981479822790w=2 Can we just merge this instead? Short answer: No Long answer - it doesn't take this path. Different bug, both want fixing I suspect. Actually, it does take this path ... one of the things we've been doing in SCSI is slowly eliminating the old direct submission paths in favour of sending everything through the correct block layer paths. scsi_execute(), which the sr ioctl uses is just such a fixed path ... the bug is that it should be bouncing the request but because of an oversight (which Mike's patch corrects) it doesn't. The ultimate goal is to be able to eliminate the unchecked_isa_dma flag entirely and have the block layer (or device mask allocations) fix all of this in every ULD. James - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
Long answer - it doesn't take this path. Different bug, both want fixing I suspect. Actually, it does take this path ... one of the things we've been doing in SCSI is slowly eliminating the old direct submission paths in favour of sending everything through the correct block layer paths. scsi_execute(), which the sr ioctl uses is just such a fixed path ... the bug is that it should be bouncing the request but because of an oversight (which Mike's patch corrects) it doesn't. Well if Mike's patch is going in and it fixes this then I'll be more than happy to withdraw the pending one. The ultimate goal is to be able to eliminate the unchecked_isa_dma flag entirely and have the block layer (or device mask allocations) fix all of this in every ULD. About time ;) Alan - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
On Tue, 2007-05-08 at 18:53 +0100, Alan Cox wrote: The ultimate goal is to be able to eliminate the unchecked_isa_dma flag entirely and have the block layer (or device mask allocations) fix all of this in every ULD. About time ;) Actually, I should point out (before those who did the work get justifiably irritated) that the overall goal is to remove the special case non-scatter/gather path from all the drivers ... eliminating the need to worry about DMA zone allocations is just a nice bonus as a side effect of sending everything through the block layer. James - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] add bidi support for block pc requests
FUJITA Tomonori wrote:
Here is an updated version of the patch to add bidi support to block
pc requests. Bugs spotted by Benny were fixed.
This patch can be applied cleanly to the scsi-misc git tree and is on
the top of the following patch to add linked request support:
http://marc.info/?l=linux-scsim=117835587615642w=2
---
From: FUJITA Tomonori [EMAIL PROTECTED]
Date: Mon, 7 May 2007 16:42:24 +0900
Subject: [PATCH] add bidi support to scsi pc commands
This adds bidi support to block pc requests.
A bidi request uses req-next_rq pointer for an in request.
This patch introduces a new structure, scsi_data_buffer to hold the
data buffer information. To avoid make scsi_cmnd structure fatter, the
scsi mid-layer uses cmnd-request-next_rq-special pointer for
a scsi_data_buffer structure. LLDs don't touch the second request
(req-next_rq) so it's safe to use req-special.
scsi_blk_pc_done() always completes the whole command so
scsi_end_request() simply completes the bidi chunk too.
A helper function, scsi_bidi_data_buffer() is for LLDs to access to
the scsi_data_buffer structure easily.
Signed-off-by: FUJITA Tomonori [EMAIL PROTECTED]
---
@@ -685,6 +696,14 @@ static struct scsi_cmnd *scsi_end_request(struct
scsi_cmnd *cmd, int uptodate,
}
}
+ /*
+ * a REQ_BLOCK_PC command is always completed fully so just do
+ * end the bidi chunk.
+ */
+ if (sdb)
+ end_that_request_chunk(req-next_rq, uptodate,
+sdb-request_bufflen);
+
sdb-request_bufflen was zeroed in scsi_release_buffers which is called before
scsi_end_request.
static void scsi_blk_pc_done(struct scsi_cmnd *cmd)
{
BUG_ON(!blk_pc_request(cmd-request));
@@ -1090,10 +1159,22 @@ static void scsi_blk_pc_done(struct scsi_cmnd *cmd)
static int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request
*req)
{
struct scsi_cmnd *cmd;
+ struct scsi_data_buffer *sdb = NULL;
+
+ if (blk_bidi_rq(req)) {
+ sdb = kzalloc(sizeof(*sdb), GFP_ATOMIC);
+ if (unlikely(!sdb))
+ return BLKPREP_DEFER;
+ }
cmd = scsi_get_cmd_from_req(sdev, req);
- if (unlikely(!cmd))
+ if (unlikely(!cmd)) {
+ kfree(sdb);
return BLKPREP_DEFER;
+ }
+
+ if (sdb)
+ req-next_rq-special = sdb;
/*
* BLOCK_PC requests may transfer data, in which case they must
Before I get to my main concern here I have one comment. in
scsi_get_cmd_from_req()
there is a code path in which a scsi_cmnd is taken from special and is not newly
allocated. It is best to move bidi allocation to scsi_get_cmd_from_req() and
allocate
new sdb only if we get a new Command. (See my attached patch for an example)
OK! My main concern is with kzalloc(sizeof(*sdb), GFP_ATOMIC);
All IO allocations should come from slabs in case of a memory starved system.
There are 3 possible solutions I see:
1. Use struct scsi_cmnd for bidi_read and not a special scsi_data_buffer.
Attached is above solution. It is by far the simplest of the three.
So simple that even Jens's BigIO patch can almost apply at scsi_lib.c. (and
vise versa)
What's hanged on the request-next_rq-special is a second scsi_cmnd.
The command is taken from regular command pool. This solution, though
wasteful of some memory is very stable.
2. Force the users of BIDI to allocate scsi_data_buffer(s)
Users will allocate a slab for scsi_data_buffers and hang them on
nex_rq-special before
hand. Than free them together with second request. This approach can be very
stable, But
it is bad because it is a layering violation. When block and SCSI layers
decide to change
the wiring of bidi. Users will have to change with them.
3. Let SCSI-ml manage a slab for scsi_data_buff's
Have a flag to scsi_setup_command_freelist() or a new API. When a device is
flagged
bidi-able we could 1-Allocate bigger slots to have extra memory for SDBs
together
with the command itself. or 2-allocate yet another slab for SDBs.
The 3rd approach is clean, and I can submit a patch for it. But I think it is
not currently
necessary. The first solution (See attached patch) we can all live with, I
hope. Since for
me it gives me the stability I need. And for the rest of the kernel it is the
minimum
change, layered on existing building blocks.
If any one wants to try it out I put up the usual patchset that exercise this
approach here.
http://www.bhalevy.com/open-osd/download/double_rq_bidi
Thanks
Boaz
From ac8f0d3df711c5d01a269fde6a4ecce3000c3f68 Mon Sep 17 00:00:00 2001
From: Boaz Harrosh [EMAIL PROTECTED]
Date: Tue, 8 May 2007 14:04:46 +0300
Subject: [PATCH] scsi-ml bidi support
At the block level bidi request uses req-next_rq pointer for a second
bidi_read request.
At Scsi-midlayer a second scsi_cmnd structure is used for the sglists
of
Re: SG_IO with 4k buffer size to iscsi sg device causes Bad page panic
Qi, Yanling wrote: Hi All, This panic is related to the interactions between scsi/sg.c, iscsi initiator and tcp on the RHEL 2.6.9-42 kernel. But we may also have the similar problem with open-iscsi initiator. I will explain why we see the Yeah, this problem should occur in the upstream open-iscsi iscsi code. open-iscsi works very similar to linux-scsi where it just sends pages around with sock-ops-sendpage, and it looks like sg uses __get_free_pages in RHEL's kernel and upstream it uses alloc_pages so unless there was a change in those functions or the network layer then we should have a similar problem. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] add bidi support for block pc requests
On Tue, 2007-05-08 at 21:53 +0300, Boaz Harrosh wrote: Before I get to my main concern here I have one comment. in scsi_get_cmd_from_req() there is a code path in which a scsi_cmnd is taken from special and is not newly allocated. It is best to move bidi allocation to scsi_get_cmd_from_req() and allocate new sdb only if we get a new Command. (See my attached patch for an example) OK! My main concern is with kzalloc(sizeof(*sdb), GFP_ATOMIC); All IO allocations should come from slabs in case of a memory starved system. I think you'll find that kzalloc comes directly out of a slab for this size of allocation anyway ... you mean you want to see a dedicated pool for this specific allocation? There are 3 possible solutions I see: 1. Use struct scsi_cmnd for bidi_read and not a special scsi_data_buffer. Attached is above solution. It is by far the simplest of the three. So simple that even Jens's BigIO patch can almost apply at scsi_lib.c. (and vise versa) What's hanged on the request-next_rq-special is a second scsi_cmnd. The command is taken from regular command pool. This solution, though wasteful of some memory is very stable. There's another problem in that it destroys our forward progress guarantee. There's always a single reserve command for every HBA so that forward progress for freeing memory can always be made in the system even if the command slab is out and we have to reclaim memory through a HBA with no outstanding commands. Allocating two commands per bidirectional request hoses that guarantee ... it could be fixed up by increasing the reserve pool to 2, but that's adding further unwanted complexity ... 2. Force the users of BIDI to allocate scsi_data_buffer(s) Users will allocate a slab for scsi_data_buffers and hang them on nex_rq-special before hand. Than free them together with second request. This approach can be very stable, But it is bad because it is a layering violation. When block and SCSI layers decide to change the wiring of bidi. Users will have to change with them. I Agree. 3. Let SCSI-ml manage a slab for scsi_data_buff's Have a flag to scsi_setup_command_freelist() or a new API. When a device is flagged bidi-able we could 1-Allocate bigger slots to have extra memory for SDBs together with the command itself. or 2-allocate yet another slab for SDBs. if you're worried about allocations, doing a single allocate is better The 3rd approach is clean, and I can submit a patch for it. But I think it is not currently necessary. The first solution (See attached patch) we can all live with, I hope. Since for me it gives me the stability I need. And for the rest of the kernel it is the minimum change, layered on existing building blocks. There's actually a fourth option you haven't considered: Roll all the required sglist definitions (request_bufflen, request_buffer, use_sg and sglist_len) into the sgtable pools. We're getting very close to the point where someone gets to sweep through the drivers eliminating the now superfluous non-sg path in the queuecommand. When that happens the only cases become no transfer or SG backed commands. At this point we can do a consolidation of the struct scsi_cmnd fields. This does represent the ideal time to sweep the sg list handling fields into the sgtable and simply have a single pointer to struct sgtable in the scsi_cmnd (== NULL is the signal for a no transfer command). James - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 16/30] drivers/scsi/aic7xxx_old: Convert to generic boolean-values
On Tue, 08 May 2007 14:24:32 -0500 James Bottomley [EMAIL PROTECTED] wrote: However, could we compromise and just add TRUE = true, FALSE = false to the enum? That sounds sane. But I don't recall all the details of the discussion and perhaps I'm missing something. I think the whole bool/true/false thing is pretty dissatisfactory really. Java gets this right and C cannot and will not and we end up with people using true and false as plain old 1 and 0. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 16/30] drivers/scsi/aic7xxx_old: Convert to generic boolean-values
On Tue, 2007-05-08 at 13:11 -0700, Andrew Morton wrote: On Tue, 08 May 2007 14:24:32 -0500 James Bottomley [EMAIL PROTECTED] wrote: However, could we compromise and just add TRUE = true, FALSE = false to the enum? That sounds sane. But I don't recall all the details of the discussion and perhaps I'm missing something. I think the whole bool/true/false thing is pretty dissatisfactory really. Java gets this right and C cannot and will not and we end up with people using true and false as plain old 1 and 0. Right, that's the core of my complaint ... zero is false and anything else is true ... I rather like the C isms that allow things like if (ptr) *ptr = ... If there were evidence that the C treatment of logicals as arithmeticals was causing huge problems for driver writers, then yes I'd reconsider my position ... but really any driver writer has to use bare metal arithmetical values interchangeably with logical values because that's the way device registers work. James - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 16/30] drivers/scsi/aic7xxx_old: Convert to generic boolean-values
From: Andrew Morton [EMAIL PROTECTED] Date: Tue, 8 May 2007 13:11:52 -0700 On Tue, 08 May 2007 14:24:32 -0500 James Bottomley [EMAIL PROTECTED] wrote: However, could we compromise and just add TRUE = true, FALSE = false to the enum? That sounds sane. But I don't recall all the details of the discussion and perhaps I'm missing something. I think the whole bool/true/false thing is pretty dissatisfactory really. Java gets this right and C cannot and will not and we end up with people using true and false as plain old 1 and 0. I think it's more important to be consistent across the entire tree, whatever we choose, than to be nice and add compat define hacks for the sake of a select few stubborn drivers. If you're going to add those ugly #define TRUE true bits, the whole point of the change is lost so you might as not make it at all. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 16/30] drivers/scsi/aic7xxx_old: Convert to generic boolean-values
On Tue, 2007-05-08 at 14:44 -0700, David Miller wrote: From: Andrew Morton [EMAIL PROTECTED] Date: Tue, 8 May 2007 13:11:52 -0700 On Tue, 08 May 2007 14:24:32 -0500 James Bottomley [EMAIL PROTECTED] wrote: However, could we compromise and just add TRUE = true, FALSE = false to the enum? That sounds sane. But I don't recall all the details of the discussion and perhaps I'm missing something. I think the whole bool/true/false thing is pretty dissatisfactory really. Java gets this right and C cannot and will not and we end up with people using true and false as plain old 1 and 0. In that case, I think we go with what's there, which seems to be predominantly TRUE/FALSE. I think it's more important to be consistent across the entire tree, whatever we choose, than to be nice and add compat define hacks for the sake of a select few stubborn drivers. If you're going to add those ugly #define TRUE true bits, the whole point of the change is lost so you might as not make it at all. That too wouldn't be an unacceptable outcome ... James - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PATCH] final SCSI updates for 2.6.21
This should be the second half of the SCSI tree, mainly assorted driver updates and fixes. The patch is available from: master.kernel.org:/pub/scm/linux/kernel/git/jejb/scsi-misc-2.6.git The short changelog is: Adrian Bunk (1): qla4xxx: possible cleanups Amol Lad (1): megaraid: replace yield() with cond_resched() Andrew Vasquez (6): qla2xxx: Update version number to 8.01.07-k7. qla2xxx: Add MSI support. qla2xxx: Correct pci_set_msi() usage semantics. qla2xxx: Attempt to stop firmware only if it had been previously executed. qla2xxx: Honor NVRAM port-down-retry-count settings. qla2xxx: Error-out during probe() if we're unable to complete HBA initiali Brian King (11): ipr: Use PCI-E reset API for new ipr adapter ipr: Better handle adapter boot time errors ipr: Enable multi-initator RAID support ipr: Improved dual adapter errors ipr: Increase adapter operational timeout ipr: Handle IOA reset request ipr: Handle check condition status from disk array device ipr: Handle UA on disk array following an adapter reset ipr: Allow driver_data to be passed for dynamic ids ipr: Prevent overlapped adapter resets use sysfs configured timeout for EH Start Unit timeout Christof Schmitt (4): zfcp: Fix deadlock between zfcp ERP and SCSI zfcp: Locking for req_no and req_seq_no zfcp: print S_ID and D_ID with 3 bytes zfcp: Stop system after memory corruption Christoph Hellwig (2): deprecate the old NCR53C9x driver sas_scsi_host: Convert to use the kthread API David Milburn (1): megaraid: update version reported by MEGAIOC_QDRVRVER Guennadi Liakhovetski (3): tmscsim: Remove the last bus_to_virt() tmscsim: remove bogus endianness conversions tmscsim: remove long dead DMA_INT Heiko Carstens (3): zfcp: clear boxed flag on unit reopen. zfcp: clear adapter failed flag if an fsf request times out. zfcp: rework request ID management. James Smart (17): fc_transport: make all rports wait dev_loss_tmo before removing them lpfc 8.1.12 : Change version number to 8.1.12 lpfc 8.1.12 : Update copyright year to 2007 lpfc 8.1.12 : Added support for 8G speed and new HBAs lpfc 8.1.12 : Add support for async scanning lpfc 8.1.12 : Don't process ERATT interrupts when issuing KILL_BOARD mbx c lpfc 8.1.12 : Collapse discovery lists to a single node list lpfc 8.1.12 : Reference count node structures for node lifetime management lpfc 8.1.12 : Improve handling of failed ELS aborts lpfc 8.1.12 : Improve diagnostic messages and change local loopback messag lpfc 8.1.12 : Fixed recovery of rport after race with dev_loss_tmo lpfc 8.1.12 : Round 2 of Miscellaneous fixes lpfc 8.1.12 : Reorganize lpfc_nlp_list() and callers to prepare for nodeli lpfc 8.1.12 : Fix unlock inside list traversal lpfc 8.1.12 : Rework offline path to solve HBA reset issues lpfc 8.1.12 : Modify ELS abort handling to prevent double completion lpfc 8.1.12 : Misc bug fixes and code cleanup Matthias Kaehlcke (1): qla1280: use DMA_64BIT_MASK instead of ~ 0ULL Olaf Hering (1): mesh: cleanup variable usage in interrupt handler Salyzyn, Mark (4): aacraid: correct SUN products to README aacraid: superfluous adapter reset for IBM 8 series ServeRAID controllers aacraid: kexec fix (reset interrupt handler) aacraid: kmalloc/memset-kzalloc [EMAIL PROTECTED] (2): dpt_i2o: kmalloc/memset-kzalloc ch: kmalloc/memset-kzalloc walter harms (1): megaraid: fix warnings when CONFIG_PROC_FS=n And the diffstat: Documentation/feature-removal-schedule.txt |9 Documentation/scsi/aacraid.txt |4 drivers/s390/scsi/zfcp_aux.c | 90 -- drivers/s390/scsi/zfcp_def.h | 41 + drivers/s390/scsi/zfcp_erp.c | 89 ++ drivers/s390/scsi/zfcp_ext.h |4 drivers/s390/scsi/zfcp_fsf.c | 50 - drivers/s390/scsi/zfcp_qdio.c | 49 - drivers/s390/scsi/zfcp_scsi.c |9 drivers/scsi/aacraid/comminit.c|3 drivers/scsi/aacraid/commsup.c |6 drivers/scsi/aacraid/dpcsup.c |6 drivers/scsi/aacraid/rx.c |4 drivers/scsi/ch.c |9 drivers/scsi/dpt_i2o.c | 17 drivers/scsi/ipr.c | 349 ++- drivers/scsi/ipr.h | 33 + drivers/scsi/libsas/sas_scsi_host.c| 36 - drivers/scsi/lpfc/lpfc.h | 33 - drivers/scsi/lpfc/lpfc_attr.c | 212 -- drivers/scsi/lpfc/lpfc_crtn.h | 32 - drivers/scsi/lpfc/lpfc_ct.c| 24 drivers/scsi/lpfc/lpfc_disc.h | 28 drivers/scsi/lpfc/lpfc_els.c
