qla4xxx: added srb referance counter
Subsystems (2)
PATCH 6/11 qla4xxx: added srb referance counter
From: Vikas Chaudhary <[email protected]>
Serialization between the error handler and recovery code.
--- drivers/scsi/qla4xxx/ql4_glbl.h | 2 + drivers/scsi/qla4xxx/ql4_isr.c | 6 ++-- drivers/scsi/qla4xxx/ql4_os.c | 58 +++++++++++++++++++++++++++++++++++---- 3 files changed, 57 insertions(+), 9 deletions(-)
drivers/scsi/qla4xxx/ql4_glbl.h +2 -0
@@ -73,6 +73,8 @@ void qla4xxx_dump_buffer(void *b, uint32_t size);int qla4xxx_send_marker_iocb (struct scsi_qla_host *ha,struct ddb_entry *ddb_entry,int lun,uint16_t mrkr_mod); +void sp_put (struct scsi_qla_host *ha,struct srb *sp); +extern int ql4xextended_error_logging;extern int ql4xdiscoverywait;extern int ql4xdontresethba;
drivers/scsi/qla4xxx/ql4_isr.c +3 -3
@@ -97,7 +97,7 @@ qla4xxx_status_cont_entry(struct scsi_qla_host *ha,/* Place command on done queue. */ if (srb->req_sense_len ==0 ) { -qla4xxx_srb_compl (ha, srb); +sp_put (ha, srb); ha->status_srb =NULL ; } }
@@ -329,7 +329,7 @@ status_entry_exit:/* complete the request, if not waiting for status_continuation pkt */ srb->cc_stat = sts_entry->completionStatus;if (ha->status_srb ==NULL ) -qla4xxx_srb_compl (ha, srb); +sp_put (ha, srb); }/**
@@ -393,7 +393,7 @@ static void qla4xxx_process_response_queue(struct scsi_qla_host * ha)/* ETRY normally by sending it back with * DID_BUS_BUSY */ srb->cmd->result =DID_BUS_BUSY <<16 ; -qla4xxx_srb_compl (ha, srb); +sp_put (ha, srb);break ;case ET_CONTINUE :
drivers/scsi/qla4xxx/ql4_os.c +52 -6
102 lines not on this page · raw message
Signature · 1 line
1.6.0.2
drivers/scsi/qla4xxx/ql4_os.c static int qla4xxx_eh_wait_on_command(struct scsi_qla_host *ha, · line 1530
Quoted 9 lines · Ravi Anand
drivers/scsi/qla4xxx/ql4_os.c +6
-2-msleep (2000 ); - }while (max_wait_time--); +if (got_ref&& (atomic_read (&rp->ref_count) ==1 )) { + done++; +break ; + } + +msleep (ABORT_POLLING_PERIOD );
Did you want to use krefs for the refcounting?
And why is this so funky (got_ref arg and refcount peak) compared to the qla2xxx one?
Quoted 14 lines · Mike Christie
drivers/scsi/qla4xxx/ql4_os.c static int qla4xxx_eh_wait_on_command(struct scsi_qla_host *ha, · line 1530
Quoted 9 lines · Ravi Anand
drivers/scsi/qla4xxx/ql4_os.c +6
-2-msleep (2000 ); - }while (max_wait_time--); +if (got_ref&& (atomic_read (&rp->ref_count) ==1 )) { + done++; +break ; + } + +msleep (ABORT_POLLING_PERIOD );Did you want to use krefs for the refcounting?
We will add it to our to do list and submit a patch later on. For right now we will like to stick to it as kref will require additional testing.
Quoted 2 lines
And why is this so funky (
got_refarg and refcount peak) compared to the qla2xxx one?
I don't think qla2xxx is doing any reference counting in eh_abort() path. Basically its trying to differentiate for case where it takes an additional reference when the cmd is with the F/W. In that case if its the last guy, then it can go ahead and complete the command.
Hope this helps.
THanks
Ravi
Quoted 20 lines · Ravi Anand
Quoted 14 lines · Mike Christie
drivers/scsi/qla4xxx/ql4_os.c static int qla4xxx_eh_wait_on_command(struct scsi_qla_host *ha, · line 1530
Quoted 9 lines · Ravi Anand
drivers/scsi/qla4xxx/ql4_os.c +6
-2-msleep (2000 ); - }while (max_wait_time--); +if (got_ref&& (atomic_read (&rp->ref_count) ==1 )) { + done++; +break ; + } + +msleep (ABORT_POLLING_PERIOD );Did you want to use krefs for the refcounting?
We will add it to our to do list and submit a patch later on. For right now we will like to stick to it as kref will require additional testing.
Quoted 8 lines
Quoted 2 lines
And why is this so funky (
got_refarg and refcount peak) compared to the qla2xxx one?I don't think qla2xxx is doing any reference counting in
eh_abort()path. Basically its trying to differentiate for case where it takes an additional reference when the cmd is with the F/W. In that case if its the last guy, then it can go ahead and complete the command.
got_ref is always 0 isn't it (at least in the patch it is)? It seems like you can just get rid of all that and just copy qla2xxx's code.
Quoted 31 lines · Mike Christie
Quoted 20 lines · Ravi Anand
Quoted 13 lines · Mike Christie
Quoted 9 lines · Ravi Anand
- msleep(2000);
- } while (max_wait_time--);
+ if (got_ref&& (atomic_read(&rp->ref_count)== 1)) {
+ done++;
+ break;
+ }
+
+msleep(ABORT_POLLING_PERIOD);Quoted 1 line
Did you want to use krefs for the refcounting?
We will add it to our to do list and submit a patch later on. For right now we will like to stick to it as kref will require additional testing.
Quoted 9 lines
Quoted 2 lines
And why is this so funky (
got_refarg and refcount peak) compared to the qla2xxx one?I don't think qla2xxx is doing any reference counting in
eh_abort()path. Basically its trying to differentiate for case where it takes an additional
reference when the cmd is with the F/W. In that case if its the last guy, then it can go ahead and complete the command.
Quoted 2 lines
got_refis always 0 isn't it (at least in the patch it is)? It seems like you can just get rid of all that and just copy qla2xxx's code.
Hey, since it is always 0, then
if (got_ref && (atomic_read(&rp->ref_count) == 1)) {
will never be true, right? I think you can just drop that chunk or let me know if it is used in another patch I missed or something.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 38 lines · ravi.anand
Quoted 35 lines · Mike Christie
Quoted 34 lines · Mike Christie
Quoted 32 lines · Ravi Anand
Quoted 13 lines · Mike Christie
Quoted 9 lines · Ravi Anand
- msleep(2000);
- } while (max_wait_time--);
+ if (got_ref&& (atomic_read(&rp->ref_count)== 1)) {
+ done++;
+ break;
+ }
+
+msleep(ABORT_POLLING_PERIOD);Quoted 1 line
Did you want to use krefs for the refcounting?
We will add it to our to do list and submit a patch later on. For right now we will like to stick to it as kref will require additional testing.
Quoted 3 lines
And why is this so funky (
got_refarg and refcount peak) compared to the
qla2xxx one?I don't think qla2xxx is doing any reference counting in
eh_abort()path.
Basically its trying to differentiate for case where it takes an additional
reference when the cmd is with the F/W. In that case if its the last guy,
then it can go ahead and complete the command.
Quoted 5 lines
Quoted 3 lines
Quoted 2 lines
got_refis always 0 isn't it (at least in the patch it is)? It seems like you can just get rid of all that and just copy qla2xxx's code.In the abort path (PATCH 10/11) it gets incremented. So its not
Ah I see.
Quoted 5 lines
always zero. And the reason we need a reference is because it can get completed behind our back leading to a dangling srb. Since now two guys are working on it :
eh_abort()path as well as the normal completion path.
Except for the code you added above, I am not seeing how qla4xxx_eh_wait_on_command interacts with the srb or why it needs a refcount on it. I do see why qla4xxx_abort_task needs it. If qla4xxx_eh_wait_on_command does need the ref then it seems like the device reset path is going to have a issue.
Can't you do sp_put() after qla4xxx_abort_task(). It seems like after the qla4xxx_abort_task() call, then no one needs to touch the srb. Without your patch, qla4xxx_eh_wait_on_command does not access the srb. It only checks if the cmd has been completed by checking if SCp.ptr has been cleared.
And why does qla4xxx_eh_abort() do this loop
+ /* Check active list for command */
+ spin_lock_irqsave(&ha->hardware_lock, flags);
+ for (i = 1; i < MAX_SRBS; i++) {+ srb_cmd = scsi_host_find_tag(ha->host, i);
+ if (srb_cmd == NULL)
+ continue;
It seems like you would want to do
qla4xxx_wait_for_hba_online()spin_lock_irqsave(&ha->hardware_lock, flags);
srb = (struct srb *) cmd->SCp.ptr;
if (!srb)
/* raced while waiting for hba online */ return SUCCESS;
/* Get a reference to the sp and drop the lock.*/ sp_get(srb); spin_unlock_irqrestore(&ha->hardware_lock, flags); qla4xxx_abort_task()sp_put(ha, srb) spin_lock_irqsave(&ha->hardware_lock, flags);
qla4xxx_eh_wait_on_command(ha, cmd);
Quoted 2 lines · Mike Christie
spin_lock_irqsave(&ha->hardware_lock, flags);
The lock should not be held while we sleep of course :)
Quoted 5 lines
qla4xxx_eh_wait_on_command(ha, cmd);
--
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
Quoted 2 lines · Mike Christie
It seems like you would want to do
Maybe one more fix.
Quoted 8 lines
qla4xxx_wait_for_hba_online()spin_lock_irqsave(&ha->hardware_lock, flags);srb = (struct srb *)
cmd->SCp.ptr;
if (!srb)
/* raced while waiting for hba online */
return SUCCESS;
Quoted 4 lines
/* Get a reference to the sp and drop the lock.*/
sp_get(srb);spin_unlock_irqrestore(&ha->hardware_lock, flags);qla4xxx_abort_task()
From what I can tell, we used to call qla4xxx_srb_compl (and now do the final sp_put in the normal completion path) while holding the hardware_lock. If this is not the case then the loop in the eh abort patch is racey and my pseudo code above is too).
If I am right, then I think here we want to do
spin_lock_irqsave(&ha->hardware_lock, flags);sp_put(ha, srb);
So it is consistent with the other code, and then I think we want to add a comment in sp_put() that we should be holing the hardware_lock when calling it.
Quoted 8 lines
sp_put(ha, srb)spin_lock_irqsave(&ha->hardware_lock, flags);
qla4xxx_eh_wait_on_command(ha, cmd);
--
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
Quoted 16 lines · Ravi Anand
Quoted 14 lines · Mike Christie
drivers/scsi/qla4xxx/ql4_os.c static int qla4xxx_eh_wait_on_command(struct scsi_qla_host *ha, · line 1530
Quoted 9 lines · Ravi Anand
drivers/scsi/qla4xxx/ql4_os.c +6
-2-msleep (2000 ); - }while (max_wait_time--); +if (got_ref&& (atomic_read (&rp->ref_count) ==1 )) { + done++; +break ; + } + +msleep (ABORT_POLLING_PERIOD );Did you want to use krefs for the refcounting?
Quoted 4 lines
We will add it to our to do list and submit a patch later on. For right now we will like to stick to it as kref will require additional testing.
Please use krefs from the start. Other drivers like beiscsi have had to switch from its own refcount code to krefs, and have had to retest their patchsets. You have to change your code in some other patches and maybe this one too, so you have to retest everything either way.
Quoted 22 lines · Mike Christie
Quoted 16 lines · Ravi Anand
Quoted 14 lines · Mike Christie
drivers/scsi/qla4xxx/ql4_os.c static int qla4xxx_eh_wait_on_command(struct scsi_qla_host *ha, · line 1530
Quoted 9 lines · Ravi Anand
drivers/scsi/qla4xxx/ql4_os.c +6
-2-msleep (2000 ); - }while (max_wait_time--); +if (got_ref&& (atomic_read (&rp->ref_count) ==1 )) { + done++; +break ; + } + +msleep (ABORT_POLLING_PERIOD );Did you want to use krefs for the refcounting?
Quoted 4 lines
We will add it to our to do list and submit a patch later on. For right now we will like to stick to it as kref will require additional testing.
Quoted 4 lines
Please use krefs from the start. Other drivers like beiscsi have had to switch from its own refcount code to krefs, and have had to retest their patchsets. You have to change your code in some other patches and maybe this one too, so you have to retest everything either way.
That goes for me too, please ... it's a standard kernel paradigm and all the queries about what the refs were doing earlier would have been a lot simpler since the embedded krefs would largely have answered the questions.
James