Configfs is really sysfs

Hi Andrew,

Configfs blithely ingests kobject.h and kobject.c into itself, just changing the names. Furthermore, more than half of configfs is copied verbatim from sysfs, the only difference being the name changes. After undoing the name changes and adding a few new fields to kobject structures, configfs is able to use the real thing instead of its own imitation.

The changes I made to kobject.h and sysfs.h are:

* add module owner to kobj_type.

* add group_operations to kobj_type (because configfs does it this way not because it is right)

* add a children field to kset. This is likely the same as the blandly named "list" field but I haven't confirmed it.

* add a default_groups field to kset, analogous to the default_attrs of kobj_type. Hmm, somebody seems to be mixing up types and containers here, but let's just close our eyes for now.

* add an s_links field to sysfs_dirent to support configfs's user createable symlinks.

* add two new methods to sysfs_ops for fancy symlink hooks

* add a questionable release method to sysfs_ops. Sysfs and configfs have slightly different notions of when to release objects, one of them is probably wrong.

That's it, no new fields in kobjects themselves, and just three or four fields
in other allocateable structures. After these changes, no structures at all
are left in configfs.h. Configfs is now running happily using the kobject machinery instead of its own mutated clones and unsurprisingly, sysfs still runs happily too. These changes are all found in the first patch of this series.

I then looked into exactly how configfs and sysfs are different. To reduce the noise, I concatentated all the files in each directory into two single files. With redundant declarations removed, configfs came in at 1897 lines and sysfs at 1680. Diffing those two files shows:

diff -u fs/sysfs/sysfs.c fs/configfs/configfs.c | diffstat configfs.c | 1497
++++++++++++++++++++++++++++++++++---------------------------

 1 files changed, 857 insertions(+), 640 deletions(-)

So we see that two thirds of sysfs made it into configfs unchanged. Of the remaining one third that configfs has not copied, about one third supports read/write/mmappable attribute files (why should configfs not have them too?), a little less than a third involves needlessly importing its own version of setattr, and the remainder, about 300 lines, exports the kernel interface for manipulating the user-visible sysfs tree.

Allowing for a few lines of fluff, configfs's value add is about 750 lines of user space glue for namespace operations. Nothing below that glue layer is changed, except cosmetically. So configfs really is sysfs. By adding about 300 lines to configfs we can add the vfs-bypass code, and voila, configfs becomess sysfs. Another 200 lines gives us the binary blob attributes as well. There is no reason whatsover for configfs and sysfs to live on as separate code bases. If we really want to make a distinction, we can make the distinction with a flag.

But it would be stupid to forbid users from creating directories in sysfs or to forbid kernel modules from directly tweaking a configfs namespace. Why should the kernel not be able to add objects to a directory a user created? It should be up to the module author to decide these things.

Please do not push configfs to stable in this form. It is not actually a new filesystem, it is an extension to sysfs. Merging it as is would add more than a thousand lines of pointless kernel bloat. If indeed we wish to present exactly the semantics configfs now offers, we do not need a separate code base to do so.

The four patches in this patch set:

1) Add new fields to kobjects; update other headers to match
2) Sysfs all in one file
3) Configfs all in one file
4) A configfs kernel example using sysfs instead of configfs structures

Regards,

Daniel

diff -up --recursive 2.6.13-rc5-mm1.clean/include/linux/configfs.h 2.6.13-rc5-mm1/include/linux/configfs.h
@@ -46,120 +46,32 @@
 
 #define CONFIGFS_ITEM_NAME_LEN	20
 
-struct module;
-
-struct configfs_item_operations;
-struct configfs_group_operations;
-struct configfs_attribute;
-struct configfs_subsystem;
-
-struct config_item {
-	char			*ci_name;
-	char			ci_namebuf[CONFIGFS_ITEM_NAME_LEN];
-	struct kref		ci_kref;
-	struct list_head	ci_entry;
-	struct config_item	*ci_parent;
-	struct config_group	*ci_group;
-	struct config_item_type	*ci_type;
-	struct dentry		*ci_dentry;
-};
-
-extern int config_item_set_name(struct config_item *, const char *, ...);
-
-static inline char *config_item_name(struct config_item * item)
-{
-	return item->ci_name;
-}
-
-extern void config_item_init(struct config_item *);
-extern void config_item_init_type_name(struct config_item *item,
-				       const char *name,
-				       struct config_item_type *type);
-extern void config_item_cleanup(struct config_item *);
-
-extern struct config_item * config_item_get(struct config_item *);
-extern void config_item_put(struct config_item *);
-
-struct config_item_type {
-	struct module				*ct_owner;
-	struct configfs_item_operations		*ct_item_ops;
-	struct configfs_group_operations	*ct_group_ops;
-	struct configfs_attribute		**ct_attrs;
-};
-
+extern void kobject_init_type_name(struct kobject *item, const char *name, struct kobj_type *type);
 
 /**
- *	group - a group of config_items of a specific type, belonging
+ *	group - a group of kobjects of a specific type, belonging
  *	to a specific subsystem.
  */
 
-struct config_group {
-	struct config_item		cg_item;
-	struct list_head		cg_children;
-	struct configfs_subsystem 	*cg_subsys;
-	struct config_group		**default_groups;
-};
-
+extern void config_group_init(struct kset *group);
+extern void config_group_init_type_name(struct kset *group, const char *name, struct kobj_type *type);
 
-extern void config_group_init(struct config_group *group);
-extern void config_group_init_type_name(struct config_group *group,
-					const char *name,
-					struct config_item_type *type);
-
-
-static inline struct config_group *to_config_group(struct config_item *item)
+static inline struct kset *to_config_group(struct kobject *item)
 {
-	return item ? container_of(item,struct config_group,cg_item) : NULL;
+	return item ? container_of(item,struct kset,kobj) : NULL;
 }
 
-static inline struct config_group *config_group_get(struct config_group *group)
+static inline struct kset *config_group_get(struct kset *group)
 {
-	return group ? to_config_group(config_item_get(&group->cg_item)) : NULL;
+	return group ? to_config_group(kobject_get(&group->kobj)) : NULL;
 }
 
-static inline void config_group_put(struct config_group *group)
+static inline void config_group_put(struct kset *group)
 {
-	config_item_put(&group->cg_item);
+	kobject_put(&group->kobj);
 }
 
-extern struct config_item *config_group_find_obj(struct config_group *, const char *);
-
-
-struct configfs_attribute {
-	char			*ca_name;
-	struct module 		*ca_owner;
-	mode_t			ca_mode;
-};
-
-
-/*
- * If allow_link() exists, the item can symlink(2) out to other
- * items.  If the item is a group, it may support mkdir(2).
- * Groups supply one of make_group() and make_item().  If the
- * group supports make_group(), one can create group children.  If it
- * supports make_item(), one can create config_item children.  If it has
- * default_groups on group->default_groups, it has automatically created
- * group children.  default_groups may coexist alongsize make_group() or
- * make_item(), but if the group wishes to have only default_groups
- * children (disallowing mkdir(2)), it need not provide either function.
- * If the group has commit(), it supports pending and commited (active)
- * items.
- */
-struct configfs_item_operations {
-	void (*release)(struct config_item *);
-	ssize_t	(*show_attribute)(struct config_item *, struct configfs_attribute *,char *);
-	ssize_t	(*store_attribute)(struct config_item *,struct configfs_attribute *,const char *, size_t);
-	int (*allow_link)(struct config_item *src, struct config_item *target);
-	int (*drop_link)(struct config_item *src, struct config_item *target);
-};
-
-struct configfs_group_operations {
-	struct config_item *(*make_item)(struct config_group *group, const char *name);
-	struct config_group *(*make_group)(struct config_group *group, const char *name);
-	int (*commit_item)(struct config_item *item);
-	void (*drop_item)(struct config_group *group, struct config_item *item);
-};
-
+extern struct kobject *config_group_find_obj(struct kset *, const char *);
 
 
 /**

@@ -185,20 +97,13 @@ struct configfs_group_operations {
 #endif
 
 
-struct configfs_subsystem {
-	struct config_group	su_group;
-	struct semaphore	su_sem;
-};
-
-static inline struct configfs_subsystem *to_configfs_subsystem(struct config_group *group)
+static inline struct subsystem *to_configfs_subsystem(struct kset *kset)
 {
-	return group ?
-		container_of(group, struct configfs_subsystem, su_group) :
-		NULL;
+	return kset ? container_of(kset, struct subsystem, kset) : NULL;
 }
 
-int configfs_register_subsystem(struct configfs_subsystem *subsys);
-void configfs_unregister_subsystem(struct configfs_subsystem *subsys);
+int configfs_register_subsystem(struct subsystem *subsys);
+void configfs_unregister_subsystem(struct subsystem *subsys);
 
 #endif  /* __KERNEL__ */
 

diff -up --recursive 2.6.13-rc5-mm1.clean/include/linux/kobject.h 2.6.13-rc5-mm1/include/linux/kobject.h
@@ -69,7 +69,9 @@ extern char * kobject_get_path(struct ko
 
 struct kobj_type {
 	void (*release)(struct kobject *);
+	struct module		*ct_owner;
 	struct sysfs_ops	* sysfs_ops;
+	struct configfs_group_operations *ct_group_ops;
 	struct attribute	** default_attrs;
 };
 

@@ -106,6 +108,10 @@ struct kset {
 	spinlock_t		list_lock;
 	struct kobject		kobj;
 	struct kset_hotplug_ops	* hotplug_ops;
+
+	/* configfs fields: merge us! */
+	struct list_head	cg_children;
+	struct kset 		**default_groups;
 };
 
 

@@ -144,7 +150,7 @@ extern struct kobject * kset_find_obj(st
  * Use this when initializing an embedded kset with no other 
  * fields to initialize.
  */
-#define set_kset_name(str)	.kset = { .kobj = { .name = str } }
+#define set_kset_name(str)	.kset = { .kobj = { .k_name = str } }
 
 
 

@@ -156,7 +162,7 @@ struct subsystem {
 #define decl_subsys(_name,_type,_hotplug_ops) \
 struct subsystem _name##_subsys = { \
 	.kset = { \
-		.kobj = { .name = __stringify(_name) }, \
+		.kobj = { .k_name = __stringify(_name) }, \
 		.ktype = _type, \
 		.hotplug_ops =_hotplug_ops, \
 	} \

@@ -164,7 +170,7 @@ struct subsystem _name##_subsys = { \
 #define decl_subsys_name(_varname,_name,_type,_hotplug_ops) \
 struct subsystem _varname##_subsys = { \
 	.kset = { \
-		.kobj = { .name = __stringify(_name) }, \
+		.kobj = { .k_name = __stringify(_name) }, \
 		.ktype = _type, \
 		.hotplug_ops =_hotplug_ops, \
 	} \

diff -up --recursive 2.6.13-rc5-mm1.clean/include/linux/sysfs.h 2.6.13-rc5-mm1/include/linux/sysfs.h
@@ -12,6 +12,7 @@
 
 #include <asm/atomic.h>
 
+struct kset;
 struct kobject;
 struct module;
 

@@ -63,12 +64,36 @@ struct bin_attribute {
 struct sysfs_ops {
 	ssize_t	(*show)(struct kobject *, struct attribute *,char *);
 	ssize_t	(*store)(struct kobject *,struct attribute *,const char *, size_t);
+	int (*allow_link)(struct kobject *src, struct kobject *target);
+	int (*drop_link)(struct kobject *src, struct kobject *target);
+	void (*release)(struct kobject *);
+};
+
+/*
+ * If allow_link() exists, the item can symlink(2) out to other
+ * items.  If the item is a group, it may support mkdir(2).
+ * Groups supply one of make_group() and make_item().  If the
+ * group supports make_group(), one can create group children.  If it
+ * supports make_item(), one can create kobject children.  If it has
+ * default_groups on group->default_groups, it has automatically created
+ * group children.  default_groups may coexist alongsize make_group() or
+ * make_item(), but if the group wishes to have only default_groups
+ * children (disallowing mkdir(2)), it need not provide either function.
+ * If the group has commit(), it supports pending and commited (active)
+ * items.
+ */
+struct configfs_group_operations {
+	struct kobject *(*make_item)(struct kset *group, const char *name);
+	struct kset *(*make_group)(struct kset *group, const char *name);
+	int (*commit_item)(struct kobject *item);
+	void (*drop_item)(struct kset *group, struct kobject *item);
 };
 
 struct sysfs_dirent {
 	atomic_t		s_count;
 	struct list_head	s_sibling;
 	struct list_head	s_children;
+	struct list_head	s_links; /* configfs */
 	void 			* s_element;
 	int			s_type;
 	umode_t			s_mode;

diff -up --recursive 2.6.13-rc5-mm1.clean/lib/kobject.c 2.6.13-rc5-mm1/lib/kobject.c
@@ -344,7 +344,9 @@ void kobject_cleanup(struct kobject * ko
 		kfree(kobj->k_name);
 	kobj->k_name = NULL;
 	if (t && t->release)
-		t->release(kobj);
+		t->release(kobj); /* which one of us... */
+	if (t && t->sysfs_ops && t->sysfs_ops->release)
+		t->sysfs_ops->release(kobj); /* ...is bogus? */
 	if (s)
 		kset_put(s);
 	if (parent) 

Sysfs rearranged as a single file for analysis purposes.

diff -up --recursive 2.6.13-rc5-mm1.clean/fs/sysfs/Makefile 2.6.13-rc5-mm1/fs/sysfs/Makefile
@@ -2,5 +2,4 @@
 # Makefile for the sysfs virtual filesystem
 #
 
-obj-y  := inode.o file.o dir.o symlink.o mount.o bin.o \
-     group.o
+obj-y := sysfs.o

Configfs rewritten as a single file and updated to use kobjects instead of its own clone of kobjects (config_items).

diff -up --recursive 2.6.13-rc5-mm1.clean/fs/configfs/Makefile 2.6.13-rc5-mm1/fs/configfs/Makefile
@@ -2,6 +2,5 @@
 # Makefile for the configfs virtual filesystem
 #
 
-obj-$(CONFIG_CONFIGFS_FS) += configfs.o
+obj-$(CONFIG_CONFIGFS_FS) += configfs.o ddbond.config.o
 
-configfs-objs := inode.o file.o dir.o symlink.o mount.o item.o

A kernel code example that uses the modified configfs to define a simple configuration interface. Note the use of kobjects and ksets instead of config_items and config_groups.

Also notice how much code is required to get a simple value from userspace to kernel space. This is a big problem that needs to be addressed soon, before we end up with tens or hundreds of thousands of lines of code code bloat just to get and set variables from user space.

Regards,

Daniel

#include <linux/init.h>
#include <linux/module.h>
#include <linux/slab.h>

#include <linux/configfs.h>

struct ddbond_info {
struct kobject item;
int controlsock;
};

static inline struct ddbond_info *to_ddbond_info(struct kobject *item) {
return container_of(item, struct ddbond_info, item);
}

static ssize_t ddbond_info_attr_show(struct kobject *item,
struct attribute *attr, char *page)
{
ssize_t count;
struct ddbond_info *ddbond_info = to_ddbond_info(item); count = sprintf(page, "%d\n", ddbond_info->controlsock); return count;
}

static ssize_t ddbond_info_attr_store(struct kobject *item,
struct attribute *attr, const char *page, size_t count)
{
struct ddbond_info *ddbond_info = to_ddbond_info(item); unsigned long tmp;
char *p = (char *)page;

tmp = simple_strtoul(p, &p, 10);
if (!p || (*p && (*p != '\n')))
return -EINVAL;
if (tmp > INT_MAX)
return -ERANGE;
ddbond_info->controlsock = tmp;
return count;
}

static void ddbond_info_release(struct kobject *item) {
kfree(to_ddbond_info(item));
}

static struct kobj_type ddbond_info_type = {
.sysfs_ops = &(struct sysfs_ops){
.show = ddbond_info_attr_show,
.store = ddbond_info_attr_store,
.release = ddbond_info_release,
},
.default_attrs = (struct attribute *[]){
&(struct attribute){
.owner = THIS_MODULE,
.name = "sockname",
.mode = S_IRUGO | S_IWUSR,
},
NULL,
},
.ct_owner = THIS_MODULE,
};

static struct kobject *ddbond_make_item(struct kset *group, const char *name) {
struct ddbond_info *ddbond_info;
if (!(ddbond_info = kcalloc(1, sizeof(struct ddbond_info), GFP_KERNEL)))
return NULL;
kobject_init_type_name(&ddbond_info->item, name, &ddbond_info_type); return &ddbond_info->item;
}

static ssize_t ddbond_description(struct kobject *item,
struct attribute *attr, char *page)
{
return sprintf(page,
"A ddbond block server has two components: a userspace server and a kernel\n" "io daemon. First start the server and give it the name of a socket it will\n" "create, then create a ddbond directory and write the same name into the\n" "socket attribute\n");
}

static struct kobj_type ddbond_type = {
.sysfs_ops = &(struct sysfs_ops){
.show = ddbond_description,
},
.ct_group_ops = &(struct configfs_group_operations){
.make_item = ddbond_make_item,
},
.default_attrs = (struct attribute *[]){
&(struct attribute){
.owner = THIS_MODULE,
.name = "description",
.mode = S_IRUGO,
},
NULL,
}
};

static struct subsystem ddbond_subsys = {
.kset = {
.kobj = {
.k_name = "ddbond",
.ktype = &ddbond_type,
},
},
};

static int __init init_ddbond_config(void)
{
int ret;
config_group_init(&ddbond_subsys.kset); init_rwsem(&ddbond_subsys.rwsem);
if ((ret = configfs_register_subsystem(&ddbond_subsys)))
printk(KERN_ERR "Error %d while registering subsystem %s\n",

         ret, ddbond_subsys.kset.kobj.k_name);

return ret;
}

static void __exit exit_ddbond_config(void)
{
configfs_unregister_subsystem(&ddbond_subsys);
}

module_init(init_ddbond_config);
module_exit(exit_ddbond_config);
MODULE_LICENSE("GPL");

(without kmail bugs this time)

A kernel code example that uses the modified configfs to define a simple configuration interface. Note the use of kobjects and ksets instead of config_items and config_groups.

Also notice how much code is required to get a simple value from userspace to kernel space. This is a big problem that needs to be addressed soon, before we end up with tens or hundreds of thousands of lines of code code bloat just to get and set variables from user space.

Regards,

Daniel

#include <linux/init.h>
#include <linux/module.h>
#include <linux/slab.h>

#include <linux/configfs.h>

struct ddbond_info {
struct kobject item;
int controlsock;
};

static inline struct ddbond_info *to_ddbond_info(struct kobject *item) {
return container_of(item, struct ddbond_info, item);
}

static ssize_t ddbond_info_attr_show(struct kobject *item,
struct attribute *attr, char *page)
{
ssize_t count;
struct ddbond_info *ddbond_info = to_ddbond_info(item); count = sprintf(page, "%d\n", ddbond_info->controlsock); return count;
}

static ssize_t ddbond_info_attr_store(struct kobject *item,
struct attribute *attr, const char *page, size_t count)
{
struct ddbond_info *ddbond_info = to_ddbond_info(item); unsigned long tmp;
char *p = (char *)page;

tmp = simple_strtoul(p, &p, 10);
if (!p || (*p && (*p != '\n')))
return -EINVAL;
if (tmp > INT_MAX)
return -ERANGE;
ddbond_info->controlsock = tmp;
return count;
}

static void ddbond_info_release(struct kobject *item) {
kfree(to_ddbond_info(item));
}

static struct kobj_type ddbond_info_type = {
.sysfs_ops = &(struct sysfs_ops){
.show = ddbond_info_attr_show,
.store = ddbond_info_attr_store,
.release = ddbond_info_release,
},
.default_attrs = (struct attribute *[]){
&(struct attribute){
.owner = THIS_MODULE,
.name = "sockname",
.mode = S_IRUGO | S_IWUSR,
},
NULL,
},
.ct_owner = THIS_MODULE,
};

static struct kobject *ddbond_make_item(struct kset *group, const char *name) {
struct ddbond_info *ddbond_info;
if (!(ddbond_info = kcalloc(1, sizeof(struct ddbond_info), GFP_KERNEL)))
return NULL;
kobject_init_type_name(&ddbond_info->item, name, &ddbond_info_type); return &ddbond_info->item;
}

static ssize_t ddbond_description(struct kobject *item,
struct attribute *attr, char *page)
{
return sprintf(page,
"A ddbond block server has two components: a userspace server and a kernel\n" "io daemon. First start the server and give it the name of a socket it will\n" "create, then create a ddbond directory and write the same name into the\n" "socket attribute\n");
}

static struct kobj_type ddbond_type = {
.sysfs_ops = &(struct sysfs_ops){
.show = ddbond_description,
},
.ct_group_ops = &(struct configfs_group_operations){
.make_item = ddbond_make_item,
},
.default_attrs = (struct attribute *[]){
&(struct attribute){
.owner = THIS_MODULE,
.name = "description",
.mode = S_IRUGO,
},
NULL,
}
};

static struct subsystem ddbond_subsys = {
.kset = {
.kobj = {
.k_name = "ddbond",
.ktype = &ddbond_type,
},
},
};

static int __init init_ddbond_config(void)
{
int ret;
config_group_init(&ddbond_subsys.kset); init_rwsem(&ddbond_subsys.rwsem);
if ((ret = configfs_register_subsystem(&ddbond_subsys)))
printk(KERN_ERR "Error %d while registering subsystem %s\n",

         ret, ddbond_subsys.kset.kobj.k_name);

return ret;
}

static void __exit exit_ddbond_config(void)
{
configfs_unregister_subsystem(&ddbond_subsys);
}

module_init(init_ddbond_config);
module_exit(exit_ddbond_config);
MODULE_LICENSE("GPL");

Some style issues:
Mixed case in labels
Bad identation

I certainly agree. This is strictly for comparison purposes and so I did not clean up the stylistic problems from the original... this time.

I did lindent it however :-)

It shall die.

Urk. Kmail did that to me, it has been broken that way for a year or so. I will have to repost the whole set from a mailer that works.

Regards,

Daniel

(avoiding the kmail formatting problems this time.)

Sysfs rearranged as a single file for analysis purposes.

diff -up --recursive 2.6.13-rc5-mm1.clean/fs/sysfs/Makefile 2.6.13-rc5-mm1/fs/sysfs/Makefile
@@ -2,5 +2,4 @@
 # Makefile for the sysfs virtual filesystem
 #
 
-obj-y		:= inode.o file.o dir.o symlink.o mount.o bin.o \
-		   group.o
+obj-y := sysfs.o

This is precisely why configfs is separate from sysfs. If both
user and kernel can create objects, the lifetime of the object and its filesystem representation is very complex. Sysfs already has problems with people getting this wrong. configfs does not.
The fact that sysfs and configfs have similar backing stores
does not make them the same thing.

Joel

"Against stupidity the Gods themselves contend in vain."
	- Freidrich von Schiller

			http://www.jlbec.org/
			[email protected]

Could you please give a specific case?

It is not just the backing store, it is most of the code, all the structures, most of the functionality, a good deal of the bugs...

Regards,

Daniel

More to the point: what makes you think that this apparent ruggedness will diminish after being re-integrated with sysfs? If you wish, you can avoid any dangers by not using sysfs's vfs bypass api. It should be up to the subsystem author.

Regards,

Daniel

Sure, but all that copying-and-pasting really sucks. I'm sure there's some way of providing the slightly different semantics from the same codebase?

Careful - you've almost reinvented the concept of library, which would violate any number of patents...

I will keep my eyes open for library candidates as I go. For example, the binary blob operations really cry out for it.

Regards,

Daniel

I will have that patch ready later this week.

Regards,

Daniel

The way that configfs and sysfs create/destroy dentries and
their associated inodes is very different from the top, yet similar from the bottom. I suspect that some of it could be libraryized. When I first looked started configfs, I was starting from an "add on to sysfs" perspective, after all. The sysfs maintainers and I agreed, after much discussion, that we should go to a separate tree.

Joel

"Here's a nickle -- get yourself a better X server."
	- Keith Packard

			http://www.jlbec.org/
			[email protected]

First, let's look at sharing the primary structures.

[kobject vs config_item]

When I first started configfs, I tried to make it part of sysfs.
That was obviously a bad idea before I got very far at all (more on this as we go along). So I forked the codebase. I wanted, however, to preserve kobject and kset, because I figured we didn't need another kernel object type. For the longest time usysfs (user-sysfs, as it was originally called) used kobjects and jumped through hoops to make it work.
The base structures, kobject and config_item, are indeed
identical in physical layout. But in use, they are very different. A kobject, when created, is expected to appear sysfs. You mistakenly kobject_add(), whoops, there it is. kobject_init() tries to link a kobject to the kobject->kset pointer, because it's expecting the kernel to be handling the relationship.
In configfs, the relationship is handled by the VFS. The
linkage is purposefully controlled by the ->mkdir() codepath. The client driver using this relationship can follow it, but never modify it or call anything that modifies it.
In the earlier configfs incarnations that used kobject/kset, I
found myself having to fool the kobject infrastructure to get kobject->kset correct. In addition, it was a complexity I had to always think about. In configfs with config_item, it's not an issue because it doesn't exist.

[kobj_type vs config_item_type]

kobj_type has three pointers. A release function for the
object, a set of sysfs_ops for attribute functions, and a set of default_attrs.
config_item_type needs to know the module owner to pin the
client module. It has 5 item operations (this includes the parallels for sysfs_ops) and 4 group operations (this is for config_group, the parallel of kset). Because operation structures are often shared, these are made into structures.
kobj_type would have to know all the features of
config_item_type, even though it is useless stuff for sysfs. Waste of memory and code.

[subsystem vs configfs_subsystem]

The sysfs folks are trying to lose the semaphore from a sysfs
subsystem, because it makes sense to have locking that is finer grained. It should apply to the granularity of the specific device or so.
configfs_subsystem needs the semaphore, because it protects only
one thing: the config_group/config_item hierarchy. When a client module wants to navigate the tree of its groups and items, it merely takes the semaphore and navigates. Once it finds and grabs a kref to what it needs, it can drop it. There is no need for any more complex locking.

[kset vs config_group]

Here is where it gets really different. A kset may or may not
represent a directory exactly. kobjects can be part of a kset they aren't under in the sysfs tree. configfs_items must be part of the parent config_group, becuase the VFS tree (the dcache) is the exact arbiter of the relationship.
Then there is the physical structure. A kset contains a pointer
to a kobj_type that its children will be assigned. This field is useless in configfs, and more importantly, it could result in an assignemnt that isn't related to the real object type, becuase of kset's ability to have non-children members. This adds more complexity to the code, as you have to handle, prevent, or kludge around this case.
kset contains a lock around its list of children. configfs
doesn't need this because the subsystem lock is protecting the hierarchy here.
kset contains a hotplug ops pointer. configfs has nothing to do
with hotplug. So here are three pointers any configfs object has to carry around for no reason other than sharing a structure.
Conversely, a config_group contains a list of default subgroups.
They are not attributes, they are groups. This is not the same as config_item's default_attrs. Here, kset would carry around an extra pointer for every object.
Given the stress everyone places on the memory usage of these
objects, adding useless pointers to both sides just to share the structure seems a very bad idea.

"Well-timed silence hath more eloquence than speech."  
         - Martin Fraquhar Tupper

Joel Becker
Senior Member of Technical Staff
Oracle
E-mail: [email protected]
Phone: (650) 506-8127

What about the backing store? Specifically, sysfs_dirent vs
configfs_dirent.
The structures are almost identical. What's different?
configfs has a list of symlinks, as these are hard linkages and involve pinning and reference counting. So, to merge the structures, you have to add two pointers (a list_head) to every sysfs object.
Allocating, initializing, and freeing them really does appear to
be virtually identical. The functions that call the creation are very different, but they could call the same thing. There are more types of things in configfs, so all shared calls would have to be able to handle them.
Oh, but the get_name() functions, the one that return the string
name of a _dirent, are very different. So you'd have to add another pointer to the structure, a ->get_name() callback. That's an additional pointer for every sysfs object.
The attach_attr() functions are different. Some of that is the
BIN_ATTR type of sysfs, which configfs doesn't and shouldn't have. In that case, the code still works, as BIN_ATTR test wouldn't succeed. They configure dentry_ops, which are different in sysfs and configfs. So the API would have to change to specify the appropriate dentry_ops.
This is certainly not insurmountable. I don't know what you'd
call it, fs/libfs/backing_store.c? I'm interested in what the sysfs folks have to say on this, and how much they'd like to help.

Joel

A good programming language should have features that make the
kind of people who use the phrase "software engineering" shake
their heads disapprovingly.
	- Paul Graham

Joel Becker
Senior Member of Technical Staff
Oracle
E-mail: [email protected]
Phone: (650) 506-8127

The final piece of similar code is the buffered I/O setup for
attribute files. Here, the major difference is how config_items and kobjects refer to their show/store operations. The functions have different rules on this.
A kobject doesn't need show/store, it can provide one for the
entire kset or subsystem. A config_item necessarily has one for its type, and cannot chain up. So somehow the code would need to know which was the case.
A kobject directly has a sysfs_ops structure. The config_item
has show/store in a config_item_operations strucutre. If you split them out, you add a pointer and some needless complexity. So the different code paths need to refer to the functions differently.
The attribute and configfs_attribute structures are physically
identical, but the sysfs one has poor naming (IMHO). If struct attribute wasn't going to change, and no magic would be added to its usage (magic like kobject_add()'s intertwining with sysfs), they could probably be shared.

Joel

"The doctrine of human equality reposes on this: that there is no
 man really clever who has not found that he is stupid."
	- Gilbert K. Chesterson

Joel Becker
Senior Member of Technical Staff
Oracle
E-mail: [email protected]
Phone: (650) 506-8127

So, what conclusions can we draw from looking at this again? First, merging the kobject/kset structures with the
config_item/config_group structures has one benefit: they have the same name. However, that's also the first problem. As it currently stands, you know from the name what you are working with. If they have the same name, you don't know if you are working with an object in sysfs or configfs. We call a pid "pid_t" rather than "unsigned int" for the same reason, clarity of usage.
Merging them has a lot of other problems. Far from removing
code bloat (a configfs object and a sysfs object need different initialization, etc, so there would be two paths anyway), it adds significant memory bloat, as configfs objects and sysfs objects are carrying around members for the other filesystem. Finally, there are surprises in store if you try to use some of the API that isn't appropriate. With a different name and type, you just _can't_ do that.
We could perhaps share the attribute structure. This has the
same name confusion problem above, but that's about it, assuming that sysfs never wants to do anything smarter with them. I'm hoping the sysfs folks comment on this.
Sharing the attribute file code is much harder. They operate on
different types, and those types are laid out differently. I think we'd have to make the API far more invasive. Instead of black-boxing the API and having only show/store to implement, we'd have to provide some sort of translation from the native structure to a shared type. Basically, repeating what the VFS has already done for us.
Finally, assuming that the sysfs_dirent/configfs_dirent
arrangement is pretty fleshed out, I think that perhaps this backing store could be joined. Again, no more magic could be added, and it would have to handle the sysfs and configfs types in concurrence, but I think it could be done. Again, sysfs folks, please comment.

Joel

"Hell is oneself, hell is alone, the other figures in it, merely projections."
        - T. S. Eliot

Joel Becker
Senior Member of Technical Staff
Oracle
E-mail: [email protected]
Phone: (650) 506-8127

Thanks for the good explainations. I still agree with Joel that these need to be separate fses.

thanks,

greg k-h