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

[Andrei Borzenkov <arvidjaar@gmail.com>]

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

22.02.2016 17:02, Leif Lindholm пишет:
> On Mon, Feb 22, 2016 at 10:57:24AM +0300, Andrei Borzenkov wrote:
>> 19.02.2016 19:18, Leif Lindholm пишет:
>>> Some disk types have allocation requirements beyond normal grub_malloc.
>>> Add a function pointer to grub_disk_t and a wrapper function in
>>> kern/disk.c making use of that function if available, to enable these
>>> disk drivers to implement their own malloc.
>>
>> The problem is not (only) grub_disk_read_small(), but this part in
>> grub_disk_read:
>>
>>       if (agglomerate)
>>         {
>>           grub_disk_addr_t i;
>>
>>           err = (disk->dev->read) (disk, transform_sector (disk, sector),
>>                                    agglomerate << (GRUB_DISK_CACHE_BITS
>>                                                    + GRUB_DISK_SECTOR_BITS
>>                                                    - disk->log_sector_size),
>>                                    buf);
>>
>> which reads directly into user supplied buffer. May be we can allocate
>> contiguous cache block here but put pointers to multiple chunks inside
>> it. Possible implementation is to have second layer of reference counted
>> memory blocks with cache entries containing pointer + offset into them.
> 
> Whoops!
> 
> Understood.
> 
> So how about merging the two concepts?
> Including a patch to go with (after) the previous two to catch any
> remaining unaligned accesses in grub_efidisk_readwrite().
> With this applied, I get no fixups from a normal Linux boot (linux +
> initrd), but see them when exploring filesystems from the command
> line.
> 
> Whilst a bit clunky, this seems much short-term preferable to going
> back and redesigning the disk subsystem to understand that alignment
> matters. Although given the number of exceptions we seem to be
> amassing, that does not sound like a bad idea for future.
> 

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.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: disk-direct-cache-read.patch --]
[-- Type: text/x-patch; name="disk-direct-cache-read.patch", Size: 6984 bytes --]

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

<patch description>

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

diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c
index 789f8c0..945c146 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,35 @@ 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;
+      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 +371,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 +392,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 +496,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)