Re: [PATCH 1/1] sysfs: add more info to the oops dump

[Phil Carmody <ext-phil.2.carmody@nokia.com>]

On 10/03/11 08:25 -0800, ext Greg KH wrote:
> On Thu, Mar 10, 2011 at 05:53:08PM +0200, Phil Carmody wrote:
> > If we're going to remember which file we accessed, we might as well
> > also remember whether it was a read or a write, and if the latter,
> > some indication of what was written.
> > 
> > e.g.
> > $ echo 1 > /sys/kernel/slab/:at-0000064/sanity_checks
> > $ echo c > /proc/sysrq-trigger
> > ...
> > [  112.457580] last sysfs file (w): /sys/kernel/slab/:at-0000064/sanity_checks
> > [  112.464569]   written: 1
> 
> Has this actually helped you out?  I've been thinking of removing this
> line in the dmesg entirely as I haven't seen it help in a very long time
> to track anything down.

I'm glad I started my commit message with that 'if', that tells you
something. I have considered it, in its original form, not particularly
helpful too. But I blamed that on it not telling me enough. Today I saw
an oops related to a sysfs write, and this just patch basically wrote
itself instantly. But it's not stood the test of time, that's for sure.

Phil
 
> > 
> > Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
> > ---
> >  fs/sysfs/file.c |   21 ++++++++++++++++++++-
> >  1 files changed, 20 insertions(+), 1 deletions(-)
> > 
> > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> > index da3fefe..281e4dd 100644
> > --- a/fs/sysfs/file.c
> > +++ b/fs/sysfs/file.c
> > @@ -26,9 +26,18 @@
> >  
> >  /* used in crash dumps to help with debugging */
> >  static char last_sysfs_file[PATH_MAX];
> > +static char last_sysfs_write[16];
> > +static int last_sysfs_write_len; /* magic value < 0 => read */
> >  void sysfs_printk_last_file(void)
> >  {
> > -	printk(KERN_EMERG "last sysfs file: %s\n", last_sysfs_file);
> > +	printk(KERN_EMERG "last sysfs file (%c): %s\n",
> > +	       (last_sysfs_write_len < 0) ? 'r' : 'w',
> > +	       last_sysfs_file);
> > +	if (last_sysfs_write_len >= sizeof(last_sysfs_write))
> > +		printk(KERN_EMERG "  written: %s...(%d chars)\n",
> > +		       last_sysfs_write, last_sysfs_write_len);
> > +	else if (last_sysfs_write_len > 0)
> > +		printk(KERN_EMERG "  written: %s\n", last_sysfs_write);
> >  }
> >  
> >  /*
> > @@ -200,12 +209,19 @@ flush_write_buffer(struct dentry * dentry, struct sysfs_buffer * buffer, size_t
> >  	struct sysfs_dirent *attr_sd = dentry->d_fsdata;
> >  	struct kobject *kobj = attr_sd->s_parent->s_dir.kobj;
> >  	const struct sysfs_ops * ops = buffer->ops;
> > +	int copylen = min(count, sizeof(last_sysfs_write) - 1);
> >  	int rc;
> >  
> >  	/* need attr_sd for attr and ops, its parent for kobj */
> >  	if (!sysfs_get_active(attr_sd))
> >  		return -ENODEV;
> >  
> > +	while (copylen > 0 && buffer->page[copylen-1] == '\n')
> > +		--copylen; /* never print trailing \n's */
> > +	memcpy(last_sysfs_write, buffer->page, copylen);
> > +	last_sysfs_write[copylen] = '\0';
> > +	last_sysfs_write_len = count;
> > +
> >  	rc = ops->store(kobj, attr_sd->s_attr.attr, buffer->page, count);
> >  
> >  	sysfs_put_active(attr_sd);
> > @@ -363,6 +379,9 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
> >  	if (file->f_mode & FMODE_WRITE) {
> >  		if (!(inode->i_mode & S_IWUGO) || !ops->store)
> >  			goto err_out;
> > +		last_sysfs_write_len = 0;
> > +	} else {
> > +		last_sysfs_write_len = -1; /* magic value */
> 
> "magic" values are bad, please use a #define or something else that
> makes more sense to those looking at the code in 5 years.
> 
> thanks,
> 
> greg k-h