Re: [PATCH v2 3/3] disk: read into cache directly

[Vladimir 'phcoder' Serbinenko]

Is there any way this patch reclaims unused memory in case of partial cache eviction? If not it could potentially water lots of memory. One thing you need to consider is that initrd for Solaris can easily be 60% of total RAM (300 MiB on 512MiB machine) What if requested read is bigger than risk cache size?

Le mer. 2 mars 2016 01:15, Leif Lindholm <address@hidden> a écrit :

From: Andrei Borzenkov <address@hidden>

<patch description>
---
 grub-core/kern/disk.c | 96 +++++++++++++++++++++++++++++++--------------------
 grub-core/lib/disk.c  |  6 ++--
 include/grub/disk.h   | 11 +++++-
 3 files changed, 71 insertions(+), 42 deletions(-)

diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c
index 60a920b..0acf660 100644
--- a/grub-core/kern/disk.c
+++ b/grub-core/kern/disk.c
@@ -56,6 +56,20 @@ grub_err_t (*grub_disk_write_weak) (grub_disk_t disk,
 #include "disk_common.c"

 void
+grub_disk_cache_free (struct grub_cache_buffer *buf)
+{
+  if (!buf->count)
+    /* FIXME This means corruption; what can we do? */
+    return;
+
+  if (!--buf->count)
+    {
+      grub_free (buf->data);
+      grub_free (buf);
+    }
+}
+
+void
 grub_disk_cache_invalidate_all (void)
 {
   unsigned i;
@@ -64,10 +78,10 @@ grub_disk_cache_invalidate_all (void)
     {
       struct grub_disk_cache *cache = grub_disk_cache_table + i;

-      if (cache->data && ! cache->lock)
+      if (cache->buffer && ! cache->lock)
        {
-         grub_free (cache->data);
-         cache->data = ""> +         grub_disk_cache_free (cache->buffer);
+         cache->buffer = 0;
        }
     }
 }
@@ -82,14 +96,14 @@ grub_disk_cache_fetch (unsigned long dev_id, unsigned long disk_id,
   cache_index = grub_disk_cache_get_index (dev_id, disk_id, sector);
   cache = grub_disk_cache_table + cache_index;

-  if (cache->dev_id == dev_id && cache->disk_id == disk_id
+  if (cache->buffer && cache->dev_id == dev_id && cache->disk_id == disk_id
       && cache->sector == sector)
     {
       cache->lock = 1;
 #if DISK_CACHE_STATS
       grub_disk_cache_hits++;
 #endif
-      return cache->data;
+      return cache->buffer->data + cache->offset;
     }

 #if DISK_CACHE_STATS
@@ -116,28 +130,36 @@ grub_disk_cache_unlock (unsigned long dev_id, unsigned long disk_id,

 static grub_err_t
 grub_disk_cache_store (unsigned long dev_id, unsigned long disk_id,
-                      grub_disk_addr_t sector, const char *data)
+                      grub_disk_addr_t sector, grub_disk_addr_t n, char *data)
 {
-  unsigned cache_index;
-  struct grub_disk_cache *cache;
+  struct grub_cache_buffer *buf;
+  grub_addr_t offset;

-  cache_index = grub_disk_cache_get_index (dev_id, disk_id, sector);
-  cache = grub_disk_cache_table + cache_index;
+  buf = grub_malloc (sizeof (*buf));
+  if (! buf)
+    return grub_errno;
+  buf->data = ""> +  buf->count = 0;

-  cache->lock = 1;
-  grub_free (cache->data);
-  cache->data = ""> -  cache->lock = 0;
+  for (offset = 0 ; n > 0; sector += GRUB_DISK_CACHE_SIZE, offset += (GRUB_DISK_CACHE_SIZE * GRUB_DISK_SECTOR_SIZE), n--)
+    {
+      unsigned cache_index;
+      struct grub_disk_cache *cache;

-  cache->data = "" (GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS);
-  if (! cache->data)
-    return grub_errno;
+      cache_index = grub_disk_cache_get_index (dev_id, disk_id, sector);
+      cache = grub_disk_cache_table + cache_index;

-  grub_memcpy (cache->data, data,
-              GRUB_DISK_SECTOR_SIZE << GRUB_DISK_CACHE_BITS);
-  cache->dev_id = dev_id;
-  cache->disk_id = disk_id;
-  cache->sector = sector;
+      cache->lock = 1;
+      if (cache->buffer)
+       grub_disk_cache_free (cache->buffer);
+      cache->buffer = buf;
+      cache->offset = offset;
+      buf->count++;
+      cache->lock = 0;
+      cache->dev_id = dev_id;
+      cache->disk_id = disk_id;
+      cache->sector = sector;
+    }

   return GRUB_ERR_NONE;
 }
@@ -357,13 +379,11 @@ grub_disk_read_small_real (grub_disk_t disk, grub_disk_addr_t sector,
          /* Copy it and store it in the disk cache.  */
          grub_memcpy (buf, tmp_buf + offset, size);
          grub_disk_cache_store (disk->dev->id, disk->id,
-                                sector, tmp_buf);
-         grub_free (tmp_buf);
+                                sector, 1, tmp_buf);
          return GRUB_ERR_NONE;
        }
     }

-  grub_free (tmp_buf);
   grub_errno = GRUB_ERR_NONE;

   {
@@ -380,10 +400,6 @@ grub_disk_read_small_real (grub_disk_t disk, grub_disk_addr_t sector,
     num = ((size + offset + (1ULL << (disk->log_sector_size))
            - 1) >> (disk->log_sector_size));

-    tmp_buf = disk->malloc (disk, num << disk->log_sector_size);
-    if (!tmp_buf)
-      return grub_errno;
-
     if ((disk->dev->read) (disk, transform_sector (disk, aligned_sector),
                           num, tmp_buf))
       {
@@ -488,22 +504,26 @@ grub_disk_read (grub_disk_t disk, grub_disk_addr_t sector,

       if (agglomerate)
        {
-         grub_disk_addr_t i;
+         void *cache = disk->malloc (disk, agglomerate << (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS));
+
+         if (!cache)
+           return grub_errno;

          err = (disk->dev->read) (disk, transform_sector (disk, sector),
                                   agglomerate << (GRUB_DISK_CACHE_BITS
                                                   + GRUB_DISK_SECTOR_BITS
                                                   - disk->log_sector_size),
-                                  buf);
+                                  cache);
          if (err)
-           return err;
+           {
+             grub_free (cache);
+             return err;
+           }

-         for (i = 0; i < agglomerate; i ++)
-           grub_disk_cache_store (disk->dev->id, disk->id,
-                                  sector + (i << GRUB_DISK_CACHE_BITS),
-                                  (char *) buf
-                                  + (i << (GRUB_DISK_CACHE_BITS
-                                           + GRUB_DISK_SECTOR_BITS)));
+         grub_memcpy (buf, cache,
+                      agglomerate << (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS));
+         grub_disk_cache_store (disk->dev->id, disk->id,
+                                sector, agglomerate, cache);


          if (disk->read_hook)
diff --git a/grub-core/lib/disk.c b/grub-core/lib/disk.c
index 0f18688..07fb117 100644
--- a/grub-core/lib/disk.c
+++ b/grub-core/lib/disk.c
@@ -42,11 +42,11 @@ grub_disk_cache_invalidate (unsigned long dev_id, unsigned long disk_id,
   cache = grub_disk_cache_table + cache_index;

   if (cache->dev_id == dev_id && cache->disk_id == disk_id
-      && cache->sector == sector && cache->data)
+      && cache->sector == sector && cache->buffer)
     {
       cache->lock = 1;
-      grub_free (cache->data);
-      cache->data = ""> +      grub_disk_cache_free (cache->buffer);
+      cache->buffer = 0;
       cache->lock = 0;
     }
 }
diff --git a/include/grub/disk.h b/include/grub/disk.h
index 0fdd779..bbb7830 100644
--- a/include/grub/disk.h
+++ b/include/grub/disk.h
@@ -238,16 +238,25 @@ grub_stop_disk_firmware (void)
     }
 }

+struct grub_cache_buffer
+  {
+    char *data;
+    unsigned count;
+  };
+
 /* Disk cache.  */
 struct grub_disk_cache
 {
   enum grub_disk_dev_id dev_id;
   unsigned long disk_id;
   grub_disk_addr_t sector;
-  char *data;
+  struct grub_cache_buffer *buffer;
+  grub_addr_t offset;
   int lock;
 };

+void EXPORT_FUNC(grub_disk_cache_free) (struct grub_cache_buffer *buf);
+
 extern struct grub_disk_cache EXPORT_VAR(grub_disk_cache_table)[GRUB_DISK_CACHE_NUM];

 #if defined (GRUB_UTIL)
--
2.1.4


_______________________________________________
Grub-devel mailing list
address@hidden
https://lists.gnu.org/mailman/listinfo/grub-devel