Fixing halt/shutdown for libata spindown handling

Fixing halt/shutdown for libata spindown handling

[Daniel Drake]

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

Hi,

I just took a quick look into modifying halt to work as suggested for 
the new libata spindown code.

Here's how it works in Gentoo at the moment: /sbin/halt is called 
typically with the "-d -p -i -h" arguments. After doing some 
halt-specific things (such as spinning down disks per the -h argument), 
halt calls /sbin/shutdown which presumably finishes the task. This is 
with sysvinit-2.86

So, I was expecting to jump into the halt source code, find the "-h" 
argument parsing, and see code for it spinning down all the disks.

I was surprised to find that the /sbin/halt spin down implementation is 
very limited, it only works for IDE disks (by working through 
/proc/ide). This doesn't make sense to me, the libata commits state that 
userspace shutdown is spinning down libata disks.

So, this means that other distro's do it differently? I'd appreciate 
some pointers to what happens elsewhere.

I have attached the code from sysvinit which spins down the disks. It's 
not modified by the patches below.

Gentoo uses sysvinit from here:
ftp://sunsite.unc.edu/pub/Linux/system/daemons/init/sysvinit-2.86.tar.gz

Patched with:
http://sources.gentoo.org/viewcvs.py/*checkout*/gentoo-x86/sys-apps/sysvinit/files/sysvinit-2.86-docs.patch
http://sources.gentoo.org/viewcvs.py/*checkout*/gentoo-x86/sys-apps/sysvinit/files/sysvinit-2.86-shutdown-usage.patch
http://sources.gentoo.org/viewcvs.py/*checkout*/gentoo-x86/sys-apps/sysvinit/files/sysvinit-2.86-off-by-one.patch
http://distfiles.gentoo.org/distfiles/sysvinit-2.86-kexec.patch
http://sources.gentoo.org/viewcvs.py/*checkout*/gentoo-x86/sys-apps/sysvinit/files/sysvinit-2.86-execl.patch
http://sources.gentoo.org/viewcvs.py/*checkout*/gentoo-x86/sys-apps/sysvinit/files/sysvinit-2.86-utmp-64bit.patch
http://sources.gentoo.org/viewcvs.py/*checkout*/gentoo-x86/sys-apps/sysvinit/files/sysvinit-2.86-shutdown-single.patch

If I'm right and Gentoo is currently not spinning down SCSI/libata 
disks, the only /sbin/halt modification required is to write 0 into
/sys/modules/libata/parameters/spindown_compat right?

Final question: should spindown_compat be set to 0 for both shutdown and 
reboot, or just shutdown?

Thanks,
Daniel

[-- Attachment #2: hddown.c --]
[-- Type: text/x-csrc, Size: 2214 bytes --]

/*
 * hddown.c	Find all disks on the system and
 *		shut them down.
 *
 */
char *v_hddown = "@(#)hddown.c  1.02  22-Apr-2003  miquels@cistron.nl";

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <time.h>
#include <string.h>
#include <fcntl.h>
#include <dirent.h>

#ifdef __linux__

#include <sys/ioctl.h>
#include <linux/hdreg.h>

#define MAX_DISKS	64
#define PROC_IDE	"/proc/ide"
#define DEV_BASE	"/dev"

/*
 *	Find all IDE disks through /proc.
 */
static int find_idedisks(char **dev, int maxdev)
{
	DIR *dd;
	FILE *fp;
	struct dirent *d;
	char buf[256];
	int i = 0;

	if ((dd = opendir(PROC_IDE)) == NULL)
		return -1;

	while ((d = readdir(dd)) != NULL) {
		if (strncmp(d->d_name, "hd", 2) != 0)
			continue;
		buf[0] = 0;
		snprintf(buf, sizeof(buf), PROC_IDE "/%s/media", d->d_name);
		if ((fp = fopen(buf, "r")) == NULL)
			continue;
		if (fgets(buf, sizeof(buf), fp) == 0 ||
		    strcmp(buf, "disk\n") != 0) {
			fclose(fp);
			continue;
		}
		fclose(fp);
		snprintf(buf, sizeof(buf), DEV_BASE "/%s", d->d_name);
		dev[i++] = strdup(buf);
		if (i >= maxdev)
			break;
	}
	closedir(dd);
	if (i < maxdev) dev[i] = NULL;

	return 0;
}

/*
 *	Put an IDE disk in standby mode.
 *	Code stolen from hdparm.c
 */
static int do_standby_idedisk(char *device)
{
#ifndef WIN_STANDBYNOW1
#define WIN_STANDBYNOW1 0xE0
#endif
#ifndef WIN_STANDBYNOW2
#define WIN_STANDBYNOW2 0x94
#endif
	unsigned char args1[4] = {WIN_STANDBYNOW1,0,0,0};
	unsigned char args2[4] = {WIN_STANDBYNOW2,0,0,0};
	int fd;

	if ((fd = open(device, O_RDWR)) < 0)
		return -1;

	if (ioctl(fd, HDIO_DRIVE_CMD, &args1) &&
	    ioctl(fd, HDIO_DRIVE_CMD, &args2))
		return -1;

	return 0;
}

/*
 *	First find all IDE disks, then put them in standby mode.
 *	This has the side-effect of flushing the writecache,
 *	which is exactly what we want on poweroff.
 */
int hddown(void)
{
	char *disks[MAX_DISKS+1];
	int i;

	if (find_idedisks(disks, MAX_DISKS) < 0)
		return -1;

	for (i = 0; disks[i] && i < MAX_DISKS; i++)
		do_standby_idedisk(disks[i]);

	return 0;
}

#else /* __linux__ */

int hddown(void)
{
	return 0;
}

#endif /* __linux__ */

#ifdef STANDALONE
int main(int argc, char **argv)
{
	return (hddown() == 0);
}
#endif

Re: Fixing halt/shutdown for libata spindown handling

[Tejun Heo]

Hello, Daniel.

Daniel Drake wrote:
> I was surprised to find that the /sbin/halt spin down implementation is
> very limited, it only works for IDE disks (by working through
> /proc/ide). This doesn't make sense to me, the libata commits state that
> userspace shutdown is spinning down libata disks.
> 
> So, this means that other distro's do it differently? I'd appreciate
> some pointers to what happens elsewhere.

Debian (and thus probably ubuntu) issues STADNBYNOW without FLUSH.
Opensuse doesn't seem to do anything and there's another distro which
does both, not sure which tho.  Hmm... If the upstream sysvinit code
doesn't contain spindown code, it's more likely we have more distros
which don't do anything.  If that's the case, it will be worthwhile to
detect that STANDBYNOW isn't issued and skip compat checking and just do
it.  I'll come up with a patch, please wait a bit before releasing the
update.

[--snip--]
> If I'm right and Gentoo is currently not spinning down SCSI/libata
> disks, the only /sbin/halt modification required is to write 0 into
> /sys/modules/libata/parameters/spindown_compat right?

Yes, that will make all kernels from now on behave correctly, but doing
FLUSH CACHE followed by STANDBYNOW on older kernels (kernels without
manage_start_stop) would help.

> Final question: should spindown_compat be set to 0 for both shutdown and
> reboot, or just shutdown?

It doesn't really matter.  It just needs to be set while powering down.
 You can set it anywhere, during boot, in shutdown.

Thanks.

-- 
tejun

Re: Fixing halt/shutdown for libata spindown handling

[Francesco Pretto]

Tejun Heo <htejun <at> gmail.com> writes:
> 
> > Final question: should spindown_compat be set to 0 for both shutdown and
> > reboot, or just shutdown?
> 
> It doesn't really matter.  It just needs to be set while powering down.
>  You can set it anywhere, during boot, in shutdown.
> 
> Thanks.
> 

Hibernate/suspend shall be also considered. Setting it during boot it's one time
for every poweroff conditions, but i can't say if it's the best approach.

Regards

Re: Fixing halt/shutdown for libata spindown handling

[Tejun Heo]

Francesco Pretto wrote:
> Tejun Heo <htejun <at> gmail.com> writes:
>>> Final question: should spindown_compat be set to 0 for both shutdown and
>>> reboot, or just shutdown?
>> It doesn't really matter.  It just needs to be set while powering down.
>>  You can set it anywhere, during boot, in shutdown.
>>
>> Thanks.
>>
> 
> Hibernate/suspend shall be also considered. Setting it during boot it's one time
> for every poweroff conditions, but i can't say if it's the best approach.

The compat thing is only for shutdown/powerdown.  Kernel will always
flush and spindown disks on suspends.  So, really, it doesn't matter
when you set the variable.

-- 
tejun

Re: Fixing halt/shutdown for libata spindown handling

[Daniel Drake]

Hi Tejun,

Thanks for the fast response and action.

Tejun Heo wrote:
> it.  I'll come up with a patch, please wait a bit before releasing the
> update.

The patch has been merged into libata-dev.git so I'd like to revisit 
this topic now.

Am I right in saying, to fix 2.6.21, the following patches are needed, 
and no others, in this order:

sd: fix return value of sd_sync_cache()
3721050afc6cb6ddf6de0f782e2054ebcc225e9b
(not sure if this one is required?)

[SCSI] sd: implement START/STOP management
3c94c5a2fb43a654e777f509d5032b0db8ed09f

libata: reimplement suspend/resume support using sdev->manage_start_stop
9666f4009c22f6520ac3fb8a19c9e32ab973e828

libata: implement libata.spindown_compat
920a4b1038e442700a1cfac77ea7e20bd615a2c3

libata: fix shutdown warning message printing
da071b42f73dabbd0daf7ea4c3ff157d53b00648

libata: track spindown status and skip spindown_compat if possible
13b8d09f5de0aaa3153bbccc98baf247387823dc


Additionally, no userspace modifications are needed, at least while 
Gentoo's shutdown/halt programs are not attempting to spin down libata 
disks?

I note that the patch titled "SCSI: kill sht->suspend/resume" (not 
included in the above tree) is not yet merged into Linus' tree. Am I 
right in saying it's not required for the disk spindown stuff to work 
properly?

Thanks!
Daniel

Re: Fixing halt/shutdown for libata spindown handling

[Tejun Heo]

Hello,

Daniel Drake wrote:
> Am I right in saying, to fix 2.6.21, the following patches are needed,
> and no others, in this order:
> 
> sd: fix return value of sd_sync_cache()
> 3721050afc6cb6ddf6de0f782e2054ebcc225e9b
> (not sure if this one is required?)

It's a bug fix but not necessary for manage_start_stop.

> [SCSI] sd: implement START/STOP management
> 3c94c5a2fb43a654e777f509d5032b0db8ed09f

The commit is is missing the first 'c', so it's
c3c94c5a2fb43a654e777f509d5032b0db8ed09f.

> libata: reimplement suspend/resume support using sdev->manage_start_stop
> 9666f4009c22f6520ac3fb8a19c9e32ab973e828
> 
> libata: implement libata.spindown_compat
> 920a4b1038e442700a1cfac77ea7e20bd615a2c3
> 
> libata: fix shutdown warning message printing
> da071b42f73dabbd0daf7ea4c3ff157d53b00648
> 
> libata: track spindown status and skip spindown_compat if possible
> 13b8d09f5de0aaa3153bbccc98baf247387823dc

Now there's one more patch.

  http://article.gmane.org/gmane.linux.ide/18934

> Additionally, no userspace modifications are needed, at least while
> Gentoo's shutdown/halt programs are not attempting to spin down libata
> disks?

Yes.

> I note that the patch titled "SCSI: kill sht->suspend/resume" (not
> included in the above tree) is not yet merged into Linus' tree. Am I
> right in saying it's not required for the disk spindown stuff to work
> properly?

Yes, your right.  It's clean up of now unused feature and doesn't affect
any functionality.

One thing I'm worried about is reimplement-suspend-resume (9666f400).
It hasn't received testing in -mm and got merged into -rc, so it would
be a good idea to wait for a week or two and see whether any regression
is caused by the change before applying it to distro kernel.

Thanks.

-- 
tejun

Re: Fixing halt/shutdown for libata spindown handling

[Daniel Drake]

Tejun Heo wrote:
> One thing I'm worried about is reimplement-suspend-resume (9666f400).
> It hasn't received testing in -mm and got merged into -rc, so it would
> be a good idea to wait for a week or two and see whether any regression
> is caused by the change before applying it to distro kernel.

Thanks for all your help. I included all these patches in a release made 
last week and have only received positive feedback so far.

For anyone else interested, here are the patches backported to 2.6.21. A 
couple needed small rediffs, there were a couple more than originally 
described in the earlier mails (patch dependencies etc), but if you 
apply the following as a whole it seems to work nicely:

http://dev.gentoo.org/~dsd/genpatches/trunk/2.6.21/2110_scsi-sd-printing.patch
http://dev.gentoo.org/~dsd/genpatches/trunk/2.6.21/2111_sd-start-stop.patch
http://dev.gentoo.org/~dsd/genpatches/trunk/2.6.21/2112_libata-suspend.patch
http://dev.gentoo.org/~dsd/genpatches/trunk/2.6.21/2113_libata-spindown-compat.patch
http://dev.gentoo.org/~dsd/genpatches/trunk/2.6.21/2114_libata-shutdown-warning.patch
http://dev.gentoo.org/~dsd/genpatches/trunk/2.6.21/2115_libata-spindown-status.patch
http://dev.gentoo.org/~dsd/genpatches/trunk/2.6.21/2116_libata-remove-spindown-compat.patch
http://dev.gentoo.org/~dsd/genpatches/trunk/2.6.21/2117_sata-via-suspend.patch
http://dev.gentoo.org/~dsd/genpatches/trunk/2.6.21/2118_scsi-constants.patch

These are all contained in
http://dev.gentoo.org/~dsd/genpatches/tarballs/genpatches-2.6.21-3.base.tar.bz2

Daniel