[PATCH 0/2 V3] Allow access to sysfs attributes without mem allocations.

[PATCH 0/2 V3] Allow access to sysfs attributes without mem allocations.

[NeilBrown]

Hi Greg,
 please consider these two patches for a future release.
 They now have
   Reviewed-by: Tejun Heo <tj@kernel.org>
 and I've addressed Tejun's final nits.

 Thanks Tejun for your review and comments.

 As soon as they appear in an -rc1, I'll submit updates to md to use
 the new functionality for the -rc2.

Thanks,
NeilBrown


---

NeilBrown (2):
      sysfs/kernfs: allow attributes to request write buffer be pre-allocated.
      sysfs/kernfs: make read requests on pre-alloc files use the buffer.


 fs/kernfs/file.c       |   73 +++++++++++++++++++++++++++++++++---------------
 fs/sysfs/file.c        |   57 ++++++++++++++++++++++++++++++++-----
 include/linux/kernfs.h |    8 +++++
 include/linux/sysfs.h  |    9 ++++++
 4 files changed, 116 insertions(+), 31 deletions(-)

-- 
Signature

[PATCH 2/2] sysfs/kernfs: make read requests on pre-alloc files use the buffer.

[NeilBrown]

To match the previous patch which used the pre-alloc buffer for
writes, this patch causes reads to use the same buffer.
This is not strictly necessary as the current seq_read() will allocate
on first read, so user-space can trigger the required pre-alloc.  But
consistency is valuable.

The read function is somewhat simpler than seq_read() and, for example,
does not support reading from an offset into the file: reads must be
at the start of the file.

As seq_read() does not use the prealloc buffer, ->seq_show is
incompatible with ->prealloc and caused an EINVAL return from open().
sysfs code which calls into kernfs always chooses the correct function.

As the buffer is shared with writes and other reads, the mutex is
extended to cover the copy_to_user.

Signed-off-by: NeilBrown <neilb@suse.de>
Reviewed-by: Tejun Heo <tj@kernel.org>
---
 fs/kernfs/file.c |   30 +++++++++++++++++++++---------
 fs/sysfs/file.c  |   32 ++++++++++++++++++++++++++++----
 2 files changed, 49 insertions(+), 13 deletions(-)

diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 70186e2e692a..697390ea47b8 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -189,13 +189,16 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
 	const struct kernfs_ops *ops;
 	char *buf;
 
-	buf = kmalloc(len, GFP_KERNEL);
+	buf = of->prealloc_buf;
+	if (!buf)
+		buf = kmalloc(len, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
 	/*
-	 * @of->mutex nests outside active ref and is primarily to ensure that
-	 * the ops aren't called concurrently for the same open file.
+	 * @of->mutex nests outside active ref and is used both to ensure that
+	 * the ops aren't called concurrently for the same open file, and
+	 * to provide exclusive access to ->prealloc_buf (when that exists).
 	 */
 	mutex_lock(&of->mutex);
 	if (!kernfs_get_active(of->kn)) {
@@ -210,21 +213,22 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
 	else
 		len = -EINVAL;
 
-	kernfs_put_active(of->kn);
-	mutex_unlock(&of->mutex);
-
 	if (len < 0)
-		goto out_free;
+		goto out_unlock;
 
 	if (copy_to_user(user_buf, buf, len)) {
 		len = -EFAULT;
-		goto out_free;
+		goto out_unlock;
 	}
 
 	*ppos += len;
 
+ out_unlock:
+	kernfs_put_active(of->kn);
+	mutex_unlock(&of->mutex);
  out_free:
-	kfree(buf);
+	if (buf != of->prealloc_buf)
+		kfree(buf);
 	return len;
 }
 
@@ -690,6 +694,14 @@ static int kernfs_fop_open(struct inode *inode, struct file *file)
 	 */
 	of->atomic_write_len = ops->atomic_write_len;
 
+	error = -EINVAL;
+	/*
+	 * ->seq_show is incompatible with ->prealloc,
+	 * as seq_read does its own allocation.
+	 * ->read must be used instead.
+	 */
+	if (ops->prealloc && ops->seq_show)
+		goto err_free;
 	if (ops->prealloc) {
 		int len = of->atomic_write_len ?: PAGE_SIZE;
 		of->prealloc_buf = kmalloc(len + 1, GFP_KERNEL);
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 4a959d231b43..9ff511a4e2da 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -102,6 +102,22 @@ static ssize_t sysfs_kf_bin_read(struct kernfs_open_file *of, char *buf,
 	return battr->read(of->file, kobj, battr, buf, pos, count);
 }
 
+/* kernfs read callback for regular sysfs files with pre-alloc */
+static ssize_t sysfs_kf_read(struct kernfs_open_file *of, char *buf,
+			     size_t count, loff_t pos)
+{
+	const struct sysfs_ops *ops = sysfs_file_ops(of->kn);
+	struct kobject *kobj = of->kn->parent->priv;
+
+	/*
+	 * If buf != of->prealloc_buf, we don't know how
+	 * large it is, so cannot safely pass it to ->show
+	 */
+	if (pos || WARN_ON_ONCE(buf != of->prealloc_buf)) {
+		return 0;
+	return ops->show(kobj, of->kn->priv, buf);
+}
+
 /* kernfs write callback for regular sysfs files */
 static ssize_t sysfs_kf_write(struct kernfs_open_file *of, char *buf,
 			      size_t count, loff_t pos)
@@ -184,13 +200,18 @@ static const struct kernfs_ops sysfs_file_kfops_rw = {
 	.write		= sysfs_kf_write,
 };
 
+static const struct kernfs_ops sysfs_prealloc_kfops_ro = {
+	.read		= sysfs_kf_read,
+	.prealloc	= true,
+};
+
 static const struct kernfs_ops sysfs_prealloc_kfops_wo = {
 	.write		= sysfs_kf_write,
 	.prealloc	= true,
 };
 
 static const struct kernfs_ops sysfs_prealloc_kfops_rw = {
-	.seq_show	= sysfs_kf_seq_show,
+	.read		= sysfs_kf_read,
 	.write		= sysfs_kf_write,
 	.prealloc	= true,
 };
@@ -238,9 +259,12 @@ int sysfs_add_file_mode_ns(struct kernfs_node *parent,
 				ops = &sysfs_prealloc_kfops_rw;
 			else
 				ops = &sysfs_file_kfops_rw;
-		} else if (sysfs_ops->show)
-			ops = &sysfs_file_kfops_ro;
-		else if (sysfs_ops->store) {
+		} else if (sysfs_ops->show) {
+			if (mode & SYSFS_PREALLOC)
+				ops = &sysfs_prealloc_kfops_ro;
+			else
+				ops = &sysfs_file_kfops_ro;
+		} else if (sysfs_ops->store) {
 			if (mode & SYSFS_PREALLOC)
 				ops = &sysfs_prealloc_kfops_wo;
 			else

[PATCH 2/2 - revised] sysfs/kernfs: make read requests on pre-alloc files use the buffer.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4933 bytes --]



To match the previous patch which used the pre-alloc buffer for
writes, this patch causes reads to use the same buffer.
This is not strictly necessary as the current seq_read() will allocate
on first read, so user-space can trigger the required pre-alloc.  But
consistency is valuable.

The read function is somewhat simpler than seq_read() and, for example,
does not support reading from an offset into the file: reads must be
at the start of the file.

As seq_read() does not use the prealloc buffer, ->seq_show is
incompatible with ->prealloc and caused an EINVAL return from open().
sysfs code which calls into kernfs always chooses the correct function.

As the buffer is shared with writes and other reads, the mutex is
extended to cover the copy_to_user.

Signed-off-by: NeilBrown <neilb@suse.de>
Reviewed-by: Tejun Heo <tj@kernel.org>

---

Please use this version instead.  Original had a last minute change which
lead to a compile error - sorry.

NeilBrown


diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 70186e2e692a..697390ea47b8 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -189,13 +189,16 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
 	const struct kernfs_ops *ops;
 	char *buf;
 
-	buf = kmalloc(len, GFP_KERNEL);
+	buf = of->prealloc_buf;
+	if (!buf)
+		buf = kmalloc(len, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
 	/*
-	 * @of->mutex nests outside active ref and is primarily to ensure that
-	 * the ops aren't called concurrently for the same open file.
+	 * @of->mutex nests outside active ref and is used both to ensure that
+	 * the ops aren't called concurrently for the same open file, and
+	 * to provide exclusive access to ->prealloc_buf (when that exists).
 	 */
 	mutex_lock(&of->mutex);
 	if (!kernfs_get_active(of->kn)) {
@@ -210,21 +213,22 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
 	else
 		len = -EINVAL;
 
-	kernfs_put_active(of->kn);
-	mutex_unlock(&of->mutex);
-
 	if (len < 0)
-		goto out_free;
+		goto out_unlock;
 
 	if (copy_to_user(user_buf, buf, len)) {
 		len = -EFAULT;
-		goto out_free;
+		goto out_unlock;
 	}
 
 	*ppos += len;
 
+ out_unlock:
+	kernfs_put_active(of->kn);
+	mutex_unlock(&of->mutex);
  out_free:
-	kfree(buf);
+	if (buf != of->prealloc_buf)
+		kfree(buf);
 	return len;
 }
 
@@ -690,6 +694,14 @@ static int kernfs_fop_open(struct inode *inode, struct file *file)
 	 */
 	of->atomic_write_len = ops->atomic_write_len;
 
+	error = -EINVAL;
+	/*
+	 * ->seq_show is incompatible with ->prealloc,
+	 * as seq_read does its own allocation.
+	 * ->read must be used instead.
+	 */
+	if (ops->prealloc && ops->seq_show)
+		goto err_free;
 	if (ops->prealloc) {
 		int len = of->atomic_write_len ?: PAGE_SIZE;
 		of->prealloc_buf = kmalloc(len + 1, GFP_KERNEL);
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 4a959d231b43..29a7c6f9dd9c 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -102,6 +102,22 @@ static ssize_t sysfs_kf_bin_read(struct kernfs_open_file *of, char *buf,
 	return battr->read(of->file, kobj, battr, buf, pos, count);
 }
 
+/* kernfs read callback for regular sysfs files with pre-alloc */
+static ssize_t sysfs_kf_read(struct kernfs_open_file *of, char *buf,
+			     size_t count, loff_t pos)
+{
+	const struct sysfs_ops *ops = sysfs_file_ops(of->kn);
+	struct kobject *kobj = of->kn->parent->priv;
+
+	/*
+	 * If buf != of->prealloc_buf, we don't know how
+	 * large it is, so cannot safely pass it to ->show
+	 */
+	if (pos || WARN_ON_ONCE(buf != of->prealloc_buf))
+		return 0;
+	return ops->show(kobj, of->kn->priv, buf);
+}
+
 /* kernfs write callback for regular sysfs files */
 static ssize_t sysfs_kf_write(struct kernfs_open_file *of, char *buf,
 			      size_t count, loff_t pos)
@@ -184,13 +200,18 @@ static const struct kernfs_ops sysfs_file_kfops_rw = {
 	.write		= sysfs_kf_write,
 };
 
+static const struct kernfs_ops sysfs_prealloc_kfops_ro = {
+	.read		= sysfs_kf_read,
+	.prealloc	= true,
+};
+
 static const struct kernfs_ops sysfs_prealloc_kfops_wo = {
 	.write		= sysfs_kf_write,
 	.prealloc	= true,
 };
 
 static const struct kernfs_ops sysfs_prealloc_kfops_rw = {
-	.seq_show	= sysfs_kf_seq_show,
+	.read		= sysfs_kf_read,
 	.write		= sysfs_kf_write,
 	.prealloc	= true,
 };
@@ -238,9 +259,12 @@ int sysfs_add_file_mode_ns(struct kernfs_node *parent,
 				ops = &sysfs_prealloc_kfops_rw;
 			else
 				ops = &sysfs_file_kfops_rw;
-		} else if (sysfs_ops->show)
-			ops = &sysfs_file_kfops_ro;
-		else if (sysfs_ops->store) {
+		} else if (sysfs_ops->show) {
+			if (mode & SYSFS_PREALLOC)
+				ops = &sysfs_prealloc_kfops_ro;
+			else
+				ops = &sysfs_file_kfops_ro;
+		} else if (sysfs_ops->store) {
 			if (mode & SYSFS_PREALLOC)
 				ops = &sysfs_prealloc_kfops_wo;
 			else

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[PATCH 1/2] sysfs/kernfs: allow attributes to request write buffer be pre-allocated.

[NeilBrown]

md/raid allows metadata management to be performed in user-space.
A various times, particularly on device failure, the metadata needs
to be updated before further writes can be permitted.
This means that the user-space program which updates metadata much
not block on writeout, and so must not allocate memory.

mlockall(MCL_CURRENT|MCL_FUTURE) and pre-allocation can avoid all
memory allocation issues for user-memory, but that does not help
kernel memory.
Several kernel objects can be pre-allocated.  e.g. files opened before
any writes to the array are permitted.
However some kernel allocation happens in places that cannot be
pre-allocated.
In particular, writes to sysfs files (to tell md that it can now
allow writes to the array) allocate a buffer using GFP_KERNEL.

This patch allows attributes to be marked as "PREALLOC".  In that case
the maximal buffer is allocated when the file is opened, and then used
on each write instead of allocating a new buffer.

As the same buffer is now shared for all writes on the same file
description, the mutex is extended to cover full use of the buffer
including the copy_from_user().

The new __ATTR_PREALLOC() 'or's a new flag in to the 'mode', which is
inspected by sysfs_add_file_mode_ns() to determine if the file should be
marked as requiring prealloc.

Despite the comment, we *do* use ->seq_show together with ->prealloc
in this patch.  The next patch fixes that.

Signed-off-by: NeilBrown  <neilb@suse.de>
Reviewed-by: Tejun Heo <tj@kernel.org>
---
 fs/kernfs/file.c       |   45 ++++++++++++++++++++++++++++++---------------
 fs/sysfs/file.c        |   31 ++++++++++++++++++++++++-------
 include/linux/kernfs.h |    8 ++++++++
 include/linux/sysfs.h  |    9 +++++++++
 4 files changed, 71 insertions(+), 22 deletions(-)

diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 4429d6d9217f..70186e2e692a 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -106,7 +106,7 @@ static void *kernfs_seq_start(struct seq_file *sf, loff_t *ppos)
 	const struct kernfs_ops *ops;
 
 	/*
-	 * @of->mutex nests outside active ref and is just to ensure that
+	 * @of->mutex nests outside active ref and is primarily to ensure that
 	 * the ops aren't called concurrently for the same open file.
 	 */
 	mutex_lock(&of->mutex);
@@ -194,7 +194,7 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
 		return -ENOMEM;
 
 	/*
-	 * @of->mutex nests outside active ref and is just to ensure that
+	 * @of->mutex nests outside active ref and is primarily to ensure that
 	 * the ops aren't called concurrently for the same open file.
 	 */
 	mutex_lock(&of->mutex);
@@ -278,19 +278,16 @@ static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf,
 		len = min_t(size_t, count, PAGE_SIZE);
 	}
 
-	buf = kmalloc(len + 1, GFP_KERNEL);
+	buf = of->prealloc_buf;
+	if (!buf)
+		buf = kmalloc(len + 1, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
-	if (copy_from_user(buf, user_buf, len)) {
-		len = -EFAULT;
-		goto out_free;
-	}
-	buf[len] = '\0';	/* guarantee string termination */
-
 	/*
-	 * @of->mutex nests outside active ref and is just to ensure that
-	 * the ops aren't called concurrently for the same open file.
+	 * @of->mutex nests outside active ref and is used both to ensure that
+	 * the ops aren't called concurrently for the same open file, and
+	 * to provide exclusive access to ->prealloc_buf (when that exists).
 	 */
 	mutex_lock(&of->mutex);
 	if (!kernfs_get_active(of->kn)) {
@@ -299,19 +296,27 @@ static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf,
 		goto out_free;
 	}
 
+	if (copy_from_user(buf, user_buf, len)) {
+		len = -EFAULT;
+		goto out_unlock;
+	}
+	buf[len] = '\0';	/* guarantee string termination */
+
 	ops = kernfs_ops(of->kn);
 	if (ops->write)
 		len = ops->write(of, buf, len, *ppos);
 	else
 		len = -EINVAL;
 
-	kernfs_put_active(of->kn);
-	mutex_unlock(&of->mutex);
-
 	if (len > 0)
 		*ppos += len;
+
+out_unlock:
+	kernfs_put_active(of->kn);
+	mutex_unlock(&of->mutex);
 out_free:
-	kfree(buf);
+	if (buf != of->prealloc_buf)
+		kfree(buf);
 	return len;
 }
 
@@ -685,6 +690,14 @@ static int kernfs_fop_open(struct inode *inode, struct file *file)
 	 */
 	of->atomic_write_len = ops->atomic_write_len;
 
+	if (ops->prealloc) {
+		int len = of->atomic_write_len ?: PAGE_SIZE;
+		of->prealloc_buf = kmalloc(len + 1, GFP_KERNEL);
+		error = -ENOMEM;
+		if (!of->prealloc_buf)
+			goto err_free;
+	}
+
 	/*
 	 * Always instantiate seq_file even if read access doesn't use
 	 * seq_file or is not requested.  This unifies private data access
@@ -715,6 +728,7 @@ static int kernfs_fop_open(struct inode *inode, struct file *file)
 err_close:
 	seq_release(inode, file);
 err_free:
+	kfree(of->prealloc_buf);
 	kfree(of);
 err_out:
 	kernfs_put_active(kn);
@@ -728,6 +742,7 @@ static int kernfs_fop_release(struct inode *inode, struct file *filp)
 
 	kernfs_put_open_node(kn, of);
 	seq_release(inode, filp);
+	kfree(of->prealloc_buf);
 	kfree(of);
 
 	return 0;
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index e9ef59b3abb1..4a959d231b43 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -184,6 +184,17 @@ static const struct kernfs_ops sysfs_file_kfops_rw = {
 	.write		= sysfs_kf_write,
 };
 
+static const struct kernfs_ops sysfs_prealloc_kfops_wo = {
+	.write		= sysfs_kf_write,
+	.prealloc	= true,
+};
+
+static const struct kernfs_ops sysfs_prealloc_kfops_rw = {
+	.seq_show	= sysfs_kf_seq_show,
+	.write		= sysfs_kf_write,
+	.prealloc	= true,
+};
+
 static const struct kernfs_ops sysfs_bin_kfops_ro = {
 	.read		= sysfs_kf_bin_read,
 };
@@ -222,13 +233,19 @@ int sysfs_add_file_mode_ns(struct kernfs_node *parent,
 			 kobject_name(kobj)))
 			return -EINVAL;
 
-		if (sysfs_ops->show && sysfs_ops->store)
-			ops = &sysfs_file_kfops_rw;
-		else if (sysfs_ops->show)
+		if (sysfs_ops->show && sysfs_ops->store) {
+			if (mode & SYSFS_PREALLOC)
+				ops = &sysfs_prealloc_kfops_rw;
+			else
+				ops = &sysfs_file_kfops_rw;
+		} else if (sysfs_ops->show)
 			ops = &sysfs_file_kfops_ro;
-		else if (sysfs_ops->store)
-			ops = &sysfs_file_kfops_wo;
-		else
+		else if (sysfs_ops->store) {
+			if (mode & SYSFS_PREALLOC)
+				ops = &sysfs_prealloc_kfops_wo;
+			else
+				ops = &sysfs_file_kfops_wo;
+		} else
 			ops = &sysfs_file_kfops_empty;
 
 		size = PAGE_SIZE;
@@ -253,7 +270,7 @@ int sysfs_add_file_mode_ns(struct kernfs_node *parent,
 	if (!attr->ignore_lockdep)
 		key = attr->key ?: (struct lock_class_key *)&attr->skey;
 #endif
-	kn = __kernfs_create_file(parent, attr->name, mode, size, ops,
+	kn = __kernfs_create_file(parent, attr->name, mode & 0777, size, ops,
 				  (void *)attr, ns, true, key);
 	if (IS_ERR(kn)) {
 		if (PTR_ERR(kn) == -EEXIST)
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index 30faf797c2c3..d4e01b358341 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -179,6 +179,7 @@ struct kernfs_open_file {
 	struct mutex		mutex;
 	int			event;
 	struct list_head	list;
+	char			*prealloc_buf;
 
 	size_t			atomic_write_len;
 	bool			mmapped;
@@ -214,6 +215,13 @@ struct kernfs_ops {
 	 * larger ones are rejected with -E2BIG.
 	 */
 	size_t atomic_write_len;
+	/*
+	 * "prealloc" causes a buffer to be allocated at open for
+	 * all read/write requests.  As ->seq_show uses seq_read()
+	 * which does its own allocation, it is incompatible with
+	 * ->prealloc.  Provide ->read and ->write with ->prealloc.
+	 */
+	bool prealloc;
 	ssize_t (*write)(struct kernfs_open_file *of, char *buf, size_t bytes,
 			 loff_t off);
 
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index f97d0dbb59fa..ddad16148bd6 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -70,6 +70,8 @@ struct attribute_group {
  * for examples..
  */
 
+#define SYSFS_PREALLOC 010000
+
 #define __ATTR(_name, _mode, _show, _store) {				\
 	.attr = {.name = __stringify(_name),				\
 		 .mode = VERIFY_OCTAL_PERMISSIONS(_mode) },		\
@@ -77,6 +79,13 @@ struct attribute_group {
 	.store	= _store,						\
 }
 
+#define __ATTR_PREALLOC(_name, _mode, _show, _store) {			\
+	.attr = {.name = __stringify(_name),				\
+		 .mode = SYSFS_PREALLOC | VERIFY_OCTAL_PERMISSIONS(_mode) },\
+	.show	= _show,						\
+	.store	= _store,						\
+}
+
 #define __ATTR_RO(_name) {						\
 	.attr	= { .name = __stringify(_name), .mode = S_IRUGO },	\
 	.show	= _name##_show,						\

Re: [PATCH 0/2 V3] Allow access to sysfs attributes without mem allocations.

[Greg Kroah-Hartman]

On Mon, Oct 13, 2014 at 04:41:28PM +1100, NeilBrown wrote:
> Hi Greg,
>  please consider these two patches for a future release.
>  They now have
>    Reviewed-by: Tejun Heo <tj@kernel.org>
>  and I've addressed Tejun's final nits.
> 
>  Thanks Tejun for your review and comments.
> 
>  As soon as they appear in an -rc1, I'll submit updates to md to use
>  the new functionality for the -rc2.

It will have to wait until 3.19-rc1, I'll queue it up in my tree after
3.18-rc1 is out.  You can merge from my driver-core tree into yours if
you want to build off of it, as my tree will not be rebased.

thanks,

greg k-h

Re: [PATCH 0/2 V3] Allow access to sysfs attributes without mem allocations.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 790 bytes --]

On Mon, 13 Oct 2014 10:35:12 +0200 Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:

> On Mon, Oct 13, 2014 at 04:41:28PM +1100, NeilBrown wrote:
> > Hi Greg,
> >  please consider these two patches for a future release.
> >  They now have
> >    Reviewed-by: Tejun Heo <tj@kernel.org>
> >  and I've addressed Tejun's final nits.
> > 
> >  Thanks Tejun for your review and comments.
> > 
> >  As soon as they appear in an -rc1, I'll submit updates to md to use
> >  the new functionality for the -rc2.
> 
> It will have to wait until 3.19-rc1, I'll queue it up in my tree after
> 3.18-rc1 is out.  You can merge from my driver-core tree into yours if
> you want to build off of it, as my tree will not be rebased.
> 

All fine with me - thanks a lot.

NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[PATCH 0/2 V2] Allow access to sysfs attributes without mem allocations.

[NeilBrown]

Hi Tejun,
 thanks for the review.  This version incorporates your suggestions.

 You are certainly right about there being issues deeper than just the
 sysfs/kernfs level - I've been cleaning up the locking in md to avoid
 the "reconfig_mutex" as much as possible.  It is something that has
 been at the back of my mind for a while, but this need has pushed it
 to the fore-front.  All the attributes that mdmon needs to touch will
 soon only take a spinlock or nothing.

 On the topic of inadvertently locking in semantics, I noticed that
 prior to kernfs, sysfs would allocate a buffer on first read or
 write, and continue to use that buffer.  I could try to argue that I
 was using those semantics, which have now changed ... though I wasn't
 doing it consciously...  Certainly this is a thorny issue.

 As well as checking that ->seq_show isn't used with ->prealloc,
 I've also made sure that sysfs_kf_read() doesn't call the attribute
 ->show() function unless it is certain it has a preallocated (and
 hence >= PAGE_SIZE) buffer.

Thanks,
NeilBrown

---

NeilBrown (2):
      sysfs/kernfs: allow attributes to request write buffer be pre-allocated.
      sysfs/kernfs: make read requests on pre-alloc files use the buffer.


 fs/kernfs/file.c       |   70 ++++++++++++++++++++++++++++++++----------------
 fs/sysfs/file.c        |   53 +++++++++++++++++++++++++++++++-----
 include/linux/kernfs.h |    2 +
 include/linux/sysfs.h  |    9 ++++++
 4 files changed, 103 insertions(+), 31 deletions(-)

-- 
Signature

[PATCH 2/2] sysfs/kernfs: make read requests on pre-alloc files use the buffer.

[NeilBrown]

To match the previous patch which used the pre-alloc buffer for
writes, this patch causes reads to use the same buffer.
This is not strictly necessary as the current seq_read() will allocate
on first read, so user-space can trigger the required pre-alloc.  But
consistency is valuable.

The read function is somewhat simpler than seq_read() and, for example,
does not support reading from an offset into the file: reads must be
at the start of the file.

As seq_read() does not use the prealloc buffer, ->seq_show is
incompatible with ->prealloc and caused an EINVAL return from open().
sysfs code which calls into kernfs always chooses the correct function.

As the buffer is shared with writes and other reads, the mutex is
extended to cover the copy_to_user.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/kernfs/file.c |   28 +++++++++++++++++++---------
 fs/sysfs/file.c  |   31 +++++++++++++++++++++++++++----
 2 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 137c172bccc5..e055c6a01495 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -189,13 +189,16 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
 	const struct kernfs_ops *ops;
 	char *buf;
 
-	buf = kmalloc(len, GFP_KERNEL);
+	buf = of->prealloc_buf;
+	if (!buf)
+		buf = kmalloc(len, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
 	/*
-	 * @of->mutex nests outside active ref and is primarily to ensure that
-	 * the ops aren't called concurrently for the same open file.
+	 * @of->mutex nests outside active ref and is used both to ensure that
+	 * the ops aren't called concurrently for the same open file, and
+	 * to provide exclusive access to ->prealloc_buf (when that exists).
 	 */
 	mutex_lock(&of->mutex);
 	if (!kernfs_get_active(of->kn)) {
@@ -210,21 +213,22 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
 	else
 		len = -EINVAL;
 
-	kernfs_put_active(of->kn);
-	mutex_unlock(&of->mutex);
-
 	if (len < 0)
-		goto out_free;
+		goto out_unlock;
 
 	if (copy_to_user(user_buf, buf, len)) {
 		len = -EFAULT;
-		goto out_free;
+		goto out_unlock;
 	}
 
 	*ppos += len;
 
+ out_unlock:
+	kernfs_put_active(of->kn);
+	mutex_unlock(&of->mutex);
  out_free:
-	kfree(buf);
+	if (buf != of->prealloc_buf)
+		kfree(buf);
 	return len;
 }
 
@@ -690,6 +694,12 @@ static int kernfs_fop_open(struct inode *inode, struct file *file)
 	 */
 	of->atomic_write_len = ops->atomic_write_len;
 
+	error = -EINVAL;
+	if (ops->prealloc && ops->seq_show)
+		/* ->seq_show is incompatible with ->prealloc,
+		 * ->read must be used instead.
+		 */
+		goto err_free;
 	if (ops->prealloc) {
 		int len = of->atomic_write_len ?: PAGE_SIZE;
 		of->prealloc_buf = kmalloc(len + 1, GFP_KERNEL);
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 4a959d231b43..0b02cc515273 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -102,6 +102,21 @@ static ssize_t sysfs_kf_bin_read(struct kernfs_open_file *of, char *buf,
 	return battr->read(of->file, kobj, battr, buf, pos, count);
 }
 
+/* kernfs read callback for regular sysfs files with pre-alloc */
+static ssize_t sysfs_kf_read(struct kernfs_open_file *of, char *buf,
+                            size_t count, loff_t pos)
+{
+       const struct sysfs_ops *ops = sysfs_file_ops(of->kn);
+       struct kobject *kobj = of->kn->parent->priv;
+
+       if (pos || buf != of->prealloc_buf)
+	       /* If buf != of->prealloc_buf, we don't know how
+		* large it is, so cannot safely pass it to ->show
+		*/
+               return 0;
+       return ops->show(kobj, of->kn->priv, buf);
+}
+
 /* kernfs write callback for regular sysfs files */
 static ssize_t sysfs_kf_write(struct kernfs_open_file *of, char *buf,
 			      size_t count, loff_t pos)
@@ -184,13 +199,18 @@ static const struct kernfs_ops sysfs_file_kfops_rw = {
 	.write		= sysfs_kf_write,
 };
 
+static const struct kernfs_ops sysfs_prealloc_kfops_ro = {
+	.read		= sysfs_kf_read,
+	.prealloc	= true,
+};
+
 static const struct kernfs_ops sysfs_prealloc_kfops_wo = {
 	.write		= sysfs_kf_write,
 	.prealloc	= true,
 };
 
 static const struct kernfs_ops sysfs_prealloc_kfops_rw = {
-	.seq_show	= sysfs_kf_seq_show,
+	.read		= sysfs_kf_read,
 	.write		= sysfs_kf_write,
 	.prealloc	= true,
 };
@@ -238,9 +258,12 @@ int sysfs_add_file_mode_ns(struct kernfs_node *parent,
 				ops = &sysfs_prealloc_kfops_rw;
 			else
 				ops = &sysfs_file_kfops_rw;
-		} else if (sysfs_ops->show)
-			ops = &sysfs_file_kfops_ro;
-		else if (sysfs_ops->store) {
+		} else if (sysfs_ops->show) {
+			if (mode & SYSFS_PREALLOC)
+				ops = &sysfs_prealloc_kfops_ro;
+			else
+				ops = &sysfs_file_kfops_ro;
+		} else if (sysfs_ops->store) {
 			if (mode & SYSFS_PREALLOC)
 				ops = &sysfs_prealloc_kfops_wo;
 			else

Re: [PATCH 2/2] sysfs/kernfs: make read requests on pre-alloc files use the buffer.

[Tejun Heo]

On Thu, Oct 09, 2014 at 10:57:06AM +1100, NeilBrown wrote:
> To match the previous patch which used the pre-alloc buffer for
> writes, this patch causes reads to use the same buffer.
> This is not strictly necessary as the current seq_read() will allocate
> on first read, so user-space can trigger the required pre-alloc.  But
> consistency is valuable.
> 
> The read function is somewhat simpler than seq_read() and, for example,
> does not support reading from an offset into the file: reads must be
> at the start of the file.
> 
> As seq_read() does not use the prealloc buffer, ->seq_show is
> incompatible with ->prealloc and caused an EINVAL return from open().
> sysfs code which calls into kernfs always chooses the correct function.
> 
> As the buffer is shared with writes and other reads, the mutex is
> extended to cover the copy_to_user.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>

Reviewed-by: Tejun Heo <tj@kernel.org>

Some nitpicks.

> @@ -690,6 +694,12 @@ static int kernfs_fop_open(struct inode *inode, struct file *file)
>  	 */
>  	of->atomic_write_len = ops->atomic_write_len;
>  
> +	error = -EINVAL;
> +	if (ops->prealloc && ops->seq_show)
> +		/* ->seq_show is incompatible with ->prealloc,
> +		 * ->read must be used instead.
> +		 */
> +		goto err_free;

Let's please use fully-winged comments.  If it looks weird inside the
if block, it can just be located right on top, I think.  Also,
wouldn't it be better if the comment explained the reason for the
incompatibility?  Along the same line, I think it'd be better if this
is also explicitly explained where ->prealloc is defined.

> +/* kernfs read callback for regular sysfs files with pre-alloc */
> +static ssize_t sysfs_kf_read(struct kernfs_open_file *of, char *buf,
> +                            size_t count, loff_t pos)
> +{
> +       const struct sysfs_ops *ops = sysfs_file_ops(of->kn);
> +       struct kobject *kobj = of->kn->parent->priv;
> +
> +       if (pos || buf != of->prealloc_buf)
> +	       /* If buf != of->prealloc_buf, we don't know how
> +		* large it is, so cannot safely pass it to ->show
> +		*/
> +               return 0;
> +       return ops->show(kobj, of->kn->priv, buf);
> +}

Ditto on the comment formatting also shouldn't the latter condition be
a WARN_ON_ONCE()?

Thanks.

-- 
tejun

[PATCH 0/2] Allow access to sysfs attributes without mem allocations.

[NeilBrown]

Hi Tejun,
 is this closer to what you would like?

 I do really need this functionality, but I honestly don't like this
 patch.
 The need to identify just which attributes need special care seems
 backwards to me.
  1/ it is the process (which has called mlock etc) which needs
     special care, more than the attribute.  Everything that process
     does needs to avoid allocating memory, whether that this is
     particularly related to enabling write-out or not.
  2/ It is also backwards because the vast majority of sysfs
     attributes only support bool/enum/int which means at most
     23 chars including sign and newline (maybe more for reads if the
     read provides a list of options).  So almost everything
     doesn't need an allocation at all - just some on-stack space.
     I would be quite happy to identify those attributes that
     may need care when accessing and could possibly supports
     read or write > 128 characters, because there wouldn't be any.

  I also don't like this approach because we end up allocating 2 pages
  for the buffer, as we have to ask for "PAGE_SIZE+1" bytes, for the
  hypothetical case that an important attribute actually requires a
  full PAGE_SIZE write...

  Would you be happy to have all specially identified attributes be
  limited to 127 characters IO max?  Then I would just use an on-stack
  buffer for those which would remove the allocation and simplify some
  of the code.

Thanks,
NeilBrown

---

NeilBrown (2):
      sysfs - allow attributes to request write buffer be pre-allocated.
      sysfs/kernfs: make read requests on pre-alloc files use the buffer.


 fs/kernfs/file.c       |   52 +++++++++++++++++++++++++++++++----------------
 fs/sysfs/file.c        |   53 +++++++++++++++++++++++++++++++++++++++++-------
 include/linux/kernfs.h |    2 ++
 include/linux/sysfs.h  |    9 ++++++++
 4 files changed, 90 insertions(+), 26 deletions(-)

-- 
Signature

[PATCH 2/2] sysfs/kernfs: make read requests on pre-alloc files use the buffer.

[NeilBrown]

To match the previous patch which used the pre-alloc buffer for
writes, this patch causes reads to use the same buffer.
This is not strictly necessary as the current seq_read() will allocate
on first read, so user-space can trigger the required pre-alloc.  But
consistency is valuable.

The read function is somewhat simpler than seq_read() and, for example,
does not support reading from an offset into the file: reads must be
at the start of the file.

As the buffer is shared with writes and other reads, the mutex is
extended to cover the copy_to_user.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/kernfs/file.c |   17 ++++++++++-------
 fs/sysfs/file.c  |   28 ++++++++++++++++++++++++----
 2 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 73bd5ed143cd..7072240604f5 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -189,7 +189,9 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
 	const struct kernfs_ops *ops;
 	char *buf;
 
-	buf = kmalloc(len, GFP_KERNEL);
+	buf = of->buf;
+	if (!buf)
+		buf = kmalloc(len, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
@@ -210,21 +212,22 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
 	else
 		len = -EINVAL;
 
-	kernfs_put_active(of->kn);
-	mutex_unlock(&of->mutex);
-
 	if (len < 0)
-		goto out_free;
+		goto out_unlock;
 
 	if (copy_to_user(user_buf, buf, len)) {
 		len = -EFAULT;
-		goto out_free;
+		goto out_unlock;
 	}
 
 	*ppos += len;
 
+ out_unlock:
+	kernfs_put_active(of->kn);
+	mutex_unlock(&of->mutex);
  out_free:
-	kfree(buf);
+	if (buf != of->buf)
+		kfree(buf);
 	return len;
 }
 
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 4a959d231b43..3f7e4d341546 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -102,6 +102,18 @@ static ssize_t sysfs_kf_bin_read(struct kernfs_open_file *of, char *buf,
 	return battr->read(of->file, kobj, battr, buf, pos, count);
 }
 
+/* kernfs read callback for regular sysfs files with pre-alloc */
+static ssize_t sysfs_kf_read(struct kernfs_open_file *of, char *buf,
+                            size_t count, loff_t pos)
+{
+       const struct sysfs_ops *ops = sysfs_file_ops(of->kn);
+       struct kobject *kobj = of->kn->parent->priv;
+
+       if (pos)
+               return 0;
+       return ops->show(kobj, of->kn->priv, buf);
+}
+
 /* kernfs write callback for regular sysfs files */
 static ssize_t sysfs_kf_write(struct kernfs_open_file *of, char *buf,
 			      size_t count, loff_t pos)
@@ -184,13 +196,18 @@ static const struct kernfs_ops sysfs_file_kfops_rw = {
 	.write		= sysfs_kf_write,
 };
 
+static const struct kernfs_ops sysfs_prealloc_kfops_ro = {
+	.read		= sysfs_kf_read,
+	.prealloc	= true,
+};
+
 static const struct kernfs_ops sysfs_prealloc_kfops_wo = {
 	.write		= sysfs_kf_write,
 	.prealloc	= true,
 };
 
 static const struct kernfs_ops sysfs_prealloc_kfops_rw = {
-	.seq_show	= sysfs_kf_seq_show,
+	.read		= sysfs_kf_read,
 	.write		= sysfs_kf_write,
 	.prealloc	= true,
 };
@@ -238,9 +255,12 @@ int sysfs_add_file_mode_ns(struct kernfs_node *parent,
 				ops = &sysfs_prealloc_kfops_rw;
 			else
 				ops = &sysfs_file_kfops_rw;
-		} else if (sysfs_ops->show)
-			ops = &sysfs_file_kfops_ro;
-		else if (sysfs_ops->store) {
+		} else if (sysfs_ops->show) {
+			if (mode & SYSFS_PREALLOC)
+				ops = &sysfs_prealloc_kfops_ro;
+			else
+				ops = &sysfs_file_kfops_ro;
+		} else if (sysfs_ops->store) {
 			if (mode & SYSFS_PREALLOC)
 				ops = &sysfs_prealloc_kfops_wo;
 			else

Re: [PATCH 2/2] sysfs/kernfs: make read requests on pre-alloc files use the buffer.

[Tejun Heo]

On Tue, Sep 30, 2014 at 12:33:34PM +1000, NeilBrown wrote:
> To match the previous patch which used the pre-alloc buffer for
> writes, this patch causes reads to use the same buffer.
> This is not strictly necessary as the current seq_read() will allocate
> on first read, so user-space can trigger the required pre-alloc.  But
> consistency is valuable.
> 
> The read function is somewhat simpler than seq_read() and, for example,
> does not support reading from an offset into the file: reads must be
> at the start of the file.
> 
> As the buffer is shared with writes and other reads, the mutex is
> extended to cover the copy_to_user.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/kernfs/file.c |   17 ++++++++++-------
>  fs/sysfs/file.c  |   28 ++++++++++++++++++++++++----
>  2 files changed, 34 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> index 73bd5ed143cd..7072240604f5 100644
> --- a/fs/kernfs/file.c
> +++ b/fs/kernfs/file.c
> @@ -189,7 +189,9 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
>  	const struct kernfs_ops *ops;
>  	char *buf;
>  
> -	buf = kmalloc(len, GFP_KERNEL);
> +	buf = of->buf;
> +	if (!buf)
> +		buf = kmalloc(len, GFP_KERNEL);
>  	if (!buf)
>  		return -ENOMEM;
>  
> @@ -210,21 +212,22 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
>  	else
>  		len = -EINVAL;
>  
> -	kernfs_put_active(of->kn);
> -	mutex_unlock(&of->mutex);
> -
>  	if (len < 0)
> -		goto out_free;
> +		goto out_unlock;
>  
>  	if (copy_to_user(user_buf, buf, len)) {
>  		len = -EFAULT;
> -		goto out_free;
> +		goto out_unlock;
>  	}
>  
>  	*ppos += len;
>  
> + out_unlock:
> +	kernfs_put_active(of->kn);
> +	mutex_unlock(&of->mutex);
>   out_free:
> -	kfree(buf);
> +	if (buf != of->buf)
> +		kfree(buf);
>  	return len;
>  }

Can you please also make kernfs reject ->seq_show() if .prealloc is
being used?

> +/* kernfs read callback for regular sysfs files with pre-alloc */
> +static ssize_t sysfs_kf_read(struct kernfs_open_file *of, char *buf,
> +                            size_t count, loff_t pos)
> +{
> +       const struct sysfs_ops *ops = sysfs_file_ops(of->kn);
> +       struct kobject *kobj = of->kn->parent->priv;
> +
> +       if (pos)
> +               return 0;
> +       return ops->show(kobj, of->kn->priv, buf);

Shouldn't it also memset?

Thanks.

-- 
tejun