FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Device-mapper Development

 
 
LinkBack Thread Tools
 
Old 07-29-2010, 04:46 AM
Shyam Iyer
 
Default scsi_dh : increment the refcounts while calling activate

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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 07-30-2010, 12:08 AM
James Bottomley
 
Default 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.

James


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 07-30-2010, 06:15 PM
"Moger, Babu"
 
Default scsi_dh : increment the refcounts while calling activate

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


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 

Thread Tools




All times are GMT. The time now is 01:36 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org