Re: Unclean sync in current

Thus spake Kevin Oberman <oberman_at_es.net>:
> I've been seeing this for a couple of weeks since I updated my laptop to
> CURRENT. I do a normal shutdown (-p or -r) and reboot. The shutdown
> looked normal, with no problems reported with the sync, but, when the
> system is rebooted, the partitions are all shown as possibly
> unclean. From my dmesg:
> Mounting root from ufs:/dev/ad0s3a
> WARNING: / was not properly dismounted
> WARNING: / was not properly dismounted
> WARNING: /tmp was not properly dismounted
> WARNING: /usr was not properly dismounted
> WARNING: /var was not properly dismounted
> 
> All disks are mounted with soft-updates enabled. 
> 
> I don't see any other reports of this. Is this unique to my system?

Unlike the SCSI driver, the ATA driver does not send a flush cache
command to your disks before powering off the system.  The kernel
waits for five seconds in either case, but for some disks that may
not be sufficient.

The following patch should fix that, although it may have rotted a
bit in the last two months given Soeren's sweeping ATA changes.  I
also edited an unrelated change out of the diff, which might
confuse patch(1).  If you run into problems getting it to apply,
let me know and I'll fix it when I'm back from vacation.

Index: sys/dev/ata/ata-all.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/ata/ata-all.c,v
retrieving revision 1.163
diff -u -r1.163 ata-all.c
--- sys/dev/ata/ata-all.c	2003/01/19 20:18:07	1.163
+++ sys/dev/ata/ata-all.c	2003/01/27 09:17:02
_at__at_ -77,6 +77,7 _at__at_
 static void ata_intr(void *);
 static int ata_getparam(struct ata_device *, u_int8_t);
 static int ata_service(struct ata_channel *);
+static void ata_shutdown(void *arg, int howto);
 static void bswap(int8_t *, int);
 static void btrim(int8_t *, int);
 static void bpack(int8_t *, int8_t *, int);
_at__at_ -1160,7 +1161,32 _at__at_
     return error;
 }
 
+/*
+ * This procedure is called during shutdown,
+ * after all dirty buffers have been flushed.
+ */
 static void
+ata_shutdown(void *arg, int howto)
+{
+    struct ata_channel *ch;
+    int ctlr;
+
+#ifdef DEV_ATADISK
+    /* Flush the caches of each open ATA disk device. */
+    for (ctlr = 0; ctlr < devclass_get_maxunit(ata_devclass); ctlr++) {
+	if (!(ch = devclass_get_softc(ata_devclass, ctlr)))
+	    continue;
+	ch->lock_func(ch, ATA_LF_LOCK);
+	if (ch->devices & ATA_ATA_MASTER && ch->device[MASTER].driver)
+	    ad_sync(&ch->device[MASTER]);
+	if (ch->devices & ATA_ATA_SLAVE && ch->device[SLAVE].driver)
+	    ad_sync(&ch->device[SLAVE]);
+	ch->lock_func(ch, ATA_LF_UNLOCK);
+    }
+#endif /* DEV_ATADISK */
+}
+
+static void
 ata_drawer_start(struct ata_device *atadev)
 {
     ATA_INB(atadev->channel->r_io, ATA_DRIVE);	  
_at__at_ -1516,5 +1542,10 _at__at_
 	printf("ata: config_intrhook_establish failed\n");
 	free(ata_delayed_attach, M_TEMP);
     }
+
+    /* Register a handler to flush write caches on shutdown */
+    if ((EVENTHANDLER_REGISTER(shutdown_post_sync, ata_shutdown,
+			       NULL, SHUTDOWN_PRI_DEFAULT)) == NULL)
+	printf("ata: shutdown event registration failed!\n");
 }
 SYSINIT(atadev, SI_SUB_DRIVERS, SI_ORDER_SECOND, ata_init, NULL)
Index: sys/dev/ata/ata-disk.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/ata/ata-disk.c,v
retrieving revision 1.139
diff -u -r1.139 ata-disk.c
--- sys/dev/ata/ata-disk.c	2002/12/17 16:26:22	1.139
+++ sys/dev/ata/ata-disk.c	2003/01/27 09:17:04
_at__at_ -255,10 +255,8 _at__at_
     disk_invalidate(&adp->disk);
     disk_destroy(adp->dev);
     devstat_remove_entry(&adp->stats);
-    if (flush) {
-	if (ata_command(atadev, ATA_C_FLUSHCACHE, 0, 0, 0, ATA_WAIT_READY))
-	    ata_prtdev(atadev, "flushing cache on detach failed\n");
-    }
+    if (flush)
+	ad_sync(atadev);
     if (adp->flags & AD_F_RAID_SUBDISK)
 	ata_raiddisk_detach(adp);
     ata_free_name(atadev);
_at__at_ -289,8 +287,7 _at__at_
 
     adp->device->channel->lock_func(adp->device->channel, ATA_LF_LOCK);
     ATA_SLEEPLOCK_CH(adp->device->channel, ATA_CONTROL);
-    if (ata_command(adp->device, ATA_C_FLUSHCACHE, 0, 0, 0, ATA_WAIT_READY))
-	ata_prtdev(adp->device, "flushing cache on close failed\n");
+    ad_sync(adp->device);
     ATA_UNLOCK_CH(adp->device->channel);
     adp->device->channel->lock_func(adp->device->channel, ATA_LF_UNLOCK);
     return 0;
_at__at_ -765,6 +762,20 _at__at_
 	return ATA_OP_CONTINUES;
     }
     return ATA_OP_FINISHED;
+}
+
+/*
+ * Flush the write cache of the given device.
+ */
+int
+ad_sync(struct ata_device *atadev)
+{
+    int error;
+
+    /* XXX The ATA standard says this command can take up to 30 seconds. */
+    if ((error = ata_command(atadev,ATA_C_FLUSHCACHE,0,0,0,ATA_WAIT_READY)))
+	ata_prtdev(atadev, "flushing cache failed\n");
+    return error;
 }
 
 static void
Index: sys/dev/ata/ata-disk.h
===================================================================
RCS file: /home/ncvs/src/sys/dev/ata/ata-disk.h,v
retrieving revision 1.42
diff -u -r1.42 ata-disk.h
--- sys/dev/ata/ata-disk.h	2002/07/22 18:35:01	1.42
+++ sys/dev/ata/ata-disk.h	2003/01/27 09:17:04
_at__at_ -80,6 +80,7 _at__at_
 void ad_detach(struct ata_device *, int);
 void ad_reinit(struct ata_device *);
 void ad_start(struct ata_device *);
+int ad_sync(struct ata_device *);
 int ad_transfer(struct ad_request *);
 int ad_interrupt(struct ad_request *);
 int ad_service(struct ad_softc *, int);