PATCH: ide: ide-disk freeze support for hdaps

PATCH: ide: ide-disk freeze support for hdaps

[Yani Ioannou]

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

Hi all,

Attached below is a patch heavily based on Jon Escombe's patch, but
implemented as a sysfs attribute as Jens described, with a timeout
(configurable by module/kernel parameter) to ensure the queue isn't
stopped forever.

The driver creates a sysfs attribute "/sys/block/hdX/device/freeze",
which when set (echo 1 >
/sys/block/hda/device/freeze/sys/block/hda/device/freeze) freezes the
queue and parks the head on the drive, protecting the drive
(theoretically) from severe damage, until the drive is thawed (echo 0
> /sys/block/hda/device/freeze) or a timeout is reached (default 10s).

The patch is meant for usage with the hdaps (hard drive active
protection system) for Thinkpads, however would also be useful for
similair systems implemented on powerbooks, and other laptops out
there.

I've tested it out on my T42p and it seems to work well, but as Jon
warns this is experimental...

Thanks,
Yani

Signed-off-by: Yani Ioannou <yani.ioannou@gmail.com>

[-- Attachment #2: patch-linux-2.6.13-rc7-idefreeze.diff --]
[-- Type: text/plain, Size: 7144 bytes --]


---

 drivers/ide/Kconfig    |   18 +++++
 drivers/ide/ide-disk.c |  163 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/ide/ide-io.c   |   13 ++++
 3 files changed, 194 insertions(+), 0 deletions(-)

diff --git a/drivers/ide/Kconfig b/drivers/ide/Kconfig
--- a/drivers/ide/Kconfig
+++ b/drivers/ide/Kconfig
@@ -148,6 +148,24 @@ config BLK_DEV_IDEDISK
 
 	  If unsure, say Y.
 
+if BLK_DEV_IDEDISK
+
+config IDEDISK_FREEZE
+	bool "Enable IDE DISK Freeze support"
+	depends on EXPERIMENTAL 
+	help
+	  This will include support for freezing an IDE drive (parking the 
+	  head, freezing the queue), allowing hard drive protection drivers
+	  to park the head quickly and keep it parked. If you don't use such
+	  a driver, you can say N here.
+
+	  If you say yes here a sysfs attribute "/sys/block/hda/device/freeze"
+	  will be created which can be used to freeze/thaw the drive.
+	  
+	  If in doubt, say N.
+
+endif
+
 config IDEDISK_MULTI_MODE
 	bool "Use multi-mode by default"
 	help
diff --git a/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
--- a/drivers/ide/ide-disk.c
+++ b/drivers/ide/ide-disk.c
@@ -48,6 +48,9 @@
 //#define DEBUG
 
 #include <linux/config.h>
+#ifdef CONFIG_IDEDISK_FREEZE
+#include <linux/device.h>
+#endif /* CONFIG_IDEDISK_FREEZE */
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/string.h>
@@ -97,6 +100,158 @@ static struct ide_disk_obj *ide_disk_get
 	return idkp;
 }
 
+#ifdef CONFIG_IDEDISK_FREEZE
+/* IDE Freeze support - park the head and stop the queue, 
+ * 			for hard drive protection systems.
+ * Jon Escombe
+ * Yani Ioannou <yani.ioannou@gmail.com> 
+ */
+/* (spins down drive - more obvious for testing) */
+#define STANDBY_IMMEDIATE_ARGS {\
+	0xe0, 			\
+	0x44, 			\
+	0x00, 			\
+	0x4c, 			\
+	0x4e, 			\
+	0x55, 			\
+	0x00, 			\
+}
+
+#define UNLOAD_IMMEDIATE_ARGS {\
+	0xe1, 			\
+	0x44, 			\
+	0x00, 			\
+	0x4c, 			\
+	0x4e, 			\
+	0x55, 			\
+	0x00, 			\
+}
+
+#define IDE_FREEZE_TIMEOUT_SEC 10
+static int ide_freeze_timeout = IDE_FREEZE_TIMEOUT_SEC;
+module_param(ide_freeze_timeout, int, 0);
+MODULE_PARM_DESC(ide_freeze_timeout, 
+		"IDE Freeze timeout in seconds. (0<ide_freeze_timeout<65536, default=" 
+		__MODULE_STRING(IDE_FREEZE_TIMEOUT_SEC) ")");
+static void freeze_expire(unsigned long data);
+static struct timer_list freeze_timer = 
+	TIMER_INITIALIZER(freeze_expire, 0, 0);
+
+static ssize_t ide_freeze_show(struct device *dev, 
+		struct device_attribute *attr, char *buf){
+	ide_drive_t *drive = to_ide_device(dev);
+	int queue_stopped = 
+		test_bit(QUEUE_FLAG_STOPPED, &drive->queue->queue_flags);
+	
+	return snprintf(buf, 10, "%u\n", queue_stopped);
+}
+
+void ide_end_freeze_rq(struct request *rq)
+{
+	struct completion *waiting = rq->waiting;
+	u8 *argbuf = rq->buffer;
+
+	/* Spinlock is already acquired */
+	if (argbuf[3] == 0xc4) {
+		blk_stop_queue(rq->q);
+		printk(KERN_DEBUG "ide_end_freeze_rq(): Head parked\n");
+	}
+	else
+		printk(KERN_DEBUG "ide_end_freeze_rq(): Head not parked\n");
+	
+	complete(waiting);
+}
+
+static int ide_freeze_drive(ide_drive_t *drive)
+{
+	DECLARE_COMPLETION(wait);
+	struct request rq;
+	int error = 0;
+	
+	/* STANDBY IMMEDIATE COMMAND (testing) */
+	/* u8 argbuf[] = STANDBY_IMMEDIATE_ARGS; */
+
+	/* UNLOAD IMMEDIATE COMMAND */
+	u8 argbuf[] = UNLOAD_IMMEDIATE_ARGS;
+
+	/* Check we're not already frozen */
+	if(blk_queue_stopped(drive->queue)){
+		printk(KERN_DEBUG "ide_freeze_drive: Queue already stopped\n");
+		return error;
+	}
+	
+	memset(&rq, 0, sizeof(rq));
+	
+	ide_init_drive_cmd(&rq);
+
+	rq.flags = REQ_DRIVE_TASK;
+	rq.buffer = argbuf;
+	rq.waiting = &wait;
+	rq.end_io = ide_end_freeze_rq;
+
+	error = ide_do_drive_cmd(drive, &rq, ide_next);
+	wait_for_completion(&wait);
+	rq.waiting = NULL;
+	
+	if(error)
+		printk(KERN_ERR "ide_freeze_drive: Error sending command to drive %s: %d\n",
+				drive->name, error);
+	else{
+		printk(KERN_DEBUG "ide_freeze_drive: Queue stopped\n");
+		
+		freeze_timer.data = (unsigned long) drive;
+		mod_timer(&freeze_timer, jiffies+(ide_freeze_timeout*HZ));
+		printk(KERN_DEBUG "ide_freeze_drive: Timer (re)initialized to %ds timeout\n", 
+				ide_freeze_timeout);
+	}
+	return error;
+}
+
+static void ide_thaw_drive(ide_drive_t *drive)
+{
+	unsigned long flags;
+	if (!blk_queue_stopped(drive->queue))
+	{
+		printk(KERN_DEBUG "ide_thaw_drive(): Queue not stopped\n");
+		return;
+	}
+	
+	spin_lock_irqsave(&ide_lock, flags);
+	blk_start_queue(drive->queue);
+	spin_unlock_irqrestore(&ide_lock, flags);
+	printk(KERN_DEBUG "ide_thaw_drive(): Queue started\n");
+	del_timer(&freeze_timer);
+	printk(KERN_DEBUG "ide_thaw_drive: Timer stopped\n");
+}
+
+static void freeze_expire(unsigned long data)
+{
+	ide_drive_t *drive = (ide_drive_t *) data;
+	printk(KERN_DEBUG "freeze_expire(): Freeze timer timeout, thawing...\n");
+	ide_thaw_drive(drive);
+}
+	
+static ssize_t ide_freeze_store(struct device *dev, 
+		struct device_attribute *attr, const char *buf, size_t count){
+	ide_drive_t *drive = to_ide_device(dev);
+	int freeze = 0;
+	int error = 0;
+	
+	freeze = simple_strtoul(buf, NULL, 10);
+	
+	if(freeze)
+		error = ide_freeze_drive(drive);
+	else
+		ide_thaw_drive(drive);
+	
+	return error ? error : count;
+}
+
+static DEVICE_ATTR(freeze, S_IRUSR | S_IWUSR, ide_freeze_show, 
+		ide_freeze_store);
+
+#endif /* CONFIG_IDEDISK_FREEZE */
+
 static void ide_disk_release(struct kref *);
 
 static void ide_disk_put(struct ide_disk_obj *idkp)
@@ -1034,6 +1189,10 @@ static int ide_disk_remove(struct device
 	struct ide_disk_obj *idkp = drive->driver_data;
 	struct gendisk *g = idkp->disk;
 
+	#ifdef CONFIG_IDEDISK_FREEZE
+	device_remove_file(dev, &dev_attr_freeze);
+	#endif /* CONFIG_IDEDISK_FREEZE */
+	
 	ide_cacheflush_p(drive);
 
 	ide_unregister_subdriver(drive, idkp->driver);
@@ -1248,6 +1407,10 @@ static int ide_disk_probe(struct device 
 	} else
 		drive->attach = 1;
 
+	#ifdef CONFIG_IDEDISK_FREEZE
+	device_create_file(dev, &dev_attr_freeze);
+	#endif /* CONFIG_IDEDISK_FREEZE */
+	
 	g->minors = 1 << PARTN_BITS;
 	strcpy(g->devfs_name, drive->devfs_name);
 	g->driverfs_dev = &drive->gendev;
diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -1181,6 +1181,16 @@ static void ide_do_request (ide_hwgroup_
 		}
 
 		/*
+		 * Don't accept a request when the queue is stopped
+		 * (unless we are resuming from suspend)
+		 */
+		if (test_bit(QUEUE_FLAG_STOPPED, &drive->queue->queue_flags) && !blk_pm_resume_request(rq)) {
+			printk(KERN_ERR "%s: queue is stopped!\n", drive->name);
+			hwgroup->busy = 0;
+			break;
+		}
+
+		/*
 		 * Sanity: don't accept a request that isn't a PM request
 		 * if we are currently power managed. This is very important as
 		 * blk_stop_queue() doesn't prevent the elv_next_request()
@@ -1661,6 +1671,9 @@ int ide_do_drive_cmd (ide_drive_t *drive
 		where = ELEVATOR_INSERT_FRONT;
 		rq->flags |= REQ_PREEMPT;
 	}
+	if (action == ide_next)
+		where = ELEVATOR_INSERT_FRONT;
+
 	__elv_add_request(drive->queue, rq, where, 0);
 	ide_do_request(hwgroup, IDE_NO_IRQ);
 	spin_unlock_irqrestore(&ide_lock, flags);

Re: PATCH: ide: ide-disk freeze support for hdaps

[Bartlomiej Zolnierkiewicz]

Hi,

On 8/25/05, Yani Ioannou <yani.ioannou@gmail.com> wrote:
> Hi all,
> 
> Attached below is a patch heavily based on Jon Escombe's patch, but
> implemented as a sysfs attribute as Jens described, with a timeout
> (configurable by module/kernel parameter) to ensure the queue isn't
> stopped forever.
> 
> The driver creates a sysfs attribute "/sys/block/hdX/device/freeze",
> which when set (echo 1 >
> /sys/block/hda/device/freeze/sys/block/hda/device/freeze) freezes the
> queue and parks the head on the drive, protecting the drive
> (theoretically) from severe damage, until the drive is thawed (echo 0
> > /sys/block/hda/device/freeze) or a timeout is reached (default 10s).
> 
> The patch is meant for usage with the hdaps (hard drive active
> protection system) for Thinkpads, however would also be useful for
> similair systems implemented on powerbooks, and other laptops out
> there.
> 
> I've tested it out on my T42p and it seems to work well, but as Jon
> warns this is experimental...
> 
> Thanks,
> Yani
> 
> Signed-off-by: Yani Ioannou <yani.ioannou@gmail.com>

Few comments about the patch below:

diff --git a/drivers/ide/Kconfig b/drivers/ide/Kconfig
--- a/drivers/ide/Kconfig
+++ b/drivers/ide/Kconfig
@@ -148,6 +148,24 @@ config BLK_DEV_IDEDISK
 
 	  If unsure, say Y.
 
+if BLK_DEV_IDEDISK
+
+config IDEDISK_FREEZE

Is there any advantage of having it as a config option?

+	bool "Enable IDE DISK Freeze support"
+	depends on EXPERIMENTAL 
+	help
+	  This will include support for freezing an IDE drive (parking the 
+	  head, freezing the queue), allowing hard drive protection drivers
+	  to park the head quickly and keep it parked. If you don't use such
+	  a driver, you can say N here.
+
+	  If you say yes here a sysfs attribute "/sys/block/hda/device/freeze"
+	  will be created which can be used to freeze/thaw the drive.
+	  
+	  If in doubt, say N.
+
+endif
+
  config IDEDISK_MULTI_MODE
  	bool "Use multi-mode by default"
 	help
diff --git a/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
--- a/drivers/ide/ide-disk.c
+++ b/drivers/ide/ide-disk.c
@@ -48,6 +48,9 @@
 //#define DEBUG
 
  #include <linux/config.h>
+#ifdef CONFIG_IDEDISK_FREEZE
+#include <linux/device.h>
+#endif /* CONFIG_IDEDISK_FREEZE */

This shouldn't be needed, 
<linux/device.h> is already included by <linux/ide.h>.

 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/string.h>
@@ -97,6 +100,158 @@ static struct ide_disk_obj *ide_disk_get
  	return idkp;
 }
 
+#ifdef CONFIG_IDEDISK_FREEZE
+/* IDE Freeze support - park the head and stop the queue, 
+ * 			for hard drive protection systems.
+ * Jon Escombe
+ * Yani Ioannou <yani.ioannou@gmail.com> 
+ */
+/* (spins down drive - more obvious for testing) */
+#define STANDBY_IMMEDIATE_ARGS {\
+	0xe0, 			\
+	0x44, 			\
+	0x00, 			\
+	0x4c, 			\
+	0x4e, 			\
+	0x55, 			\
+	0x00, 			\
+}
+
+#define UNLOAD_IMMEDIATE_ARGS {\
+	0xe1, 			\
+	0x44, 			\
+	0x00, 			\
+	0x4c, 			\
+	0x4e, 			\
+	0x55, 			\
+	0x00, 			\
+}
+
+#define IDE_FREEZE_TIMEOUT_SEC 10
+static int ide_freeze_timeout = IDE_FREEZE_TIMEOUT_SEC;
+module_param(ide_freeze_timeout, int, 0);
+MODULE_PARM_DESC(ide_freeze_timeout, 
+		"IDE Freeze timeout in seconds. (0<ide_freeze_timeout<65536, default=" 
+		__MODULE_STRING(IDE_FREEZE_TIMEOUT_SEC) ")");

Please make the interface accept number of seconds (as suggested by Jens)
and remove this module parameter. This way interface will be more flexible
and cleaner.  I really don't see any advantage in doing "echo 1 > ..." instead
of "echo x > ..." (Pavel, please explain).

+static void freeze_expire(unsigned long data);
+static struct timer_list freeze_timer = 
+	TIMER_INITIALIZER(freeze_expire, 0, 0);

There needs to be a timer per device not a global one
(it works for a current special case of T42 but sooner
 or later we will hit this problem).

+static ssize_t ide_freeze_show(struct device *dev, 
+		struct device_attribute *attr, char *buf){
+	ide_drive_t *drive = to_ide_device(dev);
+	int queue_stopped = 
+		test_bit(QUEUE_FLAG_STOPPED, &drive->queue->queue_flags);
+	
+	return snprintf(buf, 10, "%u\n", queue_stopped);
+}
+
+void ide_end_freeze_rq(struct request *rq)
+{
+	struct completion *waiting = rq->waiting;
+	u8 *argbuf = rq->buffer;
+
+	/* Spinlock is already acquired */
+	if (argbuf[3] == 0xc4) {
+		blk_stop_queue(rq->q);
+		printk(KERN_DEBUG "ide_end_freeze_rq(): Head parked\n");
+	}
+	else
+		printk(KERN_DEBUG "ide_end_freeze_rq(): Head not parked\n");
+	
+	complete(waiting);
+}
+
+static int ide_freeze_drive(ide_drive_t *drive)
+{
+	DECLARE_COMPLETION(wait);
+	struct request rq;
+	int error = 0;
+	
+	/* STANDBY IMMEDIATE COMMAND (testing) */
+	/* u8 argbuf[] = STANDBY_IMMEDIATE_ARGS; */
+
+	/* UNLOAD IMMEDIATE COMMAND */
+	u8 argbuf[] = UNLOAD_IMMEDIATE_ARGS;
+
+	/* Check we're not already frozen */
+	if(blk_queue_stopped(drive->queue)){
+		printk(KERN_DEBUG "ide_freeze_drive: Queue already stopped\n");
+		return error;
+	}
+	
+	memset(&rq, 0, sizeof(rq));
+	
+	ide_init_drive_cmd(&rq);
+
+	rq.flags = REQ_DRIVE_TASK;
+	rq.buffer = argbuf;
+	rq.waiting = &wait;
+	rq.end_io = ide_end_freeze_rq;
+
+	error = ide_do_drive_cmd(drive, &rq, ide_next);
+	wait_for_completion(&wait);
+	rq.waiting = NULL;
+	
+	if(error)
+		printk(KERN_ERR "ide_freeze_drive: Error sending command to drive %s: %d\n",
+				drive->name, error);
+	else{
+		printk(KERN_DEBUG "ide_freeze_drive: Queue stopped\n");
+		
+		freeze_timer.data = (unsigned long) drive;
+		mod_timer(&freeze_timer, jiffies+(ide_freeze_timeout*HZ));
+		printk(KERN_DEBUG "ide_freeze_drive: Timer (re)initialized to %ds
timeout\n",
+				ide_freeze_timeout);
+	}
+	return error;
+}
+
+static void ide_thaw_drive(ide_drive_t *drive)
+{
+	unsigned long flags;
+	if (!blk_queue_stopped(drive->queue))
+	{
+		printk(KERN_DEBUG "ide_thaw_drive(): Queue not stopped\n");
+		return;
+	}
+	
+	spin_lock_irqsave(&ide_lock, flags);
+	blk_start_queue(drive->queue);
+	spin_unlock_irqrestore(&ide_lock, flags);
+	printk(KERN_DEBUG "ide_thaw_drive(): Queue started\n");
+	del_timer(&freeze_timer);
+	printk(KERN_DEBUG "ide_thaw_drive: Timer stopped\n");
+}

queue handling should be done through block layer helpers
(as described in Jens' email) - we will need them for libata too.

+static void freeze_expire(unsigned long data)
+{
+	ide_drive_t *drive = (ide_drive_t *) data;
+	printk(KERN_DEBUG "freeze_expire(): Freeze timer timeout, thawing...\n");
+	ide_thaw_drive(drive);
+}
+	
+static ssize_t ide_freeze_store(struct device *dev, 
+		struct device_attribute *attr, const char *buf, size_t count){
+	ide_drive_t *drive = to_ide_device(dev);
+	int freeze = 0;
+	int error = 0;
+	
+	freeze = simple_strtoul(buf, NULL, 10);
+	
+	if(freeze)
+		error = ide_freeze_drive(drive);
+	else
+		ide_thaw_drive(drive);
+	
+	return error ? error : count;
+}
+
+static DEVICE_ATTR(freeze, S_IRUSR | S_IWUSR, ide_freeze_show, 
+		ide_freeze_store);

sorry but this needs to be CLASS_DEVICE_ATTR, see below

+#endif /* CONFIG_IDEDISK_FREEZE */
+
  static void ide_disk_release(struct kref *);
 
  static void ide_disk_put(struct ide_disk_obj *idkp)
@@ -1034,6 +1189,10 @@ static int ide_disk_remove(struct device
  	struct ide_disk_obj *idkp = drive->driver_data;
  	struct gendisk *g = idkp->disk;
 
+	#ifdef CONFIG_IDEDISK_FREEZE
+	device_remove_file(dev, &dev_attr_freeze);
+	#endif /* CONFIG_IDEDISK_FREEZE */
+	

At this time attribute can still be in use (because refcounting is done
on drive->gendev), you need to add "disk" class to ide-disk driver
(drivers/scsi/st.c looks like a good example how to do it).

 	ide_cacheflush_p(drive);
 
  	ide_unregister_subdriver(drive, idkp->driver);
@@ -1248,6 +1407,10 @@ static int ide_disk_probe(struct device 
 	} else
 		drive->attach = 1;
 
+	#ifdef CONFIG_IDEDISK_FREEZE
+	device_create_file(dev, &dev_attr_freeze);
+	#endif /* CONFIG_IDEDISK_FREEZE */
+	
 	g->minors = 1 << PARTN_BITS;
 	strcpy(g->devfs_name, drive->devfs_name);
  	g->driverfs_dev = &drive->gendev;
diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -1181,6 +1181,16 @@ static void ide_do_request (ide_hwgroup_
 		}
 
 		/*
+		 * Don't accept a request when the queue is stopped
+		 * (unless we are resuming from suspend)
+		 */
+		if (test_bit(QUEUE_FLAG_STOPPED, &drive->queue->queue_flags) &&
!blk_pm_resume_request(rq)) {
+			printk(KERN_ERR "%s: queue is stopped!\n", drive->name);
+			hwgroup->busy = 0;
+			break;
+		}

IMO this should also be handled by block layer
which has all needed information, Jens?

While at it: I think that sysfs support should be moved to block layer (queue
attributes) and storage driver should only need to provide queue_freeze_fn
and queue_thaw_fn functions (similarly to cache flush support).  This should
be done now not later because this stuff is exposed to the user-space.

+		/*
 		 * Sanity: don't accept a request that isn't a PM request
 		 * if we are currently power managed. This is very important as
 		 * blk_stop_queue() doesn't prevent the elv_next_request()
@@ -1661,6 +1671,9 @@ int ide_do_drive_cmd (ide_drive_t *drive
 		where = ELEVATOR_INSERT_FRONT;
  		rq->flags |= REQ_PREEMPT;
 	}
+	if (action == ide_next)
+		where = ELEVATOR_INSERT_FRONT;
+
  	__elv_add_request(drive->queue, rq, where, 0);
  	ide_do_request(hwgroup, IDE_NO_IRQ);
 	spin_unlock_irqrestore(&ide_lock, flags);

Why is this needed?


Overall, very promising work!

Thanks,
Bartlomiej

Re: PATCH: ide: ide-disk freeze support for hdaps

[Yani Ioannou]

Hi Bartlomiej,

Thank you for your feedback :), as this is my first dabble in
ide/block drivers I certainly need it!

On 8/25/05, Bartlomiej Zolnierkiewicz <bzolnier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> +config IDEDISK_FREEZE
> 
> Is there any advantage of having it as a config option?

The main reasons I added the config option:
- the freeze feature is really only useful to an (increasing) niche of
mobile computers with an accelerometer.

- it might actually be detrimental to most other systems, you would
never want to freeze the queue on most machines - especially a
production system, and for that reason alone it seemed sensible to me
to be able to selectively remove it completely.

- to re-inforce the experimental nature of the patch, and disable it
by default (although this could be achieved just with EXPERIMENTAL I
suppose).

> Please make the interface accept number of seconds (as suggested by Jens)
> and remove this module parameter. This way interface will be more flexible
> and cleaner.  I really don't see any advantage in doing "echo 1 > ..." instead
> of "echo x > ..." (Pavel, please explain).

Either way is pretty easy enough to implement. Note though that I'd
expect the userspace app should thaw the device when danger is out of
the way (the timeout is mainly there to ensure that the queue isn't
frozen forever, and should probably be higher). Personally I don't
have too much of an opinion either way though... what's the consensus?
:).

I can understand Pavel's opinion in that a enable/disable attribute in
sysfs seems the norm, and is more intuitive. Also what should 'cat
/sys/block/hda/device/freeze' return for a 'echo x >
/sys/block/hda/device/freeze' sysfs attribute? The seconds remaining?
1/0 for frozen/thawed?

> +static void freeze_expire(unsigned long data);
> +static struct timer_list freeze_timer =
> +       TIMER_INITIALIZER(freeze_expire, 0, 0);
> 
> There needs to be a timer per device not a global one
> (it works for a current special case of T42 but sooner
>  or later we will hit this problem).

I was considering that, but I am confused as to whether each drive has
it's own queue or not? (I really am a newbie to this stuff...). If so
then yes there should be a per-device timer.

> queue handling should be done through block layer helpers
> (as described in Jens' email) - we will need them for libata too.

Good point, I'll try to move as much as I can up to the block layer,
it helps when it comes to implementing freeze for libata as you point
out too.

> At this time attribute can still be in use (because refcounting is done
> on drive->gendev), you need to add "disk" class to ide-disk driver
> (drivers/scsi/st.c looks like a good example how to do it).

I missed that completely, I'll look at changing it.

> IMO this should also be handled by block layer
> which has all needed information, Jens?
> 
> While at it: I think that sysfs support should be moved to block layer (queue
> attributes) and storage driver should only need to provide queue_freeze_fn
> and queue_thaw_fn functions (similarly to cache flush support).  This should
> be done now not later because this stuff is exposed to the user-space.

I was actually considering using a queue attribute originally, but in
my indecision I decided to go with Jen's suggestion. A queue attribute
does make sense in that the attribute primarily is there to freeze the
queue, but it would also be performing the head park. Would a queue
attribute be confusing because of that?

> 
> +               /*
>                  * Sanity: don't accept a request that isn't a PM request
>                  * if we are currently power managed. This is very important as
>                  * blk_stop_queue() doesn't prevent the elv_next_request()
> @@ -1661,6 +1671,9 @@ int ide_do_drive_cmd (ide_drive_t *drive
>                 where = ELEVATOR_INSERT_FRONT;
>                 rq->flags |= REQ_PREEMPT;
>         }
> +       if (action == ide_next)
> +               where = ELEVATOR_INSERT_FRONT;
> +
>         __elv_add_request(drive->queue, rq, where, 0);
>         ide_do_request(hwgroup, IDE_NO_IRQ);
>         spin_unlock_irqrestore(&ide_lock, flags);
> 
> Why is this needed?

I think Jon discussed that in a previous thread, but basically
although ide_next is documented in the comment for ide_do_drive_cmd,
there isn't (as far as Jon or I could see) anything actually handling
it. This patch is carried over from Jon's work and adds the code to
handle ide_next by inserting the request at the front of the queue.

> Overall, very promising work!

Thanks :-), most of it is Jon's work, and Jen's suggestions though.

Yani

P.S. Sorry about the lack of [] around PATCH...lack of sleep. Its more
of a RFC anyway.


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: PATCH: ide: ide-disk freeze support for hdaps

[Jens Axboe]

On Fri, Aug 26 2005, Yani Ioannou wrote:
> > Please make the interface accept number of seconds (as suggested by Jens)
> > and remove this module parameter. This way interface will be more flexible
> > and cleaner.  I really don't see any advantage in doing "echo 1 > ..." instead
> > of "echo x > ..." (Pavel, please explain).
> 
> Either way is pretty easy enough to implement. Note though that I'd
> expect the userspace app should thaw the device when danger is out of
> the way (the timeout is mainly there to ensure that the queue isn't
> frozen forever, and should probably be higher). Personally I don't
> have too much of an opinion either way though... what's the consensus?
> :).

Yes please, I don't understand why you would want a 0/1 interface
instead, when the timer-seconds method gives you the exact same ability
plus a way to control when to unfreeze...

> > +static struct timer_list freeze_timer =
> > +       TIMER_INITIALIZER(freeze_expire, 0, 0);
> > 
> > There needs to be a timer per device not a global one
> > (it works for a current special case of T42 but sooner
> >  or later we will hit this problem).
> 
> I was considering that, but I am confused as to whether each drive has
> it's own queue or not? (I really am a newbie to this stuff...). If so
> then yes there should be a per-device timer.

Each drive has its own queue.

> > queue handling should be done through block layer helpers
> > (as described in Jens' email) - we will need them for libata too.
> 
> Good point, I'll try to move as much as I can up to the block layer,
> it helps when it comes to implementing freeze for libata as you point
> out too.

That includes things like the timer as well, reuse the queue plugging
timer as I described in my initial posting on how to implement this.

> > At this time attribute can still be in use (because refcounting is done
> > on drive->gendev), you need to add "disk" class to ide-disk driver
> > (drivers/scsi/st.c looks like a good example how to do it).
> 
> I missed that completely, I'll look at changing it.
> 
> > IMO this should also be handled by block layer
> > which has all needed information, Jens?
> > 
> > While at it: I think that sysfs support should be moved to block layer (queue
> > attributes) and storage driver should only need to provide queue_freeze_fn
> > and queue_thaw_fn functions (similarly to cache flush support).  This should
> > be done now not later because this stuff is exposed to the user-space.
> 
> I was actually considering using a queue attribute originally, but in
> my indecision I decided to go with Jen's suggestion. A queue attribute
> does make sense in that the attribute primarily is there to freeze the
> queue, but it would also be performing the head park. Would a queue
> attribute be confusing because of that?

I fully agree with Bart here. The only stuff that should be ide special
is the actual command setup and completion check, since that is a
hardware property. libata will get a few little helpers for that as
well. The rest should be block layer implementation.

> >                  * Sanity: don't accept a request that isn't a PM request
> >                  * if we are currently power managed. This is very important as
> >                  * blk_stop_queue() doesn't prevent the elv_next_request()
> > @@ -1661,6 +1671,9 @@ int ide_do_drive_cmd (ide_drive_t *drive
> >                 where = ELEVATOR_INSERT_FRONT;
> >                 rq->flags |= REQ_PREEMPT;
> >         }
> > +       if (action == ide_next)
> > +               where = ELEVATOR_INSERT_FRONT;
> > +
> >         __elv_add_request(drive->queue, rq, where, 0);
> >         ide_do_request(hwgroup, IDE_NO_IRQ);
> >         spin_unlock_irqrestore(&ide_lock, flags);
> > 
> > Why is this needed?
> 
> I think Jon discussed that in a previous thread, but basically
> although ide_next is documented in the comment for ide_do_drive_cmd,
> there isn't (as far as Jon or I could see) anything actually handling
> it. This patch is carried over from Jon's work and adds the code to
> handle ide_next by inserting the request at the front of the queue.

As per my previous mail, I will ack that bit.

> > Overall, very promising work!
> 
> Thanks :-), most of it is Jon's work, and Jen's suggestions though.

My name is Jens, not Jen :-)

-- 
Jens Axboe

Re: PATCH: ide: ide-disk freeze support for hdaps

[Pavel Machek]

Hi!

> > > Please make the interface accept number of seconds (as suggested by Jens)
> > > and remove this module parameter. This way interface will be more flexible
> > > and cleaner.  I really don't see any advantage in doing "echo 1 > ..." instead
> > > of "echo x > ..." (Pavel, please explain).
> > 
> > Either way is pretty easy enough to implement. Note though that I'd
> > expect the userspace app should thaw the device when danger is out of
> > the way (the timeout is mainly there to ensure that the queue isn't
> > frozen forever, and should probably be higher). Personally I don't
> > have too much of an opinion either way though... what's the consensus?
> > :).
> 
> Yes please, I don't understand why you would want a 0/1 interface
> instead, when the timer-seconds method gives you the exact same ability
> plus a way to control when to unfreeze...

Well, with my power-managment hat on:

we probably want "freeze" functionality to be generic; it makes sense
for other devices, too.

"My battery is so low I can not use wifi any more" => userspace
freezes wifi.

Now, having this kind of timeout in all the cases looks pretty ugly to my eyes.

				Pavel
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: PATCH: ide: ide-disk freeze support for hdaps

[Yani Ioannou]

Hi Pavel,

On 8/27/05, Pavel Machek <pavel@suse.cz> wrote: 
> Well, with my power-managment hat on:
> 
> we probably want "freeze" functionality to be generic; it makes sense
> for other devices, too.
> 
> "My battery is so low I can not use wifi any more" => userspace
> freezes wifi.
> 
> Now, having this kind of timeout in all the cases looks pretty ugly to my eyes.

Thing is the freeze attribute hasn't got much to do with power
management, this is just to freeze the queue, and park the drive head
ASAP (preferably with the unload immediate command supported by new
drives) in order to protect the drive in an impact. Unload immediate
doesn't even stop spinning the drive, so little power is saved.

Maybe a suspend attribute would be a good idea for something along the
lines of what you have in mind? A enable/disable attribute would
definitely make sense for that application.

I suppose renaming the attribute to "ramming_speed" or
"brace_for_impact", might make the purpose more clear ;).

Thanks,
Yani