Re: [PATCH 01/22] libfc: Revisit kref handling

2016-08-18 Thread Chad Dupuis


On Wed, 3 Aug 2016, 1:13pm -, Hannes Reinecke wrote:

> The kref handling in fc_rport is a mess. This patch updates
> the kref handling according to the following rules:
> 
> - Take a reference whenever scheduling a workqueue
> - Take a reference whenever an ELS command is send
> - Drop the reference at the end of the workqueue function
> - Drop the reference at the end of handling ELS replies
> - Take a reference when allocating an rport
> - Drop the reference when removing an rport
> 
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/scsi/libfc/fc_rport.c | 134 
> --
>  1 file changed, 103 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
> index 93f5961..6a98bb8 100644
> --- a/drivers/scsi/libfc/fc_rport.c
> +++ b/drivers/scsi/libfc/fc_rport.c
> @@ -44,6 +44,17 @@
>   * path this potential over-use of the mutex is acceptable.
>   */
>  

<-- snip -->

Yes, this took quite a few iterations to get right but your rules make 
sense.  I've tested with this patch and it successfully passed.

Tested-by: Chad Dupuis 
--
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 01/22] libfc: Revisit kref handling

2016-08-04 Thread Johannes Thumshirn
On Wed, Aug 03, 2016 at 03:13:01PM +0200, Hannes Reinecke wrote:
> The kref handling in fc_rport is a mess. This patch updates
> the kref handling according to the following rules:
> 
> - Take a reference whenever scheduling a workqueue
> - Take a reference whenever an ELS command is send
> - Drop the reference at the end of the workqueue function
> - Drop the reference at the end of handling ELS replies
> - Take a reference when allocating an rport
> - Drop the reference when removing an rport
> 
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/scsi/libfc/fc_rport.c | 134 
> --
>  1 file changed, 103 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
> index 93f5961..6a98bb8 100644
> --- a/drivers/scsi/libfc/fc_rport.c
> +++ b/drivers/scsi/libfc/fc_rport.c
> @@ -44,6 +44,17 @@
>   * path this potential over-use of the mutex is acceptable.
>   */
>  
> +/*
> + * RPORT REFERENCE COUNTING
> + *
> + * A rport reference should be taken when:
> + * - a workqueue item is scheduled
> + * - an ELS request is send
> + * The reference should be dropped when:
> + * - the workqueue function has finished
> + * - the ELS response is handled
> + */

Please sync with the rules in the commit message.

> +
>  #include 
>  #include 
>  #include 
> @@ -242,6 +253,8 @@ static void fc_rport_state_enter(struct fc_rport_priv 
> *rdata,
>  /**
>   * fc_rport_work() - Handler for remote port events in the rport_event_queue
>   * @work: Handle to the remote port being dequeued
> + *
> + * Reference counting: drops kref on return
>   */
>  static void fc_rport_work(struct work_struct *work)
>  {
> @@ -272,8 +285,10 @@ static void fc_rport_work(struct work_struct *work)
>   kref_get(&rdata->kref);
>   mutex_unlock(&rdata->rp_mutex);
>  
> - if (!rport)
> + if (!rport) {
> + FC_RPORT_DBG(rdata, "No rport!\n");

If you're re-sending the series, this and the other added debug statements
might be better suited in '[PATCH 02/22] libfc: additional debugging messages'.

Otherwise

Acked-by: Johannes Thumshirn 

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
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 01/22] libfc: Revisit kref handling

2016-08-03 Thread Hannes Reinecke
The kref handling in fc_rport is a mess. This patch updates
the kref handling according to the following rules:

- Take a reference whenever scheduling a workqueue
- Take a reference whenever an ELS command is send
- Drop the reference at the end of the workqueue function
- Drop the reference at the end of handling ELS replies
- Take a reference when allocating an rport
- Drop the reference when removing an rport

Signed-off-by: Hannes Reinecke 
---
 drivers/scsi/libfc/fc_rport.c | 134 --
 1 file changed, 103 insertions(+), 31 deletions(-)

diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
index 93f5961..6a98bb8 100644
--- a/drivers/scsi/libfc/fc_rport.c
+++ b/drivers/scsi/libfc/fc_rport.c
@@ -44,6 +44,17 @@
  * path this potential over-use of the mutex is acceptable.
  */
 
+/*
+ * RPORT REFERENCE COUNTING
+ *
+ * A rport reference should be taken when:
+ * - a workqueue item is scheduled
+ * - an ELS request is send
+ * The reference should be dropped when:
+ * - the workqueue function has finished
+ * - the ELS response is handled
+ */
+
 #include 
 #include 
 #include 
@@ -242,6 +253,8 @@ static void fc_rport_state_enter(struct fc_rport_priv 
*rdata,
 /**
  * fc_rport_work() - Handler for remote port events in the rport_event_queue
  * @work: Handle to the remote port being dequeued
+ *
+ * Reference counting: drops kref on return
  */
 static void fc_rport_work(struct work_struct *work)
 {
@@ -272,8 +285,10 @@ static void fc_rport_work(struct work_struct *work)
kref_get(&rdata->kref);
mutex_unlock(&rdata->rp_mutex);
 
-   if (!rport)
+   if (!rport) {
+   FC_RPORT_DBG(rdata, "No rport!\n");
rport = fc_remote_port_add(lport->host, 0, &ids);
+   }
if (!rport) {
FC_RPORT_DBG(rdata, "Failed to add the rport\n");
lport->tt.rport_logoff(rdata);
@@ -329,7 +344,8 @@ static void fc_rport_work(struct work_struct *work)
FC_RPORT_DBG(rdata, "lld callback ev %d\n", event);
rdata->lld_event_callback(lport, rdata, event);
}
-   cancel_delayed_work_sync(&rdata->retry_work);
+   if (cancel_delayed_work_sync(&rdata->retry_work))
+   kref_put(&rdata->kref, lport->tt.rport_destroy);
 
/*
 * Reset any outstanding exchanges before freeing rport.
@@ -381,6 +397,7 @@ static void fc_rport_work(struct work_struct *work)
mutex_unlock(&rdata->rp_mutex);
break;
}
+   kref_put(&rdata->kref, lport->tt.rport_destroy);
 }
 
 /**
@@ -431,10 +448,14 @@ static int fc_rport_login(struct fc_rport_priv *rdata)
  * Set the new event so that the old pending event will not occur.
  * Since we have the mutex, even if fc_rport_work() is already started,
  * it'll see the new event.
+ *
+ * Reference counting: does not modify kref
  */
 static void fc_rport_enter_delete(struct fc_rport_priv *rdata,
  enum fc_rport_event event)
 {
+   struct fc_lport *lport = rdata->local_port;
+
if (rdata->rp_state == RPORT_ST_DELETE)
return;
 
@@ -442,8 +463,11 @@ static void fc_rport_enter_delete(struct fc_rport_priv 
*rdata,
 
fc_rport_state_enter(rdata, RPORT_ST_DELETE);
 
-   if (rdata->event == RPORT_EV_NONE)
-   queue_work(rport_event_queue, &rdata->event_work);
+   kref_get(&rdata->kref);
+   if (rdata->event == RPORT_EV_NONE &&
+   !queue_work(rport_event_queue, &rdata->event_work))
+   kref_put(&rdata->kref, lport->tt.rport_destroy);
+
rdata->event = event;
 }
 
@@ -484,15 +508,22 @@ out:
  *
  * Locking Note: The rport lock is expected to be held before calling
  * this routine.
+ *
+ * Reference counting: schedules workqueue, does not modify kref
  */
 static void fc_rport_enter_ready(struct fc_rport_priv *rdata)
 {
+   struct fc_lport *lport = rdata->local_port;
+
fc_rport_state_enter(rdata, RPORT_ST_READY);
 
FC_RPORT_DBG(rdata, "Port is Ready\n");
 
-   if (rdata->event == RPORT_EV_NONE)
-   queue_work(rport_event_queue, &rdata->event_work);
+   kref_get(&rdata->kref);
+   if (rdata->event == RPORT_EV_NONE &&
+   !queue_work(rport_event_queue, &rdata->event_work))
+   kref_put(&rdata->kref, lport->tt.rport_destroy);
+
rdata->event = RPORT_EV_READY;
 }
 
@@ -503,13 +534,17 @@ static void fc_rport_enter_ready(struct fc_rport_priv 
*rdata)
  * Locking Note: Called without the rport lock held. This
  * function will hold the rport lock, call an _enter_*
  * function and then unlock the rport.
+ *
+ * Reference counting: Drops kref on return.
  */
 static void fc_rport_timeout(struct work_struct *work)
 {
struct fc_rport_priv *rdata =