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 <ne...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
