mvsas: Deadlocks meet when TMF tasks issue

PATCH In review Ying Chu 4 participants 6 messages May 8, 2009 – May 9, 2009 SCSI

lore

Share & tools

lore

Ying Chu

author

2009-05-08 06:16 UTC #0 lore raw

PATCH 2/4 mvsas: Deadlocks meet when TMF tasks issue

Quoted 1 line

From 58f754f51fced4a39adcf708319e3de946a0d61a Mon Sep 17 00:00:00 2001

From: Andy <[email protected]>
Date: Mon, 4 May 2009 23:31:43 +0800
Subject: [PATCH 2/4] bug fix: deal lock

TMF task need be issued with Interrupt Disabled, or Deadlock may take place.

Signed-off-by Ying Chu <[email protected]>

Signed-off-by Andy Yan <[email protected]>

Signed-off-by Ke Wei <[email protected]>

---
 drivers/scsi/mvsas/mv_sas.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/scsi/mvsas/mv_sas.c +1 -1

index 318ec01..cb002ef 100644
@@ -1446,7 +1446,7 @@ static int mvs_exec_internal_tmf_task(struct domain_device *dev,
 		task->timer.expires = jiffies + MVS_TASK_TIMEOUT*HZ;
 		add_timer(&task->timer);
 
-		res = mvs_task_exec(task, 1, GFP_KERNEL, NULL, 0, 1, tmf);
+		res = mvs_task_exec(task, 1, GFP_KERNEL, NULL, 1, 1, tmf);
 
 		if (res) {
 			del_timer(&task->timer);

Signature · 5 lines

1.6.0.3

-- 
Regards,
Ying Chu

James Bottomley

maintainer

2009-05-08 15:49 UTC #1 lore raw

Quoted 19 lines · Ying Chu

Quoted 1 line
From 58f754f51fced4a39adcf708319e3de946a0d61a Mon Sep 17 00:00:00 2001
From: Andy <[email protected]>
Date: Mon, 4 May 2009 23:31:43 +0800
Subject: [PATCH 2/4] bug fix: deal lock
TMF task need be issued with Interrupt Disabled, or Deadlock may take place.
Signed-off-by Ying Chu <[email protected]>
Signed-off-by Andy Yan <[email protected]>
Signed-off-by Ke Wei <[email protected]>
---
 drivers/scsi/mvsas/mv_sas.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/scsi/mvsas/mv_sas.c +0 -0
index 318ec01..cb002ef 100644

drivers/scsi/mvsas/mv_sas.c static int mvs_exec_internal_tmf_task(struct domain_device *dev, · line 1449

Quoted 5 lines

drivers/scsi/mvsas/mv_sas.c +1 -1

@@ -1446,7 +1446,7 @@ static int mvs_exec_internal_tmf_task(struct domain_device *dev,
 		task->timer.expires = jiffies + MVS_TASK_TIMEOUT*HZ;
 		add_timer(&task->timer);
 
-		res = mvs_task_exec(task, 1, GFP_KERNEL, NULL, 0, 1, tmf);
+		res = mvs_task_exec(task, 1, GFP_KERNEL, NULL, 1, 1, tmf);

That eliminates the last user of the unlocked case.

Can't we just drop the conditional lock from mvs_task_exec() now instead? It will make the code much cleaner.

James

Jeff Garzik

2009-05-08 22:05 UTC #2 lore raw

Quoted 23 lines · James Bottomley

Quoted 22 lines · Ying Chu
Quoted 1 line
From 58f754f51fced4a39adcf708319e3de946a0d61a Mon Sep 17 00:00:00 2001
From: Andy <[email protected]>
Date: Mon, 4 May 2009 23:31:43 +0800
Subject: [PATCH 2/4] bug fix: deal lock
TMF task need be issued with Interrupt Disabled, or Deadlock may take place.
Signed-off-by Ying Chu <[email protected]>
Signed-off-by Andy Yan <[email protected]>
Signed-off-by Ke Wei <[email protected]>
---
 drivers/scsi/mvsas/mv_sas.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/scsi/mvsas/mv_sas.c +0 -0
index 318ec01..cb002ef 100644
@@ -1446,7 +1446,7 @@ static int mvs_exec_internal_tmf_task(struct domain_device *dev,
 		task->timer.expires = jiffies + MVS_TASK_TIMEOUT*HZ;
 		add_timer(&task->timer);
 

Quoted 7 lines

drivers/scsi/mvsas/mv_sas.c static int mvs_exec_internal_tmf_task(struct domain_device *dev, · line 1449
Quoted 2 lines
drivers/scsi/mvsas/mv_sas.c +1 -1
-		res = mvs_task_exec(task, 1, GFP_KERNEL, NULL, 0, 1, tmf);
+		res = mvs_task_exec(task, 1, GFP_KERNEL, NULL, 1, 1, tmf);
That eliminates the last user of the unlocked case.
Can't we just drop the conditional lock from mvs_task_exec() now instead? It will make the code much cleaner.

Agreed...

Andy Yan

2009-05-09 07:25 UTC #3 lore raw

James,
For considering the possibility of future use, I did not remove the parameter; I will check this and remove it with next patch if it is OK.

Regards
Andy Yan

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Jeff Garzik
Sent: 2009年5月9日 6:06
To: James Bottomley
Cc: Ying Chu; [email protected]
Subject: Re: [PATCH 2/4] mvsas: Deadlocks meet when TMF tasks issue

Quoted 23 lines · James Bottomley

Quoted 22 lines · Ying Chu
Quoted 1 line
From 58f754f51fced4a39adcf708319e3de946a0d61a Mon Sep 17 00:00:00 2001
From: Andy <[email protected]>
Date: Mon, 4 May 2009 23:31:43 +0800
Subject: [PATCH 2/4] bug fix: deal lock
TMF task need be issued with Interrupt Disabled, or Deadlock may take place.
Signed-off-by Ying Chu <[email protected]>
Signed-off-by Andy Yan <[email protected]>
Signed-off-by Ke Wei <[email protected]>
---
 drivers/scsi/mvsas/mv_sas.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/scsi/mvsas/mv_sas.c +0 -0
index 318ec01..cb002ef 100644
@@ -1446,7 +1446,7 @@ static int mvs_exec_internal_tmf_task(struct domain_device *dev,
 		task->timer.expires = jiffies + MVS_TASK_TIMEOUT*HZ;
 		add_timer(&task->timer);
 

Quoted 7 lines

drivers/scsi/mvsas/mv_sas.c static int mvs_exec_internal_tmf_task(struct domain_device *dev, · line 1449
Quoted 2 lines
drivers/scsi/mvsas/mv_sas.c +1 -1
-		res = mvs_task_exec(task, 1, GFP_KERNEL, NULL, 0, 1, tmf);
+		res = mvs_task_exec(task, 1, GFP_KERNEL, NULL, 1, 1, tmf);
That eliminates the last user of the unlocked case.
Can't we just drop the conditional lock from mvs_task_exec() now instead? It will make the code much cleaner.

Agreed...

--
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

Jeff Garzik

2009-05-09 07:31 UTC #4 lore raw

Quoted 2 lines · Andy Yan

James,
For considering the possibility of future use, I did not remove the parameter; I will check this and remove it with next patch if it is OK.

Generally, we encourage immediate elimination of all code not actively used.

The only major exception to this rule is structures or ABI that are visible to userland, such as ioctls, where we must maintain binary compatibility.

Jeff

James Bottomley

maintainer

2009-05-09 15:05 UTC #5 lore raw

In reply to Andy Yan

Quoted 1 line · Andy Yan

For considering the possibility of future use, I did not remove the parameter; I will check this and remove it with next patch if it is OK.

Well, there are two reasons for removing it. The first is that it's unused, but the second is that conditional locking isn't very nice programming style, since it makes control flow far harder to follow rationally when just reading the code. The preferred style is to define a function that does the unlocked operation, usually with double underscores and then have a locked version that takes the lock, calls the double underscore unlocked version and releases the lock again. In your case, if you find a use for the unlocked version, you can split it in the patch that's using it.

Looking at the routine, the only thing you call outside the lock is mvs_find_dev_mvi(). The thing that strikes me here is that this is a bit inefficient: why not store mvi in struct mvs_device? Then you can get it as a simple pointer operation instead of having to search.

James