[PATCH 0/7] ide: locking improvements

[PATCH 0/7] ide: locking improvements

[Bartlomiej Zolnierkiewicz]

Locking improvements in preparation for replacing the global ide_lock
spinlock by per-hwgroup spinlocks [1].

[1] patch (which is partially based on 2005 patch from Scalex86) for this
is also ready but it needs some more audit and testing

diffstat:
 drivers/ide/ide-cd.c     |   38 ++++++-------
 drivers/ide/ide-io.c     |  129 ++++++++++++++++++++---------------------------
 drivers/ide/ide-ioctls.c |    3 -
 drivers/ide/ide-lib.c    |    7 --
 drivers/ide/ide-proc.c   |   25 +--------
 drivers/ide/ide.c        |    7 --
 6 files changed, 80 insertions(+), 129 deletions(-)

[PATCH 1/7] ide: unify ide_intr()'s exit points

[Bartlomiej Zolnierkiewicz]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] ide: unify ide_intr()'s exit points

Just a preparation for future changes.

There should be no functional changes caused by this patch.

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-io.c |   27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

Index: b/drivers/ide/ide-io.c
===================================================================
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -1363,14 +1363,13 @@ irqreturn_t ide_intr (int irq, void *dev
 	ide_drive_t *drive;
 	ide_handler_t *handler;
 	ide_startstop_t startstop;
+	irqreturn_t irq_ret = IRQ_NONE;
 
 	spin_lock_irqsave(&ide_lock, flags);
 	hwif = hwgroup->hwif;
 
-	if (!ide_ack_intr(hwif)) {
-		spin_unlock_irqrestore(&ide_lock, flags);
-		return IRQ_NONE;
-	}
+	if (!ide_ack_intr(hwif))
+		goto out;
 
 	if ((handler = hwgroup->handler) == NULL || hwgroup->polling) {
 		/*
@@ -1406,9 +1405,9 @@ irqreturn_t ide_intr (int irq, void *dev
 			(void)hwif->tp_ops->read_status(hwif);
 #endif /* CONFIG_BLK_DEV_IDEPCI */
 		}
-		spin_unlock_irqrestore(&ide_lock, flags);
-		return IRQ_NONE;
+		goto out;
 	}
+
 	drive = hwgroup->drive;
 	if (!drive) {
 		/*
@@ -1417,10 +1416,10 @@ irqreturn_t ide_intr (int irq, void *dev
 		 *
 		 * [Note - this can occur if the drive is hot unplugged]
 		 */
-		spin_unlock_irqrestore(&ide_lock, flags);
-		return IRQ_HANDLED;
+		goto out_handled;
 	}
-	if (!drive_is_ready(drive)) {
+
+	if (!drive_is_ready(drive))
 		/*
 		 * This happens regularly when we share a PCI IRQ with
 		 * another device.  Unfortunately, it can also happen
@@ -1428,9 +1427,8 @@ irqreturn_t ide_intr (int irq, void *dev
 		 * their status register is up to date.  Hopefully we have
 		 * enough advance overhead that the latter isn't a problem.
 		 */
-		spin_unlock_irqrestore(&ide_lock, flags);
-		return IRQ_NONE;
-	}
+		goto out;
+
 	if (!hwgroup->busy) {
 		hwgroup->busy = 1;	/* paranoia */
 		printk(KERN_ERR "%s: ide_intr: hwgroup->busy was 0 ??\n", drive->name);
@@ -1467,8 +1465,11 @@ irqreturn_t ide_intr (int irq, void *dev
 				"on exit\n", drive->name);
 		}
 	}
+out_handled:
+	irq_ret = IRQ_HANDLED;
+out:
 	spin_unlock_irqrestore(&ide_lock, flags);
-	return IRQ_HANDLED;
+	return irq_ret;
 }
 
 /**

[PATCH 2/7] ide: IDE settings don't need an ide_lock held

[Bartlomiej Zolnierkiewicz]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] ide: IDE settings don't need an ide_lock held

IDE settings are protected by ide_setting_mtx mutex so there is
no need to hold ide_lock in ide_setting_ioctl(), ide_read_setting()
and ide_proc_unregister_driver().

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-ioctls.c |    3 ---
 drivers/ide/ide-proc.c   |   25 ++++---------------------
 2 files changed, 4 insertions(+), 24 deletions(-)

Index: b/drivers/ide/ide-ioctls.c
===================================================================
--- a/drivers/ide/ide-ioctls.c
+++ b/drivers/ide/ide-ioctls.c
@@ -19,7 +19,6 @@ int ide_setting_ioctl(ide_drive_t *drive
 		      const struct ide_ioctl_devset *s)
 {
 	const struct ide_devset *ds;
-	unsigned long flags;
 	int err = -EOPNOTSUPP;
 
 	for (; (ds = s->setting); s++) {
@@ -33,9 +32,7 @@ int ide_setting_ioctl(ide_drive_t *drive
 
 read_val:
 	mutex_lock(&ide_setting_mtx);
-	spin_lock_irqsave(&ide_lock, flags);
 	err = ds->get(drive);
-	spin_unlock_irqrestore(&ide_lock, flags);
 	mutex_unlock(&ide_setting_mtx);
 	return err >= 0 ? put_user(err, (long __user *)arg) : err;
 
Index: b/drivers/ide/ide-proc.c
===================================================================
--- a/drivers/ide/ide-proc.c
+++ b/drivers/ide/ide-proc.c
@@ -155,13 +155,8 @@ static int ide_read_setting(ide_drive_t 
 	const struct ide_devset *ds = setting->setting;
 	int val = -EINVAL;
 
-	if (ds->get) {
-		unsigned long flags;
-
-		spin_lock_irqsave(&ide_lock, flags);
+	if (ds->get)
 		val = ds->get(drive);
-		spin_unlock_irqrestore(&ide_lock, flags);
-	}
 
 	return val;
 }
@@ -583,31 +578,19 @@ EXPORT_SYMBOL(ide_proc_register_driver);
  *	Clean up the driver specific /proc files and IDE settings
  *	for a given drive.
  *
- *	Takes ide_setting_mtx and ide_lock.
- *	Caller must hold none of the locks.
+ *	Takes ide_setting_mtx.
  */
 
 void ide_proc_unregister_driver(ide_drive_t *drive, ide_driver_t *driver)
 {
-	unsigned long flags;
-
 	ide_remove_proc_entries(drive->proc, driver->proc_entries(drive));
 
 	mutex_lock(&ide_setting_mtx);
-	spin_lock_irqsave(&ide_lock, flags);
 	/*
-	 * ide_setting_mtx protects the settings list
-	 * ide_lock protects the use of settings
-	 *
-	 * so we need to hold both, ide_settings_sem because we want to
-	 * modify the settings list, and ide_lock because we cannot take
-	 * a setting out that is being used.
-	 *
-	 * OTOH both ide_{read,write}_setting are only ever used under
-	 * ide_setting_mtx.
+	 * ide_setting_mtx protects both the settings list and the use
+	 * of settings (we cannot take a setting out that is being used).
 	 */
 	drive->settings = NULL;
-	spin_unlock_irqrestore(&ide_lock, flags);
 	mutex_unlock(&ide_setting_mtx);
 }
 EXPORT_SYMBOL(ide_proc_unregister_driver);

[PATCH 3/7] ide: __ide_port_unregister_devices() doesn't need an ide_lock held

[Bartlomiej Zolnierkiewicz]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] ide: __ide_port_unregister_devices() doesn't need an ide_lock held

[ and ide_cfg_mtx mutex provides a sufficient protection for callers ]

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide.c |    7 -------
 1 file changed, 7 deletions(-)

Index: b/drivers/ide/ide.c
===================================================================
--- a/drivers/ide/ide.c	2008-09-17 22:17:58.000000000 -0700
+++ b/drivers/ide/ide.c	2008-09-17 22:20:15.000000000 -0700
@@ -130,7 +130,6 @@
 	}
 }
 
-/* Called with ide_lock held. */
 static void __ide_port_unregister_devices(ide_hwif_t *hwif)
 {
 	int i;
@@ -139,10 +138,8 @@
 		ide_drive_t *drive = &hwif->drives[i];
 
 		if (drive->dev_flags & IDE_DFLAG_PRESENT) {
-			spin_unlock_irq(&ide_lock);
 			device_unregister(&drive->gendev);
 			wait_for_completion(&drive->gendev_rel_comp);
-			spin_lock_irq(&ide_lock);
 		}
 	}
 }
@@ -150,11 +147,9 @@
 void ide_port_unregister_devices(ide_hwif_t *hwif)
 {
 	mutex_lock(&ide_cfg_mtx);
-	spin_lock_irq(&ide_lock);
 	__ide_port_unregister_devices(hwif);
 	hwif->present = 0;
 	ide_port_init_devices_data(hwif);
-	spin_unlock_irq(&ide_lock);
 	mutex_unlock(&ide_cfg_mtx);
 }
 EXPORT_SYMBOL_GPL(ide_port_unregister_devices);
@@ -192,12 +187,10 @@
 
 	mutex_lock(&ide_cfg_mtx);
 
-	spin_lock_irq(&ide_lock);
 	if (hwif->present) {
 		__ide_port_unregister_devices(hwif);
 		hwif->present = 0;
 	}
-	spin_unlock_irq(&ide_lock);
 
 	ide_proc_unregister_port(hwif);
 

[PATCH 4/7] ide: ide_hwgroup_t.rq doesn't need an ide_lock held

[Bartlomiej Zolnierkiewicz]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] ide: ide_hwgroup_t.rq doesn't need an ide_lock held

While at it:
- no need to check for hwgroup presence in ide_dump_opcode()

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-cd.c  |   12 ++++++++----
 drivers/ide/ide-io.c  |   40 +++++++++++++++++++---------------------
 drivers/ide/ide-lib.c |    7 +------
 3 files changed, 28 insertions(+), 31 deletions(-)

Index: b/drivers/ide/ide-cd.c
===================================================================
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -317,7 +317,8 @@ static void ide_dump_status_no_sense(ide
 static int cdrom_decode_status(ide_drive_t *drive, int good_stat, int *stat_ret)
 {
 	ide_hwif_t *hwif = drive->hwif;
-	struct request *rq = hwif->hwgroup->rq;
+	ide_hwgroup_t *hwgroup = hwif->hwgroup;
+	struct request *rq = hwgroup->rq;
 	int stat, err, sense_key;
 
 	/* check for errors */
@@ -508,9 +509,10 @@ end_request:
 
 		spin_lock_irqsave(&ide_lock, flags);
 		blkdev_dequeue_request(rq);
-		HWGROUP(drive)->rq = NULL;
 		spin_unlock_irqrestore(&ide_lock, flags);
 
+		hwgroup->rq = NULL;
+
 		cdrom_queue_request_sense(drive, rq->sense, rq);
 	} else
 		cdrom_end_request(drive, 0);
@@ -950,7 +952,8 @@ static int cdrom_newpc_intr_dummy_cb(str
 static ide_startstop_t cdrom_newpc_intr(ide_drive_t *drive)
 {
 	ide_hwif_t *hwif = drive->hwif;
-	struct request *rq = HWGROUP(drive)->rq;
+	ide_hwgroup_t *hwgroup = hwif->hwgroup;
+	struct request *rq = hwgroup->rq;
 	xfer_func_t *xferfunc;
 	ide_expiry_t *expiry = NULL;
 	int dma_error = 0, dma, stat, thislen, uptodate = 0;
@@ -1157,8 +1160,9 @@ end_request:
 		spin_lock_irqsave(&ide_lock, flags);
 		if (__blk_end_request(rq, 0, dlen))
 			BUG();
-		HWGROUP(drive)->rq = NULL;
 		spin_unlock_irqrestore(&ide_lock, flags);
+
+		hwgroup->rq = NULL;
 	} else {
 		if (!uptodate)
 			rq->cmd_flags |= REQ_FAILED;
Index: b/drivers/ide/ide-io.c
===================================================================
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -107,17 +107,10 @@ static int __ide_end_request(ide_drive_t
 int ide_end_request (ide_drive_t *drive, int uptodate, int nr_sectors)
 {
 	unsigned int nr_bytes = nr_sectors << 9;
-	struct request *rq;
+	struct request *rq = drive->hwif->hwgroup->rq;
 	unsigned long flags;
 	int ret = 1;
 
-	/*
-	 * room for locking improvements here, the calls below don't
-	 * need the queue lock held at all
-	 */
-	spin_lock_irqsave(&ide_lock, flags);
-	rq = HWGROUP(drive)->rq;
-
 	if (!nr_bytes) {
 		if (blk_pc_request(rq))
 			nr_bytes = rq->data_len;
@@ -125,9 +118,10 @@ int ide_end_request (ide_drive_t *drive,
 			nr_bytes = rq->hard_cur_sectors << 9;
 	}
 
+	spin_lock_irqsave(&ide_lock, flags);
 	ret = __ide_end_request(drive, rq, uptodate, nr_bytes, 1);
-
 	spin_unlock_irqrestore(&ide_lock, flags);
+
 	return ret;
 }
 EXPORT_SYMBOL(ide_end_request);
@@ -241,8 +235,9 @@ int ide_end_dequeued_request(ide_drive_t
 	unsigned long flags;
 	int ret;
 
-	spin_lock_irqsave(&ide_lock, flags);
 	BUG_ON(!blk_rq_started(rq));
+
+	spin_lock_irqsave(&ide_lock, flags);
 	ret = __ide_end_request(drive, rq, uptodate, nr_sectors << 9, 0);
 	spin_unlock_irqrestore(&ide_lock, flags);
 
@@ -274,7 +269,11 @@ static void ide_complete_pm_request (ide
 		drive->dev_flags &= ~IDE_DFLAG_BLOCKED;
 		blk_start_queue(drive->queue);
 	}
-	HWGROUP(drive)->rq = NULL;
+	spin_unlock_irqrestore(&ide_lock, flags);
+
+	drive->hwif->hwgroup->rq = NULL;
+
+	spin_lock_irqsave(&ide_lock, flags);
 	if (__blk_end_request(rq, 0, 0))
 		BUG();
 	spin_unlock_irqrestore(&ide_lock, flags);
@@ -296,12 +295,9 @@ static void ide_complete_pm_request (ide
  
 void ide_end_drive_cmd (ide_drive_t *drive, u8 stat, u8 err)
 {
+	ide_hwgroup_t *hwgroup = drive->hwif->hwgroup;
+	struct request *rq = hwgroup->rq;
 	unsigned long flags;
-	struct request *rq;
-
-	spin_lock_irqsave(&ide_lock, flags);
-	rq = HWGROUP(drive)->rq;
-	spin_unlock_irqrestore(&ide_lock, flags);
 
 	if (rq->cmd_type == REQ_TYPE_ATA_TASKFILE) {
 		ide_task_t *task = (ide_task_t *)rq->special;
@@ -332,15 +328,16 @@ void ide_end_drive_cmd (ide_drive_t *dri
 		return;
 	}
 
-	spin_lock_irqsave(&ide_lock, flags);
-	HWGROUP(drive)->rq = NULL;
+	hwgroup->rq = NULL;
+
 	rq->errors = err;
+
+	spin_lock_irqsave(&ide_lock, flags);
 	if (unlikely(__blk_end_request(rq, (rq->errors ? -EIO : 0),
 				       blk_rq_bytes(rq))))
 		BUG();
 	spin_unlock_irqrestore(&ide_lock, flags);
 }
-
 EXPORT_SYMBOL(ide_end_drive_cmd);
 
 static void ide_kill_rq(ide_drive_t *drive, struct request *rq)
@@ -1489,11 +1486,12 @@ out:
 
 void ide_do_drive_cmd(ide_drive_t *drive, struct request *rq)
 {
+	ide_hwgroup_t *hwgroup = drive->hwif->hwgroup;
 	unsigned long flags;
-	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
-	spin_lock_irqsave(&ide_lock, flags);
 	hwgroup->rq = NULL;
+
+	spin_lock_irqsave(&ide_lock, flags);
 	__elv_add_request(drive->queue, rq, ELEVATOR_INSERT_FRONT, 1);
 	__generic_unplug_device(drive->queue);
 	spin_unlock_irqrestore(&ide_lock, flags);
Index: b/drivers/ide/ide-lib.c
===================================================================
--- a/drivers/ide/ide-lib.c
+++ b/drivers/ide/ide-lib.c
@@ -277,14 +277,9 @@ int ide_set_xfer_rate(ide_drive_t *drive
 
 static void ide_dump_opcode(ide_drive_t *drive)
 {
-	struct request *rq;
+	struct request *rq = drive->hwif->hwgroup->rq;
 	ide_task_t *task = NULL;
 
-	spin_lock(&ide_lock);
-	rq = NULL;
-	if (HWGROUP(drive))
-		rq = HWGROUP(drive)->rq;
-	spin_unlock(&ide_lock);
 	if (!rq)
 		return;
 

Re: [PATCH 4/7] ide: ide_hwgroup_t.rq doesn't need an ide_lock held

[Elias Oltmanns]

Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Subject: [PATCH] ide: ide_hwgroup_t.rq doesn't need an ide_lock held
>
> While at it:
> - no need to check for hwgroup presence in ide_dump_opcode()
>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
[...]
> Index: b/drivers/ide/ide-io.c
> ===================================================================
> --- a/drivers/ide/ide-io.c
> +++ b/drivers/ide/ide-io.c
[...]
> @@ -274,7 +269,11 @@ static void ide_complete_pm_request (ide
>  		drive->dev_flags &= ~IDE_DFLAG_BLOCKED;
>  		blk_start_queue(drive->queue);
>  	}
> -	HWGROUP(drive)->rq = NULL;
> +	spin_unlock_irqrestore(&ide_lock, flags);
> +
> +	drive->hwif->hwgroup->rq = NULL;
> +
> +	spin_lock_irqsave(&ide_lock, flags);
>  	if (__blk_end_request(rq, 0, 0))
>  		BUG();
>  	spin_unlock_irqrestore(&ide_lock, flags);

Is it really an improvement to release the lock here?

Regards,

Elias

Re: [PATCH 4/7] ide: ide_hwgroup_t.rq doesn't need an ide_lock held

[Jens Axboe]

On Fri, Oct 10 2008, Elias Oltmanns wrote:
> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > Subject: [PATCH] ide: ide_hwgroup_t.rq doesn't need an ide_lock held
> >
> > While at it:
> > - no need to check for hwgroup presence in ide_dump_opcode()
> >
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > ---
> [...]
> > Index: b/drivers/ide/ide-io.c
> > ===================================================================
> > --- a/drivers/ide/ide-io.c
> > +++ b/drivers/ide/ide-io.c
> [...]
> > @@ -274,7 +269,11 @@ static void ide_complete_pm_request (ide
> >  		drive->dev_flags &= ~IDE_DFLAG_BLOCKED;
> >  		blk_start_queue(drive->queue);
> >  	}
> > -	HWGROUP(drive)->rq = NULL;
> > +	spin_unlock_irqrestore(&ide_lock, flags);
> > +
> > +	drive->hwif->hwgroup->rq = NULL;
> > +
> > +	spin_lock_irqsave(&ide_lock, flags);
> >  	if (__blk_end_request(rq, 0, 0))
> >  		BUG();
> >  	spin_unlock_irqrestore(&ide_lock, flags);
> 
> Is it really an improvement to release the lock here?

And more importantly, is it even safe? What serializes ->rq assignments
and checks without the ide_lock? Looks fishy.

But yes, dropping a lock for an assigment just to regrab it right after
is never a win. There's no contention gain, but possible bouncing
problems.

-- 
Jens Axboe

Re: [PATCH 4/7] ide: ide_hwgroup_t.rq doesn't need an ide_lock held

[Elias Oltmanns]

Jens Axboe <jens.axboe@oracle.com> wrote:
> On Fri, Oct 10 2008, Elias Oltmanns wrote:
>> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
>
>> > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>> > Subject: [PATCH] ide: ide_hwgroup_t.rq doesn't need an ide_lock held
>> >
>> > While at it:
>> > - no need to check for hwgroup presence in ide_dump_opcode()
>> >
>> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>> > ---
>> [...]
>> > Index: b/drivers/ide/ide-io.c
>> > ===================================================================
>> > --- a/drivers/ide/ide-io.c
>> > +++ b/drivers/ide/ide-io.c
>> [...]
>> > @@ -274,7 +269,11 @@ static void ide_complete_pm_request (ide
>> >  		drive->dev_flags &= ~IDE_DFLAG_BLOCKED;
>> >  		blk_start_queue(drive->queue);
>> >  	}
>> > -	HWGROUP(drive)->rq = NULL;
>> > +	spin_unlock_irqrestore(&ide_lock, flags);
>> > +
>> > +	drive->hwif->hwgroup->rq = NULL;
>> > +
>> > +	spin_lock_irqsave(&ide_lock, flags);
>> >  	if (__blk_end_request(rq, 0, 0))
>> >  		BUG();
>> >  	spin_unlock_irqrestore(&ide_lock, flags);
>> 
>> Is it really an improvement to release the lock here?
>
> And more importantly, is it even safe? What serializes ->rq assignments
> and checks without the ide_lock? Looks fishy.

Well, I haven't quite made up my mind whether it'll work in all cases,
but I think the hwgroup->busy flag is supposed to take care of that.

Regards,

Elias

Re: [PATCH 4/7] ide: ide_hwgroup_t.rq doesn't need an ide_lock held

[Jens Axboe]

On Fri, Oct 10 2008, Elias Oltmanns wrote:
> Jens Axboe <jens.axboe@oracle.com> wrote:
> > On Fri, Oct 10 2008, Elias Oltmanns wrote:
> >> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> >
> >> > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> >> > Subject: [PATCH] ide: ide_hwgroup_t.rq doesn't need an ide_lock held
> >> >
> >> > While at it:
> >> > - no need to check for hwgroup presence in ide_dump_opcode()
> >> >
> >> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> >> > ---
> >> [...]
> >> > Index: b/drivers/ide/ide-io.c
> >> > ===================================================================
> >> > --- a/drivers/ide/ide-io.c
> >> > +++ b/drivers/ide/ide-io.c
> >> [...]
> >> > @@ -274,7 +269,11 @@ static void ide_complete_pm_request (ide
> >> >  		drive->dev_flags &= ~IDE_DFLAG_BLOCKED;
> >> >  		blk_start_queue(drive->queue);
> >> >  	}
> >> > -	HWGROUP(drive)->rq = NULL;
> >> > +	spin_unlock_irqrestore(&ide_lock, flags);
> >> > +
> >> > +	drive->hwif->hwgroup->rq = NULL;
> >> > +
> >> > +	spin_lock_irqsave(&ide_lock, flags);
> >> >  	if (__blk_end_request(rq, 0, 0))
> >> >  		BUG();
> >> >  	spin_unlock_irqrestore(&ide_lock, flags);
> >> 
> >> Is it really an improvement to release the lock here?
> >
> > And more importantly, is it even safe? What serializes ->rq assignments
> > and checks without the ide_lock? Looks fishy.
> 
> Well, I haven't quite made up my mind whether it'll work in all cases,
> but I think the hwgroup->busy flag is supposed to take care of that.

It used to be especially problematic with multi count IO, but my
knowledge and last check on that dates back to pre-2000 I think...

-- 
Jens Axboe

Re: [PATCH 4/7] ide: ide_hwgroup_t.rq doesn't need an ide_lock held

[Bartlomiej Zolnierkiewicz]

On Friday 10 October 2008, Elias Oltmanns wrote:
> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > Subject: [PATCH] ide: ide_hwgroup_t.rq doesn't need an ide_lock held
> >
> > While at it:
> > - no need to check for hwgroup presence in ide_dump_opcode()
> >
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > ---
> [...]
> > Index: b/drivers/ide/ide-io.c
> > ===================================================================
> > --- a/drivers/ide/ide-io.c
> > +++ b/drivers/ide/ide-io.c
> [...]
> > @@ -274,7 +269,11 @@ static void ide_complete_pm_request (ide
> >  		drive->dev_flags &= ~IDE_DFLAG_BLOCKED;
> >  		blk_start_queue(drive->queue);
> >  	}
> > -	HWGROUP(drive)->rq = NULL;
> > +	spin_unlock_irqrestore(&ide_lock, flags);
> > +
> > +	drive->hwif->hwgroup->rq = NULL;
> > +
> > +	spin_lock_irqsave(&ide_lock, flags);
> >  	if (__blk_end_request(rq, 0, 0))
> >  		BUG();
> >  	spin_unlock_irqrestore(&ide_lock, flags);
> 
> Is it really an improvement to release the lock here?

Yes since it is a preparation for using the right lock here
(drive->queue->queue_lock instead of ide_lock) which is done
in patch #6/7:

@@ -263,10 +260,8 @@ static void ide_complete_pm_request (ide
 
 	drive->hwif->hwgroup->rq = NULL;
 
-	spin_lock_irqsave(&ide_lock, flags);
-	if (__blk_end_request(rq, 0, 0))
+	if (blk_end_request(rq, 0, 0))
 		BUG();
-	spin_unlock_irqrestore(&ide_lock, flags);
 }

[ ide_lock and drive->queue->queue_lock are the same lock at the moment
  (which is wrong since they are meant to protect completely different
  things) but after this patchset it should be quite easy to address  ]

Also ide_complete_pm_request() above is used only for completion of
PM suspend/resume requests and is not really performance critical.

Thanks,
Bart

[PATCH 5/7] ide: push ide_lock to __ide_end_request()

[Bartlomiej Zolnierkiewicz]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] ide: push ide_lock to __ide_end_request()

__ide_end_request() needs ide_lock only for __blk_end_request()
call so push ide_lock taking inside __ide_end_requests().

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-io.c |   28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

Index: b/drivers/ide/ide-io.c
===================================================================
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -58,6 +58,7 @@
 static int __ide_end_request(ide_drive_t *drive, struct request *rq,
 			     int uptodate, unsigned int nr_bytes, int dequeue)
 {
+	unsigned long flags;
 	int ret = 1;
 	int error = 0;
 
@@ -84,11 +85,13 @@ static int __ide_end_request(ide_drive_t
 		ide_dma_on(drive);
 	}
 
-	if (!__blk_end_request(rq, error, nr_bytes)) {
-		if (dequeue)
-			HWGROUP(drive)->rq = NULL;
+	spin_lock_irqsave(&ide_lock, flags);
+	if (!__blk_end_request(rq, error, nr_bytes))
 		ret = 0;
-	}
+	spin_unlock_irqrestore(&ide_lock, flags);
+
+	if (ret == 0 && dequeue)
+		drive->hwif->hwgroup->rq = NULL;
 
 	return ret;
 }
@@ -108,8 +111,6 @@ int ide_end_request (ide_drive_t *drive,
 {
 	unsigned int nr_bytes = nr_sectors << 9;
 	struct request *rq = drive->hwif->hwgroup->rq;
-	unsigned long flags;
-	int ret = 1;
 
 	if (!nr_bytes) {
 		if (blk_pc_request(rq))
@@ -118,11 +119,7 @@ int ide_end_request (ide_drive_t *drive,
 			nr_bytes = rq->hard_cur_sectors << 9;
 	}
 
-	spin_lock_irqsave(&ide_lock, flags);
-	ret = __ide_end_request(drive, rq, uptodate, nr_bytes, 1);
-	spin_unlock_irqrestore(&ide_lock, flags);
-
-	return ret;
+	return __ide_end_request(drive, rq, uptodate, nr_bytes, 1);
 }
 EXPORT_SYMBOL(ide_end_request);
 
@@ -232,16 +229,9 @@ out_do_tf:
 int ide_end_dequeued_request(ide_drive_t *drive, struct request *rq,
 			     int uptodate, int nr_sectors)
 {
-	unsigned long flags;
-	int ret;
-
 	BUG_ON(!blk_rq_started(rq));
 
-	spin_lock_irqsave(&ide_lock, flags);
-	ret = __ide_end_request(drive, rq, uptodate, nr_sectors << 9, 0);
-	spin_unlock_irqrestore(&ide_lock, flags);
-
-	return ret;
+	return __ide_end_request(drive, rq, uptodate, nr_sectors << 9, 0);
 }
 EXPORT_SYMBOL_GPL(ide_end_dequeued_request);
 

[PATCH 6/7] ide: ide_lock + __blk_end_request() -> blk_end_request()

[Bartlomiej Zolnierkiewicz]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] ide: ide_lock + __blk_end_request() -> blk_end_request()

Use blk_end_request() instead of ide_lock + __blk_end_request()
in cdrom_end_request(), cdrom_newpc_intr(), __ide_end_request(),
ide_complete_pm_request() and ide_end_drive_cmd().

[ ide_lock is currently also used as queue lock ]

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-cd.c |   12 +++---------
 drivers/ide/ide-io.c |   16 ++++------------
 2 files changed, 7 insertions(+), 21 deletions(-)

Index: b/drivers/ide/ide-cd.c
===================================================================
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -262,7 +262,6 @@ static void cdrom_end_request(ide_drive_
 		struct request *failed = (struct request *) rq->buffer;
 		struct cdrom_info *info = drive->driver_data;
 		void *sense = &info->sense_data;
-		unsigned long flags;
 
 		if (failed) {
 			if (failed->sense) {
@@ -278,11 +277,9 @@ static void cdrom_end_request(ide_drive_
 						failed->hard_nr_sectors))
 					BUG();
 			} else {
-				spin_lock_irqsave(&ide_lock, flags);
-				if (__blk_end_request(failed, -EIO,
-						      failed->data_len))
+				if (blk_end_request(failed, -EIO,
+						    failed->data_len))
 					BUG();
-				spin_unlock_irqrestore(&ide_lock, flags);
 			}
 		} else
 			cdrom_analyze_sense_data(drive, NULL, sense);
@@ -1151,16 +1148,13 @@ static ide_startstop_t cdrom_newpc_intr(
 
 end_request:
 	if (blk_pc_request(rq)) {
-		unsigned long flags;
 		unsigned int dlen = rq->data_len;
 
 		if (dma)
 			rq->data_len = 0;
 
-		spin_lock_irqsave(&ide_lock, flags);
-		if (__blk_end_request(rq, 0, dlen))
+		if (blk_end_request(rq, 0, dlen))
 			BUG();
-		spin_unlock_irqrestore(&ide_lock, flags);
 
 		hwgroup->rq = NULL;
 	} else {
Index: b/drivers/ide/ide-io.c
===================================================================
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -58,7 +58,6 @@
 static int __ide_end_request(ide_drive_t *drive, struct request *rq,
 			     int uptodate, unsigned int nr_bytes, int dequeue)
 {
-	unsigned long flags;
 	int ret = 1;
 	int error = 0;
 
@@ -85,10 +84,8 @@ static int __ide_end_request(ide_drive_t
 		ide_dma_on(drive);
 	}
 
-	spin_lock_irqsave(&ide_lock, flags);
-	if (!__blk_end_request(rq, error, nr_bytes))
+	if (!blk_end_request(rq, error, nr_bytes))
 		ret = 0;
-	spin_unlock_irqrestore(&ide_lock, flags);
 
 	if (ret == 0 && dequeue)
 		drive->hwif->hwgroup->rq = NULL;
@@ -263,10 +260,8 @@ static void ide_complete_pm_request (ide
 
 	drive->hwif->hwgroup->rq = NULL;
 
-	spin_lock_irqsave(&ide_lock, flags);
-	if (__blk_end_request(rq, 0, 0))
+	if (blk_end_request(rq, 0, 0))
 		BUG();
-	spin_unlock_irqrestore(&ide_lock, flags);
 }
 
 /**
@@ -287,7 +282,6 @@ void ide_end_drive_cmd (ide_drive_t *dri
 {
 	ide_hwgroup_t *hwgroup = drive->hwif->hwgroup;
 	struct request *rq = hwgroup->rq;
-	unsigned long flags;
 
 	if (rq->cmd_type == REQ_TYPE_ATA_TASKFILE) {
 		ide_task_t *task = (ide_task_t *)rq->special;
@@ -322,11 +316,9 @@ void ide_end_drive_cmd (ide_drive_t *dri
 
 	rq->errors = err;
 
-	spin_lock_irqsave(&ide_lock, flags);
-	if (unlikely(__blk_end_request(rq, (rq->errors ? -EIO : 0),
-				       blk_rq_bytes(rq))))
+	if (unlikely(blk_end_request(rq, (rq->errors ? -EIO : 0),
+				     blk_rq_bytes(rq))))
 		BUG();
-	spin_unlock_irqrestore(&ide_lock, flags);
 }
 EXPORT_SYMBOL(ide_end_drive_cmd);
 

[PATCH 7/7] ide: use queue lock instead of ide_lock when possible

[Bartlomiej Zolnierkiewicz]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] ide: use queue lock instead of ide_lock when possible

This is just a preparation for future changes and there should be no
functional changes caused by this patch since ide_lock is currently
also used as queue lock.

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-cd.c |   14 ++++++++------
 drivers/ide/ide-io.c |   19 ++++++++++---------
 2 files changed, 18 insertions(+), 15 deletions(-)

Index: b/drivers/ide/ide-cd.c
===================================================================
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -424,16 +424,17 @@ static int cdrom_decode_status(ide_drive
 				if (time_after(jiffies, info->write_timeout))
 					do_end_request = 1;
 				else {
+					struct request_queue *q = drive->queue;
 					unsigned long flags;
 
 					/*
 					 * take a breather relying on the unplug
 					 * timer to kick us again
 					 */
-					spin_lock_irqsave(&ide_lock, flags);
-					blk_plug_device(drive->queue);
-					spin_unlock_irqrestore(&ide_lock,
-								flags);
+					spin_lock_irqsave(q->queue_lock, flags);
+					blk_plug_device(q);
+					spin_unlock_irqrestore(q->queue_lock, flags);
+
 					return 1;
 				}
 			}
@@ -502,11 +503,12 @@ static int cdrom_decode_status(ide_drive
 
 end_request:
 	if (stat & ATA_ERR) {
+		struct request_queue *q = drive->queue;
 		unsigned long flags;
 
-		spin_lock_irqsave(&ide_lock, flags);
+		spin_lock_irqsave(q->queue_lock, flags);
 		blkdev_dequeue_request(rq);
-		spin_unlock_irqrestore(&ide_lock, flags);
+		spin_unlock_irqrestore(q->queue_lock, flags);
 
 		hwgroup->rq = NULL;
 
Index: b/drivers/ide/ide-io.c
===================================================================
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -243,20 +243,21 @@ EXPORT_SYMBOL_GPL(ide_end_dequeued_reque
  */
 static void ide_complete_pm_request (ide_drive_t *drive, struct request *rq)
 {
+	struct request_queue *q = drive->queue;
 	unsigned long flags;
 
 #ifdef DEBUG_PM
 	printk("%s: completing PM request, %s\n", drive->name,
 	       blk_pm_suspend_request(rq) ? "suspend" : "resume");
 #endif
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(q->queue_lock, flags);
 	if (blk_pm_suspend_request(rq)) {
-		blk_stop_queue(drive->queue);
+		blk_stop_queue(q);
 	} else {
 		drive->dev_flags &= ~IDE_DFLAG_BLOCKED;
-		blk_start_queue(drive->queue);
+		blk_start_queue(q);
 	}
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(q->queue_lock, flags);
 
 	drive->hwif->hwgroup->rq = NULL;
 
@@ -1469,16 +1470,16 @@ out:
 void ide_do_drive_cmd(ide_drive_t *drive, struct request *rq)
 {
 	ide_hwgroup_t *hwgroup = drive->hwif->hwgroup;
+	struct request_queue *q = drive->queue;
 	unsigned long flags;
 
 	hwgroup->rq = NULL;
 
-	spin_lock_irqsave(&ide_lock, flags);
-	__elv_add_request(drive->queue, rq, ELEVATOR_INSERT_FRONT, 1);
-	__generic_unplug_device(drive->queue);
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_lock_irqsave(q->queue_lock, flags);
+	__elv_add_request(q, rq, ELEVATOR_INSERT_FRONT, 1);
+	__generic_unplug_device(q);
+	spin_unlock_irqrestore(q->queue_lock, flags);
 }
-
 EXPORT_SYMBOL(ide_do_drive_cmd);
 
 void ide_pktcmd_tf_load(ide_drive_t *drive, u32 tf_flags, u16 bcount, u8 dma)

Re: [PATCH 7/7] ide: use queue lock instead of ide_lock when possible

[Elias Oltmanns]

Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Subject: [PATCH] ide: use queue lock instead of ide_lock when possible
>
> This is just a preparation for future changes and there should be no
> functional changes caused by this patch since ide_lock is currently
> also used as queue lock.
>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
[...]
> Index: b/drivers/ide/ide-io.c
> ===================================================================
> --- a/drivers/ide/ide-io.c
> +++ b/drivers/ide/ide-io.c
[...]
> @@ -1469,16 +1470,16 @@ out:
>  void ide_do_drive_cmd(ide_drive_t *drive, struct request *rq)
>  {
>  	ide_hwgroup_t *hwgroup = drive->hwif->hwgroup;
> +	struct request_queue *q = drive->queue;
>  	unsigned long flags;
>  
>  	hwgroup->rq = NULL;
>  
> -	spin_lock_irqsave(&ide_lock, flags);
> -	__elv_add_request(drive->queue, rq, ELEVATOR_INSERT_FRONT, 1);
> -	__generic_unplug_device(drive->queue);
> -	spin_unlock_irqrestore(&ide_lock, flags);
> +	spin_lock_irqsave(q->queue_lock, flags);
> +	__elv_add_request(q, rq, ELEVATOR_INSERT_FRONT, 1);
> +	__generic_unplug_device(q);

By the way, wouldn't blk_run_queue() be more appropriate here? It looks
to me as if blk_run_queue() was the thing intended for general usage by
low level drivers who don't know and care about schedulers, whereas the
usage of __generic_unplug_device() should mostly be restricted to the
block layer. On the other hand, there are other drivers in
drivers/block/ that use __generic_unplug_device(), so I may well be
wrong. Jens?

Regards,

Elias

Re: [PATCH 7/7] ide: use queue lock instead of ide_lock when possible

[Jens Axboe]

On Fri, Oct 10 2008, Elias Oltmanns wrote:
> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > Subject: [PATCH] ide: use queue lock instead of ide_lock when possible
> >
> > This is just a preparation for future changes and there should be no
> > functional changes caused by this patch since ide_lock is currently
> > also used as queue lock.
> >
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > ---
> [...]
> > Index: b/drivers/ide/ide-io.c
> > ===================================================================
> > --- a/drivers/ide/ide-io.c
> > +++ b/drivers/ide/ide-io.c
> [...]
> > @@ -1469,16 +1470,16 @@ out:
> >  void ide_do_drive_cmd(ide_drive_t *drive, struct request *rq)
> >  {
> >  	ide_hwgroup_t *hwgroup = drive->hwif->hwgroup;
> > +	struct request_queue *q = drive->queue;
> >  	unsigned long flags;
> >  
> >  	hwgroup->rq = NULL;
> >  
> > -	spin_lock_irqsave(&ide_lock, flags);
> > -	__elv_add_request(drive->queue, rq, ELEVATOR_INSERT_FRONT, 1);
> > -	__generic_unplug_device(drive->queue);
> > -	spin_unlock_irqrestore(&ide_lock, flags);
> > +	spin_lock_irqsave(q->queue_lock, flags);
> > +	__elv_add_request(q, rq, ELEVATOR_INSERT_FRONT, 1);
> > +	__generic_unplug_device(q);
> 
> By the way, wouldn't blk_run_queue() be more appropriate here? It looks
> to me as if blk_run_queue() was the thing intended for general usage by
> low level drivers who don't know and care about schedulers, whereas the
> usage of __generic_unplug_device() should mostly be restricted to the
> block layer. On the other hand, there are other drivers in
> drivers/block/ that use __generic_unplug_device(), so I may well be
> wrong. Jens?

Yes, that is correct. But it's ok for now, there are too many variants
of this around as it is already. I'm about to do a run and clean them up
and make sure we have a single sane way of doing it that is exported.

-- 
Jens Axboe

Re: [PATCH 7/7] ide: use queue lock instead of ide_lock when possible

[Jens Axboe]

On Fri, Oct 10 2008, Jens Axboe wrote:
> On Fri, Oct 10 2008, Elias Oltmanns wrote:
> > Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> > > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > > Subject: [PATCH] ide: use queue lock instead of ide_lock when possible
> > >
> > > This is just a preparation for future changes and there should be no
> > > functional changes caused by this patch since ide_lock is currently
> > > also used as queue lock.
> > >
> > > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > > ---
> > [...]
> > > Index: b/drivers/ide/ide-io.c
> > > ===================================================================
> > > --- a/drivers/ide/ide-io.c
> > > +++ b/drivers/ide/ide-io.c
> > [...]
> > > @@ -1469,16 +1470,16 @@ out:
> > >  void ide_do_drive_cmd(ide_drive_t *drive, struct request *rq)
> > >  {
> > >  	ide_hwgroup_t *hwgroup = drive->hwif->hwgroup;
> > > +	struct request_queue *q = drive->queue;
> > >  	unsigned long flags;
> > >  
> > >  	hwgroup->rq = NULL;
> > >  
> > > -	spin_lock_irqsave(&ide_lock, flags);
> > > -	__elv_add_request(drive->queue, rq, ELEVATOR_INSERT_FRONT, 1);
> > > -	__generic_unplug_device(drive->queue);
> > > -	spin_unlock_irqrestore(&ide_lock, flags);
> > > +	spin_lock_irqsave(q->queue_lock, flags);
> > > +	__elv_add_request(q, rq, ELEVATOR_INSERT_FRONT, 1);
> > > +	__generic_unplug_device(q);
> > 
> > By the way, wouldn't blk_run_queue() be more appropriate here? It looks
> > to me as if blk_run_queue() was the thing intended for general usage by
> > low level drivers who don't know and care about schedulers, whereas the
> > usage of __generic_unplug_device() should mostly be restricted to the
> > block layer. On the other hand, there are other drivers in
> > drivers/block/ that use __generic_unplug_device(), so I may well be
> > wrong. Jens?
> 
> Yes, that is correct. But it's ok for now, there are too many variants
> of this around as it is already. I'm about to do a run and clean them up
> and make sure we have a single sane way of doing it that is exported.

So the right thing to do here is:

        __elv_add_request(drive->queue, rq, ELEVATOR_INSERT_FRONT, 0);
        blk_start_queueing(drive->queue);

since you don't need to do any plugging.

-- 
Jens Axboe

Re: [PATCH 0/7] ide: locking improvements

[Jens Axboe]

On Wed, Oct 08 2008, Bartlomiej Zolnierkiewicz wrote:
> 
> Locking improvements in preparation for replacing the global ide_lock
> spinlock by per-hwgroup spinlocks [1].
> 
> [1] patch (which is partially based on 2005 patch from Scalex86) for this
> is also ready but it needs some more audit and testing
> 
> diffstat:
>  drivers/ide/ide-cd.c     |   38 ++++++-------
>  drivers/ide/ide-io.c     |  129 ++++++++++++++++++++---------------------------
>  drivers/ide/ide-ioctls.c |    3 -
>  drivers/ide/ide-lib.c    |    7 --
>  drivers/ide/ide-proc.c   |   25 +--------
>  drivers/ide/ide.c        |    7 --
>  6 files changed, 80 insertions(+), 129 deletions(-)

Sorry, but I just have to ask 'why'? IDE is seeing a whole lot of churn
for something that should essentially be a stable code base in
maintenance mode, and now scalability improvements?

Just doesn't make ANY sense to me, sorry. We may end up with a cleaner
code base, but likely also a buggier one. It's not like hardware
coverage testing is all that great, considering some of the ancient
stuff it supports :-)

-- 
Jens Axboe

Re: [PATCH 0/7] ide: locking improvements

[Bartlomiej Zolnierkiewicz]

On Thu, Oct 9, 2008 at 8:51 AM, Jens Axboe <jens.axboe@oracle.com> wrote:
> On Wed, Oct 08 2008, Bartlomiej Zolnierkiewicz wrote:
>>
>> Locking improvements in preparation for replacing the global ide_lock
>> spinlock by per-hwgroup spinlocks [1].
>>
>> [1] patch (which is partially based on 2005 patch from Scalex86) for this
>> is also ready but it needs some more audit and testing
>>
>> diffstat:
>>  drivers/ide/ide-cd.c     |   38 ++++++-------
>>  drivers/ide/ide-io.c     |  129 ++++++++++++++++++++---------------------------
>>  drivers/ide/ide-ioctls.c |    3 -
>>  drivers/ide/ide-lib.c    |    7 --
>>  drivers/ide/ide-proc.c   |   25 +--------
>>  drivers/ide/ide.c        |    7 --
>>  6 files changed, 80 insertions(+), 129 deletions(-)
>
> Sorry, but I just have to ask 'why'? IDE is seeing a whole lot of churn
> for something that should essentially be a stable code base in
> maintenance mode, and now scalability improvements?

It is the stable code but being in "maintenance only mode" has never
been true and as long as there are active users & developers there is
really no reason to change it.

> Just doesn't make ANY sense to me, sorry. We may end up with a cleaner
> code base, but likely also a buggier one. It's not like hardware
> coverage testing is all that great, considering some of the ancient
> stuff it supports :-)

The changes above are relatively safe/simple and are not hardware specific.

Thanks for worring about IDE but we should be fine. :)

Bart

Re: [PATCH 0/7] ide: locking improvements

[Jens Axboe]

On Thu, Oct 09 2008, Bartlomiej Zolnierkiewicz wrote:
> On Thu, Oct 9, 2008 at 8:51 AM, Jens Axboe <jens.axboe@oracle.com> wrote:
> > On Wed, Oct 08 2008, Bartlomiej Zolnierkiewicz wrote:
> >>
> >> Locking improvements in preparation for replacing the global ide_lock
> >> spinlock by per-hwgroup spinlocks [1].
> >>
> >> [1] patch (which is partially based on 2005 patch from Scalex86) for this
> >> is also ready but it needs some more audit and testing
> >>
> >> diffstat:
> >>  drivers/ide/ide-cd.c     |   38 ++++++-------
> >>  drivers/ide/ide-io.c     |  129 ++++++++++++++++++++---------------------------
> >>  drivers/ide/ide-ioctls.c |    3 -
> >>  drivers/ide/ide-lib.c    |    7 --
> >>  drivers/ide/ide-proc.c   |   25 +--------
> >>  drivers/ide/ide.c        |    7 --
> >>  6 files changed, 80 insertions(+), 129 deletions(-)
> >
> > Sorry, but I just have to ask 'why'? IDE is seeing a whole lot of churn
> > for something that should essentially be a stable code base in
> > maintenance mode, and now scalability improvements?
> 
> It is the stable code but being in "maintenance only mode" has never
> been true and as long as there are active users & developers there is
> really no reason to change it.

Well, maybe then it's just me who thinks that it definitely SHOULD be in
deep maintenance mode...

> > Just doesn't make ANY sense to me, sorry. We may end up with a cleaner
> > code base, but likely also a buggier one. It's not like hardware
> > coverage testing is all that great, considering some of the ancient
> > stuff it supports :-)
> 
> The changes above are relatively safe/simple and are not hardware specific.
> 
> Thanks for worring about IDE but we should be fine. :)
> 
> Bart

-- 
Jens Axboe