Re: [PATCH 3/9] blk-mq-debugfs: get rid of a bunch of boilerplate

[Omar Sandoval]

On Wed, May 03, 2017 at 08:51:11PM +, Bart Van Assche wrote:
> On Wed, 2017-05-03 at 12:18 -0700, Omar Sandoval wrote:
> > From: Omar Sandoval 
> > 
> > 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 
> > ---
> >  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(, " \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.

[PATCH 3/9] blk-mq-debugfs: get rid of a bunch of boilerplate

[Omar Sandoval]

From: Omar Sandoval 

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 
---
 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 *);
+   ssize_t (*write)(void *, const char __user *, size_t, loff_t *);
+   /* Set either .show or .seq_ops. */
+   const struct seq_operations *seq_ops;
 };
 
-static int blk_mq_debugfs_seq_open(struct inode *inode, struct file *file,
-  const struct seq_operations *ops)
-{
-   struct seq_file *m;
-   int ret;
-
-   ret = seq_open(file, ops);
-   if (!ret) {
-   m = file->private_data;
-   m->private = inode->i_private;
-   }
-   return ret;
-}
-
 static int blk_flags_show(struct seq_file *m, const unsigned long flags,
  const char *const *flag_name, int flag_name_count)
 {
@@ -97,9 +86,9 @@ static const char *const blk_queue_flag_name[] = {
 };
 #undef QUEUE_FLAG_NAME
 
-static int blk_queue_flags_show(struct seq_file *m, void *v)
+static int queue_state_show(void *data, struct seq_file *m)
 {
-   struct request_queue *q = m->private;
+   struct request_queue *q = data;
 
blk_flags_show(m, q->queue_flags, blk_queue_flag_name,
   ARRAY_SIZE(blk_queue_flag_name));
@@ -107,14 +96,13 @@ static int blk_queue_flags_show(struct seq_file *m, void 
*v)
return 0;
 }
 
-static ssize_t blk_queue_flags_store(struct file *file, const char __user 
*ubuf,
-size_t len, loff_t *offp)
+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(, " \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;
 }
 
-static const struct file_operations blk_queue_flags_fops = {
-   .open   = blk_queue_flags_open,
-   .read   = seq_read,
-   .llseek = seq_lseek,
-   .release= single_release,
-   .write  = blk_queue_flags_store,
-};
-
 static void print_stat(struct seq_file *m, struct blk_rq_stat *stat)
 {
if (stat->nr_samples) {
@@ -153,9 +128,9 @@ static void print_stat(struct seq_file *m, struct 
blk_rq_stat *stat)
}
 }
 
-static int queue_poll_stat_show(struct seq_file *m, void *v)
+static int queue_poll_stat_show(void *data, struct seq_file *m)
 {
-   struct request_queue *q = m->private;
+   struct request_queue *q = data;
int bucket;
 
for (bucket = 0; bucket < BLK_MQ_POLL_STATS_BKTS/2; bucket++) {
@@ -170,18 +145,6 @@ static int queue_poll_stat_show(struct seq_file *m, void 
*v)
return 0;
 }
 
-static int queue_poll_stat_open(struct inode *inode, struct file *file)
-{
-   return single_open(file, queue_poll_stat_show, inode->i_private);
-}
-
-static const struct file_operations queue_poll_stat_fops = {
-   .open   = queue_poll_stat_open,
-   .read   = seq_read,
-   .llseek = seq_lseek,
-   .release= single_release,
-};
-
 #define HCTX_STATE_NAME(name) [BLK_MQ_S_##name] = #name
 static const char *const hctx_state_name[] = {
HCTX_STATE_NAME(STOPPED),
@@ -192,9 +155,9 @@ static const char *const hctx_state_name[] = {
 };
 #undef HCTX_STATE_NAME
 
-static int hctx_state_show(struct seq_file *m, void *v)
+static int hctx_state_show(void *data, struct seq_file *m)
 {
-   struct blk_mq_hw_ctx *hctx = m->private;
+   struct blk_mq_hw_ctx *hctx = data;