Rely on the disk to spin itself down
authorDavid Zeuthen <davidz@redhat.com>
Sun, 9 Aug 2009 18:39:47 +0000 (14:39 -0400)
committerDavid Zeuthen <davidz@redhat.com>
Sun, 9 Aug 2009 18:39:47 +0000 (14:39 -0400)
In other words, instead of playing games with watching
/sys/block/sdX/stat files and sending a STANDBY IMMEDIATE command when
these haven't changed for a while, just use the STANDBY command itself
with the timeout value.

This avoids having to wake up the daemon every five seconds. In
addition, this change ensures that device specific minimums are
respected.

We didn't do this in the original commit because while testing the
feature, some of disks, notably ones from WD, appeared to not respect
the STANDBY command. However, when moved to another enclosure (the
disks was initially in a SATA PM enclosure, it got moved to a SAS
enclosure) things started working again. Also turns out the WD disk in
question has a device specific timeout of 10 minutes.

doc/man/devkit-disks.xml
src/Makefile.am
src/devkit-disks-daemon.c
src/devkit-disks-device-private.h
src/devkit-disks-device.c
src/job-drive-standby.c [deleted file]

index 31158be..bd1137b 100644 (file)
 
   <refsect1 id="devkit-disks-spindown"><title>SPINNING DOWN DISKS</title>
     <para>
-      Caution should be exercised when configuring disk spin down.
+      Caution should be exercised when configuring disk spin down timeouts.
     </para>
     <para>
       Note that every time a disk is spun down,
       the <quote>spin-up-time</quote> ATA SMART attribute.
     </para>
     <para>
-      For maximum compatibility (since disks from WD and others won't
-      spin down using the <quote>STANDBY</quote> ATA command),
-      <citerefentry>
-        <refentrytitle>devkit-disks-daemon</refentrytitle><manvolnum>8</manvolnum>
-      </citerefentry>
-      does all timeout handling on the host side. If a device appears
-      to be idle (this is determined by looking
-      at <filename>/sys/block/sdX/stat</filename> from time to time)
-      for the requested amount of time, the daemon will spin the disk
-      down using the <quote>STANDBY IMMEDIATE</quote> ATA COMMAND or
-      similar.
-    </para>
-    <para>
       On the other hand, cautious use (e.g. using conservative
       timeouts) of the ability to spin down disks, can be a good way
       to trade power consumption (typically 8 vs 1 Watts for 3.5&#34;
index 1c5ff99..42f21fe 100644 (file)
@@ -93,7 +93,6 @@ libexec_PROGRAMS += devkit-disks-helper-mkfs                          \
                    devkit-disks-helper-drive-detach                    \
                    devkit-disks-helper-drive-poll                      \
                    devkit-disks-helper-linux-md-check                  \
-                   devkit-disks-helper-drive-standby                   \
                    $(NULL)
 
 libexec_SCRIPTS = devkit-disks-helper-change-luks-password
@@ -146,10 +145,6 @@ devkit_disks_helper_linux_md_check_SOURCES = job-shared.h job-linux-md-check.c
 devkit_disks_helper_linux_md_check_CPPFLAGS = $(AM_CPPFLAGS)
 devkit_disks_helper_linux_md_check_LDADD = $(GLIB_LIBS)
 
-devkit_disks_helper_drive_standby_SOURCES = job-shared.h job-drive-standby.c
-devkit_disks_helper_drive_standby_CPPFLAGS = $(AM_CPPFLAGS) $(SGUTILS_CFLAGS)
-devkit_disks_helper_drive_standby_LDADD = $(SGUTILS_LIBS) $(GLIB_LIBS)
-
 devkit_disks_helper_drive_poll_SOURCES = job-shared.h job-drive-poll.c
 devkit_disks_helper_drive_poll_CPPFLAGS = $(AM_CPPFLAGS)
 devkit_disks_helper_drive_poll_LDADD =  $(GLIB_LIBS)
index 4873955..0c72601 100644 (file)
@@ -36,9 +36,6 @@
 /* delete entries older than five days */
 #define ATA_SMART_KEEP_ENTRIES_SECONDS (5*24*60*60)
 
-/* the poll frequency for IO activity when clients wants to spin down drives */
-#define SPINDOWN_POLL_FREQ_SECONDS 5
-
 /* ---------------------------------------------------------------------------------------------------- */
 
 #include <stdlib.h>
@@ -129,7 +126,6 @@ struct DevkitDisksDaemonPrivate
 
         GList *inhibitors;
 
-        guint spindown_timeout_id;
         GList *spindown_inhibitors;
 };
 
@@ -607,10 +603,6 @@ devkit_disks_daemon_finalize (GObject *object)
                 g_source_remove (daemon->priv->ata_smart_refresh_timer_id);
         }
 
-        if (daemon->priv->spindown_timeout_id > 0) {
-                g_source_remove (daemon->priv->spindown_timeout_id);
-        }
-
         for (l = daemon->priv->polling_inhibitors; l != NULL; l = l->next) {
                 DevkitDisksInhibitor *inhibitor = DEVKIT_DISKS_INHIBITOR (l->data);
                 g_signal_handlers_disconnect_by_func (inhibitor, daemon_polling_inhibitor_disconnected_cb, daemon);
@@ -780,7 +772,7 @@ device_remove (DevkitDisksDaemon *daemon, GUdevDevice *d)
 static void
 on_uevent (GUdevClient  *client,
            const char   *action,
-           GUdevDevice *device,
+           GUdevDevice  *device,
            gpointer      user_data)
 {
         DevkitDisksDaemon *daemon = DEVKIT_DISKS_DAEMON (user_data);
@@ -1452,12 +1444,14 @@ devkit_disks_daemon_local_check_auth (DevkitDisksDaemon            *daemon,
 #define SYSFS_BLOCK_STAT_MAX_SIZE 256
 
 static void
-issue_standby_child_watch_cb (GPid pid, int status, gpointer user_data)
+disk_set_standby_timeout_child_watch_cb (GPid     pid,
+                                         gint     status,
+                                         gpointer user_data)
 {
         DevkitDisksDevice *device = DEVKIT_DISKS_DEVICE (user_data);
 
         if (WIFEXITED (status) && WEXITSTATUS (status) == 0) {
-                //g_print ("**** NOTE: standby helper for %s completed successfully\n", device->priv->device_file);
+                g_print ("**** NOTE: standby helper for %s completed successfully\n", device->priv->device_file);
         } else {
                 g_warning ("standby helper for %s failed with exit code %d (if_exited=%d)\n",
                            device->priv->device_file,
@@ -1469,15 +1463,34 @@ issue_standby_child_watch_cb (GPid pid, int status, gpointer user_data)
 }
 
 static void
-issue_standby_to_drive (DevkitDisksDevice *device)
+disk_set_standby_timeout (DevkitDisksDevice *device)
 {
         GError *error;
         GPid pid;
-        gchar *argv[3] = {PACKAGE_LIBEXEC_DIR "/devkit-disks-helper-drive-standby",
-                          NULL, /* device_file */
+        gint value;
+        gchar *argv[5] = {"hdparm",
+                          "-S",
+                          NULL, /* argv[2]: timeout value */
+                          NULL, /* argv[3]: device_file */
                           NULL};
 
-        argv[1] = device->priv->device_file;
+        if (device->priv->spindown_timeout == 0) {
+                value = 0;
+        } else if (device->priv->spindown_timeout <= 240*5) {
+                /* 1...240 are blocks of 5 secs */
+                value = device->priv->spindown_timeout / 5;
+        } else if (device->priv->spindown_timeout <= (5*60 + 30)*60) {
+                /* 241...251 are blocks of 30 minutes */
+                value = device->priv->spindown_timeout / (30 * 60) + 240;
+                if (value == 240)
+                        value = 241;
+        } else {
+                /* so max timeout is 5.5 hours (252, 253, 244, 255 are vendor-specific / uninteresting) */
+                value = 251;
+        }
+
+        argv[2] = g_strdup_printf ("%d", value);
+        argv[3] = device->priv->device_file;
 
         error = NULL;
         if (!g_spawn_async_with_pipes (NULL,
@@ -1496,88 +1509,10 @@ issue_standby_to_drive (DevkitDisksDevice *device)
                 goto out;
         }
 
-        g_child_watch_add (pid, issue_standby_child_watch_cb, g_object_ref (device));
+        g_child_watch_add (pid, disk_set_standby_timeout_child_watch_cb, g_object_ref (device));
 
  out:
-        ;
-}
-
-static gboolean
-on_spindown_timeout (gpointer data)
-{
-        DevkitDisksDaemon *daemon = DEVKIT_DISKS_DAEMON (data);
-        GHashTableIter hash_iter;
-        DevkitDisksDevice *device;
-        gchar stat_file_path[PATH_MAX];
-        gchar buf[SYSFS_BLOCK_STAT_MAX_SIZE];
-        int fd;
-        time_t now;
-
-        now = time (NULL);
-
-        /* avoid allocating memory since this happens on a timer, e.g. all the FRAKKING time */
-
-        g_hash_table_iter_init (&hash_iter, daemon->priv->map_object_path_to_device);
-        while (g_hash_table_iter_next (&hash_iter, NULL, (gpointer) &device)) {
-
-                if (!device->priv->device_is_drive ||
-                    !device->priv->drive_can_spindown ||
-                    device->priv->spindown_timeout == 0)
-                        continue;
-
-                g_snprintf (stat_file_path,
-                            sizeof stat_file_path,
-                            "%s/stat",
-                            device->priv->native_path);
-                fd = open (stat_file_path, O_RDONLY);
-                if (fd == -1) {
-                        g_warning ("Error opening %s: %m", stat_file_path);
-                } else {
-                        ssize_t bytes_read;
-                        bytes_read = read (fd, buf, sizeof buf - 1);
-                        close (fd);
-                        if (bytes_read < 0) {
-                                g_warning ("Error reading from %s: %m", stat_file_path);
-                        } else {
-                                buf[bytes_read] = '\0';
-                                if (device->priv->spindown_last_stat == NULL) {
-                                        /* handle initial time this is set */
-                                        device->priv->spindown_last_stat = g_new0 (gchar, SYSFS_BLOCK_STAT_MAX_SIZE);
-                                        memcpy (device->priv->spindown_last_stat, buf, bytes_read + 1);
-                                        device->priv->spindown_last_stat_time = now;
-                                        device->priv->spindown_have_issued_standby = FALSE;
-                                } else {
-                                        if (g_strcmp0 (buf, device->priv->spindown_last_stat) == 0) {
-                                                gint64 idle_secs;
-
-                                                idle_secs = now - device->priv->spindown_last_stat_time;
-                                                /* same */
-                                                if (idle_secs > device->priv->spindown_timeout &&
-                                                    !device->priv->spindown_have_issued_standby) {
-                                                        device->priv->spindown_have_issued_standby = TRUE;
-                                                        g_print ("*** NOTE: issuing STANDBY IMMEDIATE for %s\n",
-                                                                 device->priv->device_file);
-
-                                                        /* and, now, DO IT! */
-                                                        issue_standby_to_drive (device);
-                                                }
-                                        } else {
-                                                /* differ */
-                                                memcpy (device->priv->spindown_last_stat, buf, bytes_read + 1);
-                                                device->priv->spindown_last_stat_time = now;
-                                                device->priv->spindown_have_issued_standby = FALSE;
-                                                //g_print ("*** NOTE: resetting spindown timeout on %s due "
-                                                //         "to IO activity\n",
-                                                //         device->priv->device_file);
-                                        }
-                                }
-                        }
-                }
-
-        }
-
-        /* keep timeout */
-        return TRUE;
+        g_free (argv[2]);
 }
 
 void
@@ -1586,9 +1521,7 @@ devkit_disks_daemon_local_update_spindown (DevkitDisksDaemon *daemon)
         GHashTableIter hash_iter;
         DevkitDisksDevice *device;
         GList *l;
-        gboolean watch_for_spindown;
 
-        watch_for_spindown = FALSE;
         g_hash_table_iter_init (&hash_iter, daemon->priv->map_object_path_to_device);
         while (g_hash_table_iter_next (&hash_iter, NULL, (gpointer) &device)) {
                 gint spindown_timeout;
@@ -1596,15 +1529,13 @@ devkit_disks_daemon_local_update_spindown (DevkitDisksDaemon *daemon)
                 if (!device->priv->device_is_drive || !device->priv->drive_can_spindown)
                         continue;
 
-                spindown_timeout = G_MAXINT;
+                spindown_timeout = 0;
                 if (device->priv->spindown_inhibitors == NULL && daemon->priv->spindown_inhibitors == NULL) {
                         /* no inhibitors */
-                        device->priv->spindown_timeout = 0;
-                        g_free (device->priv->spindown_last_stat);
-                        device->priv->spindown_last_stat = NULL;
-                        device->priv->spindown_last_stat_time = 0;
-                        device->priv->spindown_have_issued_standby = FALSE;
                 } else {
+
+                        spindown_timeout = G_MAXINT;
+
                         /* first go through all inhibitors on the device */
                         for (l = device->priv->spindown_inhibitors; l != NULL; l = l->next) {
                                 DevkitDisksInhibitor *inhibitor = DEVKIT_DISKS_INHIBITOR (l->data);
@@ -1612,38 +1543,30 @@ devkit_disks_daemon_local_update_spindown (DevkitDisksDaemon *daemon)
 
                                 spindown_timeout_inhibitor = GPOINTER_TO_INT (g_object_get_data (G_OBJECT (inhibitor),
                                                                                                  "spindown-timeout-seconds"));
+                                g_warn_if_fail (spindown_timeout_inhibitor > 0);
 
                                 if (spindown_timeout_inhibitor < spindown_timeout)
                                         spindown_timeout = spindown_timeout_inhibitor;
                         }
 
-                        /* the all inhibitors on the daemon */
+                        /* then all inhibitors on the daemon */
                         for (l = daemon->priv->spindown_inhibitors; l != NULL; l = l->next) {
                                 DevkitDisksInhibitor *inhibitor = DEVKIT_DISKS_INHIBITOR (l->data);
                                 gint spindown_timeout_inhibitor;
 
                                 spindown_timeout_inhibitor = GPOINTER_TO_INT (g_object_get_data (G_OBJECT (inhibitor),
                                                                                                  "spindown-timeout-seconds"));
+                                g_warn_if_fail (spindown_timeout_inhibitor > 0);
 
                                 if (spindown_timeout_inhibitor < spindown_timeout)
                                         spindown_timeout = spindown_timeout_inhibitor;
                         }
-
-                        device->priv->spindown_timeout = spindown_timeout;
-                        watch_for_spindown = TRUE;
                 }
-        }
 
-        if (watch_for_spindown) {
-                if (daemon->priv->spindown_timeout_id == 0) {
-                        daemon->priv->spindown_timeout_id = g_timeout_add_seconds (SPINDOWN_POLL_FREQ_SECONDS,
-                                                                                   on_spindown_timeout,
-                                                                                   daemon);
-                }
-        } else {
-                if (daemon->priv->spindown_timeout_id > 0) {
-                        g_source_remove (daemon->priv->spindown_timeout_id);
-                        daemon->priv->spindown_timeout_id = 0;
+                if (device->priv->spindown_timeout != spindown_timeout) {
+                        device->priv->spindown_timeout = spindown_timeout;
+                        /* just assume this always works... */
+                        disk_set_standby_timeout (device);
                 }
         }
 }
index 2bbc1f8..cb7d4c6 100644 (file)
@@ -87,21 +87,36 @@ struct DevkitDisksDevicePrivate
         /* A list of current polling inhibitors (DevkitDisksInhibitor objects) */
         GList *polling_inhibitors;
 
-        /* A list of current spindown configurators (DevkitDisksInhibitor objects) */
-        GList *spindown_inhibitors;
-
         /* if non-zero, the id of the idle for emitting a 'change' signal */
         guint emit_changed_idle_id;
 
-        /* used for spindown */
+        /*****************/
+        /* Disk spindown */
+        /*****************/
+
+        /* A list of current spindown configurators (DevkitDisksInhibitor objects)
+         *
+         * Each object will have a data element, @spindown-timeout-seconds, that is
+         * the requested timeout for the inhibitor in question. It can be read via
+         *
+         *  GPOINTER_TO_INT (g_object_get_data (G_OBJECT (inhibitor), "spindown-timeout-seconds"));
+         */
+        GList *spindown_inhibitors;
+
+        /* The timeout the disk is currently configured with, in seconds. This is 0 if spindown
+         * is not enabled. Depending on the command-set used, a slightly different rounded value
+         * may have been sent to the disk - for example, the ATA command-set has a rather peculiar
+         * mapping, see the hdparm(1) man-page, option -S.
+         *
+         * This value is computed by considering all per-disk spindown inhibitors (set
+         * via the DriveSetSpindownTimeout() method on the device) and all global spindown
+         * inhibitors (set via the DriveSetAllSpindownTimeouts() method on the daemon).
+         */
         gint spindown_timeout;
-        gchar *spindown_last_stat;
-        time_t spindown_last_stat_time;
-        gboolean spindown_have_issued_standby;
 
         /**************/
-        /* properties */
-        /*************/
+        /* Properties */
+        /**************/
 
         char *device_file;
         dev_t dev;
index 7468f95..000ba5a 100644 (file)
@@ -1259,8 +1259,6 @@ devkit_disks_device_finalize (GObject *object)
         if (device->priv->emit_changed_idle_id > 0)
                 g_source_remove (device->priv->emit_changed_idle_id);
 
-        g_free (device->priv->spindown_last_stat);
-
         /* free properties */
         g_free (device->priv->device_file);
         g_ptr_array_foreach (device->priv->device_file_by_id, (GFunc) g_free, NULL);
diff --git a/src/job-drive-standby.c b/src/job-drive-standby.c
deleted file mode 100644 (file)
index 56997f9..0000000
+++ /dev/null
@@ -1,103 +0,0 @@
-/* -*- Mode: C; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
- *
- * Copyright (C) 2009 David Zeuthen <david@fubar.dk>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
- *
- */
-
-#include <stdio.h>
-#include <string.h>
-#include <errno.h>
-#include <stdlib.h>
-#include <time.h>
-
-#include <scsi/sg_lib.h>
-#include <scsi/sg_cmds.h>
-
-#include <glib.h>
-
-static void
-usage (void)
-{
-  g_printerr ("incorrect usage\n");
-}
-
-int
-main (int argc, char *argv[])
-{
-  int rc;
-  int ret;
-  int sg_fd;
-  const gchar *device;
-
-  ret = 1;
-  sg_fd = -1;
-
-  if (argc != 2)
-    {
-      usage ();
-      goto out;
-    }
-
-  device = argv[1];
-
-  sg_fd = sg_cmds_open_device (device, 1 /* read_only */, 1);
-  if (sg_fd < 0)
-    {
-      g_printerr ("Cannot open %s: %m\n", device);
-      goto out;
-    }
-
-  if ((rc = sg_ll_sync_cache_10 (sg_fd,
-                                 0,  /* sync_nv */
-                                 0,  /* immed */
-                                 0,  /* group */
-                                 0,  /* lba */
-                                 0,  /* count */
-                                 1,  /* noisy */
-                                 0   /* verbose */
-                                 )) != 0)
-    {
-      g_printerr ("Error SYNCHRONIZE CACHE for %s: %s\n", device, safe_strerror (rc));
-      /* this is not a catastrophe, carry on */
-    }
-
-  if (sg_ll_start_stop_unit (sg_fd,
-                             0,  /* immed */
-                             0,  /* pc_mod__fl_num */
-                             0,  /* power_cond */
-                             0,  /* noflush__fl */
-                             0,  /* loej */
-                             0,  /* start */
-                             1,  /* noisy */
-                             0   /* verbose */
-                             ) != 0)
-    {
-      g_printerr ("Error STOP UNIT for %s: %s\n", device, safe_strerror (rc));
-      goto out;
-    }
-
-  /* OK, close the device */
-  sg_cmds_close_device (sg_fd);
-  sg_fd = -1;
-
-  ret = 0;
-
- out:
-  if (sg_fd > 0)
-    sg_cmds_close_device (sg_fd);
-  return ret;
-}