Remove sysfs_attr cache
authorHannes Reinecke <hare@suse.de>
Mon, 4 May 2009 12:11:18 +0000 (14:11 +0200)
committerHannes Reinecke <hare@suse.de>
Mon, 16 May 2011 10:01:31 +0000 (12:01 +0200)
The sysfs attribute cache was meant to speed up sysfs accesses.
However, we need to update the cache values on each access, so
there is no speedup to be had.
So remove it and save memory.

Signed-off-by: Hannes Reinecke <hare@suse.de>
libmultipath/discovery.c
libmultipath/sysfs.c
libmultipath/sysfs.h
multipathd/main.c

index dde876f..73f84a7 100644 (file)
@@ -128,16 +128,22 @@ path_discovery (vector pathvec, struct config * conf, int flag)
 #define declare_sysfs_get_str(fname) \
 extern int \
 sysfs_get_##fname (struct sysfs_device * dev, char * buff, size_t len) \
-{ \
-       char *attr; \
-\
-       attr = sysfs_attr_get_value(dev->devpath, #fname); \
-       if (!attr) \
-               return 1; \
-       if (strlcpy(buff, attr, len) != strlen(attr)) \
-               return 2; \
-       strchop(buff); \
-       return 0; \
+{                                                                     \
+       int size;                                                      \
+                                                                      \
+       size = sysfs_attr_get_value(dev->devpath, #fname, buff, len);   \
+       if (!size) {                                                    \
+               condlog(3, "%s: attribute %s not found in sysfs",       \
+                       dev->kernel, #fname);                           \
+               return 1;                                               \
+       }                                                               \
+       if (size == len) {                                              \
+               condlog(3, "%s: overflow in attribute %s",              \
+                       dev->kernel, #fname);                           \
+               return 2;                                               \
+       }                                                               \
+       strchop(buff);                                                  \
+       return 0;                                                       \
 }
 
 declare_sysfs_get_str(devtype);
@@ -146,43 +152,32 @@ declare_sysfs_get_str(vendor);
 declare_sysfs_get_str(model);
 declare_sysfs_get_str(rev);
 declare_sysfs_get_str(state);
-
-int
-sysfs_get_dev (struct sysfs_device * dev, char * buff, size_t len)
-{
-       char *attr;
-
-       attr = sysfs_attr_get_value(dev->devpath, "dev");
-       if (!attr) {
-               condlog(3, "%s: no 'dev' attribute in sysfs", dev->kernel);
-               return 1;
-       }
-       if (strlcpy(buff, attr, len) != strlen(attr)) {
-               condlog(3, "%s: overflow in 'dev' attribute", dev->kernel);
-               return 2;
-       }
-       return 0;
-}
+declare_sysfs_get_str(dev);
 
 int
 sysfs_get_timeout(struct sysfs_device *dev, unsigned int *timeout)
 {
-       char *attr;
-       char attr_path[SYSFS_PATH_SIZE];
+       char attr_path[SYSFS_PATH_SIZE], attr[NAME_SIZE];
+       size_t len;
        int r;
        unsigned int t;
 
        if (safe_sprintf(attr_path, "%s/device", dev->devpath))
                return 1;
 
-       attr = sysfs_attr_get_value(dev->devpath, "timeout");
-       if (!attr)
+       len = sysfs_attr_get_value(dev->devpath, "timeout", attr, NAME_SIZE);
+       if (!len) {
+               condlog(3, "%s: No timeout value in sysfs", dev->devpath);
                return 1;
+       }
 
        r = sscanf(attr, "%u\n", &t);
 
-       if (r != 1)
+       if (r != 1) {
+               condlog(3, "%s: Cannot parse timeout attribute '%s'",
+                       dev->devpath, attr);
                return 1;
+       }
 
        *timeout = t * 1000;
 
@@ -192,17 +187,23 @@ sysfs_get_timeout(struct sysfs_device *dev, unsigned int *timeout)
 int
 sysfs_get_size (struct sysfs_device * dev, unsigned long long * size)
 {
-       char *attr;
+       char attr[NAME_SIZE];
+       size_t len;
        int r;
 
-       attr = sysfs_attr_get_value(dev->devpath, "size");
-       if (!attr)
+       len = sysfs_attr_get_value(dev->devpath, "size", attr, NAME_SIZE);
+       if (!len) {
+               condlog(3, "%s: No size attribute in sysfs", dev->devpath);
                return 1;
+       }
 
        r = sscanf(attr, "%llu\n", size);
 
-       if (r != 1)
+       if (r != 1) {
+               condlog(3, "%s: Cannot parse size attribute '%s'",
+                       dev->devpath, attr);
                return 1;
+       }
 
        return 0;
 }
@@ -212,7 +213,8 @@ sysfs_get_fc_nodename (struct sysfs_device * dev, char * node,
                       unsigned int host, unsigned int channel,
                       unsigned int target)
 {
-       char attr_path[SYSFS_PATH_SIZE], *attr;
+       char attr_path[SYSFS_PATH_SIZE];
+       size_t len;
 
        if (safe_sprintf(attr_path,
                         "/class/fc_transport/target%i:%i:%i",
@@ -221,13 +223,11 @@ sysfs_get_fc_nodename (struct sysfs_device * dev, char * node,
                return 1;
        }
 
-       attr = sysfs_attr_get_value(attr_path, "node_name");
-       if (attr) {
-               strlcpy(node, attr, strlen(attr));
-               return 0;
-       }
+       len = sysfs_attr_get_value(attr_path, "node_name", node, NODE_NAME_SIZE);
+       if (!len)
+               return 1;
 
-       return 1;
+       return 0;
 }
 
 static int
@@ -293,7 +293,7 @@ sysfs_set_scsi_tmo (struct multipath *mpp)
                if (mpp->dev_loss){
                        snprintf(value, 11, "%u", mpp->dev_loss);
                        if (sysfs_attr_set_value(attr_path, "dev_loss_tmo",
-                                                value))
+                                                value, 11))
                                return 1;
                }
                if (mpp->fast_io_fail){
@@ -302,7 +302,7 @@ sysfs_set_scsi_tmo (struct multipath *mpp)
                        else
                                snprintf(value, 11, "%u", mpp->fast_io_fail);
                        if (sysfs_attr_set_value(attr_path, "fast_io_fail_tmo",
-                                                value))
+                                                value, 11))
                                return 1;
                }
        }
@@ -615,14 +615,14 @@ cciss_sysfs_pathinfo (struct path * pp, struct sysfs_device * dev)
 static int
 common_sysfs_pathinfo (struct path * pp, struct sysfs_device *dev)
 {
-       char *attr;
+       size_t len;
 
-       attr = sysfs_attr_get_value(dev->devpath, "dev");
-       if (!attr) {
+       len = sysfs_attr_get_value(dev->devpath, "dev",
+                                   pp->dev_t, BLK_DEV_SIZE);
+       if (!len) {
                condlog(3, "%s: no 'dev' attribute in sysfs", pp->dev);
                return 1;
        }
-       strlcpy(pp->dev_t, attr, BLK_DEV_SIZE);
 
        condlog(3, "%s: dev_t = %s", pp->dev, pp->dev_t);
 
index 714dab1..2bc605c 100644 (file)
 
 char sysfs_path[PATH_SIZE];
 
-/* attribute value cache */
-static LIST_HEAD(attr_list);
-struct sysfs_attr {
-       struct list_head node;
-       char path[PATH_SIZE];
-       char *value;                    /* points to value_local if value is cached */
-       char value_local[NAME_SIZE];
-};
-
 /* list of sysfs devices */
 static LIST_HEAD(sysfs_dev_list);
 struct sysfs_dev {
@@ -62,24 +53,15 @@ int sysfs_init(char *path, size_t len)
                strlcpy(sysfs_path, "/sys", sizeof(sysfs_path));
        dbg("sysfs_path='%s'", sysfs_path);
 
-       INIT_LIST_HEAD(&attr_list);
        INIT_LIST_HEAD(&sysfs_dev_list);
        return 0;
 }
 
 void sysfs_cleanup(void)
 {
-       struct sysfs_attr *attr_loop;
-       struct sysfs_attr *attr_temp;
-
        struct sysfs_dev *sysdev_loop;
        struct sysfs_dev *sysdev_temp;
 
-       list_for_each_entry_safe(attr_loop, attr_temp, &attr_list, node) {
-               list_del(&attr_loop->node);
-               free(attr_loop);
-       }
-
        list_for_each_entry_safe(sysdev_loop, sysdev_temp, &sysfs_dev_list, node) {
                list_del(&sysdev_loop->node);
                free(sysdev_loop);
@@ -356,130 +338,95 @@ void sysfs_device_put(struct sysfs_device *dev)
        return;
 }
 
-int
-sysfs_attr_set_value(const char *devpath, const char *attr_name,
-                    const char *value)
+size_t sysfs_attr_get_value(const char *devpath, const char *attr_name,
+                           char *attr_value, int attr_len)
 {
        char path_full[PATH_SIZE];
-       int sysfs_len;
+       const char *path;
        struct stat statbuf;
-       int fd, value_len, ret = -1;
+       int fd;
+       ssize_t size;
+       size_t sysfs_len;
+
+       if (!attr_value || !attr_len)
+               return 0;
+
+       attr_value[0] = '\0';
+       size = 0;
 
        dbg("open '%s'/'%s'", devpath, attr_name);
-       sysfs_len = snprintf(path_full, PATH_SIZE, "%s%s/%s", sysfs_path,
-                            devpath, attr_name);
-       if (sysfs_len >= PATH_SIZE || sysfs_len < 0) {
-               if (sysfs_len < 0)
-                       dbg("cannot copy sysfs path %s%s/%s : %s", sysfs_path,
-                           devpath, attr_name, strerror(errno));
-               else
-                       dbg("sysfs_path %s%s/%s too large", sysfs_path,
-                           devpath, attr_name);
-               goto out;
-       }
+       sysfs_len = strlcpy(path_full, sysfs_path, sizeof(path_full));
+       if(sysfs_len >= sizeof(path_full))
+               sysfs_len = sizeof(path_full) - 1;
+       path = &path_full[sysfs_len];
+       strlcat(path_full, devpath, sizeof(path_full));
+       strlcat(path_full, "/", sizeof(path_full));
+       strlcat(path_full, attr_name, sizeof(path_full));
 
        if (stat(path_full, &statbuf) != 0) {
-               dbg("stat '%s' failed: %s" path_full, strerror(errno));
+               dbg("stat '%s' failed: %s", path_full, strerror(errno));
                goto out;
        }
 
        /* skip directories */
-        if (S_ISDIR(statbuf.st_mode))
-                goto out;
+       if (S_ISDIR(statbuf.st_mode))
+               goto out;
 
-       if ((statbuf.st_mode & S_IWUSR) == 0)
+       /* skip non-readable files */
+       if ((statbuf.st_mode & S_IRUSR) == 0)
                goto out;
 
-       fd = open(path_full, O_WRONLY);
+       /* read attribute value */
+       fd = open(path_full, O_RDONLY);
        if (fd < 0) {
                dbg("attribute '%s' can not be opened: %s",
                    path_full, strerror(errno));
                goto out;
        }
-       value_len = strlen(value) + 1;
-       ret = write(fd, value, value_len);
-       if (ret == value_len)
-               ret = 0;
-       else if (ret < 0)
-               dbg("write to %s failed: %s", path_full, strerror(errno));
-       else {
-               dbg("tried to write %d to %s. Wrote %d\n", value_len,
-                   path_full, ret);
-               ret = -1;
-       }
+       size = read(fd, attr_value, attr_len);
        close(fd);
+       if (size < 0)
+               goto out;
+       if (size == attr_len) {
+               dbg("overflow in attribute '%s', truncating", path_full);
+               size--;
+       }
+
+       /* got a valid value, store and return it */
+       attr_value[size] = '\0';
+       remove_trailing_chars(attr_value, '\n');
+
 out:
-       return ret;
+       return size;
 }
 
-
-char *sysfs_attr_get_value(const char *devpath, const char *attr_name)
+ssize_t sysfs_attr_set_value(const char *devpath, const char *attr_name,
+                            const char *value, int value_len)
 {
        char path_full[PATH_SIZE];
-       const char *path;
-       char value[NAME_SIZE];
-       struct sysfs_attr *attr_loop;
-       struct sysfs_attr *attr = NULL;
        struct stat statbuf;
        int fd;
-       ssize_t size;
+       ssize_t size = 0;
        size_t sysfs_len;
 
-       dbg("open '%s'/'%s'", devpath, attr_name);
-       sysfs_len = strlcpy(path_full, sysfs_path, sizeof(path_full));
-       if(sysfs_len >= sizeof(path_full))
-               sysfs_len = sizeof(path_full) - 1;
-       path = &path_full[sysfs_len];
-       strlcat(path_full, devpath, sizeof(path_full));
-       strlcat(path_full, "/", sizeof(path_full));
-       strlcat(path_full, attr_name, sizeof(path_full));
-
-       /* look for attribute in cache */
-       list_for_each_entry(attr_loop, &attr_list, node) {
-               if (strcmp(attr_loop->path, path) == 0) {
-                       dbg("found in cache '%s'", attr_loop->path);
-                       attr = attr_loop;
-               }
-       }
-       if (!attr) {
-               /* store attribute in cache */
-               dbg("new uncached attribute '%s'", path_full);
-               attr = malloc(sizeof(struct sysfs_attr));
-               if (attr == NULL)
-                       return NULL;
-               memset(attr, 0x00, sizeof(struct sysfs_attr));
-               strlcpy(attr->path, path, sizeof(attr->path));
-               dbg("add to cache '%s'", path_full);
-               list_add(&attr->node, &attr_list);
-       } else {
-               /* clear old value */
-               if(attr->value)
-                       memset(attr->value, 0x00, sizeof(attr->value));
-       }
+       if (!attr_name || !value || !value_len)
+               return 0;
 
-       if (lstat(path_full, &statbuf) != 0) {
-               dbg("stat '%s' failed: %s", path_full, strerror(errno));
+       dbg("open '%s'/'%s'", devpath, attr_name);
+       sysfs_len = snprintf(path_full, PATH_SIZE, "%s%s/%s", sysfs_path,
+                            devpath, attr_name);
+       if (sysfs_len >= PATH_SIZE || sysfs_len < 0) {
+               if (sysfs_len < 0)
+                       dbg("cannot copy sysfs path %s%s/%s : %s", sysfs_path,
+                           devpath, attr_name, strerror(errno));
+               else
+                       dbg("sysfs_path %s%s/%s too large", sysfs_path,
+                           devpath, attr_name);
                goto out;
        }
 
-       if (S_ISLNK(statbuf.st_mode)) {
-               /* links return the last element of the target path */
-               char link_target[PATH_SIZE];
-               int len;
-               const char *pos;
-
-               len = readlink(path_full, link_target, sizeof(link_target));
-               if (len > 0) {
-                       link_target[len] = '\0';
-                       pos = strrchr(link_target, '/');
-                       if (pos != NULL) {
-                               dbg("cache '%s' with link value '%s'",
-                                   path_full, value);
-                               strlcpy(attr->value_local, &pos[1],
-                                       sizeof(attr->value_local));
-                               attr->value = attr->value_local;
-                       }
-               }
+       if (stat(path_full, &statbuf) != 0) {
+               dbg("stat '%s' failed: %s", path_full, strerror(errno));
                goto out;
        }
 
@@ -487,33 +434,27 @@ char *sysfs_attr_get_value(const char *devpath, const char *attr_name)
        if (S_ISDIR(statbuf.st_mode))
                goto out;
 
-       /* skip non-readable files */
-       if ((statbuf.st_mode & S_IRUSR) == 0)
+       /* skip non-writeable files */
+       if ((statbuf.st_mode & S_IWUSR) == 0)
                goto out;
 
        /* read attribute value */
-       fd = open(path_full, O_RDONLY);
+       fd = open(path_full, O_WRONLY);
        if (fd < 0) {
                dbg("attribute '%s' can not be opened: %s",
                    path_full, strerror(errno));
                goto out;
        }
-       size = read(fd, value, sizeof(value));
-       close(fd);
+       size = write(fd, value, value_len);
        if (size < 0)
-               goto out;
-       if (size == sizeof(value)) {
-               dbg("overflow in attribute '%s', truncating", path_full);
-               size--;
+               dbg("write to %s failed: %s", path_full, strerror(errno));
+       else if (size < value_len) {
+               dbg("tried to write %d to %s. Wrote %d\n", value_len,
+                   path_full, size);
+               size = -1;
        }
-
-       /* got a valid value, store and return it */
-       value[size] = '\0';
-       remove_trailing_chars(value, '\n');
-       dbg("cache '%s' with attribute value '%s'", path_full, value);
-       strlcpy(attr->value_local, value, sizeof(attr->value_local));
-       attr->value = attr->value_local;
+       close(fd);
 
 out:
-       return attr && attr->value && strlen(attr->value) ? attr->value : NULL;
+       return size;
 }
index 57d4cb1..b03166b 100644 (file)
@@ -19,9 +19,10 @@ 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);
+size_t sysfs_attr_get_value(const char *devpath, const char *attr_name,
+                           char *attr_value, int attr_len);
+ssize_t sysfs_attr_set_value(const char *devpath, const char *attr_name,
+                            const char *value, int value_len);
 int sysfs_resolve_link(char *path, size_t size);
 int sysfs_get_size (struct sysfs_device * dev, unsigned long long * size);
-int sysfs_attr_set_value(const char *devpath, const char *attr_name,
-                        const char *value);
 #endif
index f342dc9..9de33b5 100644 (file)
@@ -224,18 +224,12 @@ int
 ev_add_map (struct sysfs_device * dev, struct vectors * vecs)
 {
        char * alias;
-       char *dev_t;
        int major, minor;
        char * refwwid;
        struct multipath * mpp;
        int map_present;
        int r = 1;
 
-       dev_t = sysfs_attr_get_value(dev->devpath, "dev");
-
-       if (!dev_t || sscanf(dev_t, "%d:%d", &major, &minor) != 2)
-               return 1;
-
        alias = dm_mapname(major, minor);
 
        if (!alias)