* [PATCH 0/4] configfs: implement committable items and add sample code
@ 2020-11-25 15:22 Bartosz Golaszewski
2020-11-25 15:22 ` [PATCH 1/4] configfs: increase the item name length Bartosz Golaszewski
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Bartosz Golaszewski @ 2020-11-25 15:22 UTC (permalink / raw)
To: Joel Becker, Christoph Hellwig; +Cc: linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
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.
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 | 239 ++++++++++++++++++++++++-
fs/configfs/file.c | 8 +
include/linux/configfs.h | 3 +-
samples/configfs/configfs_sample.c | 150 ++++++++++++++++
6 files changed, 408 insertions(+), 20 deletions(-)
--
2.29.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/4] configfs: increase the item name length
2020-11-25 15:22 [PATCH 0/4] configfs: implement committable items and add sample code Bartosz Golaszewski
@ 2020-11-25 15:22 ` Bartosz Golaszewski
2020-11-25 15:22 ` [PATCH 2/4] configfs: use BIT() for internal flags Bartosz Golaszewski
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Bartosz Golaszewski @ 2020-11-25 15:22 UTC (permalink / raw)
To: Joel Becker, Christoph Hellwig; +Cc: linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
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.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
include/linux/configfs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index 2e8c69b43c64..4f76dcc08134 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h
@@ -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;
--
2.29.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/4] configfs: use BIT() for internal flags
2020-11-25 15:22 [PATCH 0/4] configfs: implement committable items and add sample code Bartosz Golaszewski
2020-11-25 15:22 ` [PATCH 1/4] configfs: increase the item name length Bartosz Golaszewski
@ 2020-11-25 15:22 ` Bartosz Golaszewski
2020-11-25 15:22 ` [PATCH 3/4] configfs: implement committable items Bartosz Golaszewski
2020-11-25 15:22 ` [PATCH 4/4] samples: configfs: add a committable group Bartosz Golaszewski
3 siblings, 0 replies; 9+ messages in thread
From: Bartosz Golaszewski @ 2020-11-25 15:22 UTC (permalink / raw)
To: Joel Becker, Christoph Hellwig; +Cc: linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
For better readability and maintenance: use the BIT() macro for flag
definitions.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
fs/configfs/configfs_internal.h | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/fs/configfs/configfs_internal.h b/fs/configfs/configfs_internal.h
index 22dce2d35a4b..855f00868654 100644
--- a/fs/configfs/configfs_internal.h
+++ b/fs/configfs/configfs_internal.h
@@ -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;
--
2.29.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/4] configfs: implement committable items
2020-11-25 15:22 [PATCH 0/4] configfs: implement committable items and add sample code Bartosz Golaszewski
2020-11-25 15:22 ` [PATCH 1/4] configfs: increase the item name length Bartosz Golaszewski
2020-11-25 15:22 ` [PATCH 2/4] configfs: use BIT() for internal flags Bartosz Golaszewski
@ 2020-11-25 15:22 ` Bartosz Golaszewski
2020-11-25 18:11 ` kernel test robot
2021-01-13 23:46 ` Joel Becker
2020-11-25 15:22 ` [PATCH 4/4] samples: configfs: add a committable group Bartosz Golaszewski
3 siblings, 2 replies; 9+ messages in thread
From: Bartosz Golaszewski @ 2020-11-25 15:22 UTC (permalink / raw)
To: Joel Becker, Christoph Hellwig; +Cc: linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
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.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
Documentation/filesystems/configfs.rst | 6 +-
fs/configfs/configfs_internal.h | 2 +
fs/configfs/dir.c | 239 ++++++++++++++++++++++++-
fs/configfs/file.c | 8 +
include/linux/configfs.h | 1 +
5 files changed, 247 insertions(+), 9 deletions(-)
diff --git a/Documentation/filesystems/configfs.rst b/Documentation/filesystems/configfs.rst
index 1d3d6f4a82a9..7e0e7c356450 100644
--- a/Documentation/filesystems/configfs.rst
+++ b/Documentation/filesystems/configfs.rst
@@ -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.
diff --git a/fs/configfs/configfs_internal.h b/fs/configfs/configfs_internal.h
index 855f00868654..22b649567b13 100644
--- a/fs/configfs/configfs_internal.h
+++ b/fs/configfs/configfs_internal.h
@@ -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;
diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index b0983e2a4e2c..73e3bd30a2ad 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -655,6 +655,13 @@ static void detach_groups(struct config_group *group)
inode_unlock(d_inode(child));
+ /*
+ * Free memory allocated for the pending and live directories
+ * of committable groups.
+ */
+ if (sd->s_type & (CONFIGFS_GROUP_PENDING | CONFIGFS_GROUP_LIVE))
+ kfree(sd->s_element);
+
d_delete(child);
dput(child);
}
@@ -859,6 +866,133 @@ static void configfs_detach_item(struct config_item *item)
configfs_remove_dir(item);
}
+static bool is_committable_group(struct config_item *item)
+{
+ const struct config_item_type *type = item->ci_type;
+
+ if (type && type->ct_group_ops &&
+ type->ct_group_ops->commit_item &&
+ type->ct_group_ops->uncommit_item)
+ return true;
+
+ return false;
+}
+
+struct pending_group_data {
+ struct config_group group;
+ struct config_item_type type;
+ struct configfs_group_operations group_ops;
+};
+
+struct live_group_data {
+ struct config_group group;
+ struct config_item_type type;
+};
+
+static int create_pending_group(struct config_item *parent_item,
+ struct configfs_fragment *frag)
+{
+ const struct config_item_type *parent_type = parent_item->ci_type;
+ struct pending_group_data *pending;
+ struct configfs_dirent *sd;
+ int ret;
+
+ pending = kzalloc(sizeof(*pending), GFP_KERNEL);
+ if (!pending)
+ return -ENOMEM;
+
+ /*
+ * Let's inherit the group_ops from the parent except for item
+ * committing and uncommitting.
+ */
+ memcpy(&pending->group_ops, parent_type->ct_group_ops,
+ sizeof(struct configfs_group_operations));
+ pending->type.ct_group_ops = &pending->group_ops;
+ pending->type.ct_group_ops->commit_item = NULL;
+ pending->type.ct_group_ops->uncommit_item = NULL;
+
+ /* Let's directly reuse item_ops. */
+ pending->type.ct_item_ops = parent_type->ct_item_ops;
+ pending->type.ct_owner = parent_type->ct_owner;
+
+ config_group_init_type_name(&pending->group, "pending", &pending->type);
+
+ ret = create_default_group(to_config_group(parent_item),
+ &pending->group, frag);
+ if (ret) {
+ kfree(pending);
+ return ret;
+ }
+
+ link_group(to_config_group(parent_item), &pending->group);
+
+ sd = pending->group.cg_item.ci_dentry->d_fsdata;
+ /* Allow creating config_items in 'pending' group. */
+ sd->s_type |= (CONFIGFS_GROUP_PENDING | CONFIGFS_USET_DIR);
+
+ return 0;
+}
+
+static int create_live_group(struct config_item *parent_item,
+ struct configfs_fragment *frag)
+{
+ struct live_group_data *live;
+ struct configfs_dirent *sd;
+ int ret;
+
+ live = kzalloc(sizeof(*live), GFP_KERNEL);
+ if (!live)
+ return -ENOMEM;
+
+ live->type.ct_owner = parent_item->ci_type->ct_owner;
+
+ config_group_init_type_name(&live->group, "live", &live->type);
+
+ ret = create_default_group(to_config_group(parent_item),
+ &live->group, frag);
+ if (ret) {
+ kfree(live);
+ return ret;
+ }
+
+ link_group(to_config_group(parent_item), &live->group);
+
+ sd = live->group.cg_item.ci_dentry->d_fsdata;
+ sd->s_type |= CONFIGFS_GROUP_LIVE;
+
+ return 0;
+}
+
+static int create_committable_groups(struct config_item *parent_item,
+ struct configfs_fragment *frag)
+{
+ struct configfs_dirent *sd;
+ int ret;
+
+ ret = create_pending_group(parent_item, frag);
+ if (ret)
+ return ret;
+
+ ret = create_live_group(parent_item, frag);
+ if (ret) {
+ detach_groups(to_config_group(parent_item));
+ return ret;
+ }
+
+ /* Disallow creating items directly in the committable group. */
+ sd = parent_item->ci_dentry->d_fsdata;
+ sd->s_type &= ~CONFIGFS_USET_DIR;
+
+ return 0;
+}
+
+static void dentry_mark_dead(struct config_item *item, struct dentry *dentry)
+{
+ configfs_detach_item(item);
+ d_inode(dentry)->i_flags |= S_DEAD;
+ dont_mount(dentry);
+}
+
static int configfs_attach_group(struct config_item *parent_item,
struct config_item *item,
struct dentry *dentry,
@@ -884,11 +1018,15 @@ static int configfs_attach_group(struct config_item *parent_item,
inode_lock_nested(d_inode(dentry), I_MUTEX_CHILD);
configfs_adjust_dir_dirent_depth_before_populate(sd);
ret = populate_groups(to_config_group(item), frag);
- if (ret) {
- configfs_detach_item(item);
- d_inode(dentry)->i_flags |= S_DEAD;
- dont_mount(dentry);
+ if (ret)
+ dentry_mark_dead(item, dentry);
+
+ if (is_committable_group(item)) {
+ ret = create_committable_groups(item, frag);
+ if (ret)
+ dentry_mark_dead(item, dentry);
}
+
configfs_adjust_dir_dirent_depth_after_populate(sd);
inode_unlock(d_inode(dentry));
if (ret)
@@ -1455,7 +1593,7 @@ static int configfs_rmdir(struct inode *dir, struct dentry *dentry)
struct config_item *parent_item;
struct config_item *item;
struct configfs_subsystem *subsys;
- struct configfs_dirent *sd;
+ struct configfs_dirent *sd, *parent_sd;
struct configfs_fragment *frag;
struct module *subsys_owner = NULL, *dead_item_owner = NULL;
int ret;
@@ -1474,6 +1612,12 @@ static int configfs_rmdir(struct inode *dir, struct dentry *dentry)
return -EINVAL;
}
+ parent_sd = dentry->d_parent->d_fsdata;
+ if (parent_sd->s_type & CONFIGFS_GROUP_LIVE) {
+ config_item_put(parent_item);
+ return -EPERM;
+ }
+
/* configfs_mkdir() shouldn't have allowed this */
BUG_ON(!subsys->su_group.cg_item.ci_type);
subsys_owner = subsys->su_group.cg_item.ci_type->ct_owner;
@@ -1560,9 +1704,94 @@ static int configfs_rmdir(struct inode *dir, struct dentry *dentry)
return 0;
}
+static int configfs_rename(struct inode *old_dir, struct dentry *old_dentry,
+ struct inode *new_dir, struct dentry *new_dentry,
+ unsigned int flags)
+{
+ struct configfs_dirent *sd, *old_parent_sd, *new_parent_sd;
+ struct dentry *old_parent_dentry, *new_parent_dentry;
+ struct configfs_dirent *committable_group_sd;
+ struct dentry *committable_group_dentry;
+ struct config_item *committable_group_item, *item, *new_parent_item;
+ struct configfs_subsystem *committable_group_subsys;
+ struct configfs_group_operations *committable_group_ops;
+ int ret = 0;
+
+ if (flags)
+ return -EINVAL;
+
+ old_parent_dentry = old_dentry->d_parent;
+ new_parent_dentry = new_dentry->d_parent;
+
+ sd = old_dentry->d_fsdata;
+ old_parent_sd = old_dentry->d_parent->d_fsdata;
+ new_parent_sd = new_dentry->d_parent->d_fsdata;
+
+ if (!old_parent_sd || !new_parent_sd)
+ return -EPERM;
+
+ /*
+ * Renaming must always be between a 'pending' and a 'live' group and
+ * both need to have the same parent.
+ */
+ if (!((old_parent_sd->s_type & CONFIGFS_GROUP_PENDING) &&
+ (new_parent_sd->s_type & CONFIGFS_GROUP_LIVE)) &&
+ !((old_parent_sd->s_type & CONFIGFS_GROUP_LIVE) &&
+ (new_parent_sd->s_type & CONFIGFS_GROUP_PENDING)))
+ return -EPERM;
+
+ if (old_parent_dentry->d_parent != new_parent_dentry->d_parent)
+ return -EPERM;
+
+ committable_group_dentry = old_parent_dentry->d_parent;
+ committable_group_sd = committable_group_dentry->d_fsdata;
+ /*
+ * Grab a reference to the committable group for the duration of
+ * this function.
+ */
+ committable_group_item =
+ configfs_get_config_item(committable_group_dentry);
+ committable_group_subsys =
+ to_config_group(committable_group_item)->cg_subsys;
+ committable_group_ops = committable_group_item->ci_type->ct_group_ops;
+
+ item = sd->s_element;
+ new_parent_item = new_parent_sd->s_element;
+
+ if (WARN_ON(!is_committable_group(committable_group_item))) {
+ /* This would be a result of a programming error in configfs. */
+ config_item_put(committable_group_item);
+ return -EPERM;
+ }
+
+ mutex_lock(&committable_group_subsys->su_mutex);
+ spin_lock(&configfs_dirent_lock);
+
+ if ((old_parent_sd->s_type & CONFIGFS_GROUP_PENDING) &&
+ (new_parent_sd->s_type & CONFIGFS_GROUP_LIVE))
+ ret = committable_group_ops->commit_item(item);
+ else
+ ret = committable_group_ops->uncommit_item(item);
+ if (ret)
+ goto out;
+
+ new_dentry->d_fsdata = sd;
+ list_move(&sd->s_sibling, &new_parent_sd->s_children);
+ item->ci_parent = new_parent_item;
+ d_move(old_dentry, new_dentry);
+
+out:
+ spin_unlock(&configfs_dirent_lock);
+ mutex_unlock(&committable_group_subsys->su_mutex);
+ config_item_put(committable_group_item);
+
+ return ret;
+}
+
const struct inode_operations configfs_dir_inode_operations = {
.mkdir = configfs_mkdir,
.rmdir = configfs_rmdir,
+ .rename = configfs_rename,
.symlink = configfs_symlink,
.unlink = configfs_unlink,
.lookup = configfs_lookup,
diff --git a/fs/configfs/file.c b/fs/configfs/file.c
index 1f0270229d7b..a20e55fd05e8 100644
--- a/fs/configfs/file.c
+++ b/fs/configfs/file.c
@@ -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);
diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index 4f76dcc08134..ff6b0e408136 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h
@@ -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);
};
--
2.29.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 4/4] samples: configfs: add a committable group
2020-11-25 15:22 [PATCH 0/4] configfs: implement committable items and add sample code Bartosz Golaszewski
` (2 preceding siblings ...)
2020-11-25 15:22 ` [PATCH 3/4] configfs: implement committable items Bartosz Golaszewski
@ 2020-11-25 15:22 ` Bartosz Golaszewski
3 siblings, 0 replies; 9+ messages in thread
From: Bartosz Golaszewski @ 2020-11-25 15:22 UTC (permalink / raw)
To: Joel Becker, Christoph Hellwig; +Cc: linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
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.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
samples/configfs/configfs_sample.c | 150 +++++++++++++++++++++++++++++
1 file changed, 150 insertions(+)
diff --git a/samples/configfs/configfs_sample.c b/samples/configfs/configfs_sample.c
index f9008be7a8a1..08fc22c7aa55 100644
--- a/samples/configfs/configfs_sample.c
+++ b/samples/configfs/configfs_sample.c
@@ -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,
};
--
2.29.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] configfs: implement committable items
2020-11-25 15:22 ` [PATCH 3/4] configfs: implement committable items Bartosz Golaszewski
@ 2020-11-25 18:11 ` kernel test robot
2021-01-13 23:46 ` Joel Becker
1 sibling, 0 replies; 9+ messages in thread
From: kernel test robot @ 2020-11-25 18:11 UTC (permalink / raw)
To: Bartosz Golaszewski, Joel Becker, Christoph Hellwig
Cc: kbuild-all, linux-kernel, Bartosz Golaszewski
[-- Attachment #1: Type: text/plain, Size: 5211 bytes --]
Hi Bartosz,
I love your patch! Perhaps something to improve:
[auto build test WARNING on hch-configfs/for-next]
[also build test WARNING on linux/master linus/master v5.10-rc5 next-20201125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Bartosz-Golaszewski/configfs-implement-committable-items-and-add-sample-code/20201125-232501
base: git://git.infradead.org/users/hch/configfs.git for-next
config: arm-randconfig-r031-20201125 (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/9ddc94acfed5f87493359f719d6eb5c259b6bd6d
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Bartosz-Golaszewski/configfs-implement-committable-items-and-add-sample-code/20201125-232501
git checkout 9ddc94acfed5f87493359f719d6eb5c259b6bd6d
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
fs/configfs/dir.c: In function 'configfs_rename':
>> fs/configfs/dir.c:1714:26: warning: variable 'committable_group_sd' set but not used [-Wunused-but-set-variable]
1714 | struct configfs_dirent *committable_group_sd;
| ^~~~~~~~~~~~~~~~~~~~
vim +/committable_group_sd +1714 fs/configfs/dir.c
1707
1708 static int configfs_rename(struct inode *old_dir, struct dentry *old_dentry,
1709 struct inode *new_dir, struct dentry *new_dentry,
1710 unsigned int flags)
1711 {
1712 struct configfs_dirent *sd, *old_parent_sd, *new_parent_sd;
1713 struct dentry *old_parent_dentry, *new_parent_dentry;
> 1714 struct configfs_dirent *committable_group_sd;
1715 struct dentry *committable_group_dentry;
1716 struct config_item *committable_group_item, *item, *new_parent_item;
1717 struct configfs_subsystem *committable_group_subsys;
1718 struct configfs_group_operations *committable_group_ops;
1719 int ret = 0;
1720
1721 if (flags)
1722 return -EINVAL;
1723
1724 old_parent_dentry = old_dentry->d_parent;
1725 new_parent_dentry = new_dentry->d_parent;
1726
1727 sd = old_dentry->d_fsdata;
1728 old_parent_sd = old_dentry->d_parent->d_fsdata;
1729 new_parent_sd = new_dentry->d_parent->d_fsdata;
1730
1731 if (!old_parent_sd || !new_parent_sd)
1732 return -EPERM;
1733
1734 /*
1735 * Renaming must always be between a 'pending' and a 'live' group and
1736 * both need to have the same parent.
1737 */
1738 if (!((old_parent_sd->s_type & CONFIGFS_GROUP_PENDING) &&
1739 (new_parent_sd->s_type & CONFIGFS_GROUP_LIVE)) &&
1740 !((old_parent_sd->s_type & CONFIGFS_GROUP_LIVE) &&
1741 (new_parent_sd->s_type & CONFIGFS_GROUP_PENDING)))
1742 return -EPERM;
1743
1744 if (old_parent_dentry->d_parent != new_parent_dentry->d_parent)
1745 return -EPERM;
1746
1747 committable_group_dentry = old_parent_dentry->d_parent;
1748 committable_group_sd = committable_group_dentry->d_fsdata;
1749 /*
1750 * Grab a reference to the committable group for the duration of
1751 * this function.
1752 */
1753 committable_group_item =
1754 configfs_get_config_item(committable_group_dentry);
1755 committable_group_subsys =
1756 to_config_group(committable_group_item)->cg_subsys;
1757 committable_group_ops = committable_group_item->ci_type->ct_group_ops;
1758
1759 item = sd->s_element;
1760 new_parent_item = new_parent_sd->s_element;
1761
1762 if (WARN_ON(!is_committable_group(committable_group_item))) {
1763 /* This would be a result of a programming error in configfs. */
1764 config_item_put(committable_group_item);
1765 return -EPERM;
1766 }
1767
1768 mutex_lock(&committable_group_subsys->su_mutex);
1769 spin_lock(&configfs_dirent_lock);
1770
1771 if ((old_parent_sd->s_type & CONFIGFS_GROUP_PENDING) &&
1772 (new_parent_sd->s_type & CONFIGFS_GROUP_LIVE))
1773 ret = committable_group_ops->commit_item(item);
1774 else
1775 ret = committable_group_ops->uncommit_item(item);
1776 if (ret)
1777 goto out;
1778
1779 new_dentry->d_fsdata = sd;
1780 list_move(&sd->s_sibling, &new_parent_sd->s_children);
1781 item->ci_parent = new_parent_item;
1782 d_move(old_dentry, new_dentry);
1783
1784 out:
1785 spin_unlock(&configfs_dirent_lock);
1786 mutex_unlock(&committable_group_subsys->su_mutex);
1787 config_item_put(committable_group_item);
1788
1789 return ret;
1790 }
1791
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34449 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] configfs: implement committable items
2020-11-25 15:22 ` [PATCH 3/4] configfs: implement committable items Bartosz Golaszewski
2020-11-25 18:11 ` kernel test robot
@ 2021-01-13 23:46 ` Joel Becker
2021-01-14 8:40 ` Bartosz Golaszewski
1 sibling, 1 reply; 9+ messages in thread
From: Joel Becker @ 2021-01-13 23:46 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: Christoph Hellwig, linux-kernel, Bartosz Golaszewski
On Wed, Nov 25, 2020 at 04:22:46PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> 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().
Woohoo! A long time coming, but thank you for working on the
implementation!
> 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.
The original API intended for live items to still be modifyable. The
live/ path forbids mkdir()/rmdir(), but it allows store(). Otherwise,
items can't be adjusted at all while in use, which is severely limiting.
Obviously the store() handler must not allow transitions from
valid-value->invalid-value, but the handler would reject invalid values
anyway, wouldn't it?
> diff --git a/fs/configfs/file.c b/fs/configfs/file.c
> index 1f0270229d7b..a20e55fd05e8 100644
> --- a/fs/configfs/file.c
> +++ b/fs/configfs/file.c
> @@ -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);
Basically, I would just leave this hunk out.
Thanks,
Joel
--
"Now Someone's on the telephone, desperate in his pain.
Someone's on the bathroom floor doing her cocaine.
Someone's got his finger on the button in some room.
No one can convince me we aren't gluttons for our doom."
http://www.jlbec.org/
jlbec@evilplan.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] configfs: implement committable items
2021-01-13 23:46 ` Joel Becker
@ 2021-01-14 8:40 ` Bartosz Golaszewski
2021-01-14 14:19 ` Bartosz Golaszewski
0 siblings, 1 reply; 9+ messages in thread
From: Bartosz Golaszewski @ 2021-01-14 8:40 UTC (permalink / raw)
To: Bartosz Golaszewski, Christoph Hellwig, LKML, Bartosz Golaszewski
On Thu, Jan 14, 2021 at 12:46 AM Joel Becker <jlbec@evilplan.org> wrote:
>
> On Wed, Nov 25, 2020 at 04:22:46PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > 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().
>
> Woohoo! A long time coming, but thank you for working on the
> implementation!
>
> > 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.
>
> The original API intended for live items to still be modifyable. The
> live/ path forbids mkdir()/rmdir(), but it allows store(). Otherwise,
> items can't be adjusted at all while in use, which is severely limiting.
> Obviously the store() handler must not allow transitions from
> valid-value->invalid-value, but the handler would reject invalid values
> anyway, wouldn't it?
>
> > diff --git a/fs/configfs/file.c b/fs/configfs/file.c
> > index 1f0270229d7b..a20e55fd05e8 100644
> > --- a/fs/configfs/file.c
> > +++ b/fs/configfs/file.c
> > @@ -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);
>
> Basically, I would just leave this hunk out.
>
I would make this configurable per-attribute because for the use-case
I need this for we definitely don't want the items to be modifiable
once they're "live".
Bartosz
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] configfs: implement committable items
2021-01-14 8:40 ` Bartosz Golaszewski
@ 2021-01-14 14:19 ` Bartosz Golaszewski
0 siblings, 0 replies; 9+ messages in thread
From: Bartosz Golaszewski @ 2021-01-14 14:19 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: Christoph Hellwig, LKML
On Thu, Jan 14, 2021 at 9:40 AM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>
> On Thu, Jan 14, 2021 at 12:46 AM Joel Becker <jlbec@evilplan.org> wrote:
> >
> > On Wed, Nov 25, 2020 at 04:22:46PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > 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().
> >
> > Woohoo! A long time coming, but thank you for working on the
> > implementation!
> >
> > > 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.
> >
> > The original API intended for live items to still be modifyable. The
> > live/ path forbids mkdir()/rmdir(), but it allows store(). Otherwise,
> > items can't be adjusted at all while in use, which is severely limiting.
> > Obviously the store() handler must not allow transitions from
> > valid-value->invalid-value, but the handler would reject invalid values
> > anyway, wouldn't it?
> >
> > > diff --git a/fs/configfs/file.c b/fs/configfs/file.c
> > > index 1f0270229d7b..a20e55fd05e8 100644
> > > --- a/fs/configfs/file.c
> > > +++ b/fs/configfs/file.c
> > > @@ -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);
> >
> > Basically, I would just leave this hunk out.
> >
>
> I would make this configurable per-attribute because for the use-case
> I need this for we definitely don't want the items to be modifiable
> once they're "live".
>
> Bartosz
After a second thought: I agree this can be left to the user's
discretion so I'll just remove this and enforce it in the sample code.
One thing I'm unsure about is whether we should allow to change the
group's name when moving it from pending to live. Any thoughts?
Bartosz
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-01-14 14:20 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-25 15:22 [PATCH 0/4] configfs: implement committable items and add sample code Bartosz Golaszewski
2020-11-25 15:22 ` [PATCH 1/4] configfs: increase the item name length Bartosz Golaszewski
2020-11-25 15:22 ` [PATCH 2/4] configfs: use BIT() for internal flags Bartosz Golaszewski
2020-11-25 15:22 ` [PATCH 3/4] configfs: implement committable items Bartosz Golaszewski
2020-11-25 18:11 ` kernel test robot
2021-01-13 23:46 ` Joel Becker
2021-01-14 8:40 ` Bartosz Golaszewski
2021-01-14 14:19 ` Bartosz Golaszewski
2020-11-25 15:22 ` [PATCH 4/4] samples: configfs: add a committable group Bartosz Golaszewski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).