Skip to contentSkip to conversation

configfs: implement committable items and add sample code

PATCH Superseded Bartosz Golaszewski 2 participants 9 messages Nov 30, 2020 – Dec 1, 2020

Subsystems (2)

v2 of 8 Superseded Latest is v8 →

lore
Share & tools
lore
#0 lore raw

v2 cover configfs: implement committable items and add sample code

From: Bartosz Golaszewski <[email protected]>

Committable items in configfs are well defined and documented but unfortunately so far never implemented.

The use-case we have over at the GPIO subsystem is using configfs in conjunction with sysfs to replace our current gpio-mockup testing module with one that will be much more flexible and will allow complete coverage of the GPIO uAPI.

The current gpio-mockup module is controlled using module parameters which forces the user to reload it everytime they need to change the chip configuration or layout and makes it difficult to extend its functionality.

Testing module based on configfs would allow fine-grained control over dummy GPIO chips but since GPIO devices must be configured before they are instantiated, we need committable items.

This implements them and adds code examples to configfs_sample module. The first two patches are just cosmetic.

v1 -> v2:
- fix a 'set but not used' build warning reported by kernel test robot

Bartosz Golaszewski (4):
configfs: increase the item name length
configfs: use BIT() for internal flags
configfs: implement committable items
samples: configfs: add a committable group

 Documentation/filesystems/configfs.rst |   6 +-
 fs/configfs/configfs_internal.h        |  22 +--
 fs/configfs/dir.c                      | 237 ++++++++++++++++++++++++-
 fs/configfs/file.c                     |   8 +
 include/linux/configfs.h               |   3 +-
 samples/configfs/configfs_sample.c     | 150 ++++++++++++++++
 6 files changed, 406 insertions(+), 20 deletions(-)
Signature · 1 line
2.29.1
#1 lore raw

PATCH v2 1/4 configfs: increase the item name length

From: Bartosz Golaszewski <[email protected]>

20 characters limit for item name is relatively small. Let's increase it to 32 to fit '04-committable-children' - a name we'll use in the sample code for committable items.

---
 include/linux/configfs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/configfs.h +1 -1
index 2e8c69b43c64..4f76dcc08134 100644
@@ -27,7 +27,7 @@
 #include <linux/kref.h>   /* struct kref */
 #include <linux/mutex.h>  /* struct mutex */
 
-#define CONFIGFS_ITEM_NAME_LEN	20
+#define CONFIGFS_ITEM_NAME_LEN	32
 
 struct module;
 
Signature · 1 line
2.29.1
#2 lore raw

PATCH v2 2/4 configfs: use BIT() for internal flags

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

For better readability and maintenance: use the BIT() macro for flag definitions.

---
 fs/configfs/configfs_internal.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)
fs/configfs/configfs_internal.h +10 -10
index 22dce2d35a4b..855f00868654 100644
@@ -46,16 +46,16 @@ struct configfs_dirent {
 	struct configfs_fragment *s_frag;
 };
 
-#define CONFIGFS_ROOT		0x0001
-#define CONFIGFS_DIR		0x0002
-#define CONFIGFS_ITEM_ATTR	0x0004
-#define CONFIGFS_ITEM_BIN_ATTR	0x0008
-#define CONFIGFS_ITEM_LINK	0x0020
-#define CONFIGFS_USET_DIR	0x0040
-#define CONFIGFS_USET_DEFAULT	0x0080
-#define CONFIGFS_USET_DROPPING	0x0100
-#define CONFIGFS_USET_IN_MKDIR	0x0200
-#define CONFIGFS_USET_CREATING	0x0400
+#define CONFIGFS_ROOT			BIT(0)
+#define CONFIGFS_DIR			BIT(1)
+#define CONFIGFS_ITEM_ATTR		BIT(2)
+#define CONFIGFS_ITEM_BIN_ATTR		BIT(3)
+#define CONFIGFS_ITEM_LINK		BIT(5)
+#define CONFIGFS_USET_DIR		BIT(6)
+#define CONFIGFS_USET_DEFAULT		BIT(7)
+#define CONFIGFS_USET_DROPPING		BIT(8)
+#define CONFIGFS_USET_IN_MKDIR		BIT(9)
+#define CONFIGFS_USET_CREATING		BIT(10)
 #define CONFIGFS_NOT_PINNED	(CONFIGFS_ITEM_ATTR | CONFIGFS_ITEM_BIN_ATTR)
 
 extern struct mutex configfs_symlink_mutex;
Signature · 1 line
2.29.1
#3 lore raw

Re: [PATCH v2 2/4] configfs: use BIT() for internal flags

Quoted 4 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

For better readability and maintenance: use the BIT() macro for flag definitions.

This does not improve readability, as I now need to look up a macro, one that doesn't make any sense at all. If you really prefer bit based numbering just use the totally obvious (1 << bit) syntax.

#4 lore raw

Re: [PATCH v2 2/4] configfs: use BIT() for internal flags

Quoted 10 lines · Christoph Hellwig
Quoted 4 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

For better readability and maintenance: use the BIT() macro for flag definitions.

This does not improve readability, as I now need to look up a macro, one that doesn't make any sense at all. If you really prefer bit based numbering just use the totally obvious (1 << bit) syntax.

(1 << bit) is better than a number in hex but BIT() has been in the kernel for so long that I doubt anyone needs to look it up.

Anyway, it's your call, it'll be (1 << bit) in v3.

Bartosz

#5 lore raw

PATCH v2 3/4 configfs: implement committable items

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

This implements configfs committable items. We mostly follow the documentation except that we extend config_group_ops with uncommit_item() callback for reverting the changes made by commit_item().

Each committable group has two sub-directories: pending and live. New items can only be created in pending/. Attributes can only be modified while the item is in pending/. Once it's ready to be committed, it must be moved over to live/ using the rename() system call. This is when the commit_item() function will be called.

Implementation-wise: we reuse the default group mechanism to elegantly plug the new pseude-groups into configfs. The pending group inherits the parent group's operations so that config_items can be seamlesly created in it using the callbacks supplied by the user as part of the committable group itself.

---
 Documentation/filesystems/configfs.rst |   6 +-
 fs/configfs/configfs_internal.h        |   2 +
 fs/configfs/dir.c                      | 237 ++++++++++++++++++++++++-
 fs/configfs/file.c                     |   8 +
 include/linux/configfs.h               |   1 +
 5 files changed, 245 insertions(+), 9 deletions(-)
Documentation/filesystems/configfs.rst +2 -4
index 1d3d6f4a82a9..7e0e7c356450 100644
@@ -290,6 +290,7 @@ config_item_type::
 		struct config_group *(*make_group)(struct config_group *group,
 						   const char *name);
 		int (*commit_item)(struct config_item *item);
+		int (*uncommit_item)(struct config_item *item);
 		void (*disconnect_notify)(struct config_group *group,
 					  struct config_item *item);
 		void (*drop_item)(struct config_group *group,
@@ -490,9 +491,6 @@ pass up an error.
 Committable Items
 =================
 
-Note:
-     Committable items are currently unimplemented.
-
 Some config_items cannot have a valid initial state.  That is, no
 default values can be specified for the item's attributes such that the
 item can do its work.  Userspace must configure one or more attributes,
@@ -532,4 +530,4 @@ method returns zero and the item is moved to the "live" directory.
 As rmdir(2) does not work in the "live" directory, an item must be
 shutdown, or "uncommitted".  Again, this is done via rename(2), this
 time from the "live" directory back to the "pending" one.  The subsystem
-is notified by the ct_group_ops->uncommit_object() method.
+is notified by the ct_group_ops->uncommit_item() method.
fs/configfs/configfs_internal.h +2 -0
index 855f00868654..22b649567b13 100644
@@ -56,6 +56,8 @@ struct configfs_dirent {
 #define CONFIGFS_USET_DROPPING		BIT(8)
 #define CONFIGFS_USET_IN_MKDIR		BIT(9)
 #define CONFIGFS_USET_CREATING		BIT(10)
+#define CONFIGFS_GROUP_PENDING		BIT(11)
+#define CONFIGFS_GROUP_LIVE		BIT(12)
 #define CONFIGFS_NOT_PINNED	(CONFIGFS_ITEM_ATTR | CONFIGFS_ITEM_BIN_ATTR)
 
 extern struct mutex configfs_symlink_mutex;
fs/configfs/dir.c +232 -5

277 lines not on this page · raw message

fs/configfs/file.c +8 -0
index 1f0270229d7b..a20e55fd05e8 100644
@@ -243,9 +243,17 @@ fill_write_buffer(struct configfs_buffer * buffer, const char __user * buf, size
 static int
 flush_write_buffer(struct file *file, struct configfs_buffer *buffer, size_t count)
 {
+	struct config_item *parent_item = buffer->item->ci_parent;
 	struct configfs_fragment *frag = to_frag(file);
+	struct configfs_dirent *sd;
 	int res = -ENOENT;
 
+	if (parent_item && parent_item->ci_dentry) {
+		sd = parent_item->ci_dentry->d_fsdata;
+		if (sd->s_type & CONFIGFS_GROUP_LIVE)
+			return -EPERM;
+	}
+
 	down_read(&frag->frag_sem);
 	if (!frag->frag_dead)
 		res = buffer->attr->store(buffer->item, buffer->page, count);
include/linux/configfs.h +1 -0
index 4f76dcc08134..ff6b0e408136 100644
@@ -219,6 +219,7 @@ 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);
+	int (*uncommit_item)(struct config_item *item);
 	void (*disconnect_notify)(struct config_group *group, struct config_item *item);
 	void (*drop_item)(struct config_group *group, struct config_item *item);
 };
Signature · 1 line
2.29.1
#6 lore raw

Re: [PATCH v2 3/4] configfs: implement committable items

Quoted 12 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

This implements configfs committable items. We mostly follow the documentation except that we extend config_group_ops with uncommit_item() callback for reverting the changes made by commit_item().

Each committable group has two sub-directories: pending and live. New items can only be created in pending/. Attributes can only be modified while the item is in pending/. Once it's ready to be committed, it must be moved over to live/ using the rename() system call. This is when the commit_item() function will be called.

Quoted 5 lines

Implementation-wise: we reuse the default group mechanism to elegantly plug the new pseude-groups into configfs. The pending group inherits the parent group's operations so that config_items can be seamlesly created in it using the callbacks supplied by the user as part of the committable group itself.

This looks pretty awkward in the hierachy, but I can't really think of anything else. One idea would be to require fsync to stage updates, but that isn't really very well discoverable.

#7 lore raw

Re: [PATCH v2 3/4] configfs: implement committable items

Quoted 20 lines · Christoph Hellwig
Quoted 12 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

This implements configfs committable items. We mostly follow the documentation except that we extend config_group_ops with uncommit_item() callback for reverting the changes made by commit_item().

Each committable group has two sub-directories: pending and live. New items can only be created in pending/. Attributes can only be modified while the item is in pending/. Once it's ready to be committed, it must be moved over to live/ using the rename() system call. This is when the commit_item() function will be called.

Quoted 5 lines

Implementation-wise: we reuse the default group mechanism to elegantly plug the new pseude-groups into configfs. The pending group inherits the parent group's operations so that config_items can be seamlesly created in it using the callbacks supplied by the user as part of the committable group itself.

Quoted 3 lines

This looks pretty awkward in the hierachy, but I can't really think of anything else. One idea would be to require fsync to stage updates, but that isn't really very well discoverable.

I'm not sure how that would work. fsync() a directory once the item is configured to instantiate it?

I was thinking about different solutions other than the one already defined but it always requires at least some kind of an additional attribute (not defined by the user) to "commit" an item and in the end rename() looks like a good candidate to me. We could possibly drop the pending and live groups and simple use a magic suffix for committed items e.g.: mv myitem myitem_committed but this would be even less intuitive.

If this patch is extended with:

@@ -1103,6 +1103,8 @@ static void configfs_dump_one(struct configfs_dirent *sd, int level)
type_print(CONFIGFS_USET_DIR); type_print(CONFIGFS_USET_DEFAULT); type_print(CONFIGFS_USET_DROPPING);
+ type_print(CONFIGFS_GROUP_PENDING);
+ type_print(CONFIGFS_GROUP_LIVE);
#undef type_print
}

Then dumping the committable subsystem shows:

[  133.603035] configfs:    "04-committable-children":
[  133.603045] configfs:     CONFIGFS_DIR
[  133.603050] configfs:      "live":
[  133.603054] configfs:       CONFIGFS_DIR
[  133.603058] configfs:       CONFIGFS_USET_DIR
[  133.603062] configfs:       CONFIGFS_USET_DEFAULT
[  133.603066] configfs:       CONFIGFS_GROUP_LIVE
[  133.603070] configfs:        "dupa":
[  133.603074] configfs:         CONFIGFS_DIR
[  133.603078] configfs:          "committed":
[  133.603082] configfs:           CONFIGFS_ITEM_ATTR
[  133.603086] configfs:          "storeme":
[  133.603090] configfs:           CONFIGFS_ITEM_ATTR
[  133.603094] configfs:      "pending":
[  133.603160] configfs:       CONFIGFS_DIR
[  133.603167] configfs:       CONFIGFS_USET_DIR
[  133.603175] configfs:       CONFIGFS_USET_DEFAULT
[  133.603183] configfs:       CONFIGFS_GROUP_PENDING
[  133.603191] configfs:      "description":
[  133.603198] configfs:       CONFIGFS_ITEM_ATTR

Which doesn't look that bad IMO.

Bartosz

#8 lore raw

PATCH v2 4/4 samples: configfs: add a committable group

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Add an example of using committable items to configfs samples. Each config item has two attributes: read-write 'storeme' which works similarly to other examples in this file and a read-only 'committed' attribute which changes its value between false and true depending on whether it's committed or not at the moment.

---
 samples/configfs/configfs_sample.c | 150 +++++++++++++++++++++++++++++
 1 file changed, 150 insertions(+)
samples/configfs/configfs_sample.c +150 -0
index f9008be7a8a1..08fc22c7aa55 100644
@@ -315,6 +315,155 @@ static struct configfs_subsystem group_children_subsys = {
 
 /* ----------------------------------------------------------------- */
 
+/*
+ * 04-committable-children
+ *
+ * This is an example of a committable group.  It's similar to the simple
+ * children example but each config_item has an additional 'committed'
+ * attribute which is read-only and is only modified when the config_item
+ * is moved from the 'pending' to the 'live' directory.
+ */
+
+struct committable_child {
+	struct config_item item;
+	int storeme;
+	bool committed;
+};
+
+static inline struct committable_child *
+to_committable_child(struct config_item *item)
+{
+	return container_of(item, struct committable_child, item);
+}
+
+static ssize_t
+committable_child_storeme_show(struct config_item *item, char *page)
+{
+	return sprintf(page, "%d\n", to_committable_child(item)->storeme);
+}
+
+static ssize_t committable_child_storeme_store(struct config_item *item,
+					       const char *page, size_t count)
+{
+	struct committable_child *child = to_committable_child(item);
+	int ret;
+
+	ret = kstrtoint(page, 10, &child->storeme);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+CONFIGFS_ATTR(committable_child_, storeme);
+
+static ssize_t
+committable_child_committed_show(struct config_item *item, char *page)
+{
+	return sprintf(page, "%s\n",
+		to_committable_child(item)->committed ? "true" : "false");
+}
+
+CONFIGFS_ATTR_RO(committable_child_, committed);
+
+static struct configfs_attribute *committable_child_attrs[] = {
+	&committable_child_attr_storeme,
+	&committable_child_attr_committed,
+	NULL,
+};
+
+static void committable_child_release(struct config_item *item)
+{
+	kfree(to_committable_child(item));
+}
+
+static struct configfs_item_operations committable_child_item_ops = {
+	.release	= committable_child_release,
+};
+
+static const struct config_item_type committable_child_type = {
+	.ct_item_ops	= &committable_child_item_ops,
+	.ct_attrs	= committable_child_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+struct committable_children {
+	struct config_group group;
+};
+
+static struct config_item *
+committable_children_make_item(struct config_group *group, const char *name)
+{
+	struct committable_child *child;
+
+	child = kzalloc(sizeof(*child), GFP_KERNEL);
+	if (!child)
+		return ERR_PTR(-ENOMEM);
+
+	config_item_init_type_name(&child->item, name, &committable_child_type);
+
+	return &child->item;
+}
+
+static ssize_t
+committable_children_description_show(struct config_item *item, char *page)
+{
+	return sprintf(page,
+"[04-committable-children]\n"
+"\n"
+"This subsystem allows creation of committable config_items.  The subsystem\n"
+"has two subdirectories: pending and live.  New config_items can only be\n"
+"created in pending/ and they have one writable and readable attribute as\n"
+"well as a single read-only attribute.  The latter is only changed once the\n"
+"item is 'committed'.  This is done by moving the config_item (using\n"
+"rename()) to the live/ directory.  At that point even the read-write\n"
+"attributes can no longer be written to.\n");
+}
+
+CONFIGFS_ATTR_RO(committable_children_, description);
+
+static struct configfs_attribute *committable_children_attrs[] = {
+	&committable_children_attr_description,
+	NULL,
+};
+
+static int committable_children_commit_item(struct config_item *item)
+{
+	to_committable_child(item)->committed = true;
+
+	return 0;
+}
+
+static int committable_children_uncommit_item(struct config_item *item)
+{
+	to_committable_child(item)->committed = false;
+
+	return 0;
+}
+
+static struct configfs_group_operations committable_children_group_ops = {
+	.make_item	= committable_children_make_item,
+	.commit_item	= committable_children_commit_item,
+	.uncommit_item	= committable_children_uncommit_item,
+};
+
+static const struct config_item_type committable_children_type = {
+	.ct_group_ops	= &committable_children_group_ops,
+	.ct_attrs	= committable_children_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct configfs_subsystem committable_children_subsys = {
+	.su_group = {
+		.cg_item = {
+			.ci_namebuf = "04-committable-children",
+			.ci_type = &committable_children_type,
+		},
+	},
+};
+
+/* ----------------------------------------------------------------- */
+
 /*
  * We're now done with our subsystem definitions.
  * For convenience in this module, here's a list of them all.  It
@@ -326,6 +475,7 @@ static struct configfs_subsystem *example_subsys[] = {
 	&childless_subsys.subsys,
 	&simple_children_subsys,
 	&group_children_subsys,
+	&committable_children_subsys,
 	NULL,
 };
 
Signature · 1 line
2.29.1