Re: Re: HDAPS, Need to park the head for real

Re: Re: HDAPS, Need to park the head for real

[Jon Escombe]

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

Jens Axboe wrote:

> Ok, I'll give you some hints to get you started... What you really want
>
>to do, is:
>
>- Insert a park request at the front of the queue
>- On completion callback on that request, freeze the block queue and
>  schedule it for unfreeze after a given time
>  
>

Am attaching a first attempt at a patch - for comments only - please 
don't apply to a production system. I've not delved into the IDE code 
before, so I've just been following my nose... In other words - It 
appears to work for me - but I may be doing something crazy ;)

Having said that, I tested with a utility that repeatedly froze/thawed 
hundreds of times while really hammering the disk with file copies, and 
nothing oopsed or failed to checksum afterwards...

To do:

Move the /proc interface to sysfs. At the moment it's just a simple 
'echo -n 1 > /proc/ide/hda/freeze' to freeze, and 0 to thaw.

Address Jens concerns about our userspace code falling over and leaving 
the machine hung. I favour retaining a binary on/off interface (rather 
than specifying a timeout up front), but having the IDE code auto-thaw 
on a timer.. That way we can just keep writing 1's to it while we're 
checking the accelerometer and wanting to keep it frozen, and if we 
should die then it'll wake up by itself after a second or so...

Same again for libata (for T43 owners).

Regards,
Jon.



______________________________________________________________
Email via Mailtraq4Free from Enstar (www.mailtraqdirect.co.uk)

[-- Attachment #2: ide_freeze.patch --]
[-- Type: text/x-patch, Size: 4882 bytes --]

diff -urN linux-2.6.13-rc6.original/drivers/ide/ide-io.c linux-2.6.13-rc6/drivers/ide/ide-io.c
--- linux-2.6.13-rc6.original/drivers/ide/ide-io.c	2005-06-17 20:48:29.000000000 +0100
+++ linux-2.6.13-rc6/drivers/ide/ide-io.c	2005-08-24 20:56:31.000000000 +0100
@@ -1181,6 +1181,16 @@
 		}
 
 		/*
+		 * 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 @@
 		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);
diff -urN linux-2.6.13-rc6.original/drivers/ide/ide-proc.c linux-2.6.13-rc6/drivers/ide/ide-proc.c
--- linux-2.6.13-rc6.original/drivers/ide/ide-proc.c	2005-06-17 20:48:29.000000000 +0100
+++ linux-2.6.13-rc6/drivers/ide/ide-proc.c	2005-08-24 21:51:14.000000000 +0100
@@ -264,6 +264,122 @@
 	return -EINVAL;
 }
 
+static int proc_ide_read_freeze
+	(char *page, char **start, off_t off, int count, int *eof, void *data)
+{
+	ide_drive_t	*drive = (ide_drive_t *) data;
+	char		*out = page;
+	int		len;
+
+	proc_ide_settings_warn();
+
+	if (test_bit(QUEUE_FLAG_STOPPED, &drive->queue->queue_flags))
+		out += sprintf(out, "%s: queue is stopped\n", drive->name);
+	else
+		out += sprintf(out, "%s: queue not stopped\n", drive->name);
+
+	len = out - page;
+	PROC_IDE_READ_RETURN(page,start,off,count,eof,len);
+}
+
+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_ERR "ide_end_freeze_rq(): Queue stopped...\n");
+	}
+	else
+		printk(KERN_ERR "ide_end_freeze_rq(): Head not parked...\n");
+/*
+	blk_stop_queue(rq->q);
+	printk(KERN_ERR "ide_end_freeze_rq(): Queue stopped...\n");
+*/
+	complete(waiting);
+}
+
+static int proc_ide_write_freeze(struct file *file, const char __user *buffer,
+				   unsigned long count, void *data)
+{
+	DECLARE_COMPLETION(wait);
+	unsigned long	val, flags;
+	char 		*buf, *s;	
+	struct request	rq;
+	ide_drive_t	*drive = (ide_drive_t *) data;
+	u8 		args[7], *argbuf = args;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EACCES;
+
+	proc_ide_settings_warn();
+
+	if (count >= PAGE_SIZE)
+		return -EINVAL;
+
+	s = buf = (char *)__get_free_page(GFP_USER);
+	if (!buf)
+		return -ENOMEM;
+
+	if (copy_from_user(buf, buffer, count)) {
+		free_page((unsigned long)buf);
+		return -EFAULT;
+	}
+
+	buf[count] = '\0';
+	memset(&rq, 0, sizeof(rq));
+	memset(&args, 0, sizeof(args));
+
+	/* Ought to check we're the right sort of device - i.e. hard disk only */
+
+	/* STANDY IMMEDIATE COMMAND (spins down drive - more obvious for testing?)
+	argbuf[0] = 0xe0;
+	*/
+
+	/* UNLOAD IMMEDIATE COMMAND */
+	argbuf[0] = 0xe1;
+	argbuf[1] = 0x44;
+	argbuf[3] = 0x4c;
+	argbuf[4] = 0x4e;
+	argbuf[5] = 0x55;
+
+	/* Ought to have some sanity checking around these values */
+	val = simple_strtoul(buf, &s, 10);
+	if (val) {
+		/* Check we're not already frozen */
+		if (!test_bit(QUEUE_FLAG_STOPPED, &drive->queue->queue_flags)) {
+			/* Issue the park command & freeze */
+			ide_init_drive_cmd(&rq);
+
+			rq.flags = REQ_DRIVE_TASK;
+			rq.buffer = argbuf;
+			rq.waiting = &wait;
+			rq.end_io = ide_end_freeze_rq;
+
+			ide_do_drive_cmd(drive, &rq, ide_next);
+			wait_for_completion(&wait);
+			rq.waiting = NULL;
+		}
+		else
+			printk(KERN_ERR "proc_ide_write_freeze(): Queue already stopped...\n");
+	}
+	else {
+		/* Check we are frozen & unfreeze */ 
+		if (test_bit(QUEUE_FLAG_STOPPED, &drive->queue->queue_flags)) {
+			spin_lock_irqsave(&ide_lock, flags);
+			blk_start_queue(drive->queue);
+			spin_unlock_irqrestore(&ide_lock, flags);
+			printk(KERN_ERR "proc_ide_write_freeze(): Queue started...\n");
+		}
+		else
+			printk(KERN_ERR "proc_ide_write_freeze(): Queue not stopped...\n");
+	}
+	free_page((unsigned long)buf);
+	return count;
+}
+
 int proc_ide_read_capacity
 	(char *page, char **start, off_t off, int count, int *eof, void *data)
 {
@@ -390,6 +506,7 @@
 	{ "media",	S_IFREG|S_IRUGO,	proc_ide_read_media,	NULL },
 	{ "model",	S_IFREG|S_IRUGO,	proc_ide_read_dmodel,	NULL },
 	{ "settings",	S_IFREG|S_IRUSR|S_IWUSR,proc_ide_read_settings,	proc_ide_write_settings },
+	{ "freeze",	S_IFREG|S_IRUSR|S_IWUSR,proc_ide_read_freeze,	proc_ide_write_freeze },
 	{ NULL,	0, NULL, NULL }
 };
 

Re: [Hdaps-devel] Re: HDAPS, Need to park the head for real

[Alan Cox]

You need the kernel side timeout. Consider this case

One page of memory holds the parking code
A second page is swapped to disk and holds the resume code

You park the disk
You wakeup
You got to page in the resume code

So you really do want the kernel helping to avoid a deadlock

@@ -1661,6 +1671,9 @@
                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);

Also puzzles me- why is this needed ?

Re: [Hdaps-devel] Re: HDAPS, Need to park the head for real

[Jon Escombe]

Alan Cox wrote:
> @@ -1661,6 +1671,9 @@
>                 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);
> 
> Also puzzles me- why is this needed ?

I wanted the park command to get in at the head of the queue (behind the 
currently executing request).

Contrary to the comments for ide_do_drive_cmd(), ide_next didn't appear 
to do anything to achieve this? At least from my initial testing before 
I made this change - it could take a second or so for the park command 
to be issued if the disk was busy....

Regards,
Jon.

______________________________________________________________
Email via Mailtraq4Free from Enstar (www.mailtraqdirect.co.uk)

Re: Re: HDAPS, Need to park the head for real

[Jens Axboe]

On Thu, Aug 25 2005, Jon Escombe wrote:
> Alan Cox wrote:
> >@@ -1661,6 +1671,9 @@
> >                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);
> >
> >Also puzzles me- why is this needed ?
> 
> I wanted the park command to get in at the head of the queue (behind the 
> currently executing request).
> 
> Contrary to the comments for ide_do_drive_cmd(), ide_next didn't appear 
> to do anything to achieve this? At least from my initial testing before 
> I made this change - it could take a second or so for the park command 
> to be issued if the disk was busy....

That part seems to have been lost, apparently. The above patch is
correct.

-- 
Jens Axboe



-------------------------------------------------------
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