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

[Tejun Heo <tj@kernel.org>]

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