[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

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

[Robert Hancock]

Bartlomiej Zolnierkiewicz wrote:
>> 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.

Are there really many active users at this point? I'm not aware of any 
new distributions that are using it. The only people I can see that 
might still want to be using it would be people with old setups or old 
embedded devices.. many of those wouldn't be using newer kernels anyway.

These kinds of changes only will really help scalability on multi-core 
machines which are unlikely to be using this code anyway.. They seem 
rather like putting makeup on a corpse to me..

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

[Bartlomiej Zolnierkiewicz]

On Saturday 11 October 2008, Robert Hancock wrote:
> Bartlomiej Zolnierkiewicz wrote:
> >> 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.
> 
> Are there really many active users at this point? I'm not aware of any 
> new distributions that are using it. The only people I can see that 
> might still want to be using it would be people with old setups or old 
> embedded devices.. many of those wouldn't be using newer kernels anyway.

Like I said before: as long as there are any active users/developers
there is no real reason to stop IDE improvements (especially since there
is no complete replacement available).

I also wouldn't worry that much about what some distros are doing.  They
are free to make their own decisions based on whatever criteria they like.

> These kinds of changes only will really help scalability on multi-core 
> machines which are unlikely to be using this code anyway.. They seem 

>From my perspective the main gain of these patches is the increased
maintainability and sanity of the code, scalability improvements are
just an added bonus.

> rather like putting makeup on a corpse to me..

Please refrain from such comments.  Not only the metaphor is completely
bogus but it may sound disrespectful to some IDE developers.

Thanks,
Bart

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

[Borislav Petkov]

Hi,

On Sat, Oct 11, 2008 at 01:39:44PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Saturday 11 October 2008, Robert Hancock wrote:
> > Bartlomiej Zolnierkiewicz wrote:
> > >> 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.
> > 
> > Are there really many active users at this point? I'm not aware of any 
> > new distributions that are using it. The only people I can see that 
> > might still want to be using it would be people with old setups or old 
> > embedded devices.. many of those wouldn't be using newer kernels anyway.
> 
> Like I said before: as long as there are any active users/developers
> there is no real reason to stop IDE improvements (especially since there
> is no complete replacement available).

... and to be completely clear on things, what Bart and other guys are doing
_is_ maintenance - simply keeping the codebase from becoming a big stinking pile
of sh*t which noone can maintain with time. If you do the effort and count what
percentage of the patches have "There should be no functional change resulting
from this patch" in them you'll see that this is the majority and they rather
clean up/simplify/fix code than add anything new, not even mentioning new
features. So yes, this _is_ maintenance on a larger scale and this is a good(tm)
thing.

> I also wouldn't worry that much about what some distros are doing.  They
> are free to make their own decisions based on whatever criteria they like.
> 
> > These kinds of changes only will really help scalability on multi-core 
> > machines which are unlikely to be using this code anyway.. They seem 
> 
> >From my perspective the main gain of these patches is the increased
> maintainability and sanity of the code, scalability improvements are
> just an added bonus.

and better code/improved scalability is a bad thing because... ?!

> > rather like putting makeup on a corpse to me..

so _NOT_ true.

-- 
Regards/Gruss,
    Boris.

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

[Jens Axboe]

On Sat, Oct 11 2008, Borislav Petkov wrote:
> > >From my perspective the main gain of these patches is the increased
> > maintainability and sanity of the code, scalability improvements are
> > just an added bonus.
> 
> and better code/improved scalability is a bad thing because... ?!

It's a bad thing because nobody on earth cares about IDE scalability,
from a performance POV a modern SATA controller is just better on
several levels. I don't think anybody cares about IDE scaling on 8-16
cores or more, simply because NOBODY is using IDE on such systems.

As such, trying to improve locking is a pointless exercise. And that is
a bad thing, because code change invariably brings in code bugs. Then
see previous mail on lack of coverage testing, and it can naturally be
harmful.

> > > rather like putting makeup on a corpse to me..
> 
> so _NOT_ true.

Depends on what you think is the corpse. Since IDE is essentially dead
and frozen, it IS a corpse and the phrase is then very appropriate. This
is not a personal jab at the IDE guys and does not reflect on the
(mostly) good work they do, just a reflection on the state of IDE in
general.

-- 
Jens Axboe

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

[Bartlomiej Zolnierkiewicz]

On Saturday 11 October 2008, Jens Axboe wrote:
> On Sat, Oct 11 2008, Borislav Petkov wrote:
> > > >From my perspective the main gain of these patches is the increased
> > > maintainability and sanity of the code, scalability improvements are
> > > just an added bonus.
> > 
> > and better code/improved scalability is a bad thing because... ?!
> 
> It's a bad thing because nobody on earth cares about IDE scalability,

JFYI: just yesterday I got mail proving otherwise. ;)

> from a performance POV a modern SATA controller is just better on
> several levels. I don't think anybody cares about IDE scaling on 8-16
> cores or more, simply because NOBODY is using IDE on such systems.
> 
> As such, trying to improve locking is a pointless exercise. And that is
> a bad thing, because code change invariably brings in code bugs. Then
> see previous mail on lack of coverage testing, and it can naturally be
> harmful.

Your concerns were already addressed in my reply but I worry that having
a discussion based on technical arguments is not your goal.

Just to repeat: these patches are not hardware specific and obviously
they are not going to be merged today, tomorrow or in a week (they are
2.6.29 material after months of time in pata tree / linux-next).

> > > > rather like putting makeup on a corpse to me..
> > 
> > so _NOT_ true.
> 
> Depends on what you think is the corpse. Since IDE is essentially dead
> and frozen, it IS a corpse and the phrase is then very appropriate. This
> is not a personal jab at the IDE guys and does not reflect on the
> (mostly) good work they do, just a reflection on the state of IDE in
> general.

Interesting statement given that i.e. diffstat-wise pata tree has more
than twice as much stuff queued up for 2.6.28 than "some other" trees
(and we have history of being a _very_ conservative w.r.t. to needlessly
moving code around in drivers/ide/).

Please stop being silly and pushing your view/idea on what other people
should be doing (not to mention ignoring real facts).

Thanks,
Bart

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

[Jens Axboe]

On Sat, Oct 11 2008, Bartlomiej Zolnierkiewicz wrote:
> On Saturday 11 October 2008, Jens Axboe wrote:
> > On Sat, Oct 11 2008, Borislav Petkov wrote:
> > > > >From my perspective the main gain of these patches is the increased
> > > > maintainability and sanity of the code, scalability improvements are
> > > > just an added bonus.
> > > 
> > > and better code/improved scalability is a bad thing because... ?!
> > 
> > It's a bad thing because nobody on earth cares about IDE scalability,
> 
> JFYI: just yesterday I got mail proving otherwise. ;)

Well, there are lots of crazy people in the world, a request from
someone doesn't necessarily make it a good idea :-)

> > from a performance POV a modern SATA controller is just better on
> > several levels. I don't think anybody cares about IDE scaling on 8-16
> > cores or more, simply because NOBODY is using IDE on such systems.
> > 
> > As such, trying to improve locking is a pointless exercise. And that is
> > a bad thing, because code change invariably brings in code bugs. Then
> > see previous mail on lack of coverage testing, and it can naturally be
> > harmful.
> 
> Your concerns were already addressed in my reply but I worry that having
> a discussion based on technical arguments is not your goal.

You make it sound like I have an alterior motive, which I definitely do
not. I just wondered what all the IDE churn was supposed to be good
for...

> Just to repeat: these patches are not hardware specific and obviously
> they are not going to be merged today, tomorrow or in a week (they are
> 2.6.29 material after months of time in pata tree / linux-next).

It's less about this specific patchset than in general. The specific one
looked fine by itself, it's just the path to to 'IDE lock scaling' that
is a bit crazy to me. Moving IDE to the softirq completion like SCSI
would be a better start, imho. IDE still does most of the processing
under its ide_lock, which isn't even necessary. Making the locking more
finely grained is what I think is pretty crazy.

> > > > > rather like putting makeup on a corpse to me..
> > > 
> > > so _NOT_ true.
> > 
> > Depends on what you think is the corpse. Since IDE is essentially dead
> > and frozen, it IS a corpse and the phrase is then very appropriate. This
> > is not a personal jab at the IDE guys and does not reflect on the
> > (mostly) good work they do, just a reflection on the state of IDE in
> > general.
> 
> Interesting statement given that i.e. diffstat-wise pata tree has more
> than twice as much stuff queued up for 2.6.28 than "some other" trees
> (and we have history of being a _very_ conservative w.r.t. to needlessly
> moving code around in drivers/ide/).
> 
> Please stop being silly and pushing your view/idea on what other people
> should be doing (not to mention ignoring real facts).

Please start by actually _reading_ what I write. Your reply is based on
the code base, my statement pertains to IDE on the hardware side
(devices, controllers, etc). On the scalability front, what new hardware
do you envision shipping with IDE controllers that are actually used for
pushing large amounts of data? People would have to be borderline insane
to make such new hw today.

I'm not pushing my views on anyone, but I am free to share what I
actually think. I actually care about old code stability, hence my
concern with the amount of IDE changes.

-- 
Jens Axboe

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

[Bartlomiej Zolnierkiewicz]

On Saturday 11 October 2008, Jens Axboe wrote:
> On Sat, Oct 11 2008, Bartlomiej Zolnierkiewicz wrote:
> > On Saturday 11 October 2008, Jens Axboe wrote:
> > > On Sat, Oct 11 2008, Borislav Petkov wrote:
> > > > > >From my perspective the main gain of these patches is the increased
> > > > > maintainability and sanity of the code, scalability improvements are
> > > > > just an added bonus.
> > > > 
> > > > and better code/improved scalability is a bad thing because... ?!
> > > 
> > > It's a bad thing because nobody on earth cares about IDE scalability,
> > 
> > JFYI: just yesterday I got mail proving otherwise. ;)
> 
> Well, there are lots of crazy people in the world, a request from
> someone doesn't necessarily make it a good idea :-)
> 
> > > from a performance POV a modern SATA controller is just better on
> > > several levels. I don't think anybody cares about IDE scaling on 8-16
> > > cores or more, simply because NOBODY is using IDE on such systems.
> > > 
> > > As such, trying to improve locking is a pointless exercise. And that is
> > > a bad thing, because code change invariably brings in code bugs. Then
> > > see previous mail on lack of coverage testing, and it can naturally be
> > > harmful.
> > 
> > Your concerns were already addressed in my reply but I worry that having
> > a discussion based on technical arguments is not your goal.
> 
> You make it sound like I have an alterior motive, which I definitely do
> not. I just wondered what all the IDE churn was supposed to be good
> for...
> 
> > Just to repeat: these patches are not hardware specific and obviously
> > they are not going to be merged today, tomorrow or in a week (they are
> > 2.6.29 material after months of time in pata tree / linux-next).
> 
> It's less about this specific patchset than in general. The specific one
> looked fine by itself, it's just the path to to 'IDE lock scaling' that
> is a bit crazy to me. Moving IDE to the softirq completion like SCSI
> would be a better start, imho. IDE still does most of the processing

We are getting there but this can't be done without fixing some other
stuff (like the patch posted today to not process the next request from
hard-IRQ context).  Any help would be much appreciated.

> under its ide_lock, which isn't even necessary. Making the locking more

Well, actually it doesn't do most work under ide_lock (never has been)
as the main means of protection is hwgroup->busy (which is protected by
ide_lock).

[ OTOH some changes in this patchset were inspired by _your_ comments about
  "room for locking improvements" in ide-io.c (IIRC) so kudos to you! ]

> finely grained is what I think is pretty crazy.

The more fine grained locking changes that were posted in separate patch
were first done 3 years ago by Scalex86 guys and they were extensively
tested on Intel hardware (however since other host drivers and things
like IDE settings were abusing ide_lock applying this change back than
was impossible - all of these stuff is fixed in the current Linus' tree).

Sorry for not explaining this clearly in the changelog.

> > > > > > rather like putting makeup on a corpse to me..
> > > > 
> > > > so _NOT_ true.
> > > 
> > > Depends on what you think is the corpse. Since IDE is essentially dead
> > > and frozen, it IS a corpse and the phrase is then very appropriate. This
> > > is not a personal jab at the IDE guys and does not reflect on the
> > > (mostly) good work they do, just a reflection on the state of IDE in
> > > general.
> > 
> > Interesting statement given that i.e. diffstat-wise pata tree has more
> > than twice as much stuff queued up for 2.6.28 than "some other" trees
> > (and we have history of being a _very_ conservative w.r.t. to needlessly
> > moving code around in drivers/ide/).
> > 
> > Please stop being silly and pushing your view/idea on what other people
> > should be doing (not to mention ignoring real facts).
> 
> Please start by actually _reading_ what I write. Your reply is based on
> the code base, my statement pertains to IDE on the hardware side
> (devices, controllers, etc). On the scalability front, what new hardware
> do you envision shipping with IDE controllers that are actually used for
> pushing large amounts of data? People would have to be borderline insane
> to make such new hw today.

Please understand that we are not doing new-hardware-driven-development
here but rather a big maintainance project.  Like I said in reply to Robert
the scalability improvements are an added bonus from my POV -> the main
goal is making the code simpler, harder to abuse and easier to maintain.

> I'm not pushing my views on anyone, but I am free to share what I
> actually think. I actually care about old code stability, hence my
> concern with the amount of IDE changes.

The actual users' feedback about amount of IDE changes have been mostly
positive (some ask for even more changes :) so I don't think that it
should be a reason of a great worries.  I hope that all other concerns
have been also cleared now.

Thanks,
Bart

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

[Borislav Petkov]

On Sat, Oct 11, 2008 at 05:56:37PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Saturday 11 October 2008, Jens Axboe wrote:
> > On Sat, Oct 11 2008, Bartlomiej Zolnierkiewicz wrote:
> > > On Saturday 11 October 2008, Jens Axboe wrote:
> > > > On Sat, Oct 11 2008, Borislav Petkov wrote:

[... ]

Don't you just love it when all gets resolved peacefully and everything is fine
and dandy afterwards. I'm so happy we've straightened that one out. Just a final
quick note which by no means is targetting to resparkle the discussion: I have
another bug report, hm, well :), but still, the machine is a 8-core AMD Dell box
(with the option of upgrading to 16 cores) which _is_ using ide-cd so, there are
some people using that code, after all :). I'm sure those people will be happier
when it scales to that many cores.

http://bugzilla.kernel.org/show_bug.cgi?id=11498.

-- 
Regards/Gruss,
    Boris.

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

[Jens Axboe]

On Sat, Oct 11 2008, Borislav Petkov wrote:
> On Sat, Oct 11, 2008 at 05:56:37PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Saturday 11 October 2008, Jens Axboe wrote:
> > > On Sat, Oct 11 2008, Bartlomiej Zolnierkiewicz wrote:
> > > > On Saturday 11 October 2008, Jens Axboe wrote:
> > > > > On Sat, Oct 11 2008, Borislav Petkov wrote:
> 
> [... ]
> 
> Don't you just love it when all gets resolved peacefully and
> everything is fine and dandy afterwards. I'm so happy we've
> straightened that one out. Just a final quick note which by no means
> is targetting to resparkle the discussion: I have another bug report,
> hm, well :), but still, the machine is a 8-core AMD Dell box (with the
> option of upgrading to 16 cores) which _is_ using ide-cd so, there are
> some people using that code, after all :). I'm sure those people will
> be happier when it scales to that many cores.
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=11498.

Not to throw a monkey wrench, but I did add 'pushing data' to that IDE
statement. There is of course lots of systems still with a parallel
channel for things like atapi drives. But that is completely parallel to
the initial argument, those don't care a rats ass about scalability.
They care about it working, that is all.

-- 
Jens Axboe

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

[Jens Axboe]

On Sat, Oct 11 2008, Bartlomiej Zolnierkiewicz wrote:
> On Saturday 11 October 2008, Jens Axboe wrote:
> > On Sat, Oct 11 2008, Bartlomiej Zolnierkiewicz wrote:
> > > On Saturday 11 October 2008, Jens Axboe wrote:
> > > > On Sat, Oct 11 2008, Borislav Petkov wrote:
> > > > > > >From my perspective the main gain of these patches is the increased
> > > > > > maintainability and sanity of the code, scalability improvements are
> > > > > > just an added bonus.
> > > > > 
> > > > > and better code/improved scalability is a bad thing because... ?!
> > > > 
> > > > It's a bad thing because nobody on earth cares about IDE scalability,
> > > 
> > > JFYI: just yesterday I got mail proving otherwise. ;)
> > 
> > Well, there are lots of crazy people in the world, a request from
> > someone doesn't necessarily make it a good idea :-)
> > 
> > > > from a performance POV a modern SATA controller is just better on
> > > > several levels. I don't think anybody cares about IDE scaling on 8-16
> > > > cores or more, simply because NOBODY is using IDE on such systems.
> > > > 
> > > > As such, trying to improve locking is a pointless exercise. And that is
> > > > a bad thing, because code change invariably brings in code bugs. Then
> > > > see previous mail on lack of coverage testing, and it can naturally be
> > > > harmful.
> > > 
> > > Your concerns were already addressed in my reply but I worry that having
> > > a discussion based on technical arguments is not your goal.
> > 
> > You make it sound like I have an alterior motive, which I definitely do
> > not. I just wondered what all the IDE churn was supposed to be good
> > for...
> > 
> > > Just to repeat: these patches are not hardware specific and obviously
> > > they are not going to be merged today, tomorrow or in a week (they are
> > > 2.6.29 material after months of time in pata tree / linux-next).
> > 
> > It's less about this specific patchset than in general. The specific one
> > looked fine by itself, it's just the path to to 'IDE lock scaling' that
> > is a bit crazy to me. Moving IDE to the softirq completion like SCSI
> > would be a better start, imho. IDE still does most of the processing
> 
> We are getting there but this can't be done without fixing some other
> stuff (like the patch posted today to not process the next request from
> hard-IRQ context).  Any help would be much appreciated.
> 
> > under its ide_lock, which isn't even necessary. Making the locking more
> 
> Well, actually it doesn't do most work under ide_lock (never has been)
> as the main means of protection is hwgroup->busy (which is protected by
> ide_lock).

Yes it does, it does all of IO processing under the ide_lock, where you
only really need the lock for actually putting the last reference to the
request.

> [ OTOH some changes in this patchset were inspired by _your_ comments about
>   "room for locking improvements" in ide-io.c (IIRC) so kudos to you! ]

And that is indeed what that comment was about :-)
There's certainly a way to make that behave a lot more nicely without
splitting the lock. It's more about latency than lock contention.

> > finely grained is what I think is pretty crazy.
> 
> The more fine grained locking changes that were posted in separate patch
> were first done 3 years ago by Scalex86 guys and they were extensively
> tested on Intel hardware (however since other host drivers and things
> like IDE settings were abusing ide_lock applying this change back than
> was impossible - all of these stuff is fixed in the current Linus' tree).
> 
> Sorry for not explaining this clearly in the changelog.

Yeah I did get that reference, I am still questioning the point of
actually doing that.

> > > > > > > rather like putting makeup on a corpse to me..
> > > > > 
> > > > > so _NOT_ true.
> > > > 
> > > > Depends on what you think is the corpse. Since IDE is essentially dead
> > > > and frozen, it IS a corpse and the phrase is then very appropriate. This
> > > > is not a personal jab at the IDE guys and does not reflect on the
> > > > (mostly) good work they do, just a reflection on the state of IDE in
> > > > general.
> > > 
> > > Interesting statement given that i.e. diffstat-wise pata tree has more
> > > than twice as much stuff queued up for 2.6.28 than "some other" trees
> > > (and we have history of being a _very_ conservative w.r.t. to needlessly
> > > moving code around in drivers/ide/).
> > > 
> > > Please stop being silly and pushing your view/idea on what other people
> > > should be doing (not to mention ignoring real facts).
> > 
> > Please start by actually _reading_ what I write. Your reply is based on
> > the code base, my statement pertains to IDE on the hardware side
> > (devices, controllers, etc). On the scalability front, what new hardware
> > do you envision shipping with IDE controllers that are actually used for
> > pushing large amounts of data? People would have to be borderline insane
> > to make such new hw today.
> 
> Please understand that we are not doing new-hardware-driven-development
> here but rather a big maintainance project.  Like I said in reply to Robert
> the scalability improvements are an added bonus from my POV -> the main
> goal is making the code simpler, harder to abuse and easier to maintain.

I do understand that, as I've said all along I'm more concerned with
coverage of testing since a lot of the supported hardware (not just low
level drivers, things like ide-tape) just isn't used a whole lot these
days. Or the last 5 years, even.

> > I'm not pushing my views on anyone, but I am free to share what I
> > actually think. I actually care about old code stability, hence my
> > concern with the amount of IDE changes.
> 
> The actual users' feedback about amount of IDE changes have been mostly
> positive (some ask for even more changes :) so I don't think that it
> should be a reason of a great worries.  I hope that all other concerns
> have been also cleared now.

Heck that's good, I do hope that I'm mostly over-concerned! I'm not
trying to create a problem where there isn't one, mainly looking for
clarity into the situation.

I noticed one ide-tape tested complaining something broke, and it seems
like he was the only one out there actually using current kernels and
testing it. I worry mostly about the changes breaking somebodys distro 3
years down the line, by which point people may have moved on (and the
old code would have worked).

-- 
Jens Axboe

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

[Borislav Petkov]

> > The actual users' feedback about amount of IDE changes have been mostly
> > positive (some ask for even more changes :) so I don't think that it
> > should be a reason of a great worries.  I hope that all other concerns
> > have been also cleared now.
> 
> Heck that's good, I do hope that I'm mostly over-concerned! I'm not
> trying to create a problem where there isn't one, mainly looking for
> clarity into the situation.
> 
> I noticed one ide-tape tested complaining something broke, and it seems
> like he was the only one out there actually using current kernels and
> testing it. I worry mostly about the changes breaking somebodys distro 3
> years down the line, by which point people may have moved on (and the
> old code would have worked).

We try to fix those as fast as possible and they become
highest priority. If we're talking about the same thing, aka
801bd32e205ca6ef78dcaf80121f1eccb89b8c1e, this is obviously already fixed.

It's not like everything is fine with the older code either. For example, I'm
staring at traces w.r.t. bug 11581 which somehow broke ide-cd around the 2.6.19
timeframe and this has been broken for that particular user since then. So I
think that what we do now, generally that anyone is doing something about that
code, replying to bug reports and working with users to solve problems is, IMO,
a lot bigger advantage than doing nothing at all. But your concern is also valid
and we're doing our best to avoid such regressions.

-- 
Regards/Gruss,
    Boris.

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

[Bartlomiej Zolnierkiewicz]

On Saturday 11 October 2008, Jens Axboe wrote:
> On Sat, Oct 11 2008, Bartlomiej Zolnierkiewicz wrote:
> > On Saturday 11 October 2008, Jens Axboe wrote:
> > > On Sat, Oct 11 2008, Bartlomiej Zolnierkiewicz wrote:
> > > > On Saturday 11 October 2008, Jens Axboe wrote:
> > > > > On Sat, Oct 11 2008, Borislav Petkov wrote:
> > > > > > > >From my perspective the main gain of these patches is the increased
> > > > > > > maintainability and sanity of the code, scalability improvements are
> > > > > > > just an added bonus.
> > > > > > 
> > > > > > and better code/improved scalability is a bad thing because... ?!
> > > > > 
> > > > > It's a bad thing because nobody on earth cares about IDE scalability,
> > > > 
> > > > JFYI: just yesterday I got mail proving otherwise. ;)
> > > 
> > > Well, there are lots of crazy people in the world, a request from
> > > someone doesn't necessarily make it a good idea :-)
> > > 
> > > > > from a performance POV a modern SATA controller is just better on
> > > > > several levels. I don't think anybody cares about IDE scaling on 8-16
> > > > > cores or more, simply because NOBODY is using IDE on such systems.
> > > > > 
> > > > > As such, trying to improve locking is a pointless exercise. And that is
> > > > > a bad thing, because code change invariably brings in code bugs. Then
> > > > > see previous mail on lack of coverage testing, and it can naturally be
> > > > > harmful.
> > > > 
> > > > Your concerns were already addressed in my reply but I worry that having
> > > > a discussion based on technical arguments is not your goal.
> > > 
> > > You make it sound like I have an alterior motive, which I definitely do
> > > not. I just wondered what all the IDE churn was supposed to be good
> > > for...
> > > 
> > > > Just to repeat: these patches are not hardware specific and obviously
> > > > they are not going to be merged today, tomorrow or in a week (they are
> > > > 2.6.29 material after months of time in pata tree / linux-next).
> > > 
> > > It's less about this specific patchset than in general. The specific one
> > > looked fine by itself, it's just the path to to 'IDE lock scaling' that
> > > is a bit crazy to me. Moving IDE to the softirq completion like SCSI
> > > would be a better start, imho. IDE still does most of the processing
> > 
> > We are getting there but this can't be done without fixing some other
> > stuff (like the patch posted today to not process the next request from
> > hard-IRQ context).  Any help would be much appreciated.
> > 
> > > under its ide_lock, which isn't even necessary. Making the locking more
> > 
> > Well, actually it doesn't do most work under ide_lock (never has been)
> > as the main means of protection is hwgroup->busy (which is protected by
> > ide_lock).
> 
> Yes it does, it does all of IO processing under the ide_lock, where you
> only really need the lock for actually putting the last reference to the
> request.

When it comes to IO processing (block layer level not hardware one) you
are of course right.  I think that this patchset addresses most of it but
it would be great if you could double check and fix the places that I
missed.

> > [ OTOH some changes in this patchset were inspired by _your_ comments about
> >   "room for locking improvements" in ide-io.c (IIRC) so kudos to you! ]
> 
> And that is indeed what that comment was about :-)
> There's certainly a way to make that behave a lot more nicely without
> splitting the lock. It's more about latency than lock contention.
> 
> > > finely grained is what I think is pretty crazy.
> > 
> > The more fine grained locking changes that were posted in separate patch
> > were first done 3 years ago by Scalex86 guys and they were extensively
> > tested on Intel hardware (however since other host drivers and things
> > like IDE settings were abusing ide_lock applying this change back than
> > was impossible - all of these stuff is fixed in the current Linus' tree).
> > 
> > Sorry for not explaining this clearly in the changelog.
> 
> Yeah I did get that reference, I am still questioning the point of
> actually doing that.

The work has already been done and it is a wortwhile work.  The risk is
quite low (this is the statement based on rather deep understanding of
IDE subsystem, the complete audit of all code-paths affected and all the
testing experiences from Scalex86/me).

Moreover the patch won't be merged after few months of extra testing.

I feel that you still keep on questioning the point of improving IDE
and insist on putting it into "bug-fixes only" mode.  If this is really
the case I'm completely uninterested in discussing it any further.

> > > > > > > > rather like putting makeup on a corpse to me..
> > > > > > 
> > > > > > so _NOT_ true.
> > > > > 
> > > > > Depends on what you think is the corpse. Since IDE is essentially dead
> > > > > and frozen, it IS a corpse and the phrase is then very appropriate. This
> > > > > is not a personal jab at the IDE guys and does not reflect on the
> > > > > (mostly) good work they do, just a reflection on the state of IDE in
> > > > > general.
> > > > 
> > > > Interesting statement given that i.e. diffstat-wise pata tree has more
> > > > than twice as much stuff queued up for 2.6.28 than "some other" trees
> > > > (and we have history of being a _very_ conservative w.r.t. to needlessly
> > > > moving code around in drivers/ide/).
> > > > 
> > > > Please stop being silly and pushing your view/idea on what other people
> > > > should be doing (not to mention ignoring real facts).
> > > 
> > > Please start by actually _reading_ what I write. Your reply is based on
> > > the code base, my statement pertains to IDE on the hardware side
> > > (devices, controllers, etc). On the scalability front, what new hardware
> > > do you envision shipping with IDE controllers that are actually used for
> > > pushing large amounts of data? People would have to be borderline insane
> > > to make such new hw today.
> > 
> > Please understand that we are not doing new-hardware-driven-development
> > here but rather a big maintainance project.  Like I said in reply to Robert
> > the scalability improvements are an added bonus from my POV -> the main
> > goal is making the code simpler, harder to abuse and easier to maintain.
> 
> I do understand that, as I've said all along I'm more concerned with
> coverage of testing since a lot of the supported hardware (not just low
> level drivers, things like ide-tape) just isn't used a whole lot these
> days. Or the last 5 years, even.
> 
> > > I'm not pushing my views on anyone, but I am free to share what I
> > > actually think. I actually care about old code stability, hence my
> > > concern with the amount of IDE changes.
> > 
> > The actual users' feedback about amount of IDE changes have been mostly
> > positive (some ask for even more changes :) so I don't think that it
> > should be a reason of a great worries.  I hope that all other concerns
> > have been also cleared now.
> 
> Heck that's good, I do hope that I'm mostly over-concerned! I'm not
> trying to create a problem where there isn't one, mainly looking for
> clarity into the situation.
> 
> I noticed one ide-tape tested complaining something broke, and it seems
> like he was the only one out there actually using current kernels and

ide-tape is quite a special case since it is on life support since early
2.6.x days (when I ressurected it from somebody's broken bio changes) and
Borislav/me put quite a lot of work into keeping it alive despite having
real difficulties with finding people willing to test changes (fortunately
things seem to be improving here).

> testing it. I worry mostly about the changes breaking somebodys distro 3
> years down the line, by which point people may have moved on (and the
> old code would have worked).

I'm not quite sure I get it here.

Do you mean that we should be more worried about things like:

http://bugzilla.kernel.org/show_bug.cgi?id=11581

?

Thanks,
Bart

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

[Robert Hancock]

Bartlomiej Zolnierkiewicz wrote:
> The work has already been done and it is a wortwhile work.  The risk is
> quite low (this is the statement based on rather deep understanding of
> IDE subsystem, the complete audit of all code-paths affected and all the
> testing experiences from Scalex86/me).
> 
> Moreover the patch won't be merged after few months of extra testing.
> 
> I feel that you still keep on questioning the point of improving IDE
> and insist on putting it into "bug-fixes only" mode.  If this is really
> the case I'm completely uninterested in discussing it any further.

What, exactly, is the point of making more than bug-fix-only changes to 
the IDE code today, when we have libata around which is a much better 
code base to work from? I'm afraid it still escapes me. I don't mean to 
denigrate the work that you and other people working on IDE are doing, 
but can't help but think there would be more productive outlets for it..

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

[Bartlomiej Zolnierkiewicz]

On Sunday 12 October 2008, Robert Hancock wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > The work has already been done and it is a wortwhile work.  The risk is
> > quite low (this is the statement based on rather deep understanding of
> > IDE subsystem, the complete audit of all code-paths affected and all the
> > testing experiences from Scalex86/me).
> > 
> > Moreover the patch won't be merged after few months of extra testing.
> > 
> > I feel that you still keep on questioning the point of improving IDE
> > and insist on putting it into "bug-fixes only" mode.  If this is really
> > the case I'm completely uninterested in discussing it any further.
> 
> What, exactly, is the point of making more than bug-fix-only changes to 

Please stop this bug-fix-only nonsense already.

Take a look at the bug #11581.  I posted the link in my reply to Jens
because it is a best example that bug-fix-only mode won't really guarantee
a stable, bug-free code in the long-term.  Many of issues at such level
as driver subsystems happen because of "collateral damage" caused by
changes at the higher level.

[ The fact that #11581 was bisected to Jens' commit is just an additional
  spice.  It is likely that bisection went wrong but with git-bisect you
  are guilty-until-proven-innocent so Jens please (finally) help us with
  resolving it. ]

Additionally with open-source projects you have to keep a certain level
of developers' interests because otherwise everybody will be bored to
death and go away work on some other things (unless of course they are
paid to actually work on bugfixes).  Which in turn will result in less
people reviewing changes or doing bugfixes.

IOW in the long-term bug-fix-only code will result in less stable code.

> the IDE code today, when we have libata around which is a much better 
> code base to work from? I'm afraid it still escapes me. I don't mean to 

Simply:

* Not all hardware is supported by libata.

* Today's IDE code is not so different from libata's.

* I'm much more familiar with IDE's code than libata's. :)

> denigrate the work that you and other people working on IDE are doing, 
> but can't help but think there would be more productive outlets for it..

I don't really care.  I work on IDE because it is _fun_.

Thanks,
Bart