Remove sysfs_attr cache

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>

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index dde876fad810effb604eed1c65f18379f74d19b1..73f84a71550d6386f0d4f5a3050cbb6b855962f2 100644 (file)
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -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);
 

diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index 714dab1aad0fda1327aef39aba46bb3b7fea2aff..2bc605c44c511c889ccae33d77b6d028a57aa98e 100644 (file)
--- a/libmultipath/sysfs.c
+++ b/libmultipath/sysfs.c
@@ -37,15 +37,6 @@
 
 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;
 }

diff --git a/libmultipath/sysfs.h b/libmultipath/sysfs.h
index 57d4cb1d0e5119b8309fabce9ac7c2ca6b0ab99d..b03166b31a4851683f80d5ab40a444b0169cca69 100644 (file)
--- a/libmultipath/sysfs.h
+++ b/libmultipath/sysfs.h
@@ -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

diff --git a/multipathd/main.c b/multipathd/main.c
index f342dc95f0f1dd90b3f0ea4cd88f5433e9668267..9de33b52c8fddeaf5de7486c599a7ab14273b63c 100644 (file)
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -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)