RE: [PATCH 08/29] scsi: aacraid: Move code to wait for IO completion to shutdown func

[Raghava Aditya Renukunta]

> -Original Message-
> From: Bart Van Assche [mailto:bart.vanass...@wdc.com]
> Sent: Friday, December 22, 2017 8:27 AM
> To: j...@linux.vnet.ibm.com; Raghava Aditya Renukunta
> ; linux-scsi@vger.kernel.org;
> martin.peter...@oracle.com
> Cc: dl-esc-Aacraid Linux Driver ;
> gpicc...@linux.vnet.ibm.com; Tom White ;
> Scott Benesh 
> Subject: Re: [PATCH 08/29] scsi: aacraid: Move code to wait for IO completion
> to shutdown func
> 
> EXTERNAL EMAIL
> 
> 
> On Thu, 2017-12-21 at 17:59 +, Bart Van Assche wrote:
> > On Thu, 2017-12-21 at 09:33 -0800, Raghava Aditya Renukunta wrote:
> > > +static void aac_wait_for_io_completion(struct aac_dev *aac)
> > > +{
> > > +   unsigned long flagv = 0;
> > > +   int i = 0;
> > > +
> > > +   for (i = 60; i; --i) {
> > > +   struct scsi_device *dev;
> > > +   struct scsi_cmnd *command;
> > > +   int active = 0;
> > > +
> > > +   __shost_for_each_device(dev, aac->scsi_host_ptr) {
> > > +   spin_lock_irqsave(>list_lock, flagv);
> > > +   list_for_each_entry(command, >cmd_list, list) {
> > > +   if (command->SCp.phase == AAC_OWNER_FIRMWARE) 
> > > {
> > > +   active++;
> > > +   break;
> > > +   }
> > > +   }
> > > +   spin_unlock_irqrestore(>list_lock, flagv);
> > > +   if (active)
> > > +   break;
> > > +
> > > +   }
> > > +   /*
> > > +* We can exit If all the commands are complete
> > > +*/
> > > +   if (active == 0)
> > > +   break;
> > > +   ssleep(1);
> > > +   }
> > > +}
> >
> > Have you considered to call scsi_target_block() and scsi_target_unblock()
> instead
> > of implementing functionality like the above in a SCSI LLD?
> 
> (replying to my own e-mail)
> 
> It seems like I misread your code - calling scsi_target_block() and
> scsi_target_unblock() would not be sufficient. But calling
> blk_mq_freeze_queue()
> and blk_mq_unfreeze_queue() should be sufficient. The following commit
> made these
> functions work not only for scsi-mq but also for legacy scsi queues: commit
> 055f6e18e08f ("block: Make q_usage_counter also track legacy requests").

Hi Bart,
That piece of code is very much legacy  that I removed and placed in a function 
that would benefit all code paths that shutdown the controller. The 2 functions 
you mentioned are a god send but I think I will hold off on this patchset and 
do a full refactor with them in the next patchset. ( I suspect that I will have 
to touch a bunch of different code paths and perform extensive testing to be 
fully confident)

Regards,
Raghava Aditya

> Bart.

Re: [PATCH 08/29] scsi: aacraid: Move code to wait for IO completion to shutdown func

[Bart Van Assche]

On Thu, 2017-12-21 at 17:59 +, Bart Van Assche wrote:
> On Thu, 2017-12-21 at 09:33 -0800, Raghava Aditya Renukunta wrote:
> > +static void aac_wait_for_io_completion(struct aac_dev *aac)
> > +{
> > +   unsigned long flagv = 0;
> > +   int i = 0;
> > +
> > +   for (i = 60; i; --i) {
> > +   struct scsi_device *dev;
> > +   struct scsi_cmnd *command;
> > +   int active = 0;
> > +
> > +   __shost_for_each_device(dev, aac->scsi_host_ptr) {
> > +   spin_lock_irqsave(>list_lock, flagv);
> > +   list_for_each_entry(command, >cmd_list, list) {
> > +   if (command->SCp.phase == AAC_OWNER_FIRMWARE) {
> > +   active++;
> > +   break;
> > +   }
> > +   }
> > +   spin_unlock_irqrestore(>list_lock, flagv);
> > +   if (active)
> > +   break;
> > +
> > +   }
> > +   /*
> > +* We can exit If all the commands are complete
> > +*/
> > +   if (active == 0)
> > +   break;
> > +   ssleep(1);
> > +   }
> > +}
> 
> Have you considered to call scsi_target_block() and scsi_target_unblock() 
> instead
> of implementing functionality like the above in a SCSI LLD?

(replying to my own e-mail)

It seems like I misread your code - calling scsi_target_block() and
scsi_target_unblock() would not be sufficient. But calling blk_mq_freeze_queue()
and blk_mq_unfreeze_queue() should be sufficient. The following commit made 
these
functions work not only for scsi-mq but also for legacy scsi queues: commit
055f6e18e08f ("block: Make q_usage_counter also track legacy requests").

Bart.

Re: [PATCH 08/29] scsi: aacraid: Move code to wait for IO completion to shutdown func

[Bart Van Assche]

On Thu, 2017-12-21 at 09:33 -0800, Raghava Aditya Renukunta wrote:
> +static void aac_wait_for_io_completion(struct aac_dev *aac)
> +{
> + unsigned long flagv = 0;
> + int i = 0;
> +
> + for (i = 60; i; --i) {
> + struct scsi_device *dev;
> + struct scsi_cmnd *command;
> + int active = 0;
> +
> + __shost_for_each_device(dev, aac->scsi_host_ptr) {
> + spin_lock_irqsave(>list_lock, flagv);
> + list_for_each_entry(command, >cmd_list, list) {
> + if (command->SCp.phase == AAC_OWNER_FIRMWARE) {
> + active++;
> + break;
> + }
> + }
> + spin_unlock_irqrestore(>list_lock, flagv);
> + if (active)
> + break;
> +
> + }
> + /*
> +  * We can exit If all the commands are complete
> +  */
> + if (active == 0)
> + break;
> + ssleep(1);
> + }
> +}

Have you considered to call scsi_target_block() and scsi_target_unblock() 
instead
of implementing functionality like the above in a SCSI LLD?

Thanks,

Bart.

[PATCH 08/29] scsi: aacraid: Move code to wait for IO completion to shutdown func

[Raghava Aditya Renukunta]

Ideally driver needs to wait for IO to be submitted or responded to before
shutdown.

Move code to wait for IO completion into shutdown path

Signed-off-by: Raghava Aditya Renukunta 
---
 drivers/scsi/aacraid/comminit.c | 36 
 drivers/scsi/aacraid/commsup.c  | 25 -
 2 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c
index 9eff246..0dc7b5a 100644
--- a/drivers/scsi/aacraid/comminit.c
+++ b/drivers/scsi/aacraid/comminit.c
@@ -42,6 +42,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 #include "aacraid.h"
 
@@ -284,6 +286,38 @@ static void aac_queue_init(struct aac_dev * dev, struct 
aac_queue * q, u32 *mem,
q->entries = qsize;
 }
 
+static void aac_wait_for_io_completion(struct aac_dev *aac)
+{
+   unsigned long flagv = 0;
+   int i = 0;
+
+   for (i = 60; i; --i) {
+   struct scsi_device *dev;
+   struct scsi_cmnd *command;
+   int active = 0;
+
+   __shost_for_each_device(dev, aac->scsi_host_ptr) {
+   spin_lock_irqsave(>list_lock, flagv);
+   list_for_each_entry(command, >cmd_list, list) {
+   if (command->SCp.phase == AAC_OWNER_FIRMWARE) {
+   active++;
+   break;
+   }
+   }
+   spin_unlock_irqrestore(>list_lock, flagv);
+   if (active)
+   break;
+
+   }
+   /*
+* We can exit If all the commands are complete
+*/
+   if (active == 0)
+   break;
+   ssleep(1);
+   }
+}
+
 /**
  * aac_send_shutdown   -   shutdown an adapter
  * @dev: Adapter to shutdown
@@ -306,6 +340,8 @@ int aac_send_shutdown(struct aac_dev * dev)
mutex_unlock(>ioctl_mutex);
}
 
+   aac_wait_for_io_completion(dev);
+
fibctx = aac_fib_alloc(dev);
if (!fibctx)
return -ENOMEM;
diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index 32b8bdb..9840bd3 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -1701,31 +1701,6 @@ int aac_reset_adapter(struct aac_dev *aac, int forced, 
u8 reset_type)
 */
host = aac->scsi_host_ptr;
scsi_block_requests(host);
-   if (forced < 2) for (retval = 60; retval; --retval) {
-   struct scsi_device * dev;
-   struct scsi_cmnd * command;
-   int active = 0;
-
-   __shost_for_each_device(dev, host) {
-   spin_lock_irqsave(>list_lock, flagv);
-   list_for_each_entry(command, >cmd_list, list) {
-   if (command->SCp.phase == AAC_OWNER_FIRMWARE) {
-   active++;
-   break;
-   }
-   }
-   spin_unlock_irqrestore(>list_lock, flagv);
-   if (active)
-   break;
-
-   }
-   /*
-* We can exit If all the commands are complete
-*/
-   if (active == 0)
-   break;
-   ssleep(1);
-   }
 
/* Quiesce build, flush cache, write through mode */
if (forced < 2)
-- 
2.9.4