ide: use per-device request queue locks

* Move hack for flush requests from choose_drive() to do_ide_request().

* Add ide_plug_device() helper and convert core IDE code from using per-hwgroup lock as a request lock to use the ->queue_lock instead.

* Remove no longer needed:
- choose_drive() function
- WAKEUP() macro
- 'sleeping' flag from ide_hwif_t
- 'service_{start,time}' fields from ide_drive_t

This patch results in much simpler and more maintainable code (besides being a scalability improvement).

Cc Elias Oltmanns <[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
newer version

 drivers/ide/ide-io.c    | 213 +++++++++++++++---------------------------------
 drivers/ide/ide-park.c  |   13 +-
 drivers/ide/ide-probe.c |    3 
 include/linux/ide.h     |    4 
 4 files changed, 79 insertions(+), 154 deletions(-)

Index: b/drivers/ide/ide-io.c
===================================================================

drivers/ide/ide-io.c +0 -0

drivers/ide/ide-io.c +12 -32

@@ -780,61 +704,40 @@ repeat:	
  */
 void do_ide_request(struct request_queue *q)
 {
-	ide_drive_t	*orig_drive = q->queuedata;
-	ide_hwgroup_t	*hwgroup = orig_drive->hwif->hwgroup;
-	ide_drive_t	*drive;
-	ide_hwif_t	*hwif;
+	ide_drive_t	*drive = q->queuedata;
+	ide_hwif_t	*hwif = drive->hwif;
+	ide_hwgroup_t	*hwgroup = hwif->hwgroup;
 	struct request	*rq;
 	ide_startstop_t	startstop;
 
-	/* caller must own hwgroup->lock */
-	BUG_ON(!irqs_disabled());
-
-	while (!ide_lock_hwgroup(hwgroup)) {
-		drive = choose_drive(hwgroup);
-		if (drive == NULL) {
-			int sleeping = 0;
-			unsigned long sleep = 0; /* shut up, gcc */
-			hwgroup->rq = NULL;
-			drive = hwgroup->drive;
-			do {
-				if ((drive->dev_flags & IDE_DFLAG_SLEEPING) &&
-				    (sleeping == 0 ||
-				     time_before(drive->sleep, sleep))) {
-					sleeping = 1;
-					sleep = drive->sleep;
-				}
-			} while ((drive = drive->next) != hwgroup->drive);
-			if (sleeping) {
+	/*
+	 * drive is doing pre-flush, ordered write, post-flush sequence. even
+	 * though that is 3 requests, it must be seen as a single transaction.
+	 * we must not preempt this drive until that is complete
+	 */
+	if (blk_queue_flushing(q))
 		/*
-		 * Take a short snooze, and then wake up this hwgroup again.
-		 * This gives other hwgroups on the same a chance to
-		 * play fairly with us, just in case there are big differences
-		 * in relative throughputs.. don't want to hog the cpu too much.
+		 * small race where queue could get replugged during
+		 * the 3-request flush cycle, just yank the plug since
+		 * we want it to finish asap
 		 */
-				if (time_before(sleep, jiffies + WAIT_MIN_SLEEP))
-					sleep = jiffies + WAIT_MIN_SLEEP;
-#if 1
-				if (timer_pending(&hwgroup->timer))
-					printk(KERN_CRIT "ide_set_handler: timer already active\n");

drivers/ide/ide-io.c +15 -3

-		hwif = drive->hwif;
+	if (!ide_lock_hwgroup(hwgroup)) {
+		hwgroup->rq = NULL;
+
+		if (drive->dev_flags & IDE_DFLAG_SLEEPING) {
+			if (time_before(drive->sleep, jiffies)) {
+				ide_unlock_hwgroup(hwgroup);
+				goto plug_device;
+			}
+		}
 
 		if (hwif != hwgroup->hwif) {
 			/*

@@ -847,16 +750,20 @@ void do_ide_request(struct request_queue
 		hwgroup->hwif = hwif;
 		hwgroup->drive = drive;
 		drive->dev_flags &= ~(IDE_DFLAG_SLEEPING | IDE_DFLAG_PARKED);
-		drive->service_start = jiffies;
 
+		spin_unlock_irq(&hwgroup->lock);
+		spin_lock_irq(q->queue_lock);
 		/*
 		 * we know that the queue isn't empty, but this can happen
 		 * if the q->prep_rq_fn() decides to kill a request
 		 */
 		rq = elv_next_request(drive->queue);
+		spin_unlock_irq(q->queue_lock);
+		spin_lock_irq(&hwgroup->lock);
+
 		if (!rq) {
 			ide_unlock_hwgroup(hwgroup);
-			break;
+			goto out;
 		}
 
 		/*

drivers/ide/ide-io.c +10 -3

 		}
-	}
+	} else
+		goto plug_device;
+out:
+	spin_unlock_irq(&hwgroup->lock);
+	spin_lock_irq(q->queue_lock);
 	return;
 
 plug_device:
-	if (!elv_queue_empty(orig_drive->queue))
-		blk_plug_device(orig_drive->queue);
+	spin_unlock_irq(&hwgroup->lock);
+	spin_lock_irq(q->queue_lock);
+
+	if (!elv_queue_empty(q))
+		blk_plug_device(q);
 }
 
 /*

drivers/ide/ide-io.c +2 -0

@@ -974,10 +899,12 @@ out:
 void ide_timer_expiry (unsigned long data)
 {
 	ide_hwgroup_t	*hwgroup = (ide_hwgroup_t *) data;
+	ide_drive_t	*uninitialized_var(drive);
 	ide_handler_t	*handler;
 	ide_expiry_t	*expiry;
 	unsigned long	flags;
 	unsigned long	wait = -1;
+	int		plug_device = 0;
 
 	spin_lock_irqsave(&hwgroup->lock, flags);
 

Quoted 11 lines · Bartlomiej Zolnierkiewicz

* Move hack for flush requests from choose_drive() to do_ide_request().

* Add ide_plug_device() helper and convert core IDE code from using per-hwgroup lock as a request lock to use the ->queue_lock instead.

* Remove no longer needed:
- choose_drive() function
- WAKEUP() macro
- 'sleeping' flag from ide_hwif_t
- 'service_{start,time}' fields from ide_drive_t

Quoted 7 lines

This patch results in much simpler and more maintainable code (besides being a scalability improvement).

Cc Elias Oltmanns <[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
newer version

Eventually, I got round to having a look at this one (I reviewed the two small ones at the time). Apologies for the delay.

Quoted 11 lines

 drivers/ide/ide-io.c    | 213 +++++++++++++++---------------------------------
 drivers/ide/ide-park.c  |   13 +-
 drivers/ide/ide-probe.c |    3 
 include/linux/ide.h     |    4 
 4 files changed, 79 insertions(+), 154 deletions(-)

Index: b/drivers/ide/ide-io.c
===================================================================

drivers/ide/ide-io.c +0 -0

[...]

Quoted 61 lines

drivers/ide/ide-io.c +12 -40

@@ -780,61 +704,40 @@ repeat:	
  */
 void do_ide_request(struct request_queue *q)
 {
-	ide_drive_t	*orig_drive = q->queuedata;
-	ide_hwgroup_t	*hwgroup = orig_drive->hwif->hwgroup;
-	ide_drive_t	*drive;
-	ide_hwif_t	*hwif;
+	ide_drive_t	*drive = q->queuedata;
+	ide_hwif_t	*hwif = drive->hwif;
+	ide_hwgroup_t	*hwgroup = hwif->hwgroup;
 	struct request	*rq;
 	ide_startstop_t	startstop;
 
-	/* caller must own hwgroup->lock */
-	BUG_ON(!irqs_disabled());
-
-	while (!ide_lock_hwgroup(hwgroup)) {
-		drive = choose_drive(hwgroup);
-		if (drive == NULL) {
-			int sleeping = 0;
-			unsigned long sleep = 0; /* shut up, gcc */
-			hwgroup->rq = NULL;
-			drive = hwgroup->drive;
-			do {
-				if ((drive->dev_flags & IDE_DFLAG_SLEEPING) &&
-				    (sleeping == 0 ||
-				     time_before(drive->sleep, sleep))) {
-					sleeping = 1;
-					sleep = drive->sleep;
-				}
-			} while ((drive = drive->next) != hwgroup->drive);
-			if (sleeping) {
+	/*
+	 * drive is doing pre-flush, ordered write, post-flush sequence. even
+	 * though that is 3 requests, it must be seen as a single transaction.
+	 * we must not preempt this drive until that is complete
+	 */
+	if (blk_queue_flushing(q))
 		/*
-		 * Take a short snooze, and then wake up this hwgroup again.
-		 * This gives other hwgroups on the same a chance to
-		 * play fairly with us, just in case there are big differences
-		 * in relative throughputs.. don't want to hog the cpu too much.
+		 * small race where queue could get replugged during
+		 * the 3-request flush cycle, just yank the plug since
+		 * we want it to finish asap
 		 */
-				if (time_before(sleep, jiffies + WAIT_MIN_SLEEP))
-					sleep = jiffies + WAIT_MIN_SLEEP;
-#if 1
-				if (timer_pending(&hwgroup->timer))
-					printk(KERN_CRIT "ide_set_handler: timer already active\n");
-#endif
-				/* so that ide_timer_expiry knows what to do */
-				hwgroup->sleeping = 1;
-				hwgroup->req_gen_timer = hwgroup->req_gen;
-				mod_timer(&hwgroup->timer, sleep);
-				/* we purposely leave hwgroup locked
-				 * while sleeping */
-			} else

drivers/ide/ide-io.c repeat: · line 724

Quoted 2 lines

drivers/ide/ide-io.c +1 -1

-				ide_unlock_hwgroup(hwgroup);
+		blk_remove_plug(q);

I'm not at all convinced that this works as expected. First of all, I think we can safely assume that the plug is removed when block layer calls into the ->request_fn(). Secondly, since the ide layer doesn't call the ->request_fn() on it's own accord, I rather suspect that this check can be dropped altogether. On the other hand, I'm not sure I agree

with the rest of the patch anyway, see below.

Quoted 7 lines

drivers/ide/ide-io.c +2 -3

-			/* no more work for this hwgroup (for now) */
-			goto plug_device;
-		}
+	spin_unlock_irq(q->queue_lock);
+	spin_lock_irq(&hwgroup->lock);
 

drivers/ide/ide-io.c repeat: · line 730

Quoted 4 lines

drivers/ide/ide-io.c +2 -2

-		if (drive != orig_drive)
-			goto plug_device;
+	/* caller must own hwgroup->lock */
+	BUG_ON(!irqs_disabled());

Does this check really make any sense? The lock is being taken just two lines before, after all.

Quoted 38 lines

drivers/ide/ide-io.c +15 -3

-		hwif = drive->hwif;
+	if (!ide_lock_hwgroup(hwgroup)) {
+		hwgroup->rq = NULL;
+
+		if (drive->dev_flags & IDE_DFLAG_SLEEPING) {
+			if (time_before(drive->sleep, jiffies)) {
+				ide_unlock_hwgroup(hwgroup);
+				goto plug_device;
+			}
+		}
 
 		if (hwif != hwgroup->hwif) {
 			/*

@@ -847,16 +750,20 @@ void do_ide_request(struct request_queue
 		hwgroup->hwif = hwif;
 		hwgroup->drive = drive;
 		drive->dev_flags &= ~(IDE_DFLAG_SLEEPING | IDE_DFLAG_PARKED);
-		drive->service_start = jiffies;
 
+		spin_unlock_irq(&hwgroup->lock);
+		spin_lock_irq(q->queue_lock);
 		/*
 		 * we know that the queue isn't empty, but this can happen
 		 * if the q->prep_rq_fn() decides to kill a request
 		 */
 		rq = elv_next_request(drive->queue);
+		spin_unlock_irq(q->queue_lock);
+		spin_lock_irq(&hwgroup->lock);
+
 		if (!rq) {
 			ide_unlock_hwgroup(hwgroup);
-			break;
+			goto out;
 		}
 
 		/*

@@ -888,15 +795,22 @@ void do_ide_request(struct request_queue
 

drivers/ide/ide-io.c void do_ide_request(struct request_queue · line 799

Quoted 6 lines

drivers/ide/ide-io.c +2 -2

 		if (startstop == ide_stopped) {
 			ide_unlock_hwgroup(hwgroup);
-			if (!elv_queue_empty(orig_drive->queue))
-				blk_plug_device(orig_drive->queue);
+			/* give other devices a chance */
+			goto plug_device;

This, I think, is very wrong. The rule of thumb for a ->requst_fn() should be to take as many requests off the queue as possible. Besides, this may easily preempt an ordered sequence as mentioned earlier. In my opinion, we really need a loop of sorts (while or goto).

Quoted 20 lines

drivers/ide/ide-io.c +10 -3

 		}
-	}
+	} else
+		goto plug_device;
+out:
+	spin_unlock_irq(&hwgroup->lock);
+	spin_lock_irq(q->queue_lock);
 	return;
 
 plug_device:
-	if (!elv_queue_empty(orig_drive->queue))
-		blk_plug_device(orig_drive->queue);
+	spin_unlock_irq(&hwgroup->lock);
+	spin_lock_irq(q->queue_lock);
+
+	if (!elv_queue_empty(q))
+		blk_plug_device(q);
 }
 
 /*

[...]

Quoted 13 lines

drivers/ide/ide-io.c +2 -0

@@ -974,10 +899,12 @@ out:
 void ide_timer_expiry (unsigned long data)
 {
 	ide_hwgroup_t	*hwgroup = (ide_hwgroup_t *) data;
+	ide_drive_t	*uninitialized_var(drive);
 	ide_handler_t	*handler;
 	ide_expiry_t	*expiry;
 	unsigned long	flags;
 	unsigned long	wait = -1;
+	int		plug_device = 0;
 
 	spin_lock_irqsave(&hwgroup->lock, flags);
 

drivers/ide/ide-io.c void ide_timer_expiry (unsigned long dat · line 918

Quoted 4 lines

drivers/ide/ide-io.c +0 -0

@@ -989,12 +916,8 @@ void ide_timer_expiry (unsigned long dat
 		 * or we were "sleeping" to give other devices a chance.
 		 * Either way, we don't really want to complain about anything.
 		 */

Not that important, I suppose, but you might want to update that comment while you're at it.

drivers/ide/ide-io.c void ide_timer_expiry (unsigned long dat · line 923

Quoted 10 lines

drivers/ide/ide-io.c +1 -5

-		if (hwgroup->sleeping) {
-			hwgroup->sleeping = 0;
-			ide_unlock_hwgroup(hwgroup);
-		}
 	} else {
-		ide_drive_t *drive = hwgroup->drive;
+		drive = hwgroup->drive;
 		if (!drive) {
 			printk(KERN_ERR "ide_timer_expiry: hwgroup->drive was NULL\n");
 			hwgroup->handler = NULL;

Finally, some more general blathering on the topic at hand: A while ago, I spent some thought on the possibilities of giving the block layer a notion of linked device queues as an equivalent hwgroups in ide, scsi_hosts or ata_ports and let it take care of time / bandwidth distribution among the queues belonging to one group. This is, as I understand, pretty much what your code is relying on since you have chucked out choose_drive(). However, this turned out not to be too easy

and I'm not quite sure whether we really want to go down that road. One major problem is that there is no straight forward way for the block layer to know, whether a ->request_fn() has actually taken a request off the queue and if not (or less than queue_depth anyway), whether it's just the device that couldn't take any more or the controller instead. On the whole, it seems not exactly trivial to get it right and it would probably be a good idea to consult Jens and perhaps James before

embarking on such a venture. Short of that, I think that ide layer has to keep an appropriate equivalent of choose_drive() and also the while loop in the do_ide_request() function.

[ Once again, sorry for the long delay. ]

Quoted 1 line

Bartlomiej Zolnierkiewicz <[email protected]> wrote:

Quoted 72 lines

Quoted 10 lines

 drivers/ide/ide-io.c    | 213 +++++++++++++++---------------------------------
 drivers/ide/ide-park.c  |   13 +-
 drivers/ide/ide-probe.c |    3 
 include/linux/ide.h     |    4 
 4 files changed, 79 insertions(+), 154 deletions(-)

Index: b/drivers/ide/ide-io.c
===================================================================

drivers/ide/ide-io.c +0 -0

[...]

Quoted 61 lines

drivers/ide/ide-io.c +12 -40

@@ -780,61 +704,40 @@ repeat:	
  */
 void do_ide_request(struct request_queue *q)
 {
-	ide_drive_t	*orig_drive = q->queuedata;
-	ide_hwgroup_t	*hwgroup = orig_drive->hwif->hwgroup;
-	ide_drive_t	*drive;
-	ide_hwif_t	*hwif;
+	ide_drive_t	*drive = q->queuedata;
+	ide_hwif_t	*hwif = drive->hwif;
+	ide_hwgroup_t	*hwgroup = hwif->hwgroup;
 	struct request	*rq;
 	ide_startstop_t	startstop;
 
-	/* caller must own hwgroup->lock */
-	BUG_ON(!irqs_disabled());
-
-	while (!ide_lock_hwgroup(hwgroup)) {
-		drive = choose_drive(hwgroup);
-		if (drive == NULL) {
-			int sleeping = 0;
-			unsigned long sleep = 0; /* shut up, gcc */
-			hwgroup->rq = NULL;
-			drive = hwgroup->drive;
-			do {
-				if ((drive->dev_flags & IDE_DFLAG_SLEEPING) &&
-				    (sleeping == 0 ||
-				     time_before(drive->sleep, sleep))) {
-					sleeping = 1;
-					sleep = drive->sleep;
-				}
-			} while ((drive = drive->next) != hwgroup->drive);
-			if (sleeping) {
+	/*
+	 * drive is doing pre-flush, ordered write, post-flush sequence. even
+	 * though that is 3 requests, it must be seen as a single transaction.
+	 * we must not preempt this drive until that is complete
+	 */
+	if (blk_queue_flushing(q))
 		/*
-		 * Take a short snooze, and then wake up this hwgroup again.
-		 * This gives other hwgroups on the same a chance to
-		 * play fairly with us, just in case there are big differences
-		 * in relative throughputs.. don't want to hog the cpu too much.
+		 * small race where queue could get replugged during
+		 * the 3-request flush cycle, just yank the plug since
+		 * we want it to finish asap
 		 */
-				if (time_before(sleep, jiffies + WAIT_MIN_SLEEP))
-					sleep = jiffies + WAIT_MIN_SLEEP;
-#if 1
-				if (timer_pending(&hwgroup->timer))
-					printk(KERN_CRIT "ide_set_handler: timer already active\n");
-#endif
-				/* so that ide_timer_expiry knows what to do */
-				hwgroup->sleeping = 1;
-				hwgroup->req_gen_timer = hwgroup->req_gen;
-				mod_timer(&hwgroup->timer, sleep);
-				/* we purposely leave hwgroup locked
-				 * while sleeping */
-			} else

Quoted 8 lines

drivers/ide/ide-io.c repeat: · line 724

Quoted 2 lines

drivers/ide/ide-io.c +1 -1

-				ide_unlock_hwgroup(hwgroup);
+		blk_remove_plug(q);

I'm not at all convinced that this works as expected. First of all, I think we can safely assume that the plug is removed when block layer calls into the ->request_fn(). Secondly, since the ide layer doesn't call the ->request_fn() on it's own accord, I rather suspect that this check can be dropped altogether. On the other hand, I'm not sure I agree

I suspect that this is leftover from the old code and I also think that it can be removed completely. However mixing too many real code changes in a single patch is not a good practice and the removal can be handled independently of the discussed patch.

If you would send me a patch with the above change I will be happy to integrate it into pata tree (patch can be against current Linus' tree or linux-next, either one is completely fine with me).

Quoted 7 lines

Finally, some more general blathering on the topic at hand: A while ago, I spent some thought on the possibilities of giving the block layer a notion of linked device queues as an equivalent hwgroups in ide, scsi_hosts or ata_ports and let it take care of time / bandwidth distribution among the queues belonging to one group. This is, as I understand, pretty much what your code is relying on since you have chucked out choose_drive(). However, this turned out not to be too easy

This is the right way to go and I has always advocated for it. However after seeing how libata got away with ignoring the issue altogether I'm no longer sure of this. I haven't seen any bug reports which would indicate that simplified approach has any really negative consequences.

[ Still would be great to have the control over bandwitch of "queue-group"
at the block layer level since we could also use it for distributing the available PCI[-E] bus bandwitch. ]

Quoted 7 lines

and I'm not quite sure whether we really want to go down that road. One major problem is that there is no straight forward way for the block layer to know, whether a ->request_fn() has actually taken a request off the queue and if not (or less than queue_depth anyway), whether it's just the device that couldn't take any more or the controller instead. On the whole, it seems not exactly trivial to get it right and it would probably be a good idea to consult Jens and perhaps James before

I think that having more information returned by ->request_fn() could be beneficial to the block layer (independently whether we end up adding support for "queue-groups" to the block layer or not) but this definitely needs to be verified with Jens & James.

Quoted 4 lines

embarking on such a venture. Short of that, I think that ide layer has to keep an appropriate equivalent of choose_drive() and also the while loop in the do_ide_request() function.

Thank you for your review. v1->v2 interdiff below.

v2:
* Fixes/improvements based on review from Elias:
- take as many requests off the queue as possible
- remove now redundant BUG_ON()

Quoted 1 line · Bartlomiej Zolnierkiewicz

On Monday 24 November 2008, Elias Oltmanns wrote:

Quoted 8 lines

Quoted 7 lines

Finally, some more general blathering on the topic at hand: A while ago, I spent some thought on the possibilities of giving the block layer a notion of linked device queues as an equivalent hwgroups in ide, scsi_hosts or ata_ports and let it take care of time / bandwidth distribution among the queues belonging to one group. This is, as I understand, pretty much what your code is relying on since you have chucked out choose_drive(). However, this turned out not to be too easy

Quoted 4 lines

This is the right way to go and I has always advocated for it. However after seeing how libata got away with ignoring the issue altogether I'm no longer sure of this. I haven't seen any bug reports which would indicate that simplified approach has any really negative consequences.

Well, libata can safely ignore it since scsi takes care of that (see scsi_run_queue() which is called on command completion).

Quoted 13 lines

[ Still would be great to have the control over bandwitch of "queue-group"
at the block layer level since we could also use it for distributing the available PCI[-E] bus bandwitch. ]

Quoted 7 lines

and I'm not quite sure whether we really want to go down that road. One major problem is that there is no straight forward way for the block layer to know, whether a ->request_fn() has actually taken a request off the queue and if not (or less than queue_depth anyway), whether it's just the device that couldn't take any more or the controller instead. On the whole, it seems not exactly trivial to get it right and it would probably be a good idea to consult Jens and perhaps James before

Quoted 4 lines

I think that having more information returned by ->request_fn() could be beneficial to the block layer (independently whether we end up adding support for "queue-groups" to the block layer or not) but this definitely needs to be verified with Jens & James.

Some time back, I raised this with Jens in connection with your previous version of the patchset [1]. I didn't get an answer at the time but perhaps it would help to raise it again in its own right and to give some more examples of its potential merits.