Re: [PATCH] Optimise hostdisk device handling

[Colin Watson]

On IRC, Vladimir expressed enthusiasm, but also concerns about possible
problems with cache coherency as a result of this patch, and would
prefer to leave it until post-1.98.  I can sympathise with this and
won't push it.

Here's an updated version that fixes a regression in grub-setup by
making sure to reopen the device when the access mode (O_RDONLY vs.
O_WRONLY) changes.  It also makes sure to close data->fd if it was
previously open, fixing a file descriptor leak.

This is now in my 'hostdisk-speedup' branch, and merged into
experimental.

=== added file 'ChangeLog.hostdisk-speedup'
--- ChangeLog.hostdisk-speedup  1970-01-01 00:00:00 +0000
+++ ChangeLog.hostdisk-speedup  2010-03-03 10:43:43 +0000
@@ -0,0 +1,18 @@
+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 10:42:45 +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 10:42:45 +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 10:42:45 +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 10:42:45 +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 10:42:45 +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 20:03:03 +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,13 @@ struct
   char *device;
 } map[256];
 
+struct grub_util_biosdisk_data
+{
+  char *dev;
+  int access_mode;
+  int fd;
+};
+
 #ifdef __linux__
 /* Check if we have devfs support.  */
 static int
@@ -165,6 +173,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 +182,10 @@ 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->access_mode = 0;
+  data->fd = -1;
 
   /* Get the size.  */
 #if defined(__MINGW32__)
@@ -254,6 +267,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 +286,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 +306,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 +336,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 +358,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 +385,35 @@ 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 &&
+       data->access_mode == (flags & O_ACCMODE))
       {
-       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
+      {
+       free (data->dev);
+       if (data->fd != -1)
+         close (data->fd);
+
+       /* 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);
+
+       data->dev = xstrdup (dev);
+       data->access_mode = (flags & O_ACCMODE);
+       data->fd = fd;
+      }
 
     if (is_partition)
       sector -= disk->partition->start;
@@ -375,7 +437,26 @@ 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 &&
+      data->access_mode == (flags & O_ACCMODE))
+    {
+      grub_dprintf ("hostdisk", "reusing open device `%s'\n", data->dev);
+      fd = data->fd;
+    }
+  else
+    {
+      free (data->dev);
+      if (data->fd != -1)
+       close (data->fd);
+
+      fd = open (map[disk->id].device, flags);
+      if (fd >= 0)
+       {
+         data->dev = xstrdup (map[disk->id].device);
+         data->access_mode = (flags & O_ACCMODE);
+         data->fd = fd;
+       }
+    }
 
 #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
   if (! (sysctl_oldflags & 0x10)
@@ -535,7 +616,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 +650,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

-- 
Colin Watson                                       address@hidden