Re: Spinning down sata disk before poweroff

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Fri, Jun 17, 2016 at 01:49:43AM +0000, Anindya Mukherjee wrote:
> Hi,
> 
> I'm running NetBSD 7.0.1_PATCH (GENERIC) amd64 on a Dell laptop. Almost everything is working perfectly, except the fact that every time I shutdown using the -p switch, the hard drive makes a loud click sound as the system powers off. I checked the SMART status (atactl and smartctl) and after every poweroff the Power_Off-Retract-Count parameter increases by 1.
> 
> I did some searching on the web and came across PR #21531 where this issue was discussed from 2003-2008 and finally a patch was committed which resolved the issue by sending the STANDBY_IMMEDIATE command to the disk before powering off. Since then the code has been refactored, but it is present in src/sys/dev/ata/wd.c line 1970 (wd_shutdown) which calls line 1848 (wd_standby). This seemed strange since the disk was definitely not being spun down.
> 
> I attached a remote gdb instance and stepped through the code during shutdown, breaking on wd_flushcache() which is always called. The code path taken is wdclose()->wdlastclose() (lines 1029, 1014). I can see that the cache is flushed but then the device is deleted in line 1023. Subsequently, power is cut off during shutdown, causing an emergency retract. So, it seems at least for newer sata disks the spindown code is not being called. I'm fairly new to NetBSD code so there is a chance I read this wrong, so feel free to correct me.
> 
> Ideally I'd like the disk to spin down during poweroff (-p) and halt (-h), perhaps settable using a sysctl, but not during a reboot (-r). I am planning to patch wdlastclose() as an experiment to run the spindown code to see if it stops the click. Is this a known issue, worthy of a PR? I can file one. I can also volunteer a patch once I have tested it on my laptop. Comments welcome!


So the disk is not powered off because it's detached before the pmf framework
has a chance to power it off (see amd64/amd64/machdep.c:cpu_reboot()).
that's bad.
Doing the poweroff in wdlastclose() is bad because then you'll have a
poweroff/powerup cycle for a reboot, or even on unmount/mount events if this
is not your root device. This can be harmfull for some disks (this has already
been discussed).

The (untested) attached patch should fix this by calling pmf before detach;
can you give it a try ?

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Index: arch/amd64/amd64/machdep.c
===================================================================
RCS file: /cvsroot/src/sys/arch/amd64/amd64/machdep.c,v
retrieving revision 1.217
diff -u -p -u -r1.217 machdep.c
--- arch/amd64/amd64/machdep.c	15 May 2016 10:35:54 -0000	1.217
+++ arch/amd64/amd64/machdep.c	17 Jun 2016 11:22:45 -0000
@@ -654,14 +654,19 @@ cpu_reboot(int howto, char *bootstr)
 				resettodr();
 		}
 
-		while (vfs_unmountall1(curlwp, false, false) ||
-		       config_detach_all(boothowto) ||
-		       vfs_unmount_forceone(curlwp))
+		while (vfs_unmountall1(curlwp, false, false))
 			;	/* do nothing */
-	} else
+
+		pmf_system_shutdown(boothowto);
+		while(config_detach_all(boothowto))
+			;	/* do nothing */
+		while(vfs_unmount_forceone(curlwp))
+			;	/* do nothing */
+	} else {
 		suspendsched();
+		pmf_system_shutdown(boothowto);
+	}
 
-	pmf_system_shutdown(boothowto);
 
 	/* Disable interrupts. */
 	s = splhigh();
Index: arch/i386/i386/machdep.c
===================================================================
RCS file: /cvsroot/src/sys/arch/i386/i386/machdep.c,v
retrieving revision 1.755
diff -u -p -u -r1.755 machdep.c
--- arch/i386/i386/machdep.c	15 May 2016 10:35:54 -0000	1.755
+++ arch/i386/i386/machdep.c	17 Jun 2016 11:22:45 -0000
@@ -743,14 +743,18 @@ cpu_reboot(int howto, char *bootstr)
 				resettodr();
 		}
 
-		while (vfs_unmountall1(curlwp, false, false) ||
-		       config_detach_all(boothowto) ||
-		       vfs_unmount_forceone(curlwp))
+		while (vfs_unmountall1(curlwp, false, false))
 			;	/* do nothing */
-	} else
-		suspendsched();
 
-	pmf_system_shutdown(boothowto);
+		pmf_system_shutdown(boothowto);
+		while (config_detach_all(boothowto))
+			;	/* do nothing */
+		while (vfs_unmount_forceone(curlwp))
+			;	/* do nothing */
+	} else {
+		suspendsched();
+		pmf_system_shutdown(boothowto);
+	}
 
 	s = splhigh();