[PATCH] Optimise hostdisk device handling

[Colin Watson]

grub-probe filesystem reads are seriously slow right now:

  $ sudo time ./grub-probe -t fs /usr/share/grub/unicode.pf2
  ext2
  0.12user 3.27system 0:11.75elapsed 28%CPU (0avgtext+0avgdata 
24320maxresident)k
  43592inputs+8outputs (0major+1572minor)pagefaults 0swaps

This is a serious problem when running update-grub, which of course runs
grub-probe several times.  It's essentially due to the way hostdisk
opens and closes devices all the time, wasting lots of expensive
syscalls and probably defeating buffer caching into the bargain.  With
this patch, consecutive reads from the same device don't need to reopen
that device, speeding it up by an order of magnitude:

  $ sudo time ./grub-probe -t fs /usr/share/grub/unicode.pf2
  ext2
  0.06user 0.05system 0:00.71elapsed 15%CPU (0avgtext+0avgdata 
24304maxresident)k
  11120inputs+8outputs (0major+1571minor)pagefaults 0swaps

After review, is there any chance at all that this could get into 1.98?
I think I'm going to end up applying something like this in Ubuntu 10.04
anyway, but obviously I'd rather have it upstream if possible.


2010-03-03  Colin Watson  <address@hidden>

        * util/hostdisk.c (struct grub_util_biosdisk_data): New structure.
        (grub_util_biosdisk_open): Initialise disk->data.
        (struct linux_partition_cache): New structure.
        (linux_find_partition): Cache partition start positions; these are
        expensive to compute on every read and write.
        (open_device): Cache open file descriptor in disk->data, so that we
        don't have to reopen it and flush the buffer cache for consecutive
        operations on the same device.
        (grub_util_biosdisk_close): New function.
        (grub_util_biosdisk_dev): Set `close' member.

        * conf/common.rmk (grub_probe_SOURCES): Add kern/list.c.
        * conf/i386-efi.rmk (grub_setup_SOURCES): Likewise.
        * conf/i386-pc.rmk (grub_setup_SOURCES): Likewise.
        * conf/sparc64-ieee1275.rmk (grub_setup_SOURCES): Likewise.
        * conf/x86_64-efi.rmk (grub_setup_SOURCES): Likewise.

=== modified file 'conf/common.rmk'
--- conf/common.rmk     2010-02-25 14:10:18 +0000
+++ conf/common.rmk     2010-03-03 09:44:19 +0000
@@ -24,7 +24,7 @@ util/grub-probe.c_DEPENDENCIES = grub_pr
 grub_probe_SOURCES = gnulib/progname.c util/grub-probe.c       \
        util/hostdisk.c util/misc.c util/getroot.c              \
        kern/device.c kern/disk.c kern/err.c kern/misc.c        \
-       kern/parser.c kern/partition.c kern/file.c              \
+       kern/parser.c kern/partition.c kern/file.c kern/list.c  \
        \
        fs/affs.c fs/cpio.c fs/fat.c fs/ext2.c fs/hfs.c         \
        fs/hfsplus.c fs/iso9660.c fs/udf.c fs/jfs.c fs/minix.c  \

=== modified file 'conf/i386-efi.rmk'
--- conf/i386-efi.rmk   2010-01-20 20:31:39 +0000
+++ conf/i386-efi.rmk   2010-03-03 09:44:31 +0000
@@ -21,7 +21,7 @@ util/i386/efi/grub-mkimage.c_DEPENDENCIE
 #      kern/err.c kern/misc.c fs/fat.c fs/ext2.c fs/xfs.c fs/affs.c    \
 #      fs/sfs.c kern/parser.c kern/partition.c partmap/msdos.c         \
 #      fs/ufs.c fs/ufs2.c fs/minix.c fs/hfs.c fs/jfs.c fs/hfsplus.c 
kern/file.c        \
-#      kern/fs.c kern/env.c fs/fshelp.c
+#      kern/fs.c kern/env.c kern/list.c fs/fshelp.c
 
 # Scripts.
 sbin_SCRIPTS = grub-install

=== modified file 'conf/i386-pc.rmk'
--- conf/i386-pc.rmk    2010-02-03 00:24:07 +0000
+++ conf/i386-pc.rmk    2010-03-03 09:44:54 +0000
@@ -96,7 +96,8 @@ grub_setup_SOURCES = gnulib/progname.c \
        util/i386/pc/grub-setup.c util/hostdisk.c       \
        util/misc.c util/getroot.c kern/device.c kern/disk.c    \
        kern/err.c kern/misc.c kern/parser.c kern/partition.c   \
-       kern/file.c kern/fs.c kern/env.c fs/fshelp.c            \
+       kern/file.c kern/fs.c kern/env.c kern/list.c            \
+       fs/fshelp.c                                             \
        \
        fs/affs.c fs/cpio.c fs/ext2.c fs/fat.c fs/hfs.c         \
        fs/hfsplus.c fs/iso9660.c fs/udf.c fs/jfs.c fs/minix.c  \

=== modified file 'conf/sparc64-ieee1275.rmk'
--- conf/sparc64-ieee1275.rmk   2010-01-20 20:31:39 +0000
+++ conf/sparc64-ieee1275.rmk   2010-03-03 09:45:17 +0000
@@ -70,7 +70,8 @@ util/sparc64/ieee1275/grub-setup.c_DEPEN
 grub_setup_SOURCES = util/sparc64/ieee1275/grub-setup.c util/hostdisk.c        
\
        util/misc.c util/getroot.c kern/device.c kern/disk.c    \
        kern/err.c kern/misc.c kern/parser.c kern/partition.c   \
-       kern/file.c kern/fs.c kern/env.c fs/fshelp.c            \
+       kern/file.c kern/fs.c kern/env.c kern/list.c            \
+       fs/fshelp.c                                             \
        \
        fs/affs.c fs/cpio.c fs/ext2.c fs/fat.c fs/hfs.c         \
        fs/hfsplus.c fs/iso9660.c fs/udf.c fs/jfs.c fs/minix.c  \

=== modified file 'conf/x86_64-efi.rmk'
--- conf/x86_64-efi.rmk 2010-01-20 20:31:39 +0000
+++ conf/x86_64-efi.rmk 2010-03-03 09:45:29 +0000
@@ -20,7 +20,7 @@ grub_mkimage_SOURCES = gnulib/progname.c
 #      kern/err.c kern/misc.c fs/fat.c fs/ext2.c fs/xfs.c fs/affs.c    \
 #      fs/sfs.c kern/parser.c kern/partition.c partmap/msdos.c         \
 #      fs/ufs.c fs/ufs2.c fs/minix.c fs/hfs.c fs/jfs.c fs/hfsplus.c 
kern/file.c        \
-#      kern/fs.c kern/env.c fs/fshelp.c
+#      kern/fs.c kern/env.c kern/list.c fs/fshelp.c
 
 # Scripts.
 sbin_SCRIPTS = grub-install

=== modified file 'util/hostdisk.c'
--- util/hostdisk.c     2010-02-07 01:47:18 +0000
+++ util/hostdisk.c     2010-03-03 10:05:41 +0000
@@ -26,6 +26,7 @@
 #include <grub/util/hostdisk.h>
 #include <grub/misc.h>
 #include <grub/i18n.h>
+#include <grub/list.h>
 
 #include <stdio.h>
 #include <stdlib.h>
@@ -103,6 +104,12 @@ struct
   char *device;
 } map[256];
 
+struct grub_util_biosdisk_data
+{
+  char *dev;
+  int fd;
+};
+
 #ifdef __linux__
 /* Check if we have devfs support.  */
 static int
@@ -165,6 +172,7 @@ grub_util_biosdisk_open (const char *nam
 {
   int drive;
   struct stat st;
+  struct grub_util_biosdisk_data *data;
 
   drive = find_grub_drive (name);
   if (drive < 0)
@@ -173,6 +181,9 @@ grub_util_biosdisk_open (const char *nam
 
   disk->has_partitions = 1;
   disk->id = drive;
+  disk->data = data = xmalloc (sizeof (struct grub_util_biosdisk_data));
+  data->dev = NULL;
+  data->fd = -1;
 
   /* Get the size.  */
 #if defined(__MINGW32__)
@@ -254,6 +265,17 @@ grub_util_biosdisk_open (const char *nam
 }
 
 #ifdef __linux__
+/* Cache of partition start sectors for each disk.  */
+struct linux_partition_cache
+{
+  struct linux_partition_cache *next;
+  char *dev;
+  unsigned long start;
+  int partno;
+};
+
+struct linux_partition_cache *linux_partition_cache_list;
+
 static int
 linux_find_partition (char *dev, unsigned long sector)
 {
@@ -262,6 +284,7 @@ linux_find_partition (char *dev, unsigne
   char *p;
   int i;
   char real_dev[PATH_MAX];
+  struct linux_partition_cache *cache;
 
   strcpy(real_dev, dev);
 
@@ -281,6 +304,16 @@ linux_find_partition (char *dev, unsigne
       format = "%d";
     }
 
+  for (cache = linux_partition_cache_list; cache; cache = cache->next)
+    {
+      if (strcmp (cache->dev, dev) == 0 && cache->start == sector)
+       {
+         sprintf (p, format, cache->partno);
+         strcpy (dev, real_dev);
+         return 1;
+       }
+    }
+
   for (i = 1; i < 10000; i++)
     {
       int fd;
@@ -301,6 +334,15 @@ linux_find_partition (char *dev, unsigne
 
       if (hdg.start == sector)
        {
+         struct linux_partition_cache *new_cache_item;
+
+         new_cache_item = xmalloc (sizeof *new_cache_item);
+         new_cache_item->dev = xstrdup (dev);
+         new_cache_item->start = hdg.start;
+         new_cache_item->partno = i;
+         grub_list_push (GRUB_AS_LIST_P (&linux_partition_cache_list),
+                         GRUB_AS_LIST (new_cache_item));
+
          strcpy (dev, real_dev);
          return 1;
        }
@@ -314,6 +356,7 @@ static int
 open_device (const grub_disk_t disk, grub_disk_addr_t sector, int flags)
 {
   int fd;
+  struct grub_util_biosdisk_data *data = disk->data;
 
 #ifdef O_LARGEFILE
   flags |= O_LARGEFILE;
@@ -340,18 +383,30 @@ open_device (const grub_disk_t disk, gru
        && strncmp (map[disk->id].device, "/dev/", 5) == 0)
       is_partition = linux_find_partition (dev, disk->partition->start);
 
-    /* Open the partition.  */
-    grub_dprintf ("hostdisk", "opening the device `%s' in open_device()\n", 
dev);
-    fd = open (dev, flags);
-    if (fd < 0)
+    if (data->dev && strcmp (data->dev, dev) == 0)
       {
-       grub_error (GRUB_ERR_BAD_DEVICE, "cannot open `%s'", dev);
-       return -1;
+       grub_dprintf ("hostdisk", "reusing open device `%s'\n", dev);
+       fd = data->fd;
       }
+    else
+      {
+       /* Open the partition.  */
+       grub_dprintf ("hostdisk", "opening the device `%s' in open_device()\n", 
dev);
+       fd = open (dev, flags);
+       if (fd < 0)
+         {
+           grub_error (GRUB_ERR_BAD_DEVICE, "cannot open `%s'", dev);
+           return -1;
+         }
 
-    /* Flush the buffer cache to the physical disk.
-       XXX: This also empties the buffer cache.  */
-    ioctl (fd, BLKFLSBUF, 0);
+       /* Flush the buffer cache to the physical disk.
+          XXX: This also empties the buffer cache.  */
+       ioctl (fd, BLKFLSBUF, 0);
+
+       free (data->dev);
+       data->dev = xstrdup (dev);
+       data->fd = fd;
+      }
 
     if (is_partition)
       sector -= disk->partition->start;
@@ -375,7 +430,21 @@ open_device (const grub_disk_t disk, gru
     }
 #endif
 
-  fd = open (map[disk->id].device, flags);
+  if (data->dev && strcmp (data->dev, map[disk->id].device) == 0)
+    {
+      grub_dprintf ("hostdisk", "reusing open device `%s'\n", data->dev);
+      fd = data->fd;
+    }
+  else
+    {
+      fd = open (map[disk->id].device, flags);
+      if (fd >= 0)
+       {
+         free (data->dev);
+         data->dev = xstrdup (map[disk->id].device);
+         data->fd = fd;
+       }
+    }
 
 #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
   if (! (sysctl_oldflags & 0x10)
@@ -535,7 +604,6 @@ grub_util_biosdisk_read (grub_disk_t dis
       != (ssize_t) (size << GRUB_DISK_SECTOR_BITS))
     grub_error (GRUB_ERR_READ_ERROR, "cannot read from `%s'", 
map[disk->id].device);
 
-  close (fd);
   return grub_errno;
 }
 
@@ -570,17 +638,27 @@ grub_util_biosdisk_write (grub_disk_t di
       != (ssize_t) (size << GRUB_DISK_SECTOR_BITS))
     grub_error (GRUB_ERR_WRITE_ERROR, "cannot write to `%s'", 
map[disk->id].device);
 
-  close (fd);
   return grub_errno;
 }
 
+static void
+grub_util_biosdisk_close (struct grub_disk *disk)
+{
+  struct grub_util_biosdisk_data *data = disk->data;
+
+  free (data->dev);
+  if (data->fd != -1)
+    close (data->fd);
+  free (data);
+}
+
 static struct grub_disk_dev grub_util_biosdisk_dev =
   {
     .name = "biosdisk",
     .id = GRUB_DISK_DEVICE_BIOSDISK_ID,
     .iterate = grub_util_biosdisk_iterate,
     .open = grub_util_biosdisk_open,
-    .close = 0,
+    .close = grub_util_biosdisk_close,
     .read = grub_util_biosdisk_read,
     .write = grub_util_biosdisk_write,
     .next = 0


Thanks,

-- 
Colin Watson                                       address@hidden