Force IDE cache flush on shutdown

Force IDE cache flush on shutdown

[Arjan van de Ven]

Hi,

Alan discovered the hard way that the current 2.6 IDE code doesn't do a
cache-flush on shutdown. The patch below forward ports this from 2.4. In
addition it fixes a bug where the ->wcache value only got determined for
removable disks not all disks. (that fix is from Alan, all other bugs are
mine ;)

Greetings,
   Arjan van de Ven

diff -urNp linux-1110/drivers/ide/ide-disk.c linux-1120/drivers/ide/ide-disk.c
--- linux-1110/drivers/ide/ide-disk.c
+++ linux-1120/drivers/ide/ide-disk.c
@@ -58,6 +58,8 @@
 #include <linux/genhd.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
+#include <linux/reboot.h>
+
 
 #define _IDE_DISK
 
@@ -1729,11 +1733,11 @@ static ide_driver_t idedisk_driver = {
 
 static int idedisk_open(struct inode *inode, struct file *filp)
 {
+	u8 cf;
 	ide_drive_t *drive = inode->i_bdev->bd_disk->private_data;
 	drive->usage++;
 	if (drive->removable && drive->usage == 1) {
 		ide_task_t args;
-		u8 cf;
 		memset(&args, 0, sizeof(ide_task_t));
 		args.tfRegister[IDE_COMMAND_OFFSET] = WIN_DOORLOCK;
 		args.command_type = IDE_DRIVE_TASK_NO_DATA;
@@ -1746,18 +1750,18 @@ static int idedisk_open(struct inode *in
 		 */
 		if (drive->doorlocking && ide_raw_taskfile(drive, &args, NULL))
 			drive->doorlocking = 0;
-		drive->wcache = 0;
-		/* Cache enabled ? */
-		if (drive->id->csfo & 1)
-		drive->wcache = 1;
-		/* Cache command set available ? */
-		if (drive->id->cfs_enable_1 & (1<<5))
-			drive->wcache = 1;
-		/* ATA6 cache extended commands */
-		cf = drive->id->command_set_2 >> 24;
-		if((cf & 0xC0) == 0x40 && (cf & 0x30) != 0)
-			drive->wcache = 1;
 	}
+	drive->wcache = 0;
+	/* Cache enabled ? */
+	if (drive->id->csfo & 1)
+		drive->wcache = 1;
+	/* Cache command set available ? */
+	if (drive->id->cfs_enable_1 & (1<<5))
+		drive->wcache = 1;
+	/* ATA6 cache extended commands */
+	cf = drive->id->command_set_2 >> 24;
+	if((cf & 0xC0) == 0x40 && (cf & 0x30) != 0)
+		drive->wcache = 1;
 	return 0;
 }
 
@@ -1779,6 +1783,7 @@ static int ide_cacheflush_p(ide_drive_t 
 static int idedisk_release(struct inode *inode, struct file *filp)
 {
 	ide_drive_t *drive = inode->i_bdev->bd_disk->private_data;
+	ide_cacheflush_p(drive);
 	if (drive->removable && drive->usage == 1) {
 		ide_task_t args;
 		memset(&args, 0, sizeof(ide_task_t));
@@ -1788,7 +1793,6 @@ static int idedisk_release(struct inode 
 		if (drive->doorlocking && ide_raw_taskfile(drive, &args, NULL))
 			drive->doorlocking = 0;
 	}
-	ide_cacheflush_p(drive);
 	drive->usage--;
 	return 0;
 }
@@ -1874,14 +1878,68 @@ failed:
 	return 1;
 }
 
+
+static int ide_disk_notify_reboot (struct notifier_block *this, unsigned long event, void *x)
+{
+	ide_hwif_t *hwif;
+	ide_drive_t *drive;
+	int i, unit;
+	
+	switch (event) {
+		case SYS_HALT:
+		case SYS_POWER_OFF:
+			break;
+		default:
+			return NOTIFY_DONE;
+	}
+	printk(KERN_INFO "flushing ide devices: ");
+	for (i = 0; i < MAX_HWIFS; i++) {
+		hwif = &ide_hwifs[i];
+		if (!hwif->present)
+			continue;
+		for (unit = 0; unit < MAX_DRIVES; ++unit) {
+			drive = &hwif->drives[unit];
+			if (!drive->present)
+				continue;
+			if (drive->media != ide_disk)
+				continue;
+                                                                                                                       
+			/* set the drive to standby */
+			printk("%s ", drive->name);
+
+			ide_cacheflush_p(drive);
+		}
+	}
+	printk("\n");
+	mdelay(1000);
+        return NOTIFY_DONE;
+
+}
+
+static struct notifier_block ide_notifier = {
+        ide_disk_notify_reboot,
+        NULL,
+        5
+};
+
+
+
 static void __exit idedisk_exit (void)
 {
+	unregister_reboot_notifier(&ide_notifier);
 	ide_unregister_driver(&idedisk_driver);
 }
 
 static int idedisk_init (void)
-{
-	return ide_register_driver(&idedisk_driver);
+{	
+	int i;
+	i = ide_register_driver(&idedisk_driver);
+
+	if (i) 
+		return i;
+
+	register_reboot_notifier(&ide_notifier);
+	return 0;
 }
 
 module_init(idedisk_init);

Re: Force IDE cache flush on shutdown

[Christoph Hellwig]

On Thu, May 06, 2004 at 09:04:49AM +0200, Arjan van de Ven wrote:
> +static int ide_disk_notify_reboot (struct notifier_block *this, unsigned long event, void *x)
> +{
> +	ide_hwif_t *hwif;
> +	ide_drive_t *drive;
> +	int i, unit;
> +	
> +	switch (event) {
> +		case SYS_HALT:
> +		case SYS_POWER_OFF:
> +			break;
> +		default:
> +			return NOTIFY_DONE;
> +	}

Please don't use reboot notifiers in new driver code.  The driver
model has a ->shutdown method for that.

Re: Force IDE cache flush on shutdown

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

On Thu, May 06, 2004 at 08:49:18AM +0100, Christoph Hellwig wrote:
> On Thu, May 06, 2004 at 09:04:49AM +0200, Arjan van de Ven wrote:
> > +static int ide_disk_notify_reboot (struct notifier_block *this, unsigned long event, void *x)
> > +{
> > +	ide_hwif_t *hwif;
> > +	ide_drive_t *drive;
> > +	int i, unit;
> > +	
> > +	switch (event) {
> > +		case SYS_HALT:
> > +		case SYS_POWER_OFF:
> > +			break;
> > +		default:
> > +			return NOTIFY_DONE;
> > +	}
> 
> Please don't use reboot notifiers in new driver code.  The driver
> model has a ->shutdown method for that.

there is somewhat of a problem with that; the reboot command potentially
runs from the disk in question, so that might never get called since that
will keep things busy.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Force IDE cache flush on shutdown

[Christoph Hellwig]

On Thu, May 06, 2004 at 09:50:44AM +0200, Arjan van de Ven wrote:
> > Please don't use reboot notifiers in new driver code.  The driver
> > model has a ->shutdown method for that.
> 
> there is somewhat of a problem with that; the reboot command potentially
> runs from the disk in question, so that might never get called since that
> will keep things busy.

shutdown != remove.  Shutdown is called for all devices on shutdown, remove
isn't called at all at reboot.

Re: Force IDE cache flush on shutdown

[Arjan van de Ven]

On Thu, May 06, 2004 at 08:55:49AM +0100, Christoph Hellwig wrote:
> On Thu, May 06, 2004 at 09:50:44AM +0200, Arjan van de Ven wrote:
> > > Please don't use reboot notifiers in new driver code.  The driver
> > > model has a ->shutdown method for that.
> > 
> > there is somewhat of a problem with that; the reboot command potentially
> > runs from the disk in question, so that might never get called since that
> > will keep things busy.
> 
> shutdown != remove.  Shutdown is called for all devices on shutdown, remove
> isn't called at all at reboot.

Ok the following does the trick for me. Better?

diff -purN linux-2.6.5/drivers/ide/ide-disk.c linux/drivers/ide/ide-disk.c
--- linux-2.6.5/drivers/ide/ide-disk.c	2004-05-06 10:56:55.672139712 +0200
+++ linux/drivers/ide/ide-disk.c	2004-05-06 11:09:19.470065240 +0200
@@ -1820,6 +1820,23 @@ static int idedisk_revalidate_disk(struc
 	return 0;
 }
 
+static int ide_drive_shutdown(struct device * dev)
+{
+	ide_drive_t * drive = container_of(dev,ide_drive_t,gendev);
+	
+	/* safety checks */
+	if (!drive->present)
+		return 0;
+	if (drive->media != ide_disk)
+		return 0;
+	printk("Flushing cache: %s \n", drive->name);
+	ide_cacheflush_p(drive);
+	/* give the hardware time to finish; it may return prematurely to cheat */
+	mdelay(300);
+	return 0;
+}
+                        
+
 static struct block_device_operations idedisk_ops = {
 	.owner		= THIS_MODULE,
 	.open		= idedisk_open,
@@ -1881,6 +1898,7 @@ static void __exit idedisk_exit (void)
 
 static int idedisk_init (void)
 {
+	idedisk_driver.gen_driver.shutdown = ide_drive_shutdown;
 	return ide_register_driver(&idedisk_driver);
 }
 

Re: Force IDE cache flush on shutdown

[Christoph Hellwig]

On Thu, May 06, 2004 at 12:46:38PM +0200, Arjan van de Ven wrote:
> Ok the following does the trick for me. Better?
> 
> diff -purN linux-2.6.5/drivers/ide/ide-disk.c linux/drivers/ide/ide-disk.c
> --- linux-2.6.5/drivers/ide/ide-disk.c	2004-05-06 10:56:55.672139712 +0200
> +++ linux/drivers/ide/ide-disk.c	2004-05-06 11:09:19.470065240 +0200
> @@ -1820,6 +1820,23 @@ static int idedisk_revalidate_disk(struc
>  	return 0;
>  }
>  
> +static int ide_drive_shutdown(struct device * dev)
> +{
> +	ide_drive_t * drive = container_of(dev,ide_drive_t,gendev);
> +	
> +	/* safety checks */
> +	if (!drive->present)
> +		return 0;
> +	if (drive->media != ide_disk)
> +		return 0;
> +	printk("Flushing cache: %s \n", drive->name);
> +	ide_cacheflush_p(drive);
> +	/* give the hardware time to finish; it may return prematurely to cheat */
> +	mdelay(300);
> +	return 0;
> +}

Looks good to mz limited understanding of the ide code :)

>  static struct block_device_operations idedisk_ops = {
>  	.owner		= THIS_MODULE,
>  	.open		= idedisk_open,
> @@ -1881,6 +1898,7 @@ static void __exit idedisk_exit (void)
>  
>  static int idedisk_init (void)
>  {
> +	idedisk_driver.gen_driver.shutdown = ide_drive_shutdown;

isn't idedisk_driver initialized statically somewhere?  You should probably
add

	.gen_driver = {
		.shutdown	= ide_drive_shutdown,
	},

to that initializer instead.

Re: Force IDE cache flush on shutdown

[Arjan van de Ven]

On Thu, May 06, 2004 at 11:52:20AM +0100, Christoph Hellwig wrote:
> > +	idedisk_driver.gen_driver.shutdown = ide_drive_shutdown;
> 
> isn't idedisk_driver initialized statically somewhere?  You should probably

ok ok you win

diff -purN linux-2.6.5/drivers/ide/ide-disk.c linux/drivers/ide/ide-disk.c
--- linux-2.6.5/drivers/ide/ide-disk.c	2004-05-06 13:26:53.350284720 +0200
+++ linux/drivers/ide/ide-disk.c	2004-05-06 13:32:01.322465832 +0200
@@ -1725,6 +1725,9 @@ static ide_driver_t idedisk_driver = {
 	.drives			= LIST_HEAD_INIT(idedisk_driver.drives),
 	.start_power_step	= idedisk_start_power_step,
 	.complete_power_step	= idedisk_complete_power_step,
+	.gen_driver = {
+		.shutdown	= ide_drive_shutdown,
+	},	                                
 };
 
@@ -1820,6 +1823,23 @@ static int idedisk_revalidate_disk(struc
 	return 0;
 }
 
+static int ide_drive_shutdown(struct device * dev)
+{
+	ide_drive_t * drive = container_of(dev,ide_drive_t,gendev);
+	
+	/* safety checks */
+	if (!drive->present)
+		return 0;
+	if (drive->media != ide_disk)
+		return 0;
+	printk("Flushing cache: %s \n", drive->name);
+	ide_cacheflush_p(drive);
+	/* give the hardware time to finish; it may return prematurely to cheat */
+	mdelay(300);
+	return 0;
+}
+                        
+
 static struct block_device_operations idedisk_ops = {
 	.owner		= THIS_MODULE,
 	.open		= idedisk_open,

Re: Force IDE cache flush on shutdown

[Nigel Cunningham]

Hi.

If you want it tested on some more systems, send it to the suspend list
(swsusp-devel at lists dot sourceforge dot net). There are a bunch of
people there who'd love to give it a go and suspend2 will make any
issues show up quickly. (At the moment, it calls
drivers_suspend() prior to the power off call to force flushing).

Regards,

Nigel
-- 
Nigel & Michelle Cunningham
C/- Westminster Presbyterian Church Belconnen
61 Templeton Street, Cook, ACT 2614.
+61 (2) 6251 7727(wk); +61 (2) 6254 0216 (home)

Evolution (n): A hypothetical process whereby infinitely improbable events occur 
with alarming frequency, order arises from chaos, and no one is given credit.

Re: Force IDE cache flush on shutdown

[Arjan van de Ven]

On Thu, May 06, 2004 at 01:33:09PM +0200, Arjan van de Ven wrote:
> On Thu, May 06, 2004 at 11:52:20AM +0100, Christoph Hellwig wrote:
> > > +	idedisk_driver.gen_driver.shutdown = ide_drive_shutdown;
> > 
> > isn't idedisk_driver initialized statically somewhere?  You should probably
> 
> ok ok you win
> 
> diff -purN linux-2.6.5/drivers/ide/ide-disk.c linux/drivers/ide/ide-disk.c
> --- linux-2.6.5/drivers/ide/ide-disk.c	2004-05-06 13:26:53.350284720 +0200
> +++ linux/drivers/ide/ide-disk.c	2004-05-06 13:32:01.322465832 +0200
> @@ -1725,6 +1725,9 @@ static ide_driver_t idedisk_driver = {
>  	.drives			= LIST_HEAD_INIT(idedisk_driver.drives),
>  	.start_power_step	= idedisk_start_power_step,
>  	.complete_power_step	= idedisk_complete_power_step,
> +	.gen_driver = {
> +		.shutdown	= ide_drive_shutdown,
> +	},	                                


and that needs a prototype as well 

--- linux-2.6.5/drivers/ide/ide-disk.c~	2004-05-06 13:44:41.052969240 +0200
+++ linux-2.6.5/drivers/ide/ide-disk.c	2004-05-06 13:44:41.053969088 +0200
@@ -1701,6 +1701,7 @@
 }
 
 static int idedisk_attach(ide_drive_t *drive);
+static int ide_drive_shutdown(struct device * dev);
 
 /*
  *      IDE subdriver functions, registered with ide.c

Re: Force IDE cache flush on shutdown

[Bartlomiej Zolnierkiewicz]

On Thursday 06 of May 2004 13:33, Arjan van de Ven wrote:
> On Thu, May 06, 2004 at 11:52:20AM +0100, Christoph Hellwig wrote:
> > > +	idedisk_driver.gen_driver.shutdown = ide_drive_shutdown;
> >
> > isn't idedisk_driver initialized statically somewhere?  You should
> > probably
>
> ok ok you win
>
> diff -purN linux-2.6.5/drivers/ide/ide-disk.c linux/drivers/ide/ide-disk.c
> --- linux-2.6.5/drivers/ide/ide-disk.c	2004-05-06 13:26:53.350284720 +0200
> +++ linux/drivers/ide/ide-disk.c	2004-05-06 13:32:01.322465832 +0200
> @@ -1725,6 +1725,9 @@ static ide_driver_t idedisk_driver = {
>  	.drives			= LIST_HEAD_INIT(idedisk_driver.drives),
>  	.start_power_step	= idedisk_start_power_step,
>  	.complete_power_step	= idedisk_complete_power_step,
> +	.gen_driver = {
> +		.shutdown	= ide_drive_shutdown,
> +	},
>  };
>
> @@ -1820,6 +1823,23 @@ static int idedisk_revalidate_disk(struc
>  	return 0;
>  }
>
> +static int ide_drive_shutdown(struct device * dev)
> +{
> +	ide_drive_t * drive = container_of(dev,ide_drive_t,gendev);
> +
> +	/* safety checks */
> +	if (!drive->present)
> +		return 0;

not needed / BUG_ON

> +	if (drive->media != ide_disk)
> +		return 0;

not needed / BUG_ON

> +	printk("Flushing cache: %s \n", drive->name);
> +	ide_cacheflush_p(drive);
> +	/* give the hardware time to finish; it may return prematurely to cheat
> */ +	mdelay(300);

I really don't like it.

Is this delay arbitrary or you know about such devices?

> +	return 0;
> +}
> +
> +
>  static struct block_device_operations idedisk_ops = {
>  	.owner		= THIS_MODULE,
>  	.open		= idedisk_open,

Re: Force IDE cache flush on shutdown

[Benjamin Herrenschmidt]

> > +	printk("Flushing cache: %s \n", drive->name);
> > +	ide_cacheflush_p(drive);
> > +	/* give the hardware time to finish; it may return prematurely to cheat
> > */ +	mdelay(300);
> 
> I really don't like it.
> 
> Is this delay arbitrary or you know about such devices?

Agreed... Why not standby the disk instead ? We could re-use the exact
same code path as the Power Management... At least we know that once
standby complete, we are ok (though I noticed that some proprietary
OSes still impose a half a second delay after standby and before
removing power ... broken IDE disks ...)

In fact, I'd like to kill the shutdown() callback in drivers in general
as I think it's just a special case of a PM suspend in fact. The problem
is we need once for all to fix the meaning of the "state" passed to those
callbacks. Once that's done, we can have states for restart, shutdown and
kexec (which wants devices to be put idle -> stop DMA etc...)

Ben.

Re: Force IDE cache flush on shutdown

[Bartlomiej Zolnierkiewicz]

On Thursday 06 of May 2004 09:49, Christoph Hellwig wrote:
> On Thu, May 06, 2004 at 09:04:49AM +0200, Arjan van de Ven wrote:
> > +static int ide_disk_notify_reboot (struct notifier_block *this, unsigned
> > long event, void *x) +{
> > +	ide_hwif_t *hwif;
> > +	ide_drive_t *drive;
> > +	int i, unit;
> > +
> > +	switch (event) {
> > +		case SYS_HALT:
> > +		case SYS_POWER_OFF:
> > +			break;
> > +		default:
> > +			return NOTIFY_DONE;
> > +	}
>
> Please don't use reboot notifiers in new driver code.  The driver
> model has a ->shutdown method for that.

There is one problem with using ->shutdown:
handling of SYS_RESTART isn't the same as of SYS_HALT and SYS_POWER_OFF.

Re: Force IDE cache flush on shutdown

[Benjamin Herrenschmidt]

> 
> There is one problem with using ->shutdown:
> handling of SYS_RESTART isn't the same as of SYS_HALT and SYS_POWER_OFF.

See my other comment... I think this duplicates what the suspend 
callback does (though fixing that might not be a 2.6 option).

Ben.

Re: Force IDE cache flush on shutdown

[Pavel Machek]

Hi!

> > +static int ide_disk_notify_reboot (struct notifier_block *this, unsigned long event, void *x)
> > +{
> > +	ide_hwif_t *hwif;
> > +	ide_drive_t *drive;
> > +	int i, unit;
> > +	
> > +	switch (event) {
> > +		case SYS_HALT:
> > +		case SYS_POWER_OFF:
> > +			break;
> > +		default:
> > +			return NOTIFY_DONE;
> > +	}
> 
> Please don't use reboot notifiers in new driver code.  The driver
> model has a ->shutdown method for that.

Also you want to run this on machine entering S3 or suspend-to-disk,
too...

								Pavel
-- 
When do you have heart between your knees?

Re: Force IDE cache flush on shutdown

[Jens Axboe]

On Thu, May 06 2004, Arjan van de Ven wrote:
> Hi,
> 
> Alan discovered the hard way that the current 2.6 IDE code doesn't do a
> cache-flush on shutdown. The patch below forward ports this from 2.4. In
> addition it fixes a bug where the ->wcache value only got determined for
> removable disks not all disks. (that fix is from Alan, all other bugs are
> mine ;)

Yeah that's a dumb bug, I fixed that in the barrier patches as well (but
forgot to send it in).

Maybe you could send that in seperately first, it needs to go in
regardless.

-- 
Jens Axboe

Re: Force IDE cache flush on shutdown

[Arjan van de Ven]

On Thu, May 06, 2004 at 09:58:10AM +0200, Jens Axboe wrote:> On Thu, May 06 2004, Arjan van de Ven wrote:
> > Hi,
> > 
> > Alan discovered the hard way that the current 2.6 IDE code doesn't do a
> > cache-flush on shutdown. The patch below forward ports this from 2.4. In
> > addition it fixes a bug where the ->wcache value only got determined for
> > removable disks not all disks. (that fix is from Alan, all other bugs are
> > mine ;)
> 
> Yeah that's a dumb bug, I fixed that in the barrier patches as well (but
> forgot to send it in).
> 
> Maybe you could send that in seperately first, it needs to go in
> regardless.


ok below are the uncontended bits
1) calculate wcache for non-removable disks too
2) flush the cache BEFORE unlocking the door on removable media,
   otherwise you have a small race with the human..

it makes sense for these to go in separate; I'm working on the shutdown hook
now (and testing which is the fun part ;)

diff -urNp linux-1110/drivers/ide/ide-disk.c linux-1120/drivers/ide/ide-disk.c
--- linux-1110/drivers/ide/ide-disk.c
+++ linux-1120/drivers/ide/ide-disk.c
@@ -1729,11 +1733,11 @@ static ide_driver_t idedisk_driver = {
 
 static int idedisk_open(struct inode *inode, struct file *filp)
 {
+	u8 cf;
 	ide_drive_t *drive = inode->i_bdev->bd_disk->private_data;
 	drive->usage++;
 	if (drive->removable && drive->usage == 1) {
 		ide_task_t args;
-		u8 cf;
 		memset(&args, 0, sizeof(ide_task_t));
 		args.tfRegister[IDE_COMMAND_OFFSET] = WIN_DOORLOCK;
 		args.command_type = IDE_DRIVE_TASK_NO_DATA;
@@ -1746,18 +1750,18 @@ static int idedisk_open(struct inode *in
 		 */
 		if (drive->doorlocking && ide_raw_taskfile(drive, &args, NULL))
 			drive->doorlocking = 0;
-		drive->wcache = 0;
-		/* Cache enabled ? */
-		if (drive->id->csfo & 1)
-		drive->wcache = 1;
-		/* Cache command set available ? */
-		if (drive->id->cfs_enable_1 & (1<<5))
-			drive->wcache = 1;
-		/* ATA6 cache extended commands */
-		cf = drive->id->command_set_2 >> 24;
-		if((cf & 0xC0) == 0x40 && (cf & 0x30) != 0)
-			drive->wcache = 1;
 	}
+	drive->wcache = 0;
+	/* Cache enabled ? */
+	if (drive->id->csfo & 1)
+		drive->wcache = 1;
+	/* Cache command set available ? */
+	if (drive->id->cfs_enable_1 & (1<<5))
+		drive->wcache = 1;
+	/* ATA6 cache extended commands */
+	cf = drive->id->command_set_2 >> 24;
+	if((cf & 0xC0) == 0x40 && (cf & 0x30) != 0)
+		drive->wcache = 1;
 	return 0;
 }
 
@@ -1779,6 +1783,7 @@ static int ide_cacheflush_p(ide_drive_t 
 static int idedisk_release(struct inode *inode, struct file *filp)
 {
 	ide_drive_t *drive = inode->i_bdev->bd_disk->private_data;
+	ide_cacheflush_p(drive);
 	if (drive->removable && drive->usage == 1) {
 		ide_task_t args;
 		memset(&args, 0, sizeof(ide_task_t));
@@ -1788,7 +1793,6 @@ static int idedisk_release(struct inode 
 		if (drive->doorlocking && ide_raw_taskfile(drive, &args, NULL))
 			drive->doorlocking = 0;
 	}
-	ide_cacheflush_p(drive);
 	drive->usage--;
 	return 0;
 }

Re: Force IDE cache flush on shutdown

[Jens Axboe]

On Thu, May 06 2004, Arjan van de Ven wrote:
> 
> On Thu, May 06, 2004 at 09:58:10AM +0200, Jens Axboe wrote:> On Thu, May 06 2004, Arjan van de Ven wrote:
> > > Hi,
> > > 
> > > Alan discovered the hard way that the current 2.6 IDE code doesn't do a
> > > cache-flush on shutdown. The patch below forward ports this from 2.4. In
> > > addition it fixes a bug where the ->wcache value only got determined for
> > > removable disks not all disks. (that fix is from Alan, all other bugs are
> > > mine ;)
> > 
> > Yeah that's a dumb bug, I fixed that in the barrier patches as well (but
> > forgot to send it in).
> > 
> > Maybe you could send that in seperately first, it needs to go in
> > regardless.
> 
> 
> ok below are the uncontended bits
> 1) calculate wcache for non-removable disks too
> 2) flush the cache BEFORE unlocking the door on removable media,
>    otherwise you have a small race with the human..
> 
> it makes sense for these to go in separate; I'm working on the shutdown hook
> now (and testing which is the fun part ;)
> 
> diff -urNp linux-1110/drivers/ide/ide-disk.c linux-1120/drivers/ide/ide-disk.c
> --- linux-1110/drivers/ide/ide-disk.c
> +++ linux-1120/drivers/ide/ide-disk.c
> @@ -1729,11 +1733,11 @@ static ide_driver_t idedisk_driver = {
>  
>  static int idedisk_open(struct inode *inode, struct file *filp)
>  {
> +	u8 cf;
>  	ide_drive_t *drive = inode->i_bdev->bd_disk->private_data;
>  	drive->usage++;
>  	if (drive->removable && drive->usage == 1) {
>  		ide_task_t args;
> -		u8 cf;
>  		memset(&args, 0, sizeof(ide_task_t));
>  		args.tfRegister[IDE_COMMAND_OFFSET] = WIN_DOORLOCK;
>  		args.command_type = IDE_DRIVE_TASK_NO_DATA;
> @@ -1746,18 +1750,18 @@ static int idedisk_open(struct inode *in
>  		 */
>  		if (drive->doorlocking && ide_raw_taskfile(drive, &args, NULL))
>  			drive->doorlocking = 0;
> -		drive->wcache = 0;
> -		/* Cache enabled ? */
> -		if (drive->id->csfo & 1)
> -		drive->wcache = 1;
> -		/* Cache command set available ? */
> -		if (drive->id->cfs_enable_1 & (1<<5))
> -			drive->wcache = 1;
> -		/* ATA6 cache extended commands */
> -		cf = drive->id->command_set_2 >> 24;
> -		if((cf & 0xC0) == 0x40 && (cf & 0x30) != 0)
> -			drive->wcache = 1;
>  	}
> +	drive->wcache = 0;
> +	/* Cache enabled ? */
> +	if (drive->id->csfo & 1)
> +		drive->wcache = 1;
> +	/* Cache command set available ? */
> +	if (drive->id->cfs_enable_1 & (1<<5))
> +		drive->wcache = 1;
> +	/* ATA6 cache extended commands */
> +	cf = drive->id->command_set_2 >> 24;
> +	if((cf & 0xC0) == 0x40 && (cf & 0x30) != 0)
> +		drive->wcache = 1;
>  	return 0;
>  }
>  
> @@ -1779,6 +1783,7 @@ static int ide_cacheflush_p(ide_drive_t 
>  static int idedisk_release(struct inode *inode, struct file *filp)
>  {
>  	ide_drive_t *drive = inode->i_bdev->bd_disk->private_data;
> +	ide_cacheflush_p(drive);
>  	if (drive->removable && drive->usage == 1) {
>  		ide_task_t args;
>  		memset(&args, 0, sizeof(ide_task_t));
> @@ -1788,7 +1793,6 @@ static int idedisk_release(struct inode 
>  		if (drive->doorlocking && ide_raw_taskfile(drive, &args, NULL))
>  			drive->doorlocking = 0;
>  	}
> -	ide_cacheflush_p(drive);
>  	drive->usage--;
>  	return 0;
>  }

Looks good to me.

-- 
Jens Axboe