Skip to contentSkip to conversation

sysfs backing store (Re-splitted)

PATCH In review Maneesh Soni 2 participants 11 messages Jul 29, 2004 – Aug 3, 2004 Driver core

lore
Share & tools
lore

Maneesh Soni

author
#0 lore raw

cover sysfs backing store (Re-splitted)

Hello Andrew,

As per Viro's suggestions I have re-splitted the sysfs backing store patch set. This splitting is more meaningful and it should make the long pending review more interesting. There are no significant changes in the code except some cosmetic code re-arrangement and a couple of real changes in the error paths.

1) In fs/sysfs/dir.c:create_dir(), dput() was missing if we fail in sysfs_new_dirent()

2) Check for kmalloc() return value in fs/sysfs/symlink.c:sysfs_add_link().

I hope Viro will do the review soon. In the mean time I thought it will be nice if you could _replace_ the sysfs patches (six patches with names sysfs-leaves-xxx.patch) with the following patches (mailed separately). I will update them again based on the comments I get.

Thanks
Maneesh

Signature · 6 lines
Maneesh Soni
Linux Technology Center, 
IBM Austin
email: [email protected]
Phone: 1-512-838-1896 Fax: 
T/L : 6781896
author
#1 lore raw

PATCH 1/5 Add sysfs_dirent to sysfs dentry

o This the first patch in the series of patches implementing sysfs backing store. It introduces the new sysfs_dirent data structure. The sysfs_dirent is added to each of the element which can be represented in sysfs like, kobject (directory), text or binary attributes (files), attribute groups (directory) and symlinks.

o It uses dentry's d_fsdata field to attach the corresponding sysfs_dirent.

o The sysfs_dirents are maintained in a tree of all the sysfs entries using the s_children and s_sibling list pointers.

o The patch just adds the sysfs_dirent structure and hence should not be used independently but system can boot with it.

 fs/sysfs/bin.c        |   17 +++++++++++----
 fs/sysfs/dir.c        |   27 +++++++++++++++++++-----
 fs/sysfs/file.c       |   25 ++++++++++++++++------
 fs/sysfs/group.c      |    2 -
 fs/sysfs/mount.c      |    8 +++++++
 fs/sysfs/symlink.c    |   56 +++++++++++++++++++++++++++++++++++++++++++++-----
 fs/sysfs/sysfs.h      |   37 ++++++++++++++++++++++++++++++---
 include/linux/sysfs.h |   21 ++++++++++++++++++
 8 files changed, 169 insertions(+), 24 deletions(-)
include/linux/sysfs.h renamed from include/linux/sysfs.h~sysfs-backing-store-add-sysfs_dirent +21 -0
diff -puN include/linux/sysfs.h~sysfs-backing-store-add-sysfs_dirent include/linux/sysfs.h
@@ -9,6 +9,8 @@
 #ifndef _SYSFS_H_
 #define _SYSFS_H_
 
+#include <asm/atomic.h>
+
 struct kobject;
 struct module;
 

@@ -57,6 +59,25 @@ struct sysfs_ops {
 	ssize_t	(*store)(struct kobject *,struct attribute *,const char *, size_t);
 };
 
+struct sysfs_dirent {
+	atomic_t		s_count;
+	struct list_head	s_sibling;
+	struct list_head	s_children;
+	void 			* s_element;
+	int			s_type;
+	umode_t			s_mode;
+	struct dentry		* s_dentry;
+};
+
+#define SYSFS_ROOT		0x0001
+#define SYSFS_KOBJECT		0x0002
+#define SYSFS_KOBJ_ATTR 	0x0004
+#define SYSFS_KOBJ_BIN_ATTR	0x0008
+#define SYSFS_KOBJ_ATTR_GROUP	0x0010
+#define SYSFS_KOBJ_LINK 	0x0020
+#define SYSFS_NOT_PINNED	(SYSFS_KOBJ_ATTR | SYSFS_KOBJ_BIN_ATTR | SYSFS_KOBJ_LINK)
+
+
 #ifdef CONFIG_SYSFS
 
 extern int
fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs-backing-store-add-sysfs_dirent +22 -5
diff -puN fs/sysfs/dir.c~sysfs-backing-store-add-sysfs_dirent fs/sysfs/dir.c
@@ -27,17 +27,34 @@ static int create_dir(struct kobject * k
 		      const char * n, struct dentry ** d)
 {
 	int error;
+	umode_t mode = S_IFDIR| S_IRWXU | S_IRUGO | S_IXUGO;
 
 	down(&p->d_inode->i_sem);
 	*d = sysfs_get_dentry(p,n);
 	if (!IS_ERR(*d)) {
-		error = sysfs_create(*d,
-					 S_IFDIR| S_IRWXU | S_IRUGO | S_IXUGO,
-					 init_dir);
+		error = sysfs_create(*d, mode, init_dir);
 		if (!error) {
-			(*d)->d_fsdata = k;
-			p->d_inode->i_nlink++;
+			struct sysfs_dirent * sd, * parent_sd;
+			parent_sd = p->d_fsdata;
+			sd = sysfs_new_dirent(parent_sd, k,
+						(parent_sd->s_element == k) ?
+						SYSFS_KOBJ_ATTR_GROUP :
+						SYSFS_KOBJECT);
+			if (sd) {
+				(*d)->d_fsdata = sd;
+				p->d_inode->i_nlink++;
+				sd->s_dentry = *d;
+				sd->s_mode = mode;
+			} else {
+				/* error, release the ref taken in
+				 * sysfs_create()
+				 */
+				dput(*d);  
+				error = -ENOMEM;
+			}
 		}
+		/* This will free the dentry in case of error 
+		 */
 		dput(*d);
 	} else
 		error = PTR_ERR(*d);
fs/sysfs/file.c renamed from fs/sysfs/file.c~sysfs-backing-store-add-sysfs_dirent +19 -6
diff -puN fs/sysfs/file.c~sysfs-backing-store-add-sysfs_dirent fs/sysfs/file.c
@@ -346,10 +346,12 @@ static struct file_operations sysfs_file
 };
 
 
-int sysfs_add_file(struct dentry * dir, const struct attribute * attr)
+int sysfs_add_file(struct dentry * dir, const struct attribute * attr, int type)
 {
 	struct dentry * dentry;
-	int error;
+	struct sysfs_dirent * sd;
+	struct sysfs_dirent * parent_sd = dir->d_fsdata;
+	int error = 0;
 
 	down(&dir->d_inode->i_sem);
 	dentry = sysfs_get_dentry(dir,attr->name);

@@ -357,8 +359,18 @@ int sysfs_add_file(struct dentry * dir, 
 		error = sysfs_create(dentry,
 				     (attr->mode & S_IALLUGO) | S_IFREG,
 				     init_file);
-		if (!error)
-			dentry->d_fsdata = (void *)attr;
+		if (!error) {
+
+			sd = sysfs_new_dirent(parent_sd, (void *) attr, type);
+			if (sd) {
+				sd->s_mode = (attr->mode & S_IALLUGO) | S_IFREG;
+				dentry->d_fsdata = sd;
+				sd->s_dentry = dentry;
+			} else {
+				dput(dentry);
+				error = -ENOMEM;
+			}
+		}
 		dput(dentry);
 	} else
 		error = PTR_ERR(dentry);

@@ -375,8 +387,9 @@ int sysfs_add_file(struct dentry * dir, 
 
 int sysfs_create_file(struct kobject * kobj, const struct attribute * attr)
 {
-	if (kobj && attr)
-		return sysfs_add_file(kobj->dentry,attr);
+	if (kobj && kobj->dentry && attr)
+		return sysfs_add_file(kobj->dentry, attr, SYSFS_KOBJ_ATTR);
+
 	return -EINVAL;
 }
fs/sysfs/bin.c renamed from fs/sysfs/bin.c~sysfs-backing-store-add-sysfs_dirent +13 -4
diff -puN fs/sysfs/bin.c~sysfs-backing-store-add-sysfs_dirent fs/sysfs/bin.c
@@ -160,6 +160,8 @@ int sysfs_create_bin_file(struct kobject
 {
 	struct dentry * dentry;
 	struct dentry * parent;
+	struct sysfs_dirent * sd;
+	umode_t mode = (attr->attr.mode & S_IALLUGO) | S_IFREG;
 	int error = 0;
 
 	if (!kobj || !attr)

@@ -170,13 +172,20 @@ int sysfs_create_bin_file(struct kobject
 	down(&parent->d_inode->i_sem);
 	dentry = sysfs_get_dentry(parent,attr->attr.name);
 	if (!IS_ERR(dentry)) {
-		dentry->d_fsdata = (void *)attr;
-		error = sysfs_create(dentry,
-				     (attr->attr.mode & S_IALLUGO) | S_IFREG,
-				     NULL);
+		error = sysfs_create(dentry, mode, NULL);
 		if (!error) {
 			dentry->d_inode->i_size = attr->size;
 			dentry->d_inode->i_fop = &bin_fops;
+			sd = sysfs_new_dirent(parent->d_fsdata, (void *) attr, 
+						SYSFS_KOBJ_BIN_ATTR);
+			if (!sd) {
+				sd->s_mode = mode;
+				dentry->d_fsdata = sd;
+				sd->s_dentry = dentry;
+			} else {
+				dput(dentry);
+				error =  -ENOMEM;
+			}
 		}
 		dput(dentry);
 	} else
fs/sysfs/sysfs.h renamed from fs/sysfs/sysfs.h~sysfs-backing-store-add-sysfs_dirent +34 -3
diff -puN fs/sysfs/sysfs.h~sysfs-backing-store-add-sysfs_dirent fs/sysfs/sysfs.h
@@ -5,8 +5,9 @@ extern struct inode * sysfs_new_inode(mo
 extern int sysfs_create(struct dentry *, int mode, int (*init)(struct inode *));
 
 extern struct dentry * sysfs_get_dentry(struct dentry *, const char *);
+extern void sysfs_drop_dentry(struct sysfs_dirent *, struct dentry *);
 
-extern int sysfs_add_file(struct dentry * dir, const struct attribute * attr);
+extern int sysfs_add_file(struct dentry *, const struct attribute *, int);
 extern void sysfs_hash_and_remove(struct dentry * dir, const char * name);
 
 extern int sysfs_create_subdir(struct kobject *, const char *, struct dentry **);

@@ -16,14 +17,44 @@ extern int sysfs_readlink(struct dentry 
 extern int sysfs_follow_link(struct dentry *, struct nameidata *);
 extern struct rw_semaphore sysfs_rename_sem;
 
+struct sysfs_symlink {
+	char * link_name;
+	struct kobject * target_kobj;
+};
+
 static inline struct kobject *sysfs_get_kobject(struct dentry *dentry)
 {
 	struct kobject * kobj = NULL;
 
 	spin_lock(&dcache_lock);
-	if (!d_unhashed(dentry))
-		kobj = kobject_get(dentry->d_fsdata);
+	if (!d_unhashed(dentry)) {
+		struct sysfs_dirent * sd = dentry->d_fsdata;
+		if (sd->s_type & SYSFS_KOBJ_LINK) {
+			struct sysfs_symlink * sl = sd->s_element;
+			kobj = kobject_get(sl->target_kobj);
+		} else
+			kobj = kobject_get(sd->s_element);
+	}
 	spin_unlock(&dcache_lock);
 
 	return kobj;
 }
+
+static inline
+struct sysfs_dirent * sysfs_new_dirent(struct sysfs_dirent * p, void * e, int t)
+{
+	struct sysfs_dirent * sd;
+
+	sd = kmalloc(sizeof(*sd), GFP_KERNEL);
+	if (!sd)
+		return NULL;
+	memset(sd, 0, sizeof(*sd));
+	atomic_set(&sd->s_count, 1);
+	sd->s_element = e;
+	sd->s_type = t;
+	sd->s_dentry = NULL;
+	INIT_LIST_HEAD(&sd->s_children);
+	list_add(&sd->s_sibling, &p->s_children);
+
+	return sd;
+}
fs/sysfs/group.c renamed from fs/sysfs/group.c~sysfs-backing-store-add-sysfs_dirent +1 -1
diff -puN fs/sysfs/group.c~sysfs-backing-store-add-sysfs_dirent fs/sysfs/group.c
@@ -31,7 +31,7 @@ static int create_files(struct dentry * 
 	int error = 0;
 
 	for (attr = grp->attrs; *attr && !error; attr++) {
-		error = sysfs_add_file(dir,*attr);
+		error = sysfs_add_file(dir, *attr, SYSFS_KOBJ_ATTR);
 	}
 	if (error)
 		remove_files(dir,grp);
fs/sysfs/symlink.c renamed from fs/sysfs/symlink.c~sysfs-backing-store-add-sysfs_dirent +51 -5

72 lines not on this page · raw message

fs/sysfs/mount.c renamed from fs/sysfs/mount.c~sysfs-backing-store-add-sysfs_dirent +8 -0
diff -puN fs/sysfs/mount.c~sysfs-backing-store-add-sysfs_dirent fs/sysfs/mount.c
@@ -22,6 +22,13 @@ static struct super_operations sysfs_ops
 	.drop_inode	= generic_delete_inode,
 };
 
+struct sysfs_dirent sysfs_root = {
+	.s_sibling	= LIST_HEAD_INIT(sysfs_root.s_sibling),
+	.s_children	= LIST_HEAD_INIT(sysfs_root.s_children),
+	.s_element	= NULL,
+	.s_type		= SYSFS_ROOT,
+};
+
 static int sysfs_fill_super(struct super_block *sb, void *data, int silent)
 {
 	struct inode *inode;

@@ -50,6 +57,7 @@ static int sysfs_fill_super(struct super
 		iput(inode);
 		return -ENOMEM;
 	}
+	root->d_fsdata = &sysfs_root;
 	sb->s_root = root;
 	return 0;
 }

_

Signature · 6 lines
Maneesh Soni
Linux Technology Center, 
IBM Austin
email: [email protected]
Phone: 1-512-838-1896 Fax: 
T/L : 6781896
author
#2 lore raw

PATCH 2/5 Use sysfs_dirent tree for ->readdir etc.

o This patch implements the sysfs_dir_operations file_operations strucutre for sysfs directories. It uses the sysfs_dirent based tree for ->readdir() and ->lseek() methods instead of simple_dir_operations which use dentry based tree.

o This also has the code for sysfs_get() and sysfs_put() which manipulates the reference counting for for sysfs_dirents.

 fs/sysfs/dir.c   |  173 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 fs/sysfs/mount.c |    2 
 fs/sysfs/sysfs.h |   27 ++++++++
 3 files changed, 200 insertions(+), 2 deletions(-)
fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs-backing-store-sysfs_readdir +172 -1
diff -puN fs/sysfs/dir.c~sysfs-backing-store-sysfs_readdir fs/sysfs/dir.c
@@ -15,7 +15,7 @@ DECLARE_RWSEM(sysfs_rename_sem);
 static int init_dir(struct inode * inode)
 {
 	inode->i_op = &simple_dir_inode_operations;
-	inode->i_fop = &simple_dir_operations;
+	inode->i_fop = &sysfs_dir_operations;
 
 	/* directory inodes start off with i_nlink == 2 (for "." entry) */
 	inode->i_nlink++;

@@ -210,6 +210,177 @@ int sysfs_rename_dir(struct kobject * ko
 	return error;
 }
 
+/* get the name for corresponding element represented by the given sysfs_dirent
+ */
+const unsigned char * sysfs_get_name(struct sysfs_dirent *sd)
+{
+	struct attribute * attr;
+	struct bin_attribute * bin_attr;
+	struct sysfs_symlink  * sl;
+
+	if (!sd || !sd->s_element)
+		BUG();
+
+	switch (sd->s_type) {
+		case SYSFS_KOBJECT:
+		case SYSFS_KOBJ_ATTR_GROUP:
+			/* Always have a dentry so use that */
+			return sd->s_dentry->d_name.name;
+
+		case SYSFS_KOBJ_ATTR:
+			attr = sd->s_element;
+			return attr->name;
+
+		case SYSFS_KOBJ_BIN_ATTR:
+			bin_attr = sd->s_element;
+			return bin_attr->attr.name;
+
+		case SYSFS_KOBJ_LINK:
+			sl = sd->s_element;
+			return sl->link_name;
+	}
+	return NULL;
+}
+
+static int sysfs_dir_open(struct inode *inode, struct file *file)
+{
+	struct dentry * dentry = file->f_dentry;
+	struct sysfs_dirent * parent_sd = dentry->d_fsdata;
+
+	down(&dentry->d_inode->i_sem);
+	file->private_data = sysfs_new_dirent(parent_sd, NULL, 0);
+	up(&dentry->d_inode->i_sem);
+
+	return file->private_data ? 0 : -ENOMEM;
+
+}
+
+static int sysfs_dir_close(struct inode *inode, struct file *file)
+{
+	struct dentry * dentry = file->f_dentry;
+	struct sysfs_dirent * cursor = file->private_data;
+
+	down(&dentry->d_inode->i_sem);
+	list_del_init(&cursor->s_sibling);
+	up(&dentry->d_inode->i_sem);
+	sysfs_put(file->private_data);
+
+	return 0;
+}
+
+/* Relationship between s_mode and the DT_xxx types */
+static inline unsigned char dt_type(struct sysfs_dirent *sd)
+{
+	return (sd->s_mode >> 12) & 15;
+}
+
+static int sysfs_readdir(struct file * filp, void * dirent, filldir_t filldir)
+{
+	struct dentry *dentry = filp->f_dentry;
+	struct sysfs_dirent * parent_sd = dentry->d_fsdata;
+	struct sysfs_dirent *cursor = filp->private_data;
+	struct list_head *p, *q = &cursor->s_sibling;
+	ino_t ino;
+	int i = filp->f_pos;
+
+	switch (i) {
+		case 0:
+			ino = dentry->d_inode->i_ino;
+			if (filldir(dirent, ".", 1, i, ino, DT_DIR) < 0)
+				break;
+			filp->f_pos++;
+			i++;
+			/* fallthrough */
+		case 1:
+			ino = parent_ino(dentry);
+			if (filldir(dirent, "..", 2, i, ino, DT_DIR) < 0)
+				break;
+			filp->f_pos++;
+			i++;
+			/* fallthrough */
+		default:
+			if (filp->f_pos == 2) {
+				list_del(q);
+				list_add(q, &parent_sd->s_children);
+			}
+			for (p=q->next; p!= &parent_sd->s_children; p=p->next) {
+				struct sysfs_dirent *next;
+				const char * name;
+				int len;
+
+				next = list_entry(p, struct sysfs_dirent,
+						   s_sibling);
+				if (!next->s_element)
+					continue;
+
+				name = sysfs_get_name(next);
+				len = strlen(name);
+				if (next->s_dentry)
+					ino = next->s_dentry->d_inode->i_ino;
+				else
+					ino = iunique(sysfs_sb, 2);
+
+				if (filldir(dirent, name, len, filp->f_pos, ino,
+						 dt_type(next)) < 0)
+					return 0;
+
+				list_del(q);
+				list_add(q, p);
+				p = q;
+				filp->f_pos++;
+			}
+	}
+	return 0;
+}
+
+static loff_t sysfs_dir_lseek(struct file * file, loff_t offset, int origin)
+{
+	struct dentry * dentry = file->f_dentry;
+
+	down(&dentry->d_inode->i_sem);
+	switch (origin) {
+		case 1:
+			offset += file->f_pos;
+		case 0:
+			if (offset >= 0)
+				break;
+		default:
+			up(&file->f_dentry->d_inode->i_sem);
+			return -EINVAL;
+	}
+	if (offset != file->f_pos) {
+		file->f_pos = offset;
+		if (file->f_pos >= 2) {
+			struct sysfs_dirent *sd = dentry->d_fsdata;
+			struct sysfs_dirent *cursor = file->private_data;
+			struct list_head *p;
+			loff_t n = file->f_pos - 2;
+
+			list_del(&cursor->s_sibling);
+			p = sd->s_children.next;
+			while (n && p != &sd->s_children) {
+				struct sysfs_dirent *next;
+				next = list_entry(p, struct sysfs_dirent,
+						   s_sibling);
+				if (next->s_element)
+					n--;
+				p = p->next;
+			}
+			list_add_tail(&cursor->s_sibling, p);
+		}
+	}
+	up(&dentry->d_inode->i_sem);
+	return offset;
+}
+
+struct file_operations sysfs_dir_operations = {
+	.open		= sysfs_dir_open,
+	.release	= sysfs_dir_close,
+	.llseek		= sysfs_dir_lseek,
+	.read		= generic_read_dir,
+	.readdir	= sysfs_readdir,
+};
+
 EXPORT_SYMBOL(sysfs_create_dir);
 EXPORT_SYMBOL(sysfs_remove_dir);
 EXPORT_SYMBOL(sysfs_rename_dir);
fs/sysfs/sysfs.h renamed from fs/sysfs/sysfs.h~sysfs-backing-store-sysfs_readdir +27 -0
diff -puN fs/sysfs/sysfs.h~sysfs-backing-store-sysfs_readdir fs/sysfs/sysfs.h
@@ -4,6 +4,8 @@ extern struct vfsmount * sysfs_mount;
 extern struct inode * sysfs_new_inode(mode_t mode);
 extern int sysfs_create(struct dentry *, int mode, int (*init)(struct inode *));
 
+
+extern const unsigned char * sysfs_get_name(struct sysfs_dirent *sd);
 extern struct dentry * sysfs_get_dentry(struct dentry *, const char *);
 extern void sysfs_drop_dentry(struct sysfs_dirent *, struct dentry *);
 

@@ -16,6 +18,8 @@ extern void sysfs_remove_subdir(struct d
 extern int sysfs_readlink(struct dentry *, char __user *, int );
 extern int sysfs_follow_link(struct dentry *, struct nameidata *);
 extern struct rw_semaphore sysfs_rename_sem;
+extern struct super_block * sysfs_sb;
+extern struct file_operations sysfs_dir_operations;
 
 struct sysfs_symlink {
 	char * link_name;

@@ -58,3 +62,26 @@ struct sysfs_dirent * sysfs_new_dirent(s
 
 	return sd;
 }
+
+static inline struct sysfs_dirent * sysfs_get(struct sysfs_dirent * sd)
+{
+	if (sd) {
+		WARN_ON(!atomic_read(&sd->s_count));
+		atomic_inc(&sd->s_count);
+	}
+	return sd;
+}
+
+static inline void sysfs_put(struct sysfs_dirent * sd)
+{
+	if (atomic_dec_and_test(&sd->s_count)) {
+		if (sd->s_type & SYSFS_KOBJ_LINK) {
+			struct sysfs_symlink * sl = sd->s_element;
+			kfree(sl->link_name);
+			kobject_put(sl->target_kobj);
+			kfree(sl);
+		}
+		kfree(sd);
+	}
+}
+
fs/sysfs/mount.c renamed from fs/sysfs/mount.c~sysfs-backing-store-sysfs_readdir +1 -1
diff -puN fs/sysfs/mount.c~sysfs-backing-store-sysfs_readdir fs/sysfs/mount.c
@@ -43,7 +43,7 @@ static int sysfs_fill_super(struct super
 	inode = sysfs_new_inode(S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO);
 	if (inode) {
 		inode->i_op = &simple_dir_inode_operations;
-		inode->i_fop = &simple_dir_operations;
+		inode->i_fop = &sysfs_dir_operations;
 		/* directory inodes start off with i_nlink == 2 (for "." entry) */
 		inode->i_nlink++;	
 	} else {

_

Signature · 6 lines
Maneesh Soni
Linux Technology Center, 
IBM Austin
email: [email protected]
Phone: 1-512-838-1896 Fax: 
T/L : 6781896
author
#3 lore raw

PATCH 3/5 Free sysfs_dirent on file removal

o The following patch implements the code to free up the sysfs_dirents upon directory or file removal. It uses the sysfs_dirent based tree in order to remove the directory contents before removing the directory itself. It could do this without taking dcache_lock in sysfs_remove_dir() as it doesnot use dentry based tree.

 fs/sysfs/dir.c   |   48 ++++++++++++++----------------------------------
 fs/sysfs/inode.c |   51 ++++++++++++++++++++++++++++++---------------------
 2 files changed, 44 insertions(+), 55 deletions(-)
fs/sysfs/inode.c renamed from fs/sysfs/inode.c~sysfs-backing-store-kill-sysfs_dirent +30 -21
diff -puN fs/sysfs/inode.c~sysfs-backing-store-kill-sysfs_dirent fs/sysfs/inode.c
@@ -11,6 +11,8 @@
 #include <linux/pagemap.h>
 #include <linux/namei.h>
 #include <linux/backing-dev.h>
+#include "sysfs.h"
+
 extern struct super_block * sysfs_sb;
 
 static struct address_space_operations sysfs_aops = {

@@ -88,33 +90,40 @@ struct dentry * sysfs_get_dentry(struct 
 	return lookup_hash(&qstr,parent);
 }
 
+/* Unhashes the dentry corresponding to given sysfs_dirent
+ * Called with parent inode's i_sem held.
+ */
+void sysfs_drop_dentry(struct sysfs_dirent * sd, struct dentry * parent)
+{
+	struct dentry * dentry = sd->s_dentry;
+
+	if (dentry) {
+		if (!(d_unhashed(dentry) && dentry->d_inode)) {
+			spin_lock(&dcache_lock);
+			dget_locked(dentry);
+			__d_drop(dentry);
+			spin_unlock(&dcache_lock);
+			simple_unlink(parent->d_inode, dentry);
+		} else
+			dput(dentry);
+	}
+}
+
 void sysfs_hash_and_remove(struct dentry * dir, const char * name)
 {
-	struct dentry * victim;
+	struct sysfs_dirent * sd, * parent_sd = dir->d_fsdata;
 
 	down(&dir->d_inode->i_sem);
-	victim = sysfs_get_dentry(dir,name);
-	if (!IS_ERR(victim)) {
-		/* make sure dentry is really there */
-		if (victim->d_inode && 
-		    (victim->d_parent->d_inode == dir->d_inode)) {
-			pr_debug("sysfs: Removing %s (%d)\n", victim->d_name.name,
-				 atomic_read(&victim->d_count));
-
-			d_drop(victim);
-			/* release the target kobject in case of 
-			 * a symlink
-			 */
-			if (S_ISLNK(victim->d_inode->i_mode))
-				kobject_put(victim->d_fsdata);
-			simple_unlink(dir->d_inode,victim);
+	list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
+		if (!sd->s_element)
+			continue;
+		if (!strcmp(sysfs_get_name(sd), name)) {
+			list_del_init(&sd->s_sibling);
+			sysfs_drop_dentry(sd, dir);
+			sysfs_put(sd);
+			break;
 		}
-		/*
-		 * Drop reference from sysfs_get_dentry() above.
-		 */
-		dput(victim);
 	}
 	up(&dir->d_inode->i_sem);
 }
 
-
fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs-backing-store-kill-sysfs_dirent +14 -34

69 lines not on this page · raw message

_

Signature · 6 lines
Maneesh Soni
Linux Technology Center, 
IBM Austin
email: [email protected]
Phone: 1-512-838-1896 Fax: 
T/L : 6781896
author
#4 lore raw

PATCH 4/5 Change sysfs_file_operations

o This patch modifies the sysfs_file_operations methods so as to get the kobject and attribute pointers via corresponding sysfs_dirent.

 fs/sysfs/bin.c  |   21 ++++++++++++++-------
 fs/sysfs/file.c |   24 +++++++++++++++++-------
 2 files changed, 31 insertions(+), 14 deletions(-)
fs/sysfs/file.c renamed from fs/sysfs/file.c~sysfs-backing-store-file_operations +17 -7
diff -puN fs/sysfs/file.c~sysfs-backing-store-file_operations fs/sysfs/file.c
@@ -77,8 +77,10 @@ struct sysfs_buffer {
  */
 static int fill_read_buffer(struct file * file, struct sysfs_buffer * buffer)
 {
-	struct attribute * attr = file->f_dentry->d_fsdata;
-	struct kobject * kobj = file->f_dentry->d_parent->d_fsdata;
+	struct sysfs_dirent * attr_sd = file->f_dentry->d_fsdata;
+	struct attribute * attr = attr_sd->s_element;
+	struct sysfs_dirent * kobj_sd = file->f_dentry->d_parent->d_fsdata;
+	struct kobject * kobj = kobj_sd->s_element;
 	struct sysfs_ops * ops = buffer->ops;
 	int ret = 0;
 	ssize_t count;

@@ -135,6 +137,9 @@ static int flush_read_buffer(struct sysf
  *	is in the file's ->d_fsdata. The target object is in the directory's
  *	->d_fsdata.
  *
+ *	It is safe to use ->d_parent->d_fsdata as both dentry and the kobject
+ *	are pinned in ->open().
+ *
  *	We call fill_read_buffer() to allocate and fill the buffer from the
  *	object's show() method exactly once (if the read is happening from
  *	the beginning of the file). That should fill the entire buffer with

@@ -199,8 +204,10 @@ fill_write_buffer(struct sysfs_buffer * 
 static int 
 flush_write_buffer(struct file * file, struct sysfs_buffer * buffer, size_t count)
 {
-	struct attribute * attr = file->f_dentry->d_fsdata;
-	struct kobject * kobj = file->f_dentry->d_parent->d_fsdata;
+	struct sysfs_dirent * attr_sd = file->f_dentry->d_fsdata;
+	struct attribute * attr = attr_sd->s_element;
+	struct sysfs_dirent * kobj_sd = file->f_dentry->d_parent->d_fsdata;
+	struct kobject * kobj = kobj_sd->s_element;
 	struct sysfs_ops * ops = buffer->ops;
 
 	return ops->store(kobj,attr,buffer->page,count);

@@ -240,7 +247,8 @@ sysfs_write_file(struct file *file, cons
 static int check_perm(struct inode * inode, struct file * file)
 {
 	struct kobject *kobj = sysfs_get_kobject(file->f_dentry->d_parent);
-	struct attribute * attr = file->f_dentry->d_fsdata;
+	struct sysfs_dirent * attr_sd = file->f_dentry->d_fsdata;
+	struct attribute * attr = attr_sd->s_element;
 	struct sysfs_buffer * buffer;
 	struct sysfs_ops * ops = NULL;
 	int error = 0;

@@ -321,8 +329,10 @@ static int sysfs_open_file(struct inode 
 
 static int sysfs_release(struct inode * inode, struct file * filp)
 {
-	struct kobject * kobj = filp->f_dentry->d_parent->d_fsdata;
-	struct attribute * attr = filp->f_dentry->d_fsdata;
+	struct sysfs_dirent * attr_sd = filp->f_dentry->d_fsdata;
+	struct attribute * attr = attr_sd->s_element;
+	struct sysfs_dirent * kobj_sd = filp->f_dentry->d_parent->d_fsdata;
+	struct kobject * kobj = kobj_sd->s_element;
 	struct sysfs_buffer * buffer = filp->private_data;
 
 	if (kobj)
fs/sysfs/bin.c renamed from fs/sysfs/bin.c~sysfs-backing-store-file_operations +14 -7
diff -puN fs/sysfs/bin.c~sysfs-backing-store-file_operations fs/sysfs/bin.c
@@ -17,8 +17,10 @@
 static int
 fill_read(struct dentry *dentry, char *buffer, loff_t off, size_t count)
 {
-	struct bin_attribute * attr = dentry->d_fsdata;
-	struct kobject * kobj = dentry->d_parent->d_fsdata;
+	struct sysfs_dirent * attr_sd = dentry->d_fsdata;
+	struct bin_attribute * attr = attr_sd->s_element;
+	struct sysfs_dirent * kobj_sd = dentry->d_parent->d_fsdata;
+	struct kobject * kobj = kobj_sd->s_element;
 
 	return attr->read(kobj, buffer, off, count);
 }

@@ -60,8 +62,10 @@ read(struct file * file, char __user * u
 static int
 flush_write(struct dentry *dentry, char *buffer, loff_t offset, size_t count)
 {
-	struct bin_attribute *attr = dentry->d_fsdata;
-	struct kobject *kobj = dentry->d_parent->d_fsdata;
+	struct sysfs_dirent * attr_sd = dentry->d_fsdata;
+	struct bin_attribute * attr = attr_sd->s_element;
+	struct sysfs_dirent * kobj_sd = dentry->d_parent->d_fsdata;
+	struct kobject * kobj = kobj_sd->s_element;
 
 	return attr->write(kobj, buffer, offset, count);
 }

@@ -95,7 +99,8 @@ static ssize_t write(struct file * file,
 static int open(struct inode * inode, struct file * file)
 {
 	struct kobject *kobj = sysfs_get_kobject(file->f_dentry->d_parent);
-	struct bin_attribute * attr = file->f_dentry->d_fsdata;
+	struct sysfs_dirent * attr_sd = file->f_dentry->d_fsdata;
+	struct bin_attribute * attr = attr_sd->s_element;
 	int error = -EINVAL;
 
 	if (!kobj || !attr)

@@ -130,8 +135,10 @@ static int open(struct inode * inode, st
 
 static int release(struct inode * inode, struct file * file)
 {
-	struct kobject * kobj = file->f_dentry->d_parent->d_fsdata;
-	struct bin_attribute * attr = file->f_dentry->d_fsdata;
+	struct sysfs_dirent * sd = file->f_dentry->d_parent->d_fsdata;
+	struct kobject * kobj = sd->s_element;
+	struct sysfs_dirent * attr_sd = file->f_dentry->d_fsdata;
+	struct bin_attribute * attr = attr_sd->s_element;
 	u8 * buffer = file->private_data;
 
 	if (kobj)

_

Signature · 6 lines
Maneesh Soni
Linux Technology Center, 
IBM Austin
email: [email protected]
Phone: 1-512-838-1896 Fax: 
T/L : 6781896
author
#5 lore raw

PATCH 5/5 Stop pinning dentries & inodes for leaves

o This patch stops the pinning of non-directory or leaf dentries and inodes. The leaf dentries and inodes are created during lookup based on the entries on sysfs_dirent tree. These leaves are removed from the dcache through the VFS dentry ageing process during shrink dcache operations. Thus reducing about 80% of sysfs lowmem needs.

o This implments the ->lookup() for sysfs directory inodes and allocates dentry and inode if the lookup is successful and avoids the need of allocating and pinning of dentry and inodes during the creation of corresponding sysfs leaf entry. As of now the implementation has not required negative dentry creation on failed lookup. As sysfs is still a RAM based filesystem, negative dentries are not of any use IMO.

o The leaf dentry allocated after successful lookup is connected to the existing corresponding sysfs_dirent through the d_fsdata field. This increments the ref count of sysfs_dirent. The ref count is released through ->d_iput() dentry_operation when dentry dies.

 fs/sysfs/bin.c     |   41 ++----------------
 fs/sysfs/dir.c     |  115 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 fs/sysfs/file.c    |   37 ++---------------
 fs/sysfs/inode.c   |    3 -
 fs/sysfs/mount.c   |    2 
 fs/sysfs/symlink.c |   40 ++----------------
 fs/sysfs/sysfs.h   |    4 +
 7 files changed, 137 insertions(+), 105 deletions(-)
fs/sysfs/inode.c renamed from fs/sysfs/inode.c~sysfs-backing-store-stop-pinning +2 -1
diff -puN fs/sysfs/inode.c~sysfs-backing-store-stop-pinning fs/sysfs/inode.c
@@ -68,7 +68,8 @@ int sysfs_create(struct dentry * dentry,
 		error = init(inode);
 	if (!error) {
 		d_instantiate(dentry, inode);
-		dget(dentry); /* Extra count - pin the dentry in core */
+		if (S_ISDIR(mode))
+			dget(dentry);  /* pin only directory dentry in core */
 	} else
 		iput(inode);
  Done:
fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs-backing-store-stop-pinning +112 -3

149 lines not on this page · raw message

fs/sysfs/file.c renamed from fs/sysfs/file.c~sysfs-backing-store-stop-pinning +6 -31
diff -puN fs/sysfs/file.c~sysfs-backing-store-stop-pinning fs/sysfs/file.c
@@ -9,15 +9,6 @@
 
 #include "sysfs.h"
 
-static struct file_operations sysfs_file_operations;
-
-static int init_file(struct inode * inode)
-{
-	inode->i_size = PAGE_SIZE;
-	inode->i_fop = &sysfs_file_operations;
-	return 0;
-}
-
 #define to_subsys(k) container_of(k,struct subsystem,kset.kobj)
 #define to_sattr(a) container_of(a,struct subsys_attribute,attr)
 

@@ -347,7 +338,7 @@ static int sysfs_release(struct inode * 
 	return 0;
 }
 
-static struct file_operations sysfs_file_operations = {
+struct file_operations sysfs_file_operations = {
 	.read		= sysfs_read_file,
 	.write		= sysfs_write_file,
 	.llseek		= generic_file_llseek,

@@ -358,32 +349,16 @@ static struct file_operations sysfs_file
 
 int sysfs_add_file(struct dentry * dir, const struct attribute * attr, int type)
 {
-	struct dentry * dentry;
 	struct sysfs_dirent * sd;
 	struct sysfs_dirent * parent_sd = dir->d_fsdata;
 	int error = 0;
 
 	down(&dir->d_inode->i_sem);
-	dentry = sysfs_get_dentry(dir,attr->name);
-	if (!IS_ERR(dentry)) {
-		error = sysfs_create(dentry,
-				     (attr->mode & S_IALLUGO) | S_IFREG,
-				     init_file);
-		if (!error) {
-
-			sd = sysfs_new_dirent(parent_sd, (void *) attr, type);
-			if (sd) {
-				sd->s_mode = (attr->mode & S_IALLUGO) | S_IFREG;
-				dentry->d_fsdata = sd;
-				sd->s_dentry = dentry;
-			} else {
-				dput(dentry);
-				error = -ENOMEM;
-			}
-		}
-		dput(dentry);
-	} else
-		error = PTR_ERR(dentry);
+	sd = sysfs_new_dirent(parent_sd, (void *) attr, type);
+	if (sd)
+		sd->s_mode = (attr->mode & S_IALLUGO) | S_IFREG;
+	else
+		error = -ENOMEM;
 	up(&dir->d_inode->i_sem);
 	return error;
 }
fs/sysfs/bin.c renamed from fs/sysfs/bin.c~sysfs-backing-store-stop-pinning +6 -35

53 lines not on this page · raw message

fs/sysfs/symlink.c renamed from fs/sysfs/symlink.c~sysfs-backing-store-stop-pinning +6 -34
diff -puN fs/sysfs/symlink.c~sysfs-backing-store-stop-pinning fs/sysfs/symlink.c
@@ -8,17 +8,11 @@
 
 #include "sysfs.h"
 
-static struct inode_operations sysfs_symlink_inode_operations = {
+struct inode_operations sysfs_symlink_inode_operations = {
 	.readlink = sysfs_readlink,
 	.follow_link = sysfs_follow_link,
 };
 
-static int init_symlink(struct inode * inode)
-{
-	inode->i_op = &sysfs_symlink_inode_operations;
-	return 0;
-}
-
 static int object_depth(struct kobject * kobj)
 {
 	struct kobject * p = kobj;

@@ -53,9 +47,8 @@ static void fill_object_path(struct kobj
 	}
 }
 
-static struct sysfs_dirent * 
-sysfs_add_link(struct sysfs_dirent * parent_sd, char * name, 
-		struct kobject * target)
+static int sysfs_add_link(struct sysfs_dirent * parent_sd, char * name,
+			    struct kobject * target)
 {
 	struct sysfs_dirent * sd;
 	struct sysfs_symlink * sl;

@@ -76,14 +69,14 @@ sysfs_add_link(struct sysfs_dirent * par
 	sd = sysfs_new_dirent(parent_sd, sl, SYSFS_KOBJ_LINK);
 	if (sd) {
 		sd->s_mode = S_IFLNK|S_IRWXUGO;
-		return sd;
+		return 0;
 	}
 
 	kfree(sl->link_name);
 exit2:
 	kfree(sl);
 exit1:
-	return ERR_PTR(error);
+	return error;
 }
 
 /**

@@ -95,34 +88,13 @@ exit1:
 int sysfs_create_link(struct kobject * kobj, struct kobject * target, char * name)
 {
 	struct dentry * dentry = kobj->dentry;
-	struct dentry * d;
 	int error = 0;
 
 	if (!name)
 		return -EINVAL;
 
 	down(&dentry->d_inode->i_sem);
-	d = sysfs_get_dentry(dentry,name);
-	if (!IS_ERR(d)) {
-		error = sysfs_create(d, S_IFLNK|S_IRWXUGO, init_symlink);
-		if (!error) {
-			struct sysfs_dirent * sd;
-			sd = sysfs_add_link(dentry->d_fsdata, name, target);
-			if (!IS_ERR(sd)) {
-				/* 
-			 	* associate the link dentry with the target  
-			 	* through the corresponding sysfs_dirent.
-			 	*/
-				d->d_fsdata = sd;
-				sd->s_dentry = dentry;
-			} else {
-				dput(d);
-				error = PTR_ERR(sd);
-			}
-		}
-		dput(d);
-	} else 
-		error = PTR_ERR(d);
+	error = sysfs_add_link(dentry->d_fsdata, name, target);
 	up(&dentry->d_inode->i_sem);
 	return error;
 }
fs/sysfs/sysfs.h renamed from fs/sysfs/sysfs.h~sysfs-backing-store-stop-pinning +4 -0
diff -puN fs/sysfs/sysfs.h~sysfs-backing-store-stop-pinning fs/sysfs/sysfs.h
@@ -20,6 +20,10 @@ extern int sysfs_follow_link(struct dent
 extern struct rw_semaphore sysfs_rename_sem;
 extern struct super_block * sysfs_sb;
 extern struct file_operations sysfs_dir_operations;
+extern struct file_operations sysfs_file_operations;
+extern struct file_operations bin_fops;
+extern struct inode_operations sysfs_dir_inode_operations;
+extern struct inode_operations sysfs_symlink_inode_operations;
 
 struct sysfs_symlink {
 	char * link_name;
fs/sysfs/mount.c renamed from fs/sysfs/mount.c~sysfs-backing-store-stop-pinning +1 -1
diff -puN fs/sysfs/mount.c~sysfs-backing-store-stop-pinning fs/sysfs/mount.c
@@ -42,7 +42,7 @@ static int sysfs_fill_super(struct super
 
 	inode = sysfs_new_inode(S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO);
 	if (inode) {
-		inode->i_op = &simple_dir_inode_operations;
+		inode->i_op = &sysfs_dir_inode_operations;
 		inode->i_fop = &sysfs_dir_operations;
 		/* directory inodes start off with i_nlink == 2 (for "." entry) */
 		inode->i_nlink++;

_

Signature · 6 lines
Maneesh Soni
Linux Technology Center, 
IBM Austin
email: [email protected]
Phone: 1-512-838-1896 Fax: 
T/L : 6781896
#6 lore raw

Re: [PATCH 5/5] Stop pinning dentries & inodes for leaves

Quoted 16 lines · Maneesh Soni

o This patch stops the pinning of non-directory or leaf dentries and inodes. The leaf dentries and inodes are created during lookup based on the entries on sysfs_dirent tree. These leaves are removed from the dcache through the VFS dentry ageing process during shrink dcache operations. Thus reducing about 80% of sysfs lowmem needs.

o This implments the ->lookup() for sysfs directory inodes and allocates dentry and inode if the lookup is successful and avoids the need of allocating and pinning of dentry and inodes during the creation of corresponding sysfs leaf entry. As of now the implementation has not required negative dentry creation on failed lookup. As sysfs is still a RAM based filesystem, negative dentries are not of any use IMO.

Quoted 4 lines

o The leaf dentry allocated after successful lookup is connected to the existing corresponding sysfs_dirent through the d_fsdata field. This increments the ref count of sysfs_dirent. The ref count is released through ->d_iput() dentry_operation when dentry dies.

ACK; there's some reordering that might make sense here, but that depends on what will fall out of changes in previous chunks.

#7 lore raw

Re: [PATCH 4/5] Change sysfs_file_operations

In reply to Maneesh Soni
Quoted 5 lines · Maneesh Soni

o This patch modifies the sysfs_file_operations methods so as to get the kobject and attribute pointers via corresponding sysfs_dirent.

Hmm... After looking at that, I'd probably add typed inlined helpers (dentry to attr, dentry to kobject, dentry to bin_attr) and converted to their use *before* everything else (patch #0). And change their definitions to use sysfs_dirent in patch #1. That way we avoid breakage on intermediate stages.

#8 lore raw

Re: [PATCH 3/5] Free sysfs_dirent on file removal

In reply to Maneesh Soni
Quoted 7 lines · Maneesh Soni

o The following patch implements the code to free up the sysfs_dirents upon directory or file removal. It uses the sysfs_dirent based tree in order to remove the directory contents before removing the directory itself. It could do this without taking dcache_lock in sysfs_remove_dir() as it doesnot use dentry based tree.

ACK, but some of that (freeing sysfs_dirent upon removal, no smarts, just enough to plug the leak) belongs in the first chunk.

Note that there we shouldn't care about refcounts - that stuff belongs here.

#9 lore raw

Re: [PATCH 2/5] Use sysfs_dirent tree for ->readdir etc.

In reply to Maneesh Soni
Quoted 8 lines · Maneesh Soni

o This patch implements the sysfs_dir_operations file_operations strucutre for sysfs directories. It uses the sysfs_dirent based tree for ->readdir() and ->lseek() methods instead of simple_dir_operations which use dentry based tree.

o This also has the code for sysfs_get() and sysfs_put() which manipulates the reference counting for for sysfs_dirents.

ACK, except that I'd move refcounting primitives to the chunk that actually uses them.

#10 lore raw

Re: [PATCH 1/5] Add sysfs_dirent to sysfs dentry

In reply to Maneesh Soni

fs/sysfs/dir.c static int create_dir(struct kobject * k · line 52

Quoted 5 lines · Maneesh Soni
fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs-backing-store-add-sysfs_dirent +5 -0
+			} else {
+				/* error, release the ref taken in
+				 * sysfs_create()
+				 */
+				dput(*d);

Umm... Shouldn't we unhash here? sysfs_create() did hash it, so... Same goes for other places where we fail to create sysfs_dirent; as the matter of fact, how about making
sysfs_make_dirent(dentry, mode, data, type)
that would either allocate sysfs_dirent and bind it to dentry, or unhash and dput() dentry and return error? AFAICS, that would make life easier.

fs/sysfs/file.c int sysfs_add_file(struct dentry * dir, · line 391

Quoted 6 lines

int sysfs_create_file(struct kobject * kobj, const struct attribute * attr) {

fs/sysfs/file.c renamed from fs/sysfs/file.c~sysfs-backing-store-add-sysfs_dirent +2 -2
-	if (kobj && attr)
-		return sysfs_add_file(kobj->dentry,attr);
+	if (kobj && kobj->dentry && attr)
+		return sysfs_add_file(kobj->dentry, attr, SYSFS_KOBJ_ATTR);

Can we legitimately get NULL kobj or kobj->dentry here? If not, that'd better be BUG_ON()...

fs/sysfs/symlink.c int sysfs_create_link(struct kobject * k · line 102

Quoted 6 lines
fs/sysfs/symlink.c renamed from fs/sysfs/symlink.c~sysfs-backing-store-add-sysfs_dirent +2 -0
@@ -65,15 +98,28 @@ int sysfs_create_link(struct kobject * k
 	struct dentry * d;
 	int error = 0;
 
+	if (!name)
+		return -EINVAL;

Again, can that happen legitimately?

Quoted 13 lines
fs/sysfs/symlink.c renamed from fs/sysfs/symlink.c~sysfs-backing-store-add-sysfs_dirent +4 -5
 	down(&dentry->d_inode->i_sem);
 	d = sysfs_get_dentry(dentry,name);
 	if (!IS_ERR(d)) {
 		error = sysfs_create(d, S_IFLNK|S_IRWXUGO, init_symlink);
-		if (!error)
-			/* 
-			 * associate the link dentry with the target kobject 
-			 */
-			d->d_fsdata = kobject_get(target);
+		if (!error) {
+			struct sysfs_dirent * sd;
+			sd = sysfs_add_link(dentry->d_fsdata, name, target);
+			if (!IS_ERR(sd)) {

fs/sysfs/symlink.c int sysfs_create_link(struct kobject * k · line 121

Quoted 10 lines
fs/sysfs/symlink.c renamed from fs/sysfs/symlink.c~sysfs-backing-store-add-sysfs_dirent +10 -0
+				/* 
+			 	* associate the link dentry with the target  
+			 	* through the corresponding sysfs_dirent.
+			 	*/
+				d->d_fsdata = sd;
+				sd->s_dentry = dentry;
+			} else {
+				dput(d);
+				error = PTR_ERR(sd);
+			}

I'd pull that inside sysfs_add_link() (and then further into
sysfs_new_dirent()).