Re: [PATCH 1/2] disk: Add support for device-specific malloc function

[Andrei Borzenkov <arvidjaar@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1331 bytes --]

24.02.2016 16:57, Leif Lindholm пишет:
> On Wed, Feb 24, 2016 at 03:09:13PM +0300, Andrei Borzenkov wrote:
>>>> Could you test attached patch with your alignment fixes on top. This
>>>> implements my idea of using shared buffers. Seems to work in naive testing.
>>>
>>> Testing this with a grub-mkstandalone image, I get:
>>>
>>> kern/dl.c:556: flushing 0x10f1 bytes at 0x9ffb5ac20
>>> kern/dl.c:649: module name: tar
>>> kern/dl.c:650: init function: 0x9ffb5b220
>>> kern/disk.c:217: Opening `memdisk'...
>>> kern/fs.c:56: Detecting tarfs...
>>>
>>> And then spectacular crash in UEFI due to an EL2 translation fault.
>>
>> To be sure - is it with my patch alone or with your patches? If some
>> more patches are used - could you send exact diff to trunk to avoid
>> misunderstanding?
> 
> Double checked with only your patch on top of
> 1b782e902e69516f82158203674d4951a40c82d4 (previously running with
> _only_ my alignment fixup in efidisk.c). Same behaviour.

I cannot reproduce it on x86_64 (also with mm-debug enabled) and I do
not know how to load standalone image on ppc; is it possible to use QEMU
to run ARM64 (I assume you have it)? If not what are options to test it?

Anyway, there was one problem I fixed later (although I did not get any
issue before as well), I attach updated version. Thank you for testing!

[-- Attachment #2: disk-direct-cache-read-v2.patch --]
[-- Type: text/x-patch, Size: 6772 bytes --]

From: Andrei Borzenkov <arvidjaar@gmail.com>
Subject: [PATCH] disk: read into cache directly

<patch description>

---
 grub-core/kern/disk.c | 95 ++++++++++++++++++++++++++++++---------------------
 grub-core/lib/disk.c  |  6 ++--
 include/grub/disk.h   | 11 +++++-
 3 files changed, 70 insertions(+), 42 deletions(-)

diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c
index 789f8c0..04ab2ab 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 = 0;
+	  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 = data;
+  buf->count = 0;
 
-  cache->lock = 1;
-  grub_free (cache->data);
-  cache->data = 0;
-  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_malloc (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;
 }
@@ -350,13 +372,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;
 
   {
@@ -373,10 +393,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 = grub_malloc (num << disk->log_sector_size);
-    if (!tmp_buf)
-      return grub_errno;
-    
     if ((disk->dev->read) (disk, transform_sector (disk, aligned_sector),
 			   num, tmp_buf))
       {
@@ -481,22 +497,25 @@ grub_disk_read (grub_disk_t disk, grub_disk_addr_t sector,
 
       if (agglomerate)
 	{
-	  grub_disk_addr_t i;
+	  void *cache = grub_malloc (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 = 0;
+      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 b385af8..9e60367 100644
--- a/include/grub/disk.h
+++ b/include/grub/disk.h
@@ -233,16 +233,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)
-- 
tg: (bc22096..) e/disk-cache-align (depends on: master)