[PATCH 2/6] scsi_dh : increment the refcounts while calling activate

[PATCH 2/6] scsi_dh : increment the refcounts while calling activate

[Moger, Babu]

Hold the refcounts for device and scsi_dh_data while calling handler's activate. This will make sure that devices and scsi_dh_data are not removed while activate is still in progress. Make sure to call put_device and kref_put in the handler after activate is complete.

Signed-off-by: Babu Moger <babu.moger@lsi.com>
---
--- linux-2.6.35-rc5/drivers/scsi/device_handler/scsi_dh.c.orig	2010-07-23 05:40:11.000000000 -0500
+++ linux-2.6.35-rc5/drivers/scsi/device_handler/scsi_dh.c	2010-07-23 05:48:53.000000000 -0500
@@ -228,7 +228,8 @@ store_dh_state(struct device *dev, struc
 			 * Activate a device handler
 			 */
 			if (scsi_dh->activate)
-				err = scsi_dh->activate(sdev, NULL, NULL);
+				err = scsi_dh_activate(sdev->request_queue,
+							NULL, NULL);
 			else
 				err = 0;
 		}
@@ -431,6 +432,8 @@ EXPORT_SYMBOL_GPL(scsi_unregister_device
  *         do not hold the lock in the caller which may be needed in fn.
  * @data - data passed to the function fn upon completion.
  *
+ * NOTE  - Remember to call put_device and kref_put in the handler after
+ *         calling the callback function. Otherwise things could become messy.
  */
 int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
 {
@@ -450,9 +453,12 @@ int scsi_dh_activate(struct request_queu
 	if (err)
 		return err;
 
-	if (scsi_dh->activate)
+	if (scsi_dh->activate) {
+		kref_get(&sdev->scsi_dh_data->kref);
 		err = scsi_dh->activate(sdev, fn, data);
-	put_device(&sdev->sdev_gendev);
+	} else
+		put_device(&sdev->sdev_gendev);
+
 	return err;
 }
 EXPORT_SYMBOL_GPL(scsi_dh_activate);

Re: [dm-devel] [PATCH 2/6] scsi_dh : increment the refcounts while calling activate

[Shyam Iyer]

On 07/28/2010 06:59 PM, Moger, Babu wrote:
> Hold the refcounts for device and scsi_dh_data while calling handler's activate. This will make sure that devices and scsi_dh_data are not removed while activate is still in progress. Make sure to call put_device and kref_put in the handler after activate is complete.
>
> Signed-off-by: Babu Moger<babu.moger@lsi.com>
> ---
> --- linux-2.6.35-rc5/drivers/scsi/device_handler/scsi_dh.c.orig	2010-07-23 05:40:11.000000000 -0500
> +++ linux-2.6.35-rc5/drivers/scsi/device_handler/scsi_dh.c	2010-07-23 05:48:53.000000000 -0500
> @@ -228,7 +228,8 @@ store_dh_state(struct device *dev, struc
>   			 * Activate a device handler
>   			 */
>   			if (scsi_dh->activate)
> -				err = scsi_dh->activate(sdev, NULL, NULL);
> +				err = scsi_dh_activate(sdev->request_queue,
> +							NULL, NULL);
>   			else
>   				err = 0;
>   		}
> @@ -431,6 +432,8 @@ EXPORT_SYMBOL_GPL(scsi_unregister_device
>    *         do not hold the lock in the caller which may be needed in fn.
>    * @data - data passed to the function fn upon completion.
>    *
> + * NOTE  - Remember to call put_device and kref_put in the handler after
> + *         calling the callback function. Otherwise things could become messy.
>    */
>   int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
>   {
> @@ -450,9 +453,12 @@ int scsi_dh_activate(struct request_queu
>   	if (err)
>   		return err;
>
> -	if (scsi_dh->activate)
> +	if (scsi_dh->activate) {
> +		kref_get(&sdev->scsi_dh_data->kref);
>   		err = scsi_dh->activate(sdev, fn, data);
>    
Why not kref_put here instead of in the device handler.. It is easier to 
associate the ref counts..

Also, you are removing the put_device here and adding them to the device 
handler which can be avoided ..
> -	put_device(&sdev->sdev_gendev);
> +	} else
> +		put_device(&sdev->sdev_gendev);
> +
>   	return err;
>   }
>   EXPORT_SYMBOL_GPL(scsi_dh_activate);
>
>
>
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>    

-Shyam Iyer

Re: [PATCH 2/6] scsi_dh : increment the refcounts while calling activate

[James Bottomley]

On Wed, 2010-07-28 at 16:59 -0600, Moger, Babu wrote:
> Hold the refcounts for device and scsi_dh_data while calling handler's
> activate. This will make sure that devices and scsi_dh_data are not
> removed while activate is still in progress. Make sure to call
> put_device and kref_put in the handler after activate is complete.

This is a complete no-no.  You can't take unreleased references in a
single patch.  I know the releases come in subsequent patches, but
that's not the way to do it.  You have to have an atomic change (as in
all refcounts must balance on either side of the patch).  If someone
bisected into this, it would likely never release stuff ... which could
lead to some unnecessary debugging.

James

RE: [PATCH 2/6] scsi_dh : increment the refcounts while calling activate

[Moger, Babu]

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@suse.de]
> Sent: Thursday, July 29, 2010 7:09 PM
> To: Moger, Babu
> Cc: device-mapper development; linux-scsi@vger.kernel.org; Qi, Yanling;
> Chauhan, Vijay; Stankey, Robert; Dachepalli, Sudhir
> Subject: Re: [PATCH 2/6] scsi_dh : increment the refcounts while
> calling activate
> 
> On Wed, 2010-07-28 at 16:59 -0600, Moger, Babu wrote:
> > Hold the refcounts for device and scsi_dh_data while calling
> handler's
> > activate. This will make sure that devices and scsi_dh_data are not
> > removed while activate is still in progress. Make sure to call
> > put_device and kref_put in the handler after activate is complete.
> 
> This is a complete no-no.  You can't take unreleased references in a
> single patch.  I know the releases come in subsequent patches, but
> that's not the way to do it.  You have to have an atomic change (as in
> all refcounts must balance on either side of the patch).  If someone
> bisected into this, it would likely never release stuff ... which could
> lead to some unnecessary debugging.

  Yes.. Understood. Still investigating to see if I could avoid this..
 
> James
>