Hello Bart,
On Fri 14-04-17 15:42:53, Bart Van Assche wrote:
> On Wed, 2017-04-12 at 10:23 +0200, Jan Kara wrote:
> > +#ifndef CONFIG_BLK_WBT_MQ
> > + if (q->mq_ops)
> > + return;
> > +#endif
> > +#ifndef CONFIG_BLK_WBT_SQ
> > + if (q->request_fn)
> > + return;
> > +#endif
> > +
> > + /*
> > + * If this fails, we don't get throttling
> > + */
> > + wbt_init(q);
>
> How about using positive logic to enable WBT, e.g. as follows? Wouldn't
> that make the code easier to read?
>
> if ((IS_ENABLED(CONFIG_BLK_WBT_MQ) && q->mq_ops) ||
> (IS_ENABLED(CONFIG_BLK_WBT_SQ) && q->request_fn))
> wbt_init(q);
Yes, it is more readable. I was just copying the function around so I
didn't change it from the original but I guess I could add this cleanup to
make things more readable.
> > +static void deadline_registered_queue(struct request_queue *q)
> > +{
> > + wbt_enable_default(q);
> > +}
> > +
> > /*
> > * sysfs parts below
> > */
> > @@ -445,6 +451,7 @@ static struct elevator_type iosched_deadline = {
> > .elevator_latter_req_fn = elv_rb_latter_request,
> > .elevator_init_fn = deadline_init_queue,
> > .elevator_exit_fn = deadline_exit_queue,
> > + .elevator_registered_fn = deadline_registered_queue,
> > },
> >
> > .elevator_attrs = deadline_attrs,
> > [ ... ]
> > @@ -91,6 +92,11 @@ static void noop_exit_queue(struct elevator_queue *e)
> > kfree(nd);
> > }
> >
> > +static void noop_registered_queue(struct request_queue *q)
> > +{
> > + wbt_enable_default(q);
> > +}
> > +
> > static struct elevator_type elevator_noop = {
> > .ops.sq = {
> > .elevator_merge_req_fn = noop_merged_requests,
> > @@ -100,6 +106,7 @@ static struct elevator_type elevator_noop = {
> > .elevator_latter_req_fn = noop_latter_request,
> > .elevator_init_fn = noop_init_queue,
> > .elevator_exit_fn = noop_exit_queue,
> > + .elevator_registered_fn = noop_registered_queue,
> > },
> > .elevator_name = "noop",
> > .elevator_owner = THIS_MODULE,
>
> This approach is not suited for blk-mq because with blk-mq "none" means no
> scheduler and hence no struct elevator_type. Please consider not to add any
> elevator_registered_fn() callbacks to noop and deadline but instead to call
> wbt_enable_default() from elv_unregister_queue().
So I was not much concerned about how to make blk-mq enable the writeback
throttling as you cannot switch between legacy and blk-mq path dynamically
and thus writeback throttling cannot get disabled. And for legacy path
using elevator_registered_fn is a natural choice.
I agree that using elv_unregister_queue() to enable writeback throttling
would work as well and may be easier to reuse if we ever want to make
similar auto-disabling work e.g. with BFQ. So unless someone objects I can
redo my patch that way.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
