[PATCH V2] Add support for BTRFS raid5/6 to GRUB

[PATCH V2] Add support for BTRFS raid5/6 to GRUB

[Goffredo Baroncelli]

Hi All,

the aim of this patches set is to provide support for a BTRFS raid5/6
filesystem in GRUB.

The first patch, implements the basic support for raid5/6. I.e this works when
all the disks are present.

The next 4 patches, are preparatory ones.

The 6th patch implements the raid5 recovery for btrfs (i.e. handling the
disappearing of 1 disk).
The 7th patch makes the code for handling the raid6 recovery more generic.
The last one implements the raid6 recovery for btrfs (i.e. handling the
disappearing up to two disks).

I tested the code in grub-emu, and it works (for me) both with all the disks,
and with some disks missing. I checked the crc32 calculated from grub and
from linux and these matched. Finally I checked if the support for md raid6 
still works properly, and it does (with all the drives and with up to 2 drives
missing)

Comments are welcome.

Changelog
v1: initial support for btrfs raid5/6. No recovery allowed
v2: full support for btrfs raid5/6. Recovery allowed
v3: some minor cleanup suggested by Daniel Kiper; reusing the
    original raid6 recovery code of grub


BR
G.Baroncelli

[PATCH 1/8] Add support for reading a filesystem with a raid5 or raid6 profile.

[Goffredo Baroncelli]

Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
 grub-core/fs/btrfs.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index be195448d..7e287d0ec 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -119,6 +119,8 @@ struct grub_btrfs_chunk_item
 #define GRUB_BTRFS_CHUNK_TYPE_RAID1         0x10
 #define GRUB_BTRFS_CHUNK_TYPE_DUPLICATED    0x20
 #define GRUB_BTRFS_CHUNK_TYPE_RAID10        0x40
+#define GRUB_BTRFS_CHUNK_TYPE_RAID5         0x80
+#define GRUB_BTRFS_CHUNK_TYPE_RAID6         0x100
   grub_uint8_t dummy2[0xc];
   grub_uint16_t nstripes;
   grub_uint16_t nsubstripes;
@@ -764,6 +766,65 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
 	      stripe_offset = low + chunk_stripe_length
 		* high;
 	      csize = chunk_stripe_length - low;
+	      break;
+	    }
+	  case GRUB_BTRFS_CHUNK_TYPE_RAID5:
+	  case GRUB_BTRFS_CHUNK_TYPE_RAID6:
+	    {
+	      grub_uint64_t nparities, stripe_nr, high, low;
+
+	      redundancy = 1;	/* no redundancy for now */
+
+	      if (grub_le_to_cpu64 (chunk->type) & GRUB_BTRFS_CHUNK_TYPE_RAID5)
+		{
+		  grub_dprintf ("btrfs", "RAID5\n");
+		  nparities = 1;
+		}
+	      else
+		{
+		  grub_dprintf ("btrfs", "RAID6\n");
+		  nparities = 2;
+		}
+
+	      /*
+	       * A raid 6 layout consists in several stripes spread
+	       * on the disks, following a layout like the one below
+	       *
+	       *   Disk1  Disk2  Disk3  Ddisk4
+	       *
+	       *    A1     B1     P1     Q1
+	       *    Q2     A2     B2     P2  
+	       *    P3     Q3     A3     B3
+	       *  [...]
+	       *
+	       *  Note that the placement of the parities depends by the row
+	       *  index;
+	       *  In the code below:
+	       *  stripe_nr -> is the stripe number not considering the parities
+	       *               (A1=0, B1=1, A2 = 2, B2 = 3, ...)
+	       *  high -> is the row number (0 for A1...Q1, 1 for Q2..P2, ...)
+	       *  stripen -> is the column number (or number of disk)
+	       *  off -> logical address to read (from teh beginning of the 
+	       *         chunk space)
+	       *  chunk_stripe_length -> size of a stripe (typically 64k)
+	       *  nstripes -> number of disks
+	       * 
+	       */
+	      stripe_nr = grub_divmod64 (off, chunk_stripe_length, &low);
+
+	      high = grub_divmod64 (stripe_nr, nstripes - nparities, &stripen);
+
+	      /*
+	       * until now stripen is evaluated without the parities (0 for A1,
+	       * A2, A3... 1 for B1, B2...); now consider also the parities (0
+	       * for A1, 1 for A2, 2 for A3....); the math is done "modulo"
+	       * number of disk
+	       */
+	      grub_divmod64 (high + stripen, nstripes, &stripen);
+
+	      stripe_offset = low + chunk_stripe_length * high;
+	      csize = chunk_stripe_length - low;
+
 	      break;
 	    }
 	  default:
-- 
2.17.0

[PATCH 2/8] Add helper to check the btrfs header.

[Goffredo Baroncelli]

This helper was used in few places to help the debugging. As conservative
approach, in case of error it is only logged. This because I was unaware
if this could change something the error handling of the previous
code.

Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
 grub-core/fs/btrfs.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 7e287d0ec..186864ac9 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -77,7 +77,8 @@ struct btrfs_header
 {
   grub_btrfs_checksum_t checksum;
   grub_btrfs_uuid_t uuid;
-  grub_uint8_t dummy[0x30];
+  grub_uint64_t bytenr;
+  grub_uint8_t dummy[0x28];
   grub_uint32_t nitems;
   grub_uint8_t level;
 } GRUB_PACKED;
@@ -286,6 +287,25 @@ free_iterator (struct grub_btrfs_leaf_descriptor *desc)
   grub_free (desc->data);
 }
 
+static grub_err_t
+check_btrfs_header (struct grub_btrfs_data *data, struct btrfs_header *header,
+                    grub_disk_addr_t addr)
+{
+  if (grub_le_to_cpu64 (header->bytenr) != addr)
+    {
+      grub_dprintf ("btrfs", "btrfs_header.bytenr is not equal node addr\n");
+      return grub_error (GRUB_ERR_BAD_FS,
+			 "header bytenr is not equal node addr");
+    }
+  if (grub_memcmp (data->sblock.uuid, header->uuid, sizeof(grub_btrfs_uuid_t)))
+    {
+      grub_dprintf ("btrfs", "btrfs_header.uuid doesn't match sblock uuid\n");
+      return grub_error (GRUB_ERR_BAD_FS,
+			 "header uuid doesn't match sblock uuid");
+    }
+  return GRUB_ERR_NONE;
+}
+
 static grub_err_t
 save_ref (struct grub_btrfs_leaf_descriptor *desc,
 	  grub_disk_addr_t addr, unsigned i, unsigned m, int l)
@@ -341,6 +361,7 @@ next (struct grub_btrfs_data *data,
 
       err = grub_btrfs_read_logical (data, grub_le_to_cpu64 (node.addr),
 				     &head, sizeof (head), 0);
+      check_btrfs_header (data, &head, grub_le_to_cpu64 (node.addr));
       if (err)
 	return -err;
 
@@ -402,6 +423,7 @@ lower_bound (struct grub_btrfs_data *data,
       /* FIXME: preread few nodes into buffer. */
       err = grub_btrfs_read_logical (data, addr, &head, sizeof (head),
 				     recursion_depth + 1);
+      check_btrfs_header (data, &head, addr);
       if (err)
 	return err;
       addr += sizeof (head);
-- 
2.17.0

[PATCH 3/8] Move the error logging logic from find_device() to the callee.

[Goffredo Baroncelli]

This is a preparatory patch. The callee knows better if this
error is fatal, or if it exists another available disk.


Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
 grub-core/fs/btrfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 186864ac9..020d20ae6 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -604,9 +604,6 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t id, int do_rescan)
     grub_device_iterate (find_device_iter, &ctx);
   if (!ctx.dev_found)
     {
-      grub_error (GRUB_ERR_BAD_FS,
-		  N_("couldn't find a necessary member device "
-		     "of multi-device filesystem"));
       return NULL;
     }
   data->n_devices_attached++;
@@ -893,6 +890,9 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
 		dev = find_device (data, stripe->device_id, j);
 		if (!dev)
 		  {
+		    grub_dprintf ("btrfs",
+				  "couldn't find a necessary member device "
+				  "of multi-device filesystem\n");
 		    err = grub_errno;
 		    grub_errno = GRUB_ERR_NONE;
 		    continue;
-- 
2.17.0

[PATCH 5/8] Refactor the code that read from disk

[Goffredo Baroncelli]

This is a preparatory patch, to help the adding of the raid5/6 recovery code.
In case of availability of all disks, this code is good for all the RAID
profiles. However in case of failure, the error handling is quite different.
Refactoring this code increases the general readability.

Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
 grub-core/fs/btrfs.c | 106 +++++++++++++++++++++++++------------------
 1 file changed, 61 insertions(+), 45 deletions(-)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 32c4382e5..fc4198e39 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -625,6 +625,47 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t id)
   return ctx.dev_found;
 }
 
+static grub_err_t
+btrfs_read_from_chunk (struct grub_btrfs_data *data,
+		       struct grub_btrfs_chunk_item *chunk,
+		       grub_uint64_t stripen, grub_uint64_t stripe_offset,
+		       int redundancy, grub_uint64_t csize,
+		       void *buf)
+{
+
+    struct grub_btrfs_chunk_stripe *stripe;
+    grub_disk_addr_t paddr;
+    grub_device_t dev;
+    grub_err_t err;
+
+    stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
+    /* Right now the redundancy handling is easy.
+       With RAID5-like it will be more difficult.  */
+    stripe += stripen + redundancy;
+
+    paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
+
+    grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T
+		  " maps to 0x%" PRIxGRUB_UINT64_T "\n",
+		  stripen, stripe->offset);
+    grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T "\n", paddr);
+
+    dev = find_device (data, stripe->device_id);
+    if (!dev)
+      {
+	grub_dprintf ("btrfs",
+		      "couldn't find a necessary member device "
+		      "of multi-device filesystem\n");
+	grub_errno = GRUB_ERR_NONE;
+	return GRUB_ERR_READ_ERROR;
+      }
+
+    err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
+			  paddr & (GRUB_DISK_SECTOR_SIZE - 1),
+			  csize, buf);
+    return err;
+}
+
 static grub_err_t
 grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
 			 void *buf, grub_size_t size, int recursion_depth)
@@ -638,7 +679,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
       grub_err_t err = 0;
       struct grub_btrfs_key key_out;
       int challoc = 0;
-      grub_device_t dev;
       struct grub_btrfs_key key_in;
       grub_size_t chsize;
       grub_disk_addr_t chaddr;
@@ -858,53 +898,29 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
 
 	for (j = 0; j < 2; j++)
 	  {
+	    grub_dprintf ("btrfs", "chunk 0x%" PRIxGRUB_UINT64_T
+			  "+0x%" PRIxGRUB_UINT64_T
+			  " (%d stripes (%d substripes) of %"
+			  PRIxGRUB_UINT64_T ") \n",
+			  grub_le_to_cpu64 (key->offset),
+			  grub_le_to_cpu64 (chunk->size),
+			  grub_le_to_cpu16 (chunk->nstripes),
+			  grub_le_to_cpu16 (chunk->nsubstripes),
+			  grub_le_to_cpu64 (chunk->stripe_length));
+	    grub_dprintf ("btrfs", "reading laddr 0x%" PRIxGRUB_UINT64_T "\n",
+			  addr);
+
 	    for (i = 0; i < redundancy; i++)
 	      {
-		struct grub_btrfs_chunk_stripe *stripe;
-		grub_disk_addr_t paddr;
-
-		stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
-		/* Right now the redundancy handling is easy.
-		   With RAID5-like it will be more difficult.  */
-		stripe += stripen + i;
-
-		paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
-
-		grub_dprintf ("btrfs", "chunk 0x%" PRIxGRUB_UINT64_T
-			      "+0x%" PRIxGRUB_UINT64_T
-			      " (%d stripes (%d substripes) of %"
-			      PRIxGRUB_UINT64_T ") stripe %" PRIxGRUB_UINT64_T
-			      " maps to 0x%" PRIxGRUB_UINT64_T "\n",
-			      grub_le_to_cpu64 (key->offset),
-			      grub_le_to_cpu64 (chunk->size),
-			      grub_le_to_cpu16 (chunk->nstripes),
-			      grub_le_to_cpu16 (chunk->nsubstripes),
-			      grub_le_to_cpu64 (chunk->stripe_length),
-			      stripen, stripe->offset);
-		grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T
-			      " for laddr 0x%" PRIxGRUB_UINT64_T "\n", paddr,
-			      addr);
-
-		dev = find_device (data, stripe->device_id);
-		if (!dev)
-		  {
-		    grub_dprintf ("btrfs",
-				  "couldn't find a necessary member device "
-				  "of multi-device filesystem\n");
-		    err = grub_errno;
-		    grub_errno = GRUB_ERR_NONE;
-		    continue;
-		  }
-
-		err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
-				      paddr & (GRUB_DISK_SECTOR_SIZE - 1),
-				      csize, buf);
-		if (!err)
-		  break;
-		grub_errno = GRUB_ERR_NONE;
+		err = btrfs_read_from_chunk (data, chunk, stripen,
+					     stripe_offset,
+					     i,     /* redundancy */
+					     csize, buf);
+		if (err == GRUB_ERR_NONE)
+		    break;
 	      }
-	    if (i != redundancy)
-	      break;
+	    if (err == GRUB_ERR_NONE)
+		break;
 	  }
 	if (err)
 	  return grub_errno = err;
-- 
2.17.0

[PATCH 6/8] Add support for recovery for a raid5 btrfs profiles.

[Goffredo Baroncelli]

Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
 grub-core/fs/btrfs.c | 178 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 173 insertions(+), 5 deletions(-)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index fc4198e39..8d72607d1 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -29,6 +29,7 @@
 #include <minilzo.h>
 #include <grub/i18n.h>
 #include <grub/btrfs.h>
+#include <grub/crypto.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -663,9 +664,156 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
     err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
 			  paddr & (GRUB_DISK_SECTOR_SIZE - 1),
 			  csize, buf);
+    grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T "\n", paddr);
     return err;
 }
 
+struct raid56_buffer {
+  void *buf;
+  int  data_is_valid;
+};
+
+static void
+rebuild_raid5 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
+               grub_uint64_t csize)
+{
+  grub_uint64_t target = 0, i;
+
+  while (buffers[target].data_is_valid && target < nstripes)
+    ++target;
+
+  if (target == nstripes)
+    {
+      grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n");
+      return;
+    }
+
+  grub_dprintf ("btrfs", "rebuilding raid5 stripe #%" PRIuGRUB_UINT64_T "\n",
+	        target);
+  for (i = 0; i < nstripes ; i++)
+    if (i != target)
+      grub_crypto_xor (buffers[target].buf, buffers[target].buf, buffers[i].buf,
+                       csize);
+}
+
+static grub_err_t
+raid56_read_retry (struct grub_btrfs_data *data,
+		   struct grub_btrfs_chunk_item *chunk,
+		   grub_uint64_t stripe_offset, grub_uint64_t stripen,
+		   grub_uint64_t csize, void *buf)
+{
+
+  struct raid56_buffer *buffers = NULL;
+  grub_uint64_t nstripes = grub_le_to_cpu16 (chunk->nstripes);
+  grub_uint64_t chunk_type = grub_le_to_cpu64 (chunk->type);
+  grub_err_t ret = GRUB_ERR_NONE;
+  grub_uint64_t i, failed_devices;
+
+  buffers = grub_zalloc (sizeof(*buffers) * nstripes);
+  if (!buffers)
+    {
+      ret = GRUB_ERR_OUT_OF_MEMORY;
+      goto cleanup;
+    }
+
+  for (i = 0; i < nstripes ; i++)
+    {
+      buffers[i].buf = grub_zalloc (csize);
+      if (!buffers[i].buf)
+	{
+	  ret = GRUB_ERR_OUT_OF_MEMORY;
+	  goto cleanup;
+	}
+    }
+
+  for (i = 0; i < nstripes ; i++)
+    {
+      struct grub_btrfs_chunk_stripe *stripe;
+      grub_disk_addr_t paddr;
+      grub_device_t dev;
+      grub_err_t err2;
+
+      stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
+      stripe += i;
+
+      paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
+      grub_dprintf ("btrfs", "reading paddr %" PRIxGRUB_UINT64_T
+                    " from stripe ID %" PRIxGRUB_UINT64_T "\n", paddr,
+                    stripe->device_id);
+
+      /* FIXME: rescan the devices */
+      dev = find_device (data, stripe->device_id);
+      if (!dev)
+	{
+	  buffers[i].data_is_valid = 0;
+	  grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " FAILED (dev ID %"
+			PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
+	  continue;
+	}
+
+      err2 = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
+			     paddr & (GRUB_DISK_SECTOR_SIZE - 1),
+			     csize, buffers[i].buf);
+      if (err2 == GRUB_ERR_NONE)
+	{
+	  buffers[i].data_is_valid = 1;
+	  grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " Ok (dev ID %"
+			PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
+	}
+      else
+	{
+	  buffers[i].data_is_valid = 0;
+	  grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T
+			" FAILED (dev ID %" PRIxGRUB_UINT64_T ")\n", i,
+			stripe->device_id);
+	}
+    }
+
+  failed_devices = 0;
+  for (i = 0; i < nstripes ; i++)
+    if (!buffers[i].data_is_valid)
+      ++failed_devices;
+  if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
+    {
+      grub_dprintf ("btrfs",
+		    "not enough disks for raid5: total %" PRIuGRUB_UINT64_T
+		    ", missing %" PRIuGRUB_UINT64_T "\n",
+		    nstripes, failed_devices);
+      ret = GRUB_ERR_READ_ERROR;
+      goto cleanup;
+    }
+  else
+    {
+      grub_dprintf ("btrfs",
+                    "enough disks for raid5/6 rebuilding: total %"
+		    PRIuGRUB_UINT64_T ", missing %" PRIuGRUB_UINT64_T "\n",
+                    nstripes, failed_devices);
+    }
+
+  /* if these are enough, try to rebuild the data */
+  if (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5)
+    {
+      rebuild_raid5 (buffers, nstripes, csize);
+      grub_memcpy (buf, buffers[stripen].buf, csize);
+    }
+  else
+    {
+      grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
+    }
+
+cleanup:
+
+  if (buffers)
+    {
+      for (i = 0; i < nstripes ; i++)
+	if (buffers[i].buf)
+	  grub_free(buffers[i].buf);
+      grub_free(buffers);
+    }
+
+  return ret;
+}
+
 static grub_err_t
 grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
 			 void *buf, grub_size_t size, int recursion_depth)
@@ -743,6 +891,11 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
 	grub_uint16_t nstripes;
 	unsigned redundancy = 1;
 	unsigned i, j;
+	int is_raid56;
+	grub_uint64_t parities_pos = 0;
+
+        is_raid56 = !!(grub_le_to_cpu64 (chunk->type) &
+		    GRUB_BTRFS_CHUNK_TYPE_RAID5);
 
 	if (grub_le_to_cpu64 (chunk->size) <= off)
 	  {
@@ -868,7 +1021,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
 	       * 
 	       */
 	      stripe_nr = grub_divmod64 (off, chunk_stripe_length, &low);
-
 	      high = grub_divmod64 (stripe_nr, nstripes - nparities, &stripen);
 
 	      /*
@@ -878,6 +1030,8 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
 	       * number of disk
 	       */
 	      grub_divmod64 (high + stripen, nstripes, &stripen);
+	      grub_divmod64 (high + nstripes - nparities, nstripes,
+			     &parities_pos);
 
 	      stripe_offset = low + chunk_stripe_length * high;
 	      csize = chunk_stripe_length - low;
@@ -910,15 +1064,29 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
 	    grub_dprintf ("btrfs", "reading laddr 0x%" PRIxGRUB_UINT64_T "\n",
 			  addr);
 
-	    for (i = 0; i < redundancy; i++)
+	    if (!is_raid56)
+	      {
+		for (i = 0; i < redundancy; i++)
+		  {
+		    err = btrfs_read_from_chunk (data, chunk, stripen,
+						 stripe_offset,
+						 i,     /* redundancy */
+						 csize, buf);
+		    if (err == GRUB_ERR_NONE)
+			break;
+		  }
+	      }
+	    else
 	      {
 		err = btrfs_read_from_chunk (data, chunk, stripen,
 					     stripe_offset,
-					     i,     /* redundancy */
+					     0,     /* no mirror */
 					     csize, buf);
-		if (err == GRUB_ERR_NONE)
-		    break;
+		if (err != GRUB_ERR_NONE)
+		  err = raid56_read_retry (data, chunk, stripe_offset,
+					   stripen, csize, buf);
 	      }
+
 	    if (err == GRUB_ERR_NONE)
 		break;
 	  }
-- 
2.17.0

[PATCH 7/8] Make more generic the code for raid6 rebuilding

[Goffredo Baroncelli]

The original grub code which handles the recovery of a raid6 disks array
assumes that all the read are multiple of 1 << GRUB_DISK_SECTOR_BITS and
it also assumes that all the I/O is done via the struct
grub_diskfilter_segment.
This is not true for the btrfs code. In order to reuse the native
grub_raid6_recover() code, it is modified to not call
grub_diskfilter_read_node directly, but to call an handler passed
as argument.

Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
 grub-core/disk/raid6_recover.c | 52 ++++++++++++++++++++++------------
 include/grub/diskfilter.h      |  9 ++++++
 2 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/grub-core/disk/raid6_recover.c b/grub-core/disk/raid6_recover.c
index aa674f6ca..1b37d0552 100644
--- a/grub-core/disk/raid6_recover.c
+++ b/grub-core/disk/raid6_recover.c
@@ -74,14 +74,25 @@ mod_255 (unsigned x)
 }
 
 static grub_err_t
-grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
-                    char *buf, grub_disk_addr_t sector, grub_size_t size)
+raid_recover_read_diskfilter_array (void *data, int disk_nr,
+				    grub_uint64_t sector,
+				    void *buf, grub_size_t size)
+{
+    struct grub_diskfilter_segment *array = data;
+    return grub_diskfilter_read_node (&array->nodes[disk_nr],
+				      (grub_disk_addr_t)sector,
+				      size >> GRUB_DISK_SECTOR_BITS, buf);
+}
+
+grub_err_t
+grub_raid6_recover_generic (void *data, grub_uint64_t nstripes, int disknr, int p,
+			    char *buf, grub_uint64_t sector, grub_size_t size,
+			    raid_recover_read_t read_func, int layout)
 {
   int i, q, pos;
   int bad1 = -1, bad2 = -1;
   char *pbuf = 0, *qbuf = 0;
 
-  size <<= GRUB_DISK_SECTOR_BITS;
   pbuf = grub_zalloc (size);
   if (!pbuf)
     goto quit;
@@ -91,17 +102,17 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
     goto quit;
 
   q = p + 1;
-  if (q == (int) array->node_count)
+  if (q == (int) nstripes)
     q = 0;
 
   pos = q + 1;
-  if (pos == (int) array->node_count)
+  if (pos == (int) nstripes)
     pos = 0;
 
-  for (i = 0; i < (int) array->node_count - 2; i++)
+  for (i = 0; i < (int) nstripes - 2; i++)
     {
       int c;
-      if (array->layout & GRUB_RAID_LAYOUT_MUL_FROM_POS)
+      if (layout & GRUB_RAID_LAYOUT_MUL_FROM_POS)
 	c = pos;
       else
 	c = i;
@@ -109,8 +120,7 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
         bad1 = c;
       else
         {
-          if (! grub_diskfilter_read_node (&array->nodes[pos], sector,
-					   size >> GRUB_DISK_SECTOR_BITS, buf))
+	  if (!read_func(data, pos, sector, buf, size))
             {
               grub_crypto_xor (pbuf, pbuf, buf, size);
               grub_raid_block_mulx (c, buf, size);
@@ -128,7 +138,7 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
         }
 
       pos++;
-      if (pos == (int) array->node_count)
+      if (pos == (int) nstripes)
         pos = 0;
     }
 
@@ -139,16 +149,14 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
   if (bad2 < 0)
     {
       /* One bad device */
-      if ((! grub_diskfilter_read_node (&array->nodes[p], sector,
-					size >> GRUB_DISK_SECTOR_BITS, buf)))
+      if (!read_func(data, p, sector, buf, size))
         {
           grub_crypto_xor (buf, buf, pbuf, size);
           goto quit;
         }
 
       grub_errno = GRUB_ERR_NONE;
-      if (grub_diskfilter_read_node (&array->nodes[q], sector,
-				     size >> GRUB_DISK_SECTOR_BITS, buf))
+      if (read_func(data, q, sector, buf, size))
         goto quit;
 
       grub_crypto_xor (buf, buf, qbuf, size);
@@ -160,14 +168,12 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
       /* Two bad devices */
       unsigned c;
 
-      if (grub_diskfilter_read_node (&array->nodes[p], sector,
-				     size >> GRUB_DISK_SECTOR_BITS, buf))
+      if (read_func(data, p, sector, buf, size))
         goto quit;
 
       grub_crypto_xor (pbuf, pbuf, buf, size);
 
-      if (grub_diskfilter_read_node (&array->nodes[q], sector,
-				     size >> GRUB_DISK_SECTOR_BITS, buf))
+      if (read_func(data, q, sector, buf, size))
         goto quit;
 
       grub_crypto_xor (qbuf, qbuf, buf, size);
@@ -190,6 +196,16 @@ quit:
   return grub_errno;
 }
 
+static grub_err_t
+grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
+                    char *buf, grub_disk_addr_t sector, grub_size_t size)
+{
+  return grub_raid6_recover_generic (array, array->node_count, disknr, p, buf,
+				     sector, size << GRUB_DISK_SECTOR_BITS,
+				     raid_recover_read_diskfilter_array,
+				     array->layout);
+}
+
 GRUB_MOD_INIT(raid6rec)
 {
   grub_raid6_init_table ();
diff --git a/include/grub/diskfilter.h b/include/grub/diskfilter.h
index d89273c1b..6f9fac25f 100644
--- a/include/grub/diskfilter.h
+++ b/include/grub/diskfilter.h
@@ -189,6 +189,15 @@ typedef grub_err_t (*grub_raid6_recover_func_t) (struct grub_diskfilter_segment
 extern grub_raid5_recover_func_t grub_raid5_recover_func;
 extern grub_raid6_recover_func_t grub_raid6_recover_func;
 
+typedef grub_err_t (* raid_recover_read_t)(void *data, int disk_nr,
+					   grub_uint64_t addr, void *dest,
+					   grub_size_t size);
+
+grub_err_t
+grub_raid6_recover_generic (void *data, grub_uint64_t nstripes, int disknr, int p,
+			    char *buf, grub_uint64_t sector, grub_size_t size,
+			    raid_recover_read_t read_func, int layout);
+
 grub_err_t grub_diskfilter_vg_register (struct grub_diskfilter_vg *vg);
 
 grub_err_t
-- 
2.17.0

[PATCH 8/8] Add raid6 recovery for a btrfs filesystem.

[Goffredo Baroncelli]

Add the raid6 recovery, in order to use a raid6 filesystem even if some
disks (up to two) are missing.
This code use the old md raid6 code already present in grub.

Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
 grub-core/fs/btrfs.c | 43 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 8d72607d1..07e9db910 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -30,6 +30,7 @@
 #include <grub/i18n.h>
 #include <grub/btrfs.h>
 #include <grub/crypto.h>
+#include <grub/diskfilter.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -696,11 +697,36 @@ rebuild_raid5 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
                        csize);
 }
 
+static grub_err_t
+raid_recover_read_raid56_buffer (void *data, int disk_nr, grub_uint64_t addr,
+                                 void *dest, grub_size_t size)
+{
+    struct raid56_buffer *buffers = data;
+
+    (void)addr;
+    grub_memcpy(dest, buffers[disk_nr].buf, size);
+
+    grub_errno = buffers[disk_nr].data_is_valid ? GRUB_ERR_NONE : 
+		 GRUB_ERR_READ_ERROR;
+    return grub_errno;
+}
+
+static void
+rebuild_raid6 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
+               grub_uint64_t csize, grub_uint64_t parities_pos, void *dest,
+               grub_uint64_t stripen)
+
+{
+  grub_raid6_recover_generic (buffers, nstripes, stripen, parities_pos,
+                              dest, 0, csize,
+                              raid_recover_read_raid56_buffer, 0);
+}
+
 static grub_err_t
 raid56_read_retry (struct grub_btrfs_data *data,
 		   struct grub_btrfs_chunk_item *chunk,
 		   grub_uint64_t stripe_offset, grub_uint64_t stripen,
-		   grub_uint64_t csize, void *buf)
+		   grub_uint64_t csize, void *buf, grub_uint64_t parities_pos)
 {
 
   struct raid56_buffer *buffers = NULL;
@@ -782,6 +808,15 @@ raid56_read_retry (struct grub_btrfs_data *data,
       ret = GRUB_ERR_READ_ERROR;
       goto cleanup;
     }
+  else if (failed_devices > 2 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID6))
+    {
+      grub_dprintf ("btrfs",
+		    "not enough disks for raid6: total %" PRIuGRUB_UINT64_T
+		    ", missing %" PRIuGRUB_UINT64_T "\n",
+		    nstripes, failed_devices);
+      ret = GRUB_ERR_READ_ERROR;
+      goto cleanup;
+    }
   else
     {
       grub_dprintf ("btrfs",
@@ -798,7 +833,7 @@ raid56_read_retry (struct grub_btrfs_data *data,
     }
   else
     {
-      grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
+      rebuild_raid6 (buffers, nstripes, csize, parities_pos, buf, stripen);
     }
 
 cleanup:
@@ -895,7 +930,7 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
 	grub_uint64_t parities_pos = 0;
 
         is_raid56 = !!(grub_le_to_cpu64 (chunk->type) &
-		    GRUB_BTRFS_CHUNK_TYPE_RAID5);
+		    (GRUB_BTRFS_CHUNK_TYPE_RAID5|GRUB_BTRFS_CHUNK_TYPE_RAID6));
 
 	if (grub_le_to_cpu64 (chunk->size) <= off)
 	  {
@@ -1084,7 +1119,7 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
 					     csize, buf);
 		if (err != GRUB_ERR_NONE)
 		  err = raid56_read_retry (data, chunk, stripe_offset,
-					   stripen, csize, buf);
+					   stripen, csize, buf, parities_pos);
 	      }
 
 	    if (err == GRUB_ERR_NONE)
-- 
2.17.0

[PATCH V3] Add support for BTRFS raid5/6 to GRUB

[Goffredo Baroncelli]

I was wrong this is the V3 patches set ; the subject is wrong. Sorry for the inconvenient.

On 05/11/2018 09:24 PM, Goffredo Baroncelli wrote:
> Hi All,
> 
> the aim of this patches set is to provide support for a BTRFS raid5/6
> filesystem in GRUB.
> 
> The first patch, implements the basic support for raid5/6. I.e this works when
> all the disks are present.
> 
> The next 4 patches, are preparatory ones.
> 
> The 6th patch implements the raid5 recovery for btrfs (i.e. handling the
> disappearing of 1 disk).
> The 7th patch makes the code for handling the raid6 recovery more generic.
> The last one implements the raid6 recovery for btrfs (i.e. handling the
> disappearing up to two disks).
> 
> I tested the code in grub-emu, and it works (for me) both with all the disks,
> and with some disks missing. I checked the crc32 calculated from grub and
> from linux and these matched. Finally I checked if the support for md raid6 
> still works properly, and it does (with all the drives and with up to 2 drives
> missing)
> 
> Comments are welcome.
> 
> Changelog
> v1: initial support for btrfs raid5/6. No recovery allowed
> v2: full support for btrfs raid5/6. Recovery allowed
> v3: some minor cleanup suggested by Daniel Kiper; reusing the
>     original raid6 recovery code of grub
> 
> 
> BR
> G.Baroncelli
> 
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

[PATCH V3] Add support for BTRFS raid5/6 to GRUB

[Goffredo Baroncelli]

I was wrong this is the V3 patches set ; the subject is wrong. Sorry for the inconvenient.

On 05/11/2018 09:24 PM, Goffredo Baroncelli wrote:
> Hi All,
> 
> the aim of this patches set is to provide support for a BTRFS raid5/6
> filesystem in GRUB.
> 
> The first patch, implements the basic support for raid5/6. I.e this works when
> all the disks are present.
> 
> The next 4 patches, are preparatory ones.
> 
> The 6th patch implements the raid5 recovery for btrfs (i.e. handling the
> disappearing of 1 disk).
> The 7th patch makes the code for handling the raid6 recovery more generic.
> The last one implements the raid6 recovery for btrfs (i.e. handling the
> disappearing up to two disks).
> 
> I tested the code in grub-emu, and it works (for me) both with all the disks,
> and with some disks missing. I checked the crc32 calculated from grub and
> from linux and these matched. Finally I checked if the support for md raid6 
> still works properly, and it does (with all the drives and with up to 2 drives
> missing)
> 
> Comments are welcome.
> 
> Changelog
> v1: initial support for btrfs raid5/6. No recovery allowed
> v2: full support for btrfs raid5/6. Recovery allowed
> v3: some minor cleanup suggested by Daniel Kiper; reusing the
>     original raid6 recovery code of grub
> 
> 
> BR
> G.Baroncelli
> 
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 1/8] Add support for reading a filesystem with a raid5 or raid6 profile.

[Daniel Kiper]

On Fri, May 11, 2018 at 09:24:39PM +0200, Goffredo Baroncelli wrote:
> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
> ---
>  grub-core/fs/btrfs.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index be195448d..7e287d0ec 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -119,6 +119,8 @@ struct grub_btrfs_chunk_item
>  #define GRUB_BTRFS_CHUNK_TYPE_RAID1         0x10
>  #define GRUB_BTRFS_CHUNK_TYPE_DUPLICATED    0x20
>  #define GRUB_BTRFS_CHUNK_TYPE_RAID10        0x40
> +#define GRUB_BTRFS_CHUNK_TYPE_RAID5         0x80
> +#define GRUB_BTRFS_CHUNK_TYPE_RAID6         0x100
>    grub_uint8_t dummy2[0xc];
>    grub_uint16_t nstripes;
>    grub_uint16_t nsubstripes;
> @@ -764,6 +766,65 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
>  	      stripe_offset = low + chunk_stripe_length
>  		* high;
>  	      csize = chunk_stripe_length - low;
> +	      break;
> +	    }
> +	  case GRUB_BTRFS_CHUNK_TYPE_RAID5:
> +	  case GRUB_BTRFS_CHUNK_TYPE_RAID6:
> +	    {
> +	      grub_uint64_t nparities, stripe_nr, high, low;
> +
> +	      redundancy = 1;	/* no redundancy for now */
> +
> +	      if (grub_le_to_cpu64 (chunk->type) & GRUB_BTRFS_CHUNK_TYPE_RAID5)
> +		{
> +		  grub_dprintf ("btrfs", "RAID5\n");
> +		  nparities = 1;
> +		}
> +	      else
> +		{
> +		  grub_dprintf ("btrfs", "RAID6\n");
> +		  nparities = 2;
> +		}
> +
> +	      /*
> +	       * A raid 6 layout consists in several stripes spread

This line is confusing if you compare it with both cases a few lines above.
I think that you should explain somewhere why this math works for RAID 5
and RAID 6.

> +	       * on the disks, following a layout like the one below
> +	       *
> +	       *   Disk1  Disk2  Disk3  Ddisk4
> +	       *
> +	       *    A1     B1     P1     Q1
> +	       *    Q2     A2     B2     P2
> +	       *    P3     Q3     A3     B3
> +	       *  [...]
> +	       *
> +	       *  Note that the placement of the parities depends by the row

s/by the/on/?

> +	       *  index;
> +	       *  In the code below:
> +	       *  stripe_nr -> is the stripe number not considering the parities
> +	       *               (A1=0, B1=1, A2 = 2, B2 = 3, ...)
> +	       *  high -> is the row number (0 for A1...Q1, 1 for Q2..P2, ...)
> +	       *  stripen -> is the column number (or number of disk)

s/number of disk/number of disk in row/?

> +	       *  off -> logical address to read (from teh beginning of the

What is "teh"? nth? And it seems to me that off is the offset from the
beginning of the array.

> +	       *         chunk space)

I am not sure what you mean by "chunk space" here.

> +	       *  chunk_stripe_length -> size of a stripe (typically 64k)

I assume that stripe does not cover P1 and Q1 (parity disks)?

> +	       *  nstripes -> number of disks
> +	       *
> +	       */
> +	      stripe_nr = grub_divmod64 (off, chunk_stripe_length, &low);

What is the low?

> +	      high = grub_divmod64 (stripe_nr, nstripes - nparities, &stripen);
> +
> +	      /*
> +	       * until now stripen is evaluated without the parities (0 for A1,
> +	       * A2, A3... 1 for B1, B2...); now consider also the parities (0
> +	       * for A1, 1 for A2, 2 for A3....); the math is done "modulo"
> +	       * number of disk

May I ask you to use proper English sentences in the comments?

Daniel

Re: [PATCH 2/8] Add helper to check the btrfs header.

[Daniel Kiper]

On Fri, May 11, 2018 at 09:24:40PM +0200, Goffredo Baroncelli wrote:
> This helper was used in few places to help the debugging. As conservative

s/was/will be/?

> approach, in case of error it is only logged. This because I was unaware

s/This because I was unaware/This is because I am not sure/

> if this could change something the error handling of the previous

s/could/can/
s/the error/in the error/

> code.

s/previous code./currently existing code./

Otherwise LGTM.

Daniel

Re: [PATCH 3/8] Move the error logging logic from find_device() to the callee.

[Daniel Kiper]

On Fri, May 11, 2018 at 09:24:41PM +0200, Goffredo Baroncelli wrote:
> This is a preparatory patch. The callee knows better if this
> error is fatal, or if it exists another available disk.

WRT the subject:
  s/Move the error logging logic from find_device() to the callee./
    Move the error logging from find_device() to its caller/?

And it seems to me that every email's subject should start with "btrfs: ",
e.g. "btrfs: Move the error logging from find_device() to its caller".

s/error is fatal, or if it exists another available disk/
  error is fatal or not, i.e. another disk is available./

Otherwise LGTM.

Daniel

Re: [PATCH 5/8] Refactor the code that read from disk

[Daniel Kiper]

It looks that patch 4 is missing in this series.

On Fri, May 11, 2018 at 09:24:43PM +0200, Goffredo Baroncelli wrote:
> This is a preparatory patch, to help the adding of the raid5/6 recovery code.
> In case of availability of all disks, this code is good for all the RAID
> profiles. However in case of failure, the error handling is quite different.
> Refactoring this code increases the general readability.

s/raid/RAID/ in all patches please. And use "RAID 5" and "RAID 6" (with
space between RAID and number) respectively.

> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
> ---
>  grub-core/fs/btrfs.c | 106 +++++++++++++++++++++++++------------------
>  1 file changed, 61 insertions(+), 45 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 32c4382e5..fc4198e39 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -625,6 +625,47 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t id)
>    return ctx.dev_found;
>  }
>
> +static grub_err_t
> +btrfs_read_from_chunk (struct grub_btrfs_data *data,
> +		       struct grub_btrfs_chunk_item *chunk,
> +		       grub_uint64_t stripen, grub_uint64_t stripe_offset,
> +		       int redundancy, grub_uint64_t csize,
> +		       void *buf)
> +{
> +
> +    struct grub_btrfs_chunk_stripe *stripe;
> +    grub_disk_addr_t paddr;
> +    grub_device_t dev;
> +    grub_err_t err;
> +
> +    stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
> +    /* Right now the redundancy handling is easy.
> +       With RAID5-like it will be more difficult.  */
> +    stripe += stripen + redundancy;
> +
> +    paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
> +
> +    grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T
> +		  " maps to 0x%" PRIxGRUB_UINT64_T "\n",
> +		  stripen, stripe->offset);
> +    grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T "\n", paddr);
> +
> +    dev = find_device (data, stripe->device_id);
> +    if (!dev)
> +      {
> +	grub_dprintf ("btrfs",
> +		      "couldn't find a necessary member device "
> +		      "of multi-device filesystem\n");
> +	grub_errno = GRUB_ERR_NONE;
> +	return GRUB_ERR_READ_ERROR;
> +      }
> +
> +    err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
> +			  paddr & (GRUB_DISK_SECTOR_SIZE - 1),
> +			  csize, buf);
> +    return err;
> +}
> +
>  static grub_err_t
>  grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
>  			 void *buf, grub_size_t size, int recursion_depth)
> @@ -638,7 +679,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
>        grub_err_t err = 0;
>        struct grub_btrfs_key key_out;
>        int challoc = 0;
> -      grub_device_t dev;
>        struct grub_btrfs_key key_in;
>        grub_size_t chsize;
>        grub_disk_addr_t chaddr;
> @@ -858,53 +898,29 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
>
>  	for (j = 0; j < 2; j++)
>  	  {
> +	    grub_dprintf ("btrfs", "chunk 0x%" PRIxGRUB_UINT64_T
> +			  "+0x%" PRIxGRUB_UINT64_T
> +			  " (%d stripes (%d substripes) of %"
> +			  PRIxGRUB_UINT64_T ") \n",
> +			  grub_le_to_cpu64 (key->offset),
> +			  grub_le_to_cpu64 (chunk->size),
> +			  grub_le_to_cpu16 (chunk->nstripes),
> +			  grub_le_to_cpu16 (chunk->nsubstripes),
> +			  grub_le_to_cpu64 (chunk->stripe_length));
> +	    grub_dprintf ("btrfs", "reading laddr 0x%" PRIxGRUB_UINT64_T "\n",
> +			  addr);
> +
>  	    for (i = 0; i < redundancy; i++)
>  	      {
> -		struct grub_btrfs_chunk_stripe *stripe;
> -		grub_disk_addr_t paddr;
> -
> -		stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
> -		/* Right now the redundancy handling is easy.
> -		   With RAID5-like it will be more difficult.  */
> -		stripe += stripen + i;
> -
> -		paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
> -
> -		grub_dprintf ("btrfs", "chunk 0x%" PRIxGRUB_UINT64_T
> -			      "+0x%" PRIxGRUB_UINT64_T
> -			      " (%d stripes (%d substripes) of %"
> -			      PRIxGRUB_UINT64_T ") stripe %" PRIxGRUB_UINT64_T
> -			      " maps to 0x%" PRIxGRUB_UINT64_T "\n",
> -			      grub_le_to_cpu64 (key->offset),
> -			      grub_le_to_cpu64 (chunk->size),
> -			      grub_le_to_cpu16 (chunk->nstripes),
> -			      grub_le_to_cpu16 (chunk->nsubstripes),
> -			      grub_le_to_cpu64 (chunk->stripe_length),
> -			      stripen, stripe->offset);
> -		grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T
> -			      " for laddr 0x%" PRIxGRUB_UINT64_T "\n", paddr,
> -			      addr);
> -
> -		dev = find_device (data, stripe->device_id);
> -		if (!dev)
> -		  {
> -		    grub_dprintf ("btrfs",
> -				  "couldn't find a necessary member device "
> -				  "of multi-device filesystem\n");
> -		    err = grub_errno;
> -		    grub_errno = GRUB_ERR_NONE;
> -		    continue;
> -		  }
> -
> -		err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
> -				      paddr & (GRUB_DISK_SECTOR_SIZE - 1),
> -				      csize, buf);
> -		if (!err)
> -		  break;
> -		grub_errno = GRUB_ERR_NONE;
> +		err = btrfs_read_from_chunk (data, chunk, stripen,
> +					     stripe_offset,
> +					     i,     /* redundancy */
> +					     csize, buf);
> +		if (err == GRUB_ERR_NONE)
> +		    break;
>  	      }
> -	    if (i != redundancy)
> -	      break;
> +	    if (err == GRUB_ERR_NONE)
> +		break;
>  	  }
>  	if (err)
>  	  return grub_errno = err;

One logic change per patch please. First put the code in the function
then change the log message or vice versa. This is important to ease
bisection later.

Daniel

Re: [PATCH 6/8] Add support for recovery for a raid5 btrfs profiles.

[Daniel Kiper]

On Fri, May 11, 2018 at 09:24:44PM +0200, Goffredo Baroncelli wrote:
> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
> ---
>  grub-core/fs/btrfs.c | 178 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 173 insertions(+), 5 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index fc4198e39..8d72607d1 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -29,6 +29,7 @@
>  #include <minilzo.h>
>  #include <grub/i18n.h>
>  #include <grub/btrfs.h>
> +#include <grub/crypto.h>
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -663,9 +664,156 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
>      err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
>  			  paddr & (GRUB_DISK_SECTOR_SIZE - 1),
>  			  csize, buf);
> +    grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T "\n", paddr);
>      return err;
>  }
>
> +struct raid56_buffer {
> +  void *buf;
> +  int  data_is_valid;
> +};
> +
> +static void
> +rebuild_raid5 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
> +               grub_uint64_t csize)
> +{
> +  grub_uint64_t target = 0, i;
> +
> +  while (buffers[target].data_is_valid && target < nstripes)
> +    ++target;
> +
> +  if (target == nstripes)
> +    {
> +      grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n");
> +      return;
> +    }
> +
> +  grub_dprintf ("btrfs", "rebuilding raid5 stripe #%" PRIuGRUB_UINT64_T "\n",
> +	        target);
> +  for (i = 0; i < nstripes ; i++)

Please drop extra space behind nstripes. I have found
similar issues below too. Please fix all of them.

> +    if (i != target)
> +      grub_crypto_xor (buffers[target].buf, buffers[target].buf, buffers[i].buf,
> +                       csize);
> +}
> +
> +static grub_err_t
> +raid56_read_retry (struct grub_btrfs_data *data,
> +		   struct grub_btrfs_chunk_item *chunk,
> +		   grub_uint64_t stripe_offset, grub_uint64_t stripen,
> +		   grub_uint64_t csize, void *buf)
> +{
> +
> +  struct raid56_buffer *buffers = NULL;
> +  grub_uint64_t nstripes = grub_le_to_cpu16 (chunk->nstripes);
> +  grub_uint64_t chunk_type = grub_le_to_cpu64 (chunk->type);
> +  grub_err_t ret = GRUB_ERR_NONE;
> +  grub_uint64_t i, failed_devices;
> +
> +  buffers = grub_zalloc (sizeof(*buffers) * nstripes);
> +  if (!buffers)
> +    {
> +      ret = GRUB_ERR_OUT_OF_MEMORY;
> +      goto cleanup;
> +    }
> +
> +  for (i = 0; i < nstripes ; i++)

Ditto.

> +    {
> +      buffers[i].buf = grub_zalloc (csize);
> +      if (!buffers[i].buf)
> +	{
> +	  ret = GRUB_ERR_OUT_OF_MEMORY;
> +	  goto cleanup;
> +	}
> +    }
> +
> +  for (i = 0; i < nstripes ; i++)

Ditto.

> +    {
> +      struct grub_btrfs_chunk_stripe *stripe;
> +      grub_disk_addr_t paddr;
> +      grub_device_t dev;
> +      grub_err_t err2;
> +
> +      stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
> +      stripe += i;
> +
> +      paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
> +      grub_dprintf ("btrfs", "reading paddr %" PRIxGRUB_UINT64_T
> +                    " from stripe ID %" PRIxGRUB_UINT64_T "\n", paddr,
> +                    stripe->device_id);
> +
> +      /* FIXME: rescan the devices */

Could you be more verbose here?

> +      dev = find_device (data, stripe->device_id);
> +      if (!dev)
> +	{
> +	  buffers[i].data_is_valid = 0;
> +	  grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " FAILED (dev ID %"
> +			PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
> +	  continue;
> +	}
> +
> +      err2 = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
> +			     paddr & (GRUB_DISK_SECTOR_SIZE - 1),
> +			     csize, buffers[i].buf);
> +      if (err2 == GRUB_ERR_NONE)
> +	{
> +	  buffers[i].data_is_valid = 1;
> +	  grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " Ok (dev ID %"
> +			PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
> +	}
> +      else
> +	{
> +	  buffers[i].data_is_valid = 0;
> +	  grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T
> +			" FAILED (dev ID %" PRIxGRUB_UINT64_T ")\n", i,
> +			stripe->device_id);
> +	}
> +    }
> +
> +  failed_devices = 0;
> +  for (i = 0; i < nstripes ; i++)
> +    if (!buffers[i].data_is_valid)
> +      ++failed_devices;
> +  if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
> +    {
> +      grub_dprintf ("btrfs",
> +		    "not enough disks for raid5: total %" PRIuGRUB_UINT64_T
> +		    ", missing %" PRIuGRUB_UINT64_T "\n",
> +		    nstripes, failed_devices);
> +      ret = GRUB_ERR_READ_ERROR;
> +      goto cleanup;
> +    }
> +  else
> +    {
> +      grub_dprintf ("btrfs",
> +                    "enough disks for raid5/6 rebuilding: total %"
> +		    PRIuGRUB_UINT64_T ", missing %" PRIuGRUB_UINT64_T "\n",
> +                    nstripes, failed_devices);
> +    }
> +
> +  /* if these are enough, try to rebuild the data */
> +  if (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5)
> +    {
> +      rebuild_raid5 (buffers, nstripes, csize);
> +      grub_memcpy (buf, buffers[stripen].buf, csize);
> +    }
> +  else
> +    {
> +      grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
> +    }
> +
> +cleanup:
> +

Drop empty line after cleanup label and add space before the label.

> +  if (buffers)
> +    {
> +      for (i = 0; i < nstripes ; i++)
> +	if (buffers[i].buf)
> +	  grub_free(buffers[i].buf);
> +      grub_free(buffers);

Please be more consistent in using spaces and tabs.

> +    }
> +
> +  return ret;
> +}
> +
>  static grub_err_t
>  grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
>  			 void *buf, grub_size_t size, int recursion_depth)
> @@ -743,6 +891,11 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
>  	grub_uint16_t nstripes;
>  	unsigned redundancy = 1;
>  	unsigned i, j;
> +	int is_raid56;
> +	grub_uint64_t parities_pos = 0;
> +
> +        is_raid56 = !!(grub_le_to_cpu64 (chunk->type) &
> +		    GRUB_BTRFS_CHUNK_TYPE_RAID5);

Something is wrong with aligment.

>  	if (grub_le_to_cpu64 (chunk->size) <= off)
>  	  {
> @@ -868,7 +1021,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
>  	       *
>  	       */
>  	      stripe_nr = grub_divmod64 (off, chunk_stripe_length, &low);
> -

Drop this change please.

Daniel

Re: [PATCH 5/8] Refactor the code that read from disk

[Daniel Kiper]

On Mon, May 14, 2018 at 08:26:40PM +0200, Daniel Kiper wrote:
> It looks that patch 4 is missing in this series.
>
> On Fri, May 11, 2018 at 09:24:43PM +0200, Goffredo Baroncelli wrote:
> > This is a preparatory patch, to help the adding of the raid5/6 recovery code.
> > In case of availability of all disks, this code is good for all the RAID
> > profiles. However in case of failure, the error handling is quite different.
> > Refactoring this code increases the general readability.
>
> s/raid/RAID/ in all patches please. And use "RAID 5" and "RAID 6" (with
> space between RAID and number) respectively.

Of course I thought about the commit messages and comments. Code you
can leave as is.

Daniel

Re: [PATCH 7/8] Make more generic the code for raid6 rebuilding

[Daniel Kiper]

On Fri, May 11, 2018 at 09:24:45PM +0200, Goffredo Baroncelli wrote:
> The original grub code which handles the recovery of a raid6 disks array
> assumes that all the read are multiple of 1 << GRUB_DISK_SECTOR_BITS and

s/the read/reads/

> it also assumes that all the I/O is done via the struct
> grub_diskfilter_segment.
> This is not true for the btrfs code. In order to reuse the native
> grub_raid6_recover() code, it is modified to not call
> grub_diskfilter_read_node directly, but to call an handler passed
> as argument.

Could you fix the formating? Currently it is difficult to read.

> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
> ---
>  grub-core/disk/raid6_recover.c | 52 ++++++++++++++++++++++------------
>  include/grub/diskfilter.h      |  9 ++++++
>  2 files changed, 43 insertions(+), 18 deletions(-)
>
> diff --git a/grub-core/disk/raid6_recover.c b/grub-core/disk/raid6_recover.c
> index aa674f6ca..1b37d0552 100644
> --- a/grub-core/disk/raid6_recover.c
> +++ b/grub-core/disk/raid6_recover.c
> @@ -74,14 +74,25 @@ mod_255 (unsigned x)
>  }
>
>  static grub_err_t
> -grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
> -                    char *buf, grub_disk_addr_t sector, grub_size_t size)
> +raid_recover_read_diskfilter_array (void *data, int disk_nr,

s/raid_recover_read_diskfilter_array/raid6_read_node_gen/

> +				    grub_uint64_t sector,
> +				    void *buf, grub_size_t size)
> +{
> +    struct grub_diskfilter_segment *array = data;

Please add empty line here.

> +    return grub_diskfilter_read_node (&array->nodes[disk_nr],
> +				      (grub_disk_addr_t)sector,
> +				      size >> GRUB_DISK_SECTOR_BITS, buf);
> +}
> +
> +grub_err_t
> +grub_raid6_recover_generic (void *data, grub_uint64_t nstripes, int disknr, int p,

s/grub_raid6_recover_generic/grub_raid6_recover_gen/

> +			    char *buf, grub_uint64_t sector, grub_size_t size,
> +			    raid_recover_read_t read_func, int layout)

s/read_func/read_node/

Hmmm... Could not you create a struct for this call?

Daniel

Re: [PATCH 8/8] Add raid6 recovery for a btrfs filesystem.

[Daniel Kiper]

On Fri, May 11, 2018 at 09:24:46PM +0200, Goffredo Baroncelli wrote:
> Add the raid6 recovery, in order to use a raid6 filesystem even if some
> disks (up to two) are missing.
> This code use the old md raid6 code already present in grub.

Please fix the commit message formating.

> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
> ---
>  grub-core/fs/btrfs.c | 43 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 8d72607d1..07e9db910 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -30,6 +30,7 @@
>  #include <grub/i18n.h>
>  #include <grub/btrfs.h>
>  #include <grub/crypto.h>
> +#include <grub/diskfilter.h>
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -696,11 +697,36 @@ rebuild_raid5 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
>                         csize);
>  }
>
> +static grub_err_t
> +raid_recover_read_raid56_buffer (void *data, int disk_nr, grub_uint64_t addr,
> +                                 void *dest, grub_size_t size)

s/raid_recover_read_raid56_buffer/raid6_recover_read_node/

> +{
> +    struct raid56_buffer *buffers = data;
> +
> +    (void)addr;

I do not like this. grub_uint64_t addr __attribute__ ((unused))" in
prototype definition please.

> +    grub_memcpy(dest, buffers[disk_nr].buf, size);
> +
> +    grub_errno = buffers[disk_nr].data_is_valid ? GRUB_ERR_NONE :
> +		 GRUB_ERR_READ_ERROR;
> +    return grub_errno;
> +}
> +
> +static void
> +rebuild_raid6 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
> +               grub_uint64_t csize, grub_uint64_t parities_pos, void *dest,
> +               grub_uint64_t stripen)

struct as a argument?

> +
> +{
> +  grub_raid6_recover_generic (buffers, nstripes, stripen, parities_pos,
> +                              dest, 0, csize,
> +                              raid_recover_read_raid56_buffer, 0);
> +}
> +
>  static grub_err_t
>  raid56_read_retry (struct grub_btrfs_data *data,
>  		   struct grub_btrfs_chunk_item *chunk,
>  		   grub_uint64_t stripe_offset, grub_uint64_t stripen,
> -		   grub_uint64_t csize, void *buf)
> +		   grub_uint64_t csize, void *buf, grub_uint64_t parities_pos)

struct as a argument?

Daniel

Re: [PATCH 7/8] Make more generic the code for raid6 rebuilding

[Daniel Kiper]

On Mon, May 14, 2018 at 08:57:10PM +0200, Daniel Kiper wrote:
> On Fri, May 11, 2018 at 09:24:45PM +0200, Goffredo Baroncelli wrote:
> > The original grub code which handles the recovery of a raid6 disks array
> > assumes that all the read are multiple of 1 << GRUB_DISK_SECTOR_BITS and
>
> s/the read/reads/
>
> > it also assumes that all the I/O is done via the struct
> > grub_diskfilter_segment.
> > This is not true for the btrfs code. In order to reuse the native
> > grub_raid6_recover() code, it is modified to not call
> > grub_diskfilter_read_node directly, but to call an handler passed
> > as argument.
>
> Could you fix the formating? Currently it is difficult to read.
>
> > Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
> > ---
> >  grub-core/disk/raid6_recover.c | 52 ++++++++++++++++++++++------------
> >  include/grub/diskfilter.h      |  9 ++++++
> >  2 files changed, 43 insertions(+), 18 deletions(-)
> >
> > diff --git a/grub-core/disk/raid6_recover.c b/grub-core/disk/raid6_recover.c
> > index aa674f6ca..1b37d0552 100644
> > --- a/grub-core/disk/raid6_recover.c
> > +++ b/grub-core/disk/raid6_recover.c
> > @@ -74,14 +74,25 @@ mod_255 (unsigned x)
> >  }
> >
> >  static grub_err_t
> > -grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
> > -                    char *buf, grub_disk_addr_t sector, grub_size_t size)
> > +raid_recover_read_diskfilter_array (void *data, int disk_nr,
>
> s/raid_recover_read_diskfilter_array/raid6_read_node_gen/

s/raid_recover_read_diskfilter_array/raid6_recover_read_node_gen/

Daniel

Re: [PATCH 7/8] Make more generic the code for raid6 rebuilding

[Goffredo Baroncelli]

On 05/14/2018 08:57 PM, Daniel Kiper wrote:
> On Fri, May 11, 2018 at 09:24:45PM +0200, Goffredo Baroncelli wrote:
[...]
> 
>> +			    char *buf, grub_uint64_t sector, grub_size_t size,
>> +			    raid_recover_read_t read_func, int layout)
> 
> s/read_func/read_node/
> 
> Hmmm... Could not you create a struct for this call?

What do you mean ? Do you mean passing a (pointer of a) struct instead of passing all these arguments ?

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


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 7/8] Make more generic the code for raid6 rebuilding

[Daniel Kiper]

On Mon, May 14, 2018 at 10:52:11PM +0200, Goffredo Baroncelli wrote:
> On 05/14/2018 08:57 PM, Daniel Kiper wrote:
> > On Fri, May 11, 2018 at 09:24:45PM +0200, Goffredo Baroncelli wrote:
> [...]
> >
> >> +			    char *buf, grub_uint64_t sector, grub_size_t size,
> >> +			    raid_recover_read_t read_func, int layout)
> >
> > s/read_func/read_node/
> >
> > Hmmm... Could not you create a struct for this call?
>
> What do you mean ? Do you mean passing a (pointer of a) struct instead of passing all these arguments ?

Exactly!

Daniel

Re: [PATCH 7/8] Make more generic the code for raid6 rebuilding

[Goffredo Baroncelli]

On 05/14/2018 09:07 PM, Daniel Kiper wrote:
> On Mon, May 14, 2018 at 08:57:10PM +0200, Daniel Kiper wrote:
>> On Fri, May 11, 2018 at 09:24:45PM +0200, Goffredo Baroncelli wrote:
[...]
>>>  static grub_err_t
>>> -grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
>>> -                    char *buf, grub_disk_addr_t sector, grub_size_t size)
>>> +raid_recover_read_diskfilter_array (void *data, int disk_nr,
>>
>> s/raid_recover_read_diskfilter_array/raid6_read_node_gen/
> 
> s/raid_recover_read_diskfilter_array/raid6_recover_read_node_gen/

I am not sure about the "_gen" postfix. This is an handler for a specific case
(using the struct grub_diskfilter_segment).

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


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 8/8] Add raid6 recovery for a btrfs filesystem.

[Goffredo Baroncelli]

On 05/14/2018 09:06 PM, Daniel Kiper wrote:
> On Fri, May 11, 2018 at 09:24:46PM +0200, Goffredo Baroncelli wrote:
>> Add the raid6 recovery, in order to use a raid6 filesystem even if some
>> disks (up to two) are missing.
>> This code use the old md raid6 code already present in grub.
> 
> Please fix the commit message formating.
> 
>> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
>> ---
>>  grub-core/fs/btrfs.c | 43 +++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 39 insertions(+), 4 deletions(-)
>>
>> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
>> index 8d72607d1..07e9db910 100644
>> --- a/grub-core/fs/btrfs.c
>> +++ b/grub-core/fs/btrfs.c
>> @@ -30,6 +30,7 @@
>>  #include <grub/i18n.h>
>>  #include <grub/btrfs.h>
>>  #include <grub/crypto.h>
>> +#include <grub/diskfilter.h>
>>
>>  GRUB_MOD_LICENSE ("GPLv3+");
>>
>> @@ -696,11 +697,36 @@ rebuild_raid5 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
>>                         csize);
>>  }
>>
>> +static grub_err_t
>> +raid_recover_read_raid56_buffer (void *data, int disk_nr, grub_uint64_t addr,
>> +                                 void *dest, grub_size_t size)
> 
> s/raid_recover_read_raid56_buffer/raid6_recover_read_node/
I renamed this as raid6_recover_read_buffer
It is not a node, because it is different handler than the one of patch #7


> 
>> +{
>> +    struct raid56_buffer *buffers = data;
>> +
>> +    (void)addr;
> 
> I do not like this. grub_uint64_t addr __attribute__ ((unused))" in
> prototype definition please.
> 
>> +    grub_memcpy(dest, buffers[disk_nr].buf, size);
>> +
>> +    grub_errno = buffers[disk_nr].data_is_valid ? GRUB_ERR_NONE :
>> +		 GRUB_ERR_READ_ERROR;
>> +    return grub_errno;
>> +}
>> +
>> +static void
>> +rebuild_raid6 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
>> +               grub_uint64_t csize, grub_uint64_t parities_pos, void *dest,
>> +               grub_uint64_t stripen)
> 
> struct as a argument?

I thought a bit about that; however I think that the there too few place where 
grub_raid6_recover_gen() is called, so my feeling is that the effort is more
than the gain.
> 
>> +
>> +{
>> +  grub_raid6_recover_generic (buffers, nstripes, stripen, parities_pos,
>> +                              dest, 0, csize,
>> +                              raid_recover_read_raid56_buffer, 0);
>> +}
>> +
>>  static grub_err_t
>>  raid56_read_retry (struct grub_btrfs_data *data,
>>  		   struct grub_btrfs_chunk_item *chunk,
>>  		   grub_uint64_t stripe_offset, grub_uint64_t stripen,
>> -		   grub_uint64_t csize, void *buf)
>> +		   grub_uint64_t csize, void *buf, grub_uint64_t parities_pos)
> 
> struct as a argument?
> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 6/8] Add support for recovery for a raid5 btrfs profiles.

[Goffredo Baroncelli]

On 05/14/2018 08:40 PM, Daniel Kiper wrote:
> On Fri, May 11, 2018 at 09:24:44PM +0200, Goffredo Baroncelli wrote:
>> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
>> ---
>>  grub-core/fs/btrfs.c | 178 +++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 173 insertions(+), 5 deletions(-)
>>
>> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
>> index fc4198e39..8d72607d1 100644
>> --- a/grub-core/fs/btrfs.c
>> +++ b/grub-core/fs/btrfs.c
>> @@ -29,6 +29,7 @@
>>  #include <minilzo.h>
>>  #include <grub/i18n.h>
>>  #include <grub/btrfs.h>
>> +#include <grub/crypto.h>
>>
>>  GRUB_MOD_LICENSE ("GPLv3+");
>>
>> @@ -663,9 +664,156 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
>>      err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
>>  			  paddr & (GRUB_DISK_SECTOR_SIZE - 1),
>>  			  csize, buf);
>> +    grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T "\n", paddr);
>>      return err;
>>  }
>>
>> +struct raid56_buffer {
>> +  void *buf;
>> +  int  data_is_valid;
>> +};
>> +
>> +static void
>> +rebuild_raid5 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
>> +               grub_uint64_t csize)
>> +{
>> +  grub_uint64_t target = 0, i;
>> +
>> +  while (buffers[target].data_is_valid && target < nstripes)
>> +    ++target;
>> +
>> +  if (target == nstripes)
>> +    {
>> +      grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n");
>> +      return;
>> +    }
>> +
>> +  grub_dprintf ("btrfs", "rebuilding raid5 stripe #%" PRIuGRUB_UINT64_T "\n",
>> +	        target);
>> +  for (i = 0; i < nstripes ; i++)
> 
> Please drop extra space behind nstripes. I have found
> similar issues below too. Please fix all of them.
> 
>> +    if (i != target)
>> +      grub_crypto_xor (buffers[target].buf, buffers[target].buf, buffers[i].buf,
>> +                       csize);
>> +}
>> +
>> +static grub_err_t
>> +raid56_read_retry (struct grub_btrfs_data *data,
>> +		   struct grub_btrfs_chunk_item *chunk,
>> +		   grub_uint64_t stripe_offset, grub_uint64_t stripen,
>> +		   grub_uint64_t csize, void *buf)
>> +{
>> +
>> +  struct raid56_buffer *buffers = NULL;
>> +  grub_uint64_t nstripes = grub_le_to_cpu16 (chunk->nstripes);
>> +  grub_uint64_t chunk_type = grub_le_to_cpu64 (chunk->type);
>> +  grub_err_t ret = GRUB_ERR_NONE;
>> +  grub_uint64_t i, failed_devices;
>> +
>> +  buffers = grub_zalloc (sizeof(*buffers) * nstripes);
>> +  if (!buffers)
>> +    {
>> +      ret = GRUB_ERR_OUT_OF_MEMORY;
>> +      goto cleanup;
>> +    }
>> +
>> +  for (i = 0; i < nstripes ; i++)
> 
> Ditto.
> 
>> +    {
>> +      buffers[i].buf = grub_zalloc (csize);
>> +      if (!buffers[i].buf)
>> +	{
>> +	  ret = GRUB_ERR_OUT_OF_MEMORY;
>> +	  goto cleanup;
>> +	}
>> +    }
>> +
>> +  for (i = 0; i < nstripes ; i++)
> 
> Ditto.
> 
>> +    {
>> +      struct grub_btrfs_chunk_stripe *stripe;
>> +      grub_disk_addr_t paddr;
>> +      grub_device_t dev;
>> +      grub_err_t err2;
>> +
>> +      stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
>> +      stripe += i;
>> +
>> +      paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
>> +      grub_dprintf ("btrfs", "reading paddr %" PRIxGRUB_UINT64_T
>> +                    " from stripe ID %" PRIxGRUB_UINT64_T "\n", paddr,
>> +                    stripe->device_id);
>> +
>> +      /* FIXME: rescan the devices */
> 
> Could you be more verbose here?
This was an initial comment. After few thoughts I concluded that it was wrong. So I 
removed it in the next patches set.

> 
>> +      dev = find_device (data, stripe->device_id);
>> +      if (!dev)
>> +	{
>> +	  buffers[i].data_is_valid = 0;
>> +	  grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " FAILED (dev ID %"
>> +			PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
>> +	  continue;
>> +	}
>> +
>> +      err2 = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
>> +			     paddr & (GRUB_DISK_SECTOR_SIZE - 1),
>> +			     csize, buffers[i].buf);
>> +      if (err2 == GRUB_ERR_NONE)
>> +	{
>> +	  buffers[i].data_is_valid = 1;
>> +	  grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " Ok (dev ID %"
>> +			PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
>> +	}
>> +      else
>> +	{
>> +	  buffers[i].data_is_valid = 0;
>> +	  grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T
>> +			" FAILED (dev ID %" PRIxGRUB_UINT64_T ")\n", i,
>> +			stripe->device_id);
>> +	}
>> +    }
>> +
>> +  failed_devices = 0;
>> +  for (i = 0; i < nstripes ; i++)
>> +    if (!buffers[i].data_is_valid)
>> +      ++failed_devices;
>> +  if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
>> +    {
>> +      grub_dprintf ("btrfs",
>> +		    "not enough disks for raid5: total %" PRIuGRUB_UINT64_T
>> +		    ", missing %" PRIuGRUB_UINT64_T "\n",
>> +		    nstripes, failed_devices);
>> +      ret = GRUB_ERR_READ_ERROR;
>> +      goto cleanup;
>> +    }
>> +  else
>> +    {
>> +      grub_dprintf ("btrfs",
>> +                    "enough disks for raid5/6 rebuilding: total %"
>> +		    PRIuGRUB_UINT64_T ", missing %" PRIuGRUB_UINT64_T "\n",
>> +                    nstripes, failed_devices);
>> +    }
>> +
>> +  /* if these are enough, try to rebuild the data */
>> +  if (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5)
>> +    {
>> +      rebuild_raid5 (buffers, nstripes, csize);
>> +      grub_memcpy (buf, buffers[stripen].buf, csize);
>> +    }
>> +  else
>> +    {
>> +      grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
>> +    }
>> +
>> +cleanup:
>> +
> 
> Drop empty line after cleanup label and add space before the label.
> 
>> +  if (buffers)
>> +    {
>> +      for (i = 0; i < nstripes ; i++)
>> +	if (buffers[i].buf)
>> +	  grub_free(buffers[i].buf);
>> +      grub_free(buffers);
> 
> Please be more consistent in using spaces and tabs.
> 
>> +    }
>> +
>> +  return ret;
>> +}
>> +
>>  static grub_err_t
>>  grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
>>  			 void *buf, grub_size_t size, int recursion_depth)
>> @@ -743,6 +891,11 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
>>  	grub_uint16_t nstripes;
>>  	unsigned redundancy = 1;
>>  	unsigned i, j;
>> +	int is_raid56;
>> +	grub_uint64_t parities_pos = 0;
>> +
>> +        is_raid56 = !!(grub_le_to_cpu64 (chunk->type) &
>> +		    GRUB_BTRFS_CHUNK_TYPE_RAID5);
> 
> Something is wrong with aligment.
> 
>>  	if (grub_le_to_cpu64 (chunk->size) <= off)
>>  	  {
>> @@ -868,7 +1021,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
>>  	       *
>>  	       */
>>  	      stripe_nr = grub_divmod64 (off, chunk_stripe_length, &low);
>> -
> 
> Drop this change please.
> 
> Daniel
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 1/8] Add support for reading a filesystem with a raid5 or raid6 profile.

[Goffredo Baroncelli]

On 05/14/2018 07:52 PM, Daniel Kiper wrote:
> On Fri, May 11, 2018 at 09:24:39PM +0200, Goffredo Baroncelli wrote:
>> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
>> ---
>>  grub-core/fs/btrfs.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 61 insertions(+)
>>
>> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
>> index be195448d..7e287d0ec 100644
>> --- a/grub-core/fs/btrfs.c
>> +++ b/grub-core/fs/btrfs.c
>> @@ -119,6 +119,8 @@ struct grub_btrfs_chunk_item
>>  #define GRUB_BTRFS_CHUNK_TYPE_RAID1         0x10
>>  #define GRUB_BTRFS_CHUNK_TYPE_DUPLICATED    0x20
>>  #define GRUB_BTRFS_CHUNK_TYPE_RAID10        0x40
>> +#define GRUB_BTRFS_CHUNK_TYPE_RAID5         0x80
>> +#define GRUB_BTRFS_CHUNK_TYPE_RAID6         0x100
>>    grub_uint8_t dummy2[0xc];
>>    grub_uint16_t nstripes;
>>    grub_uint16_t nsubstripes;
>> @@ -764,6 +766,65 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
>>  	      stripe_offset = low + chunk_stripe_length
>>  		* high;
>>  	      csize = chunk_stripe_length - low;
>> +	      break;
>> +	    }
>> +	  case GRUB_BTRFS_CHUNK_TYPE_RAID5:
>> +	  case GRUB_BTRFS_CHUNK_TYPE_RAID6:
>> +	    {
>> +	      grub_uint64_t nparities, stripe_nr, high, low;
>> +
>> +	      redundancy = 1;	/* no redundancy for now */
>> +
>> +	      if (grub_le_to_cpu64 (chunk->type) & GRUB_BTRFS_CHUNK_TYPE_RAID5)
>> +		{
>> +		  grub_dprintf ("btrfs", "RAID5\n");
>> +		  nparities = 1;
>> +		}
>> +	      else
>> +		{
>> +		  grub_dprintf ("btrfs", "RAID6\n");
>> +		  nparities = 2;
>> +		}
>> +
>> +	      /*
>> +	       * A raid 6 layout consists in several stripes spread
> 
> This line is confusing if you compare it with both cases a few lines above.
> I think that you should explain somewhere why this math works for RAID 5
> and RAID 6.
> 
>> +	       * on the disks, following a layout like the one below
>> +	       *
>> +	       *   Disk1  Disk2  Disk3  Ddisk4
>> +	       *
>> +	       *    A1     B1     P1     Q1
>> +	       *    Q2     A2     B2     P2
>> +	       *    P3     Q3     A3     B3
>> +	       *  [...]
>> +	       *
>> +	       *  Note that the placement of the parities depends by the row
> 
> s/by the/on/?
> 
>> +	       *  index;
>> +	       *  In the code below:
>> +	       *  stripe_nr -> is the stripe number not considering the parities
>> +	       *               (A1=0, B1=1, A2 = 2, B2 = 3, ...)
>> +	       *  high -> is the row number (0 for A1...Q1, 1 for Q2..P2, ...)
>> +	       *  stripen -> is the column number (or number of disk)
> 
> s/number of disk/number of disk in row/?
> 
>> +	       *  off -> logical address to read (from teh beginning of the
> 
> What is "teh"? nth? And it seems to me that off is the offset from the
> beginning of the array.
> 
>> +	       *         chunk space)
> 
> I am not sure what you mean by "chunk space" here.

In BTRFS the logical space where are stored the data and the metadata is 
divided in chunks (or block group). Each chunk maps its "logical space" to the
disk(s) space; how this "logical space" is arranged on the disk(s) depends by
the chunk profile. Eg:
- RAID1: the logical space is mapped on two disks, where each disk is the mirror
  of the other.
- RAID5: the logical space is mapped on (n-1) disks, and the Nth disk is used
  as parity (where n is the number of available disks).

Each chunks maps a logical space that has a size of few GB. This means that
a btrfs filesystem has several chunks. And each chunks may have different profiles.
So it is possible that inside a btrfs filesystem there are area where a RAID1 
profile is applied, other where a RAID5 is applied and so on...


> 
>> +	       *  chunk_stripe_length -> size of a stripe (typically 64k)
> 
> I assume that stripe does not cover P1 and Q1 (parity disks)?

a stripe is a slice of a "row" (e.g. A1, B2, Q3...)
> 
>> +	       *  nstripes -> number of disks
>> +	       *
>> +	       */
>> +	      stripe_nr = grub_divmod64 (off, chunk_stripe_length, &low);
> 
> What is the low?

"low", is the offset where the data is placed inside a "stripe"


> 
>> +	      high = grub_divmod64 (stripe_nr, nstripes - nparities, &stripen);
>> +
>> +	      /*
>> +	       * until now stripen is evaluated without the parities (0 for A1,
>> +	       * A2, A3... 1 for B1, B2...); now consider also the parities (0
>> +	       * for A1, 1 for A2, 2 for A3....); the math is done "modulo"
>> +	       * number of disk
> 
> May I ask you to use proper English sentences in the comments?

I rearranged the comments; see V4 patches set.

> 
> Daniel
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5