Re: [PATCH] block: Make CFQ default to IOPS mode on SSDs

[Jens Axboe]

On 06/09/2015 04:18 AM, Romain Francoise wrote:
> Hi,
>
> On Tue, May 19, 2015 at 01:55:21PM -0700, Tahsin Erdogan wrote:
> > --- a/block/cfq-iosched.c
> > +++ b/block/cfq-iosched.c
> > @@ -4460,7 +4460,7 @@ static int cfq_init_queue(struct request_queue *q, struct elevator_type *e)
> >   	cfqd->cfq_slice[1] = cfq_slice_sync;
> >   	cfqd->cfq_target_latency = cfq_target_latency;
> >   	cfqd->cfq_slice_async_rq = cfq_slice_async_rq;
> > -	cfqd->cfq_slice_idle = cfq_slice_idle;
> > +	cfqd->cfq_slice_idle = blk_queue_nonrot(q) ? 0 : cfq_slice_idle;
> >   	cfqd->cfq_group_idle = cfq_group_idle;
> >   	cfqd->cfq_latency = 1;
> >   	cfqd->hw_tag = -1;
>
> Did you test this patch with regular AHCI SSD devices? Applying it on
> top of v4.1-rc7 makes no difference, slice_idle is still initialized to
> 8 in my setup, while rotational is 0.
>
> Isn't the elevator initialized long before the non-rotational flag is
> actually set on the device (which probably happens after it's probed on
> the scsi bus)?

You are absolutely correct. What happens is that the queue is allocated 
and initialized, and cfq checks the flag. But the flag is set later in 
the process, when we have finished probing the device checked if it's 
rotational or not.

There are a few options to handle this. The attached might work, not 
tested at all. Basically it adds an io sched registration hook, that is 
called when we are adding the disk on the queue. Non-rotational 
detection should be done at that point.

Does that work for you?

--
Jens Axboe

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index c808ad87652d..af8918eb7cd5 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -4508,7 +4508,7 @@ static int cfq_init_queue(struct request_queue *q, struct elevator_type *e)
 	cfqd->cfq_slice[1] = cfq_slice_sync;
 	cfqd->cfq_target_latency = cfq_target_latency;
 	cfqd->cfq_slice_async_rq = cfq_slice_async_rq;
-	cfqd->cfq_slice_idle = blk_queue_nonrot(q) ? 0 : cfq_slice_idle;
+	cfqd->cfq_slice_idle = cfq_slice_idle;
 	cfqd->cfq_group_idle = cfq_group_idle;
 	cfqd->cfq_latency = 1;
 	cfqd->hw_tag = -1;
@@ -4525,6 +4525,15 @@ out_free:
 	return ret;
 }
 
+static void cfq_registered_queue(struct request_queue *q)
+{
+	struct elevator_queue *e = q->elevator;
+	struct cfq_data *cfqd = e->elevator_data;
+
+	if (blk_queue_nonrot(q))
+		cfqd->cfq_slice_idle = 0;
+}
+
 /*
  * sysfs parts below -->
  */
@@ -4640,6 +4649,7 @@ static struct elevator_type iosched_cfq = {
 		.elevator_may_queue_fn =	cfq_may_queue,
 		.elevator_init_fn =		cfq_init_queue,
 		.elevator_exit_fn =		cfq_exit_queue,
+		.elevator_registered_fn =	cfq_registered_queue,
 	},
 	.icq_size	=	sizeof(struct cfq_io_cq),
 	.icq_align	=	__alignof__(struct cfq_io_cq),
diff --git a/block/elevator.c b/block/elevator.c
index 59794d0d38e3..5f0452734a40 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -810,6 +810,8 @@ int elv_register_queue(struct request_queue *q)
 		}
 		kobject_uevent(&e->kobj, KOBJ_ADD);
 		e->registered = 1;
+		if (e->type->ops.elevator_registered_fn)
+			e->type->ops.elevator_registered_fn(q);
 	}
 	return error;
 }
diff --git a/include/linux/elevator.h b/include/linux/elevator.h
index 45a91474487d..638b324f0291 100644
--- a/include/linux/elevator.h
+++ b/include/linux/elevator.h
@@ -39,6 +39,7 @@ typedef void (elevator_deactivate_req_fn) (struct request_queue *, struct reques
 typedef int (elevator_init_fn) (struct request_queue *,
 				struct elevator_type *e);
 typedef void (elevator_exit_fn) (struct elevator_queue *);
+typedef void (elevator_registered_fn) (struct request_queue *);
 
 struct elevator_ops
 {
@@ -68,6 +69,7 @@ struct elevator_ops
 
 	elevator_init_fn *elevator_init_fn;
 	elevator_exit_fn *elevator_exit_fn;
+	elevator_registered_fn *elevator_registered_fn;
 };
 
 #define ELV_NAME_MAX	(16)