[PATCH 2/3] buffer writes to sysfs

[PATCH 2/3] buffer writes to sysfs

[Mitch Williams]

This patch buffers writes to sysfs files and flushes them to the kobject
owner when the file is closed.

Generated from 2.6.11-rc1.

Signed-off-by:  Mitch Williams <mitch.a.williams@intel.com>

diff -uprN -X dontdiff linux-2.6.11-clean/fs/sysfs/file.c linux-2.6.11/fs/sysfs/file.c
--- linux-2.6.11-clean/fs/sysfs/file.c	2004-12-24 13:33:50.000000000 -0800
+++ linux-2.6.11/fs/sysfs/file.c	2005-01-21 13:09:21.000000000 -0800
@@ -62,6 +62,7 @@ struct sysfs_buffer {
 	struct sysfs_ops	* ops;
 	struct semaphore	sem;
 	int			read_filled;
+	int			needs_write_flush;
 };


@@ -178,7 +178,8 @@ out:
  */

 static int
-fill_write_buffer(struct sysfs_buffer * buffer, const char __user * buf, size_t count)
+fill_write_buffer(struct sysfs_buffer *buffer, const char __user * buf,
+		  size_t count, size_t pos)
 {
 	int error;

@@ -187,10 +189,11 @@ fill_write_buffer(struct sysfs_buffer *
 	if (!buffer->page)
 		return -ENOMEM;

-	if (count >= PAGE_SIZE)
-		count = PAGE_SIZE - 1;
+	if (count + pos > PAGE_SIZE)
+		count = (PAGE_SIZE - 1) - pos;
 	error = copy_from_user(buffer->page,buf,count);
-	buffer->needs_read_fill = 1;
+	buffer->needs_write_flush = 1;
+        buffer->count = pos + count;
 	return error ? -EFAULT : count;
 }

@@ -206,13 +209,13 @@ fill_write_buffer(struct sysfs_buffer *
  */

 static int
-flush_write_buffer(struct dentry * dentry, struct sysfs_buffer * buffer, size_t count)
+flush_write_buffer(struct dentry * dentry, struct sysfs_buffer * buffer)
 {
 	struct attribute * attr = to_attr(dentry);
 	struct kobject * kobj = to_kobj(dentry->d_parent);
 	struct sysfs_ops * ops = buffer->ops;

-	return ops->store(kobj,attr,buffer->page,count);
+	return ops->store(kobj,attr, buffer->page, buffer->count);
 }


@@ -225,12 +228,9 @@ flush_write_buffer(struct dentry * dentr
  *
  *	Similar to sysfs_read_file(), though working in the opposite direction.
  *	We allocate and fill the data from the user in fill_write_buffer(),
- *	then push it to the kobject in flush_write_buffer().
- *	There is no easy way for us to know if userspace is only doing a partial
- *	write, so we don't support them. We expect the entire buffer to come
- *	on the first write.
- *	Hint: if you're writing a value, first read the file, modify only the
- *	the value you're changing, then write entire buffer back.
+ *	but don't push it to the kobject until the file is closed.
+ *      This allows for buffered writes, but unfortunately also hides error
+ *      codes returned individual store functions until close time.
  */

 static ssize_t
@@ -238,10 +238,10 @@ sysfs_write_file(struct file *file, cons
 {
 	struct sysfs_buffer * buffer = file->private_data;

+	if (*ppos >= PAGE_SIZE)
+		return -ENOSPC;
 	down(&buffer->sem);
-	count = fill_write_buffer(buffer,buf,count);
-	if (count > 0)
-		count = flush_write_buffer(file->f_dentry,buffer,count);
+	count = fill_write_buffer(buffer, buf, count, *ppos);
 	if (count > 0)
 		*ppos += count;
 	up(&buffer->sem);
@@ -337,6 +337,17 @@ static int sysfs_release(struct inode *
 	struct attribute * attr = to_attr(filp->f_dentry);
 	struct module * owner = attr->owner;
 	struct sysfs_buffer * buffer = filp->private_data;
+        int ret;
+
+	/* If data has been written to the file, then flush it
+	 * back to the kobject's store function here.
+	 */
+	if (buffer && kobj) {
+		down(&buffer->sem);
+		if (buffer->needs_write_flush)
+			ret = flush_write_buffer(filp->f_dentry, buffer);
+		up(&buffer->sem);
+	}

 	if (kobj)
 		kobject_put(kobj);
@@ -348,7 +352,10 @@ static int sysfs_release(struct inode *
 			free_page((unsigned long)buffer->page);
 		kfree(buffer);
 	}
-	return 0;
+	/* If flush_write_buffer returned an error, pass it up.
+	 * Otherwise, return success.
+	 */
+	return (ret < 0 ? ret : 0);
 }

 struct file_operations sysfs_file_operations = {

Re: [PATCH 2/3] buffer writes to sysfs

[Greg KH]

On Fri, Jan 21, 2005 at 02:52:29PM -0800, Mitch Williams wrote:
> This patch buffers writes to sysfs files and flushes them to the kobject
> owner when the file is closed.

Why?  This breaks the way things work today, right? 

What is this patch trying to fix?

> 
> Generated from 2.6.11-rc1.
> 
> Signed-off-by:  Mitch Williams <mitch.a.williams@intel.com>
> 
> diff -uprN -X dontdiff linux-2.6.11-clean/fs/sysfs/file.c linux-2.6.11/fs/sysfs/file.c
> --- linux-2.6.11-clean/fs/sysfs/file.c	2004-12-24 13:33:50.000000000 -0800
> +++ linux-2.6.11/fs/sysfs/file.c	2005-01-21 13:09:21.000000000 -0800
> @@ -62,6 +62,7 @@ struct sysfs_buffer {
>  	struct sysfs_ops	* ops;
>  	struct semaphore	sem;
>  	int			read_filled;
> +	int			needs_write_flush;
>  };
> 
> 
> @@ -178,7 +178,8 @@ out:
>   */
> 
>  static int
> -fill_write_buffer(struct sysfs_buffer * buffer, const char __user * buf, size_t count)
> +fill_write_buffer(struct sysfs_buffer *buffer, const char __user * buf,
> +		  size_t count, size_t pos)
>  {
>  	int error;
> 
> @@ -187,10 +189,11 @@ fill_write_buffer(struct sysfs_buffer *
>  	if (!buffer->page)
>  		return -ENOMEM;
> 
> -	if (count >= PAGE_SIZE)
> -		count = PAGE_SIZE - 1;
> +	if (count + pos > PAGE_SIZE)
> +		count = (PAGE_SIZE - 1) - pos;
>  	error = copy_from_user(buffer->page,buf,count);
> -	buffer->needs_read_fill = 1;
> +	buffer->needs_write_flush = 1;
> +        buffer->count = pos + count;
>  	return error ? -EFAULT : count;
>  }
> 
> @@ -206,13 +209,13 @@ fill_write_buffer(struct sysfs_buffer *
>   */
> 
>  static int
> -flush_write_buffer(struct dentry * dentry, struct sysfs_buffer * buffer, size_t count)
> +flush_write_buffer(struct dentry * dentry, struct sysfs_buffer * buffer)
>  {
>  	struct attribute * attr = to_attr(dentry);
>  	struct kobject * kobj = to_kobj(dentry->d_parent);
>  	struct sysfs_ops * ops = buffer->ops;
> 
> -	return ops->store(kobj,attr,buffer->page,count);
> +	return ops->store(kobj,attr, buffer->page, buffer->count);
>  }
> 
> 
> @@ -225,12 +228,9 @@ flush_write_buffer(struct dentry * dentr
>   *
>   *	Similar to sysfs_read_file(), though working in the opposite direction.
>   *	We allocate and fill the data from the user in fill_write_buffer(),
> - *	then push it to the kobject in flush_write_buffer().
> - *	There is no easy way for us to know if userspace is only doing a partial
> - *	write, so we don't support them. We expect the entire buffer to come
> - *	on the first write.
> - *	Hint: if you're writing a value, first read the file, modify only the
> - *	the value you're changing, then write entire buffer back.
> + *	but don't push it to the kobject until the file is closed.
> + *      This allows for buffered writes, but unfortunately also hides error
> + *      codes returned individual store functions until close time.
>   */
> 
>  static ssize_t
> @@ -238,10 +238,10 @@ sysfs_write_file(struct file *file, cons
>  {
>  	struct sysfs_buffer * buffer = file->private_data;
> 
> +	if (*ppos >= PAGE_SIZE)
> +		return -ENOSPC;
>  	down(&buffer->sem);
> -	count = fill_write_buffer(buffer,buf,count);
> -	if (count > 0)
> -		count = flush_write_buffer(file->f_dentry,buffer,count);
> +	count = fill_write_buffer(buffer, buf, count, *ppos);
>  	if (count > 0)
>  		*ppos += count;
>  	up(&buffer->sem);
> @@ -337,6 +337,17 @@ static int sysfs_release(struct inode *
>  	struct attribute * attr = to_attr(filp->f_dentry);
>  	struct module * owner = attr->owner;
>  	struct sysfs_buffer * buffer = filp->private_data;
> +        int ret;

You used spaces instead of a tab here.

> +
> +	/* If data has been written to the file, then flush it
> +	 * back to the kobject's store function here.
> +	 */
> +	if (buffer && kobj) {
> +		down(&buffer->sem);
> +		if (buffer->needs_write_flush)
> +			ret = flush_write_buffer(filp->f_dentry, buffer);
> +		up(&buffer->sem);
> +	}
> 
>  	if (kobj)
>  		kobject_put(kobj);
> @@ -348,7 +352,10 @@ static int sysfs_release(struct inode *
>  			free_page((unsigned long)buffer->page);
>  		kfree(buffer);
>  	}
> -	return 0;
> +	/* If flush_write_buffer returned an error, pass it up.
> +	 * Otherwise, return success.
> +	 */
> +	return (ret < 0 ? ret : 0);

I think this comment is not needed.  Actually, what's wrong with just:
	return ret;

thanks,

greg k-h

Re: [PATCH 2/3] buffer writes to sysfs

[Mitch Williams]

On Sat, 22 Jan 2005, Greg KH wrote:

>
> On Fri, Jan 21, 2005 at 02:52:29PM -0800, Mitch Williams wrote:
> > This patch buffers writes to sysfs files and flushes them to the
> kobject
> > owner when the file is closed.
>
> Why?  This breaks the way things work today, right?
>
> What is this patch trying to fix?
>

To be honest, I'm a bit ambivalent about this patch.  I wrote this code in
response to a bug filed by our test lab.  If you write a bunch (e.g. > 1K)
of data to a sysfs file, the c library splits it up into multiple writes
of 1K or less.  Because the kobject store method doesn't support offsets,
and because the call to copy_from_user doesn't honor offsets, you end up
with multiple calls to the store method, with incorrect results and no
error code.

Thus, this patch, which coalesces all writes to the file (up to PAGE_SIZE)
into one write.

To the typical user, there's really no difference in behavior, unless you
are writing a ton of data into the file.  Of course, there's the obvious
question of why you'd want to do so...

If you don't like this patch, I would say that there are a couple of
alternatives:

First, we could fix the call to copy_from_user to honor offsets and allow
multiple writes of the same data to sysfs.  In other words, writing a long
block of data to a sysfs file would result in multiple calls to the store
method, each one containing more data in the buffer.  Thus, the first 1K
of data may be processed up to 4 times if the data fills the buffer.

Second, we could just have sysfs_write_file return an error anytime *ppos
is nonzero.  It's quick and dirty but at least the caller would get some
sort of error.

The patch as presented has the disadvantage of hiding possible error codes
from the store method util the file is closed, but has the advantage of
allowing > 1K writes with only a single call to the store method.  Would
it make a difference in the real world?  I don't know, but I know it'll
make our test lab guys happy.



> > +      * Otherwise, return success.
> > +      */
> > +     return (ret < 0 ? ret : 0);
>
> I think this comment is not needed.  Actually, what's wrong with just:
>         return ret;
In this case, ret could contain a positive result.  Since a lot of
user-space apps just look for a nonzero result from close(), it seemed
safer to suppres positive return codes in the case of success.

Thanks for your patience with this set of patches.

-Mitch

Re: [PATCH 2/3] buffer writes to sysfs

[Greg KH]

On Mon, Jan 24, 2005 at 10:37:00AM -0800, Mitch Williams wrote:
> 
> 
> On Sat, 22 Jan 2005, Greg KH wrote:
> 
> >
> > On Fri, Jan 21, 2005 at 02:52:29PM -0800, Mitch Williams wrote:
> > > This patch buffers writes to sysfs files and flushes them to the
> > kobject
> > > owner when the file is closed.
> >
> > Why?  This breaks the way things work today, right?
> >
> > What is this patch trying to fix?
> >
> 
> To be honest, I'm a bit ambivalent about this patch.  I wrote this code in
> response to a bug filed by our test lab.  If you write a bunch (e.g. > 1K)
> of data to a sysfs file, the c library splits it up into multiple writes
> of 1K or less.  Because the kobject store method doesn't support offsets,
> and because the call to copy_from_user doesn't honor offsets, you end up
> with multiple calls to the store method, with incorrect results and no
> error code.

Who is trying to send > 1K to a sysfs file?  Remember, sysfs files are
for 1 value only.  If you consider > 1K a "single value" please point me
to that part of the kernel that does that.

> To the typical user, there's really no difference in behavior, unless you
> are writing a ton of data into the file.  Of course, there's the obvious
> question of why you'd want to do so...

Exactly, you should not be doing that anyway.  So, because of that, I
really don't want/like this patch.

thanks,

greg k-h

Re: [PATCH 2/3] buffer writes to sysfs

[Mitch Williams]

On Mon, 24 Jan 2005, Greg KH wrote:
>
> Who is trying to send > 1K to a sysfs file?  Remember, sysfs files are
> for 1 value only.  If you consider > 1K a "single value" please point me
> to that part of the kernel that does that.
>
> > To the typical user, there's really no difference in behavior, unless
> you
> > are writing a ton of data into the file.  Of course, there's the
> obvious
> > question of why you'd want to do so...
>
> Exactly, you should not be doing that anyway.  So, because of that, I
> really don't want/like this patch.


OK, I've had a day to think about this, and I think I have a good answer
now.

Leaving aside the issue of how big a 'single object' is, we still have to
consider the possibility that a user _will_ indeed someday try to write 4K
(or more) to a sysfs file.  It's just going to happen.  And right now, the
kernel's behavior in that event is unpredictable, because we don't know
how the c library is going to buffer this write.

Right now, on my FC3 box, writing a large amount of data to a sysfs file
results in multiple writes of 1K to the file.  What my store method sees
then is multiple calls, each with 1K of data.  Each time, I have to assume
what I see is the complete contents of the write, and I have to process it
as such.

So if my sysfs file contains FOO, and the user writes BAR followed by 3k
of garbage, I'm not going to end up with FOO, or even BAR, but I'll end up
with whatever garbage I see at the beginning of the third 1K write.

The real problem is not that I get wrong values -- my store method should
handle this -- but that there are no errors returned from this operation.
The only way the user can tell that something is wrong is if a) I write a
message to the log telling what I did in my store method, and b) the user
checks the log.

My original write buffering patch fixes this problem, and allows up to 4K
to be consolidated into a single call to the store method.  It doesn't
seem to affect normal operation of my test system (nor those in our test
lab), but does hide error code returns from store methods.  And I can see
why you would be disinclined to accept such a patch.

While we may want to consider the possibility that a 'single object' may
someday grow large (crypto key maybe?), I can live without write buffering
right now.

But at the very least, we still need to handle this failure case.   I've
tested the following patch and it does resolve the issue.  However, it now
limits the size of sysfs writes to the size of the c library's buffer.

--------------------

This patch returns an error code if the user trys to write data to a sysfs
file at any offset other than 0.

Signed-off-by:  Mitch Williams <mitch.a.williams@intel.com>

diff -urpN -X dontdiff linux-2.6.11-clean/fs/sysfs/file.c linux-2.6.11/fs/sysfs/file.c
--- linux-2.6.11-clean/fs/sysfs/file.c	2004-12-24 13:33:50.000000000 -0800
+++ linux-2.6.11/fs/sysfs/file.c	2005-01-25 10:47:15.000000000 -0800
@@ -232,6 +232,8 @@ sysfs_write_file(struct file *file, cons
 {
 	struct sysfs_buffer * buffer = file->private_data;

+        if (*ppos > 0)
+            return -EIO;
 	down(&buffer->sem);
 	count = fill_write_buffer(buffer,buf,count);
 	if (count > 0)

Re: [PATCH 2/3] buffer writes to sysfs

[Bill Davidsen]

Mitch Williams wrote:
> 
> On Mon, 24 Jan 2005, Greg KH wrote:
> 
>>Who is trying to send > 1K to a sysfs file?  Remember, sysfs files are
>>for 1 value only.  If you consider > 1K a "single value" please point me
>>to that part of the kernel that does that.
>>
>>
>>>To the typical user, there's really no difference in behavior, unless
>>
>>you
>>
>>>are writing a ton of data into the file.  Of course, there's the
>>
>>obvious
>>
>>>question of why you'd want to do so...
>>
>>Exactly, you should not be doing that anyway.  So, because of that, I
>>really don't want/like this patch.
> 
> 
> 
> OK, I've had a day to think about this, and I think I have a good answer
> now.
> 
> Leaving aside the issue of how big a 'single object' is, we still have to
> consider the possibility that a user _will_ indeed someday try to write 4K
> (or more) to a sysfs file.  It's just going to happen.  And right now, the
> kernel's behavior in that event is unpredictable, because we don't know
> how the c library is going to buffer this write.
> 
> Right now, on my FC3 box, writing a large amount of data to a sysfs file
> results in multiple writes of 1K to the file.  What my store method sees
> then is multiple calls, each with 1K of data.  Each time, I have to assume
> what I see is the complete contents of the write, and I have to process it
> as such.
> 
> So if my sysfs file contains FOO, and the user writes BAR followed by 3k
> of garbage, I'm not going to end up with FOO, or even BAR, but I'll end up
> with whatever garbage I see at the beginning of the third 1K write.
> 
> The real problem is not that I get wrong values -- my store method should
> handle this -- but that there are no errors returned from this operation.
> The only way the user can tell that something is wrong is if a) I write a
> message to the log telling what I did in my store method, and b) the user
> checks the log.
> 
> My original write buffering patch fixes this problem, and allows up to 4K
> to be consolidated into a single call to the store method.  It doesn't
> seem to affect normal operation of my test system (nor those in our test
> lab), but does hide error code returns from store methods.  And I can see
> why you would be disinclined to accept such a patch.
> 
> While we may want to consider the possibility that a 'single object' may
> someday grow large (crypto key maybe?), I can live without write buffering
> right now.
> 
> But at the very least, we still need to handle this failure case.   I've
> tested the following patch and it does resolve the issue.  However, it now
> limits the size of sysfs writes to the size of the c library's buffer.
> 
> --------------------
> 
> This patch returns an error code if the user trys to write data to a sysfs
> file at any offset other than 0.

Clearly if it doesn't work it should return an error, if there's no 
(current) need to have this work then adding code to make it work is 
probably not justified.

> 
> Signed-off-by:  Mitch Williams <mitch.a.williams@intel.com>
> 
> diff -urpN -X dontdiff linux-2.6.11-clean/fs/sysfs/file.c linux-2.6.11/fs/sysfs/file.c
> --- linux-2.6.11-clean/fs/sysfs/file.c	2004-12-24 13:33:50.000000000 -0800
> +++ linux-2.6.11/fs/sysfs/file.c	2005-01-25 10:47:15.000000000 -0800
> @@ -232,6 +232,8 @@ sysfs_write_file(struct file *file, cons
>  {
>  	struct sysfs_buffer * buffer = file->private_data;
> 
> +        if (*ppos > 0)
> +            return -EIO;
>  	down(&buffer->sem);
>  	count = fill_write_buffer(buffer,buf,count);
>  	if (count > 0)

-- 
    -bill davidsen (davidsen@tmr.com)
"The secret to procrastination is to put things off until the
  last possible moment - but no longer"  -me

Re: [PATCH 2/3] buffer writes to sysfs

[Greg KH]

On Tue, Jan 25, 2005 at 03:39:00PM -0800, Mitch Williams wrote:
> 
> 
> On Mon, 24 Jan 2005, Greg KH wrote:
> >
> > Who is trying to send > 1K to a sysfs file?  Remember, sysfs files are
> > for 1 value only.  If you consider > 1K a "single value" please point me
> > to that part of the kernel that does that.
> >
> > > To the typical user, there's really no difference in behavior, unless
> > you
> > > are writing a ton of data into the file.  Of course, there's the
> > obvious
> > > question of why you'd want to do so...
> >
> > Exactly, you should not be doing that anyway.  So, because of that, I
> > really don't want/like this patch.
> 
> 
> OK, I've had a day to think about this, and I think I have a good answer
> now.
> 
> Leaving aside the issue of how big a 'single object' is, we still have to
> consider the possibility that a user _will_ indeed someday try to write 4K
> (or more) to a sysfs file.  It's just going to happen.  And right now, the
> kernel's behavior in that event is unpredictable, because we don't know
> how the c library is going to buffer this write.

That's your C library, probably not mine :)

Anyway, sure, I can see that we want to handle this in a sane manner.

> diff -urpN -X dontdiff linux-2.6.11-clean/fs/sysfs/file.c linux-2.6.11/fs/sysfs/file.c
> --- linux-2.6.11-clean/fs/sysfs/file.c	2004-12-24 13:33:50.000000000 -0800
> +++ linux-2.6.11/fs/sysfs/file.c	2005-01-25 10:47:15.000000000 -0800
> @@ -232,6 +232,8 @@ sysfs_write_file(struct file *file, cons
>  {
>  	struct sysfs_buffer * buffer = file->private_data;
> 
> +        if (*ppos > 0)
> +            return -EIO;
>  	down(&buffer->sem);
>  	count = fill_write_buffer(buffer,buf,count);
>  	if (count > 0)

You are using spaces instead of tabs :(

Care to redo it?

thanks,

greg k-h