[PATCH] Remove devices from sysfs cache

[PATCH] Remove devices from sysfs cache

[Hannes Reinecke]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

Hi Christophe,

whenever a device is finally removed from a multipath map we should also remove it from the sysfs cache. Otherwise we'll hogging up memory with multipathd. And we might rely on wrong information as the device in the sysfs cache might in fact be a different device if the system decided to assign the same number to a different device or if some values have changed.

Please apply.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

[-- Attachment #2: multipath-tools-remove-sysfs-device-from-cache --]
[-- Type: text/plain, Size: 2142 bytes --]

multipathd: remove sysfs devices from cache

Whenever a device is really removed from any multipath map we should
also remove it from the cache. Otherwise we'll induce a memory leak.

Signed-off-by: Hannes Reinecke <hare@suse.de>

diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index d8c65b2..b9621ac 100644
--- a/libmultipath/sysfs.c
+++ b/libmultipath/sysfs.c
@@ -342,6 +342,25 @@ struct sysfs_device *sysfs_device_get_parent_with_subsystem(struct sysfs_device
 	return NULL;
 }
 
+void sysfs_device_put(struct sysfs_device *dev)
+{
+	struct sysfs_dev *sysdev_loop;
+
+	list_for_each_entry(sysdev_loop, &sysfs_dev_list, node) {
+		if (&sysdev_loop->dev == dev) {
+			dbg("removed dev '%s' from cache",
+			    sysdev_loop->dev.devpath);
+			list_del(&sysdev_loop->node);
+			free(sysdev_loop);
+			return;
+		}
+	}
+	dbg("dev '%s' not found in cache",
+	    sysdev_loop->dev.devpath);
+
+	return;
+}
+
 char *sysfs_attr_get_value(const char *devpath, const char *attr_name)
 {
 	char path_full[PATH_SIZE];
diff --git a/libmultipath/sysfs.h b/libmultipath/sysfs.h
index 6d83489..e7fa3e7 100644
--- a/libmultipath/sysfs.h
+++ b/libmultipath/sysfs.h
@@ -18,6 +18,7 @@ void sysfs_device_set_values(struct sysfs_device *dev, const char *devpath,
 struct sysfs_device *sysfs_device_get(const char *devpath);
 struct sysfs_device *sysfs_device_get_parent(struct sysfs_device *dev);
 struct sysfs_device *sysfs_device_get_parent_with_subsystem(struct sysfs_device *dev, const char *subsystem);
+void sysfs_device_put(struct sysfs_device *dev);
 char *sysfs_attr_get_value(const char *devpath, const char *attr_name);
 int sysfs_resolve_link(char *path, size_t size);
 
diff --git a/multipathd/main.c b/multipathd/main.c
index b1620b5..da5fd8f 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -442,8 +442,14 @@ out:
 static int
 uev_remove_path (struct sysfs_device * dev, struct vectors * vecs)
 {
+	int retval;
+
 	condlog(2, "%s: remove path (uevent)", dev->kernel);
-	return ev_remove_path(dev->kernel, vecs);
+	retval = ev_remove_path(dev->kernel, vecs);
+	if (!retval)
+		sysfs_device_put(dev);
+
+	return retval;
 }
 
 int

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

New multipath-tools release?

[Guido Guenther]

Hi Christophe,
On Wed, Jul 25, 2007 at 12:59:39PM +0200, Hannes Reinecke wrote:
> Hi Christophe,
> 
> whenever a device is finally removed from a multipath map we should also remove it from the sysfs cache. Otherwise we'll hogging up memory with multipathd. And we might rely on wrong information as the device in the sysfs cache might in fact be a different device if the system decided to assign the same number to a different device or if some values have changed.
Now with this fix in, would it make any sense to tag a 0.4.8-rc1? The
last multipath-tools release has been about a year ago and it would
really be nice to have a new release, so there's a new base to diff
against.
Cheers,
 -- Guido

Re: New multipath-tools release?

[Hannes Reinecke]

Guido Guenther wrote:
> Hi Christophe,
> On Wed, Jul 25, 2007 at 12:59:39PM +0200, Hannes Reinecke wrote:
>> Hi Christophe,
>>
>> whenever a device is finally removed from a multipath map we should
>> also remove it from the sysfs cache. Otherwise we'll hogging up memory
>> with multipathd. And we might rely on wrong information as the device
>> in the sysfs cache might in fact be a different device if the system
>> decided to assign the same number to a different device or if some
>> values have changed.
> Now with this fix in, would it make any sense to tag a 0.4.8-rc1? The
> last multipath-tools release has been about a year ago and it would
> really be nice to have a new release, so there's a new base to diff
> against.

In priciple a good idea. However, I do have some to-do items on my list
yet:

- netlink messages: The FC transport class has a netlink interface
  nowadays. It may be an idea to utilize this information in to update
  the path status.
- Enhance path checker. Currently the path checker is run at periodic
  intervals regardless of the path state. This leads to quite some
  load on systems with many paths, even when all paths are healthy.
  We should rather call the path checker only if a path has failed,
  as we're being notified by a failed path soon enough via I/O errors.
  And we might even be using the netlink events from FC here, too.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

Re: New multipath-tools release?

[Guido Guenther]

On Mon, Jul 30, 2007 at 04:49:07PM +0200, Hannes Reinecke wrote:
> In priciple a good idea. However, I do have some to-do items on my list
> yet:
> 
> - netlink messages: The FC transport class has a netlink interface
>   nowadays. It may be an idea to utilize this information in to update
>   the path status.
> - Enhance path checker. Currently the path checker is run at periodic
>   intervals regardless of the path state. This leads to quite some
>   load on systems with many paths, even when all paths are healthy.
>   We should rather call the path checker only if a path has failed,
>   as we're being notified by a failed path soon enough via I/O errors.
>   And we might even be using the netlink events from FC here, too.
> 
My point exactly: let's tag a new stable version and add these nice
things to the (then) next release. I think the stability of the master
branch improved quiet a bit during the last couple of commits, It'd be
nice to have this out of the door for distributions to build upon before
doing more intrusive changes again.
Cheers,
 -- Guido

Re: New multipath-tools release?

[Hannes Reinecke]

Guido Guenther wrote:
> On Mon, Jul 30, 2007 at 04:49:07PM +0200, Hannes Reinecke wrote:
>> In priciple a good idea. However, I do have some to-do items on my list
>> yet:
>>
>> - netlink messages: The FC transport class has a netlink interface
>>   nowadays. It may be an idea to utilize this information in to update
>>   the path status.
>> - Enhance path checker. Currently the path checker is run at periodic
>>   intervals regardless of the path state. This leads to quite some
>>   load on systems with many paths, even when all paths are healthy.
>>   We should rather call the path checker only if a path has failed,
>>   as we're being notified by a failed path soon enough via I/O errors.
>>   And we might even be using the netlink events from FC here, too.
>>
> My point exactly: let's tag a new stable version and add these nice
> things to the (then) next release. I think the stability of the master
> branch improved quiet a bit during the last couple of commits, It'd be
> nice to have this out of the door for distributions to build upon before
> doing more intrusive changes again.

(Ah, so there is an RHEL update coming :-)

But yes, you are right. Putting out a new version before adding those
is probably a good thing.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

Re: New multipath-tools release?

[Guido Guenther]

On Tue, Jul 31, 2007 at 05:16:19PM +0200, Hannes Reinecke wrote:
> (Ah, so there is an RHEL update coming :-)
Hmm..no, the kids called lenny and still has some months before it has
to grow up but given that 0.4.7 doesn't even work with current kernels,
a release would actually make some sense.
Cheers,
 -- Guido