Re: [PATCH 7/8] be2iscsi: Fix processing cqe for cxn whose endpoint is freed
I will pull out that patch from the drivers queue, thanks. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] be2iscsi: Fix processing cqe for cxn whose endpoint is freed
On Mon, Jun 02, 2014 at 07:22:07PM +, James Bottomley wrote: Actually, can you really pull it out, not just revert it? Reverts cause problems with bisection and are unnecessary before the tree goes to Linus. If preserving history becomes a real goal, we'd have to move to a tip like model, but I'm happy coping with the rebase that dropping a patch from a monolithic tree causes. Linus has been very unappy about rebasing close to or in the merge window, and other subsystems generally revert patches that late in the cycle as well. I'd prefer to stick to that model, but if you and Linus prefer the rebase now I can do it as well of course. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] be2iscsi: Fix processing cqe for cxn whose endpoint is freed
On Mon, 2014-06-02 at 12:33 -0700, h...@infradead.org wrote: On Mon, Jun 02, 2014 at 07:22:07PM +, James Bottomley wrote: Actually, can you really pull it out, not just revert it? Reverts cause problems with bisection and are unnecessary before the tree goes to Linus. If preserving history becomes a real goal, we'd have to move to a tip like model, but I'm happy coping with the rebase that dropping a patch from a monolithic tree causes. Linus has been very unappy about rebasing close to or in the merge window, and other subsystems generally revert patches that late in the cycle as well. I'd prefer to stick to that model, but if you and Linus prefer the rebase now I can do it as well of course. Yes, please. I've done it before, but note what happened in the pull message. He doesn't usually object to this in SCSI because no-one really develops based on our tree. As I said, if they did, we'd need to follow a tip model, so we could just drop the problem driver and redo the patches. I also tend to delay the pull a day or so to give linux-next time to pick up the change, so that Stephen Rothwell doesn't complain about it. Thanks, James -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] be2iscsi: Fix processing cqe for cxn whose endpoint is freed
On Mon, Jun 2, 2014 at 12:33 PM, h...@infradead.org h...@infradead.org wrote: Linus has been very unappy about rebasing close to or in the merge window, and other subsystems generally revert patches that late in the cycle as well. I'd prefer to stick to that model, but if you and Linus prefer the rebase now I can do it as well of course. I would indeed prefer to avoid rebases, _unless_ the tree is a real mess without it. Now, what constitues real mess can vary. It can be just really ugly history, and part of that can be it doesn't build or work at all partway through. If it causes major build or boot failures the code is *not* worth merging as-is, because that ends up being really painful for bisection etc. But for it to matter for bisection, it has to be a _major_ failure that actually matters to real people. So I'm not talking about odd make randomconfig failures, but painful build failures that actually hit reasonable configurations, and boot failures that hit relevant hardware configurations. Even in the absense of major build/working problems, real mess can be about just horrible history with tons of stupid merges that just makes it much harder to figure out what was going on. So it's a judgment thing in the end, not a black-and-white never rebase. I don't know how core that revert is. Linus -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] be2iscsi: Fix processing cqe for cxn whose endpoint is freed
On Mon, Jun 02, 2014 at 01:05:38PM -0700, Linus Torvalds wrote: I would indeed prefer to avoid rebases, _unless_ the tree is a real mess without it. Now, what constitues real mess can vary. It can be just really ugly history, and part of that can be it doesn't build or work at all partway through. If it causes major build or boot failures the code is *not* worth merging as-is, because that ends up being really painful for bisection etc. But for it to matter for bisection, it has to be a _major_ failure that actually matters to real people. So I'm not talking about odd make randomconfig failures, but painful build failures that actually hit reasonable configurations, and boot failures that hit relevant hardware configurations. It's reverting a patch that just doesn't fix a problem fully, so the prime reviewer and the patch author decided to withdraw it. It won't cause any kind of problem during bisection. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] be2iscsi: Fix processing cqe for cxn whose endpoint is freed
On Mon, Jun 2, 2014 at 1:08 PM, h...@infradead.org h...@infradead.org wrote: It's reverting a patch that just doesn't fix a problem fully, so the prime reviewer and the patch author decided to withdraw it. It won't cause any kind of problem during bisection. If it isn't a particularly large patch and doesn't have any other issues, I'd suggest just reverting it. Particularly large patches can be worth undoing just to avoid unnecessary noise in git blame etc, but that's an issue mainly for things like whitespace crap that really touches a *lot* of code and so the noise is a big thing. Linus -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] be2iscsi: Fix processing cqe for cxn whose endpoint is freed
On Mon, Jun 02, 2014 at 01:24:22PM -0700, Linus Torvalds wrote: If it isn't a particularly large patch and doesn't have any other issues, I'd suggest just reverting it. Particularly large patches can be worth undoing just to avoid unnecessary noise in git blame etc, but that's an issue mainly for things like whitespace crap that really touches a *lot* of code and so the noise is a big thing. It's an smal and simple driver patch: http://git.infradead.org/users/hch/scsi-queue.git/commitdiff/4f96827dd55981ec4bfcbc7584eb155bcd8d1849 -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 7/8] be2iscsi: Fix processing cqe for cxn whose endpoint is freed
-Original Message-
From: Mike Christie [mailto:micha...@cs.wisc.edu]
Sent: Thursday, May 08, 2014 3:49 AM
To: Jay Kallickal
Cc: jbottom...@parallels.com; linux-scsi@vger.kernel.org; Jayamohan Kallickal;
Minh Duc Tran; Sony John-N
Subject: Re: [PATCH 7/8] be2iscsi: Fix processing cqe for cxn whose endpoint is
freed
On 05/05/2014 08:41 PM, Jay Kallickal wrote:
From: Jayamohan Kallickal jayamohan.kallic...@emulex.com
During heavy IO in multipath environment with many active sessions
and port-bouncing happening, there is a race condition because of
which beiscsi_prcess_cqe() gets called for a connection whose
endpoint is freed.
Checking endpoint reference for a connection before processing in
beiscsi_process_cq().
Signed-off-by: Minh Tran minhduc.t...@emulex.com
Signed-off-by: John Soni Jose sony.joh...@emulex.com
Signed-off-by: Jayamohan Kallickal jayamohan.kallic...@emulex.com
---
drivers/scsi/be2iscsi/be_main.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/scsi/be2iscsi/be_main.c
b/drivers/scsi/be2iscsi/be_main.c index dccda6c..5a7022f 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -2110,6 +2110,16 @@ static unsigned int beiscsi_process_cq(struct
be_eq_obj *pbe_eq)
cri_index = BE_GET_CRI_FROM_CID(cid);
ep = phba-ep_array[cri_index];
+ if (unlikely(ep == NULL)) {
+ /* connection has already been freed
+ * just move on to next one
+ */
+ beiscsi_log(phba, KERN_WARNING,
+ BEISCSI_LOG_INIT,
+ BM_%d : proc cqe of disconn ep: cid %d\n,
+ cid);
+ goto proc_next_cqe;
+ }
beiscsi_ep = ep-dd_data;
beiscsi_conn = beiscsi_ep-conn;
It looks like if that race is possible then we could also free the ep while
you are accessing right? I think you would need to get a ref to the ep.
We will pull out this patch from this release. This is a very corner case and
requires changes to be done in the IO path of the driver. We will redo the
patch and submit in our next release.
Please pull-in all the other patches in this set expect this particular one
[PATCH 7/8] be2iscsi: Fix processing cqe for cxn whose endpoint is freed.
What command/function tells the card to stop sending the driver
events/notifications/ios for that connection? Is it beiscsi_close_conn or
mgmt_invalidate_connection?
Mgmt_invalidate_connection tell card to stop sending events to the driver for a
particular connection.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] be2iscsi: Fix processing cqe for cxn whose endpoint is freed
On Wed, May 07, 2014 at 05:18:38PM -0500, Mike Christie wrote: It looks like if that race is possible then we could also free the ep while you are accessing right? I think you would need to get a ref to the ep. What command/function tells the card to stop sending the driver events/notifications/ios for that connection? Is it beiscsi_close_conn or mgmt_invalidate_connection? Jay, can you look into this issue please? I've applied the series to the scsi queue for now, but I'd really prefer to see this addressed ASAP. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] be2iscsi: Fix processing cqe for cxn whose endpoint is freed
On 05/05/2014 08:41 PM, Jay Kallickal wrote:
From: Jayamohan Kallickal jayamohan.kallic...@emulex.com
During heavy IO in multipath environment with many active sessions
and port-bouncing happening, there is a race condition because of
which beiscsi_prcess_cqe() gets called for a connection whose
endpoint is freed.
Checking endpoint reference for a connection before processing in
beiscsi_process_cq().
Signed-off-by: Minh Tran minhduc.t...@emulex.com
Signed-off-by: John Soni Jose sony.joh...@emulex.com
Signed-off-by: Jayamohan Kallickal jayamohan.kallic...@emulex.com
---
drivers/scsi/be2iscsi/be_main.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index dccda6c..5a7022f 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -2110,6 +2110,16 @@ static unsigned int beiscsi_process_cq(struct
be_eq_obj *pbe_eq)
cri_index = BE_GET_CRI_FROM_CID(cid);
ep = phba-ep_array[cri_index];
+ if (unlikely(ep == NULL)) {
+ /* connection has already been freed
+ * just move on to next one
+ */
+ beiscsi_log(phba, KERN_WARNING,
+ BEISCSI_LOG_INIT,
+ BM_%d : proc cqe of disconn ep: cid %d\n,
+ cid);
+ goto proc_next_cqe;
+ }
beiscsi_ep = ep-dd_data;
beiscsi_conn = beiscsi_ep-conn;
It looks like if that race is possible then we could also free the ep
while you are accessing right? I think you would need to get a ref to
the ep.
What command/function tells the card to stop sending the driver
events/notifications/ios for that connection? Is it beiscsi_close_conn
or mgmt_invalidate_connection?
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 7/8] be2iscsi: Fix processing cqe for cxn whose endpoint is freed
From: Jayamohan Kallickal jayamohan.kallic...@emulex.com
During heavy IO in multipath environment with many active sessions
and port-bouncing happening, there is a race condition because of
which beiscsi_prcess_cqe() gets called for a connection whose
endpoint is freed.
Checking endpoint reference for a connection before processing in
beiscsi_process_cq().
Signed-off-by: Minh Tran minhduc.t...@emulex.com
Signed-off-by: John Soni Jose sony.joh...@emulex.com
Signed-off-by: Jayamohan Kallickal jayamohan.kallic...@emulex.com
---
drivers/scsi/be2iscsi/be_main.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index dccda6c..5a7022f 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -2110,6 +2110,16 @@ static unsigned int beiscsi_process_cq(struct be_eq_obj
*pbe_eq)
cri_index = BE_GET_CRI_FROM_CID(cid);
ep = phba-ep_array[cri_index];
+ if (unlikely(ep == NULL)) {
+ /* connection has already been freed
+* just move on to next one
+*/
+ beiscsi_log(phba, KERN_WARNING,
+ BEISCSI_LOG_INIT,
+ BM_%d : proc cqe of disconn ep: cid %d\n,
+ cid);
+ goto proc_next_cqe;
+ }
beiscsi_ep = ep-dd_data;
beiscsi_conn = beiscsi_ep-conn;
@@ -2219,6 +2229,7 @@ static unsigned int beiscsi_process_cq(struct be_eq_obj
*pbe_eq)
break;
}
+proc_next_cqe:
AMAP_SET_BITS(struct amap_sol_cqe, valid, sol, 0);
queue_tail_inc(cq);
sol = queue_tail_node(cq);
--
1.8.5.3
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
