On Wed, May 03, 2017 at 08:51:11PM +0000, Bart Van Assche wrote:
> On Wed, 2017-05-03 at 12:18 -0700, Omar Sandoval wrote:
> > From: Omar Sandoval <osan...@fb.com>
> >
> > A large part of blk-mq-debugfs.c is file_operations and seq_file
> > boilerplate. This sucks as is but will suck even more when schedulers
> > can define their own debugfs entries. Factor it all out into a single
> > blk_mq_debugfs_fops which multiplexes as needed. We store the
> > request_queue, blk_mq_hw_ctx, or blk_mq_ctx in the parent directory
> > dentry, which is kind of hacky, but it works.
> >
> > Signed-off-by: Omar Sandoval <osan...@fb.com>
> > ---
> > block/blk-mq-debugfs.c | 469
> > +++++++++++++++----------------------------------
> > 1 file changed, 138 insertions(+), 331 deletions(-)
> >
> > diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> > index f58a116d6cca..00cc89c34590 100644
> > --- a/block/blk-mq-debugfs.c
> > +++ b/block/blk-mq-debugfs.c
> > @@ -26,23 +26,12 @@
> > struct blk_mq_debugfs_attr {
> > const char *name;
> > umode_t mode;
> > - const struct file_operations *fops;
> > + int (*show)(void *, struct seq_file *);
>
> Hello Omar,
>
> The current show functions have the seq_file pointer as first argument
> but for .show() it is the second argument. Please consider to keep that
> pointer as the first argument.
The current show functions aren't the same thing, though. The void *
argument to those is meaningless, and the data is in m->private instead
of being passed in. I don't want to conflate the two calling
conventions.
Really what I wanted were strongly-typed show/write functions, i.e., one of
int (*show)(struct request_queue *, struct seq_file *);
int (*show)(struct blk_mq_hw_ctx *, struct seq_file *);
int (*show)(struct blk_mq_ctx *, struct seq_file *);
But I couldn't come up with a nice way to do that.
> > +static ssize_t queue_state_write(void *data, const char __user *buf,
> > + size_t count, loff_t *ppos)
> > {
> > - struct request_queue *q = file_inode(file)->i_private;
> > + struct request_queue *q = data;
> > char op[16] = { }, *s;
> >
> > - len = min(len, sizeof(op) - 1);
> > - if (copy_from_user(op, ubuf, len))
> > + if (copy_from_user(op, buf, min(count, sizeof(op) - 1)))
> > return -EFAULT;
> > s = op;
> > strsep(&s, " \t\n"); /* strip trailing whitespace */
> > @@ -127,22 +115,9 @@ static ssize_t blk_queue_flags_store(struct file
> > *file, const char __user *ubuf,
> > __func__, op);
> > return -EINVAL;
> > }
> > - return len;
> > -}
> > -
> > -static int blk_queue_flags_open(struct inode *inode, struct file *file)
> > -{
> > - return single_open(file, blk_queue_flags_show, inode->i_private);
> > + return count;
> > }
>
> Please move the return value changes of queue_state_write() into a separate
> patch since these are a bug fix and not related to the removal of the
> boilerplate code.
Sure, I'll do that.
Thanks for the review.
