[PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Goffredo Baroncelli]

From: Goffredo Baroncelli 

Add support for recovery for a RAID 5 btrfs profile. In addition
it is added some code as preparatory work for RAID 6 recovery code.

Signed-off-by: Goffredo Baroncelli 
Reviewed-by: Daniel Kiper 
---
 grub-core/fs/btrfs.c | 159 +--
 1 file changed, 154 insertions(+), 5 deletions(-)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index ea97f0502..965888da4 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -665,6 +666,138 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
 return err;
 }
 
+struct raid56_buffer {
+  void *buf;
+  int  data_is_valid;
+};
+
+static void
+rebuild_raid5 (char *dest, struct raid56_buffer *buffers,
+  grub_uint64_t nstripes, grub_uint64_t csize)
+{
+  grub_uint64_t i;
+  int first;
+
+  for(i = 0; buffers[i].data_is_valid && i < nstripes; i++);
+
+  if (i == nstripes)
+{
+  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n");
+  return;
+}
+
+  grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T "\n", 
i);
+
+  for (i = 0, first = 1; i < nstripes; i++)
+{
+  if (!buffers[i].data_is_valid)
+   continue;
+
+  if (first) {
+   grub_memcpy(dest, buffers[i].buf, csize);
+   first = 0;
+  } else
+   grub_crypto_xor (dest, dest, 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 csize, void *buf)
+{
+  struct raid56_buffer *buffers;
+  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_OUT_OF_MEMORY;
+  grub_uint64_t i, failed_devices;
+
+  buffers = grub_zalloc (sizeof(*buffers) * nstripes);
+  if (!buffers)
+goto cleanup;
+
+  for (i = 0; i < nstripes; i++)
+{
+  buffers[i].buf = grub_zalloc (csize);
+  if (!buffers[i].buf)
+   goto cleanup;
+}
+
+  for (failed_devices = 0, i = 0; i < nstripes; i++)
+{
+  struct grub_btrfs_chunk_stripe *stripe;
+  grub_disk_addr_t paddr;
+  grub_device_t dev;
+  grub_err_t err;
+
+  /* The struct grub_btrfs_chunk_stripe array lives behind struct
+grub_btrfs_chunk_item. */
+  stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1) + 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);
+
+  dev = find_device (data, stripe->device_id);
+  if (!dev)
+   {
+ grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " FAILED (dev ID 
%"
+   PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
+ failed_devices++;
+ continue;
+   }
+
+  err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
+   paddr & (GRUB_DISK_SECTOR_SIZE - 1),
+   csize, buffers[i].buf);
+  if (err == 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
+   {
+ grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T
+   " READ FAILED (dev ID %" PRIxGRUB_UINT64_T ")\n", i,
+   stripe->device_id);
+ failed_devices++;
+   }
+}
+
+  if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
+{
+  grub_dprintf ("btrfs",
+   "not enough disks for RAID 5: 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 RAID 5: total %"
+ PRIuGRUB_UINT64_T ", missing %" PRIuGRUB_UINT64_T "\n",
+ nstripes, failed_devices);
+
+  /* We have enough disks. So, rebuild the data. */
+  if (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5)
+rebuild_raid5 (buf, buffers, nstripes, csize);
+  else
+grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
+
+  ret = GRUB_ERR_NONE;
+ cleanup:
+  if (buffers)
+for (i = 0; i < nstripes; i++)
+   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)
@@ -742,6 +875,10 @@ grub_btrfs_read_logical (struct 

Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Daniel Kiper]

On Thu, Oct 18, 2018 at 07:55:39PM +0200, Goffredo Baroncelli wrote:
> From: Goffredo Baroncelli 
>
> Add support for recovery for a RAID 5 btrfs profile. In addition
> it is added some code as preparatory work for RAID 6 recovery code.
>
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 161 +--
>  1 file changed, 156 insertions(+), 5 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index ea97f0502..b277f2904 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -29,6 +29,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -665,6 +666,140 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
>  return err;
>  }
>
> +struct raid56_buffer {
> +  void *buf;
> +  int  data_is_valid;
> +};
> +
> +static void
> +rebuild_raid5 (char *dest, struct raid56_buffer *buffers,
> +grub_uint64_t nstripes, grub_uint64_t csize)
> +{
> +  grub_uint64_t i;
> +  int first;
> +
> +  for(i = 0; buffers[i].data_is_valid && i < nstripes; i++);
> +
> +  if (i == nstripes)
> +{
> +  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are 
> OK\n");
> +  return;
> +}
> +
> +  grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T 
> "\n", i);
> +
> +  for (i = 0, first = 1; i < nstripes; i++)
> +{
> +  if (!buffers[i].data_is_valid)
> + continue;
> +
> +  if (first) {
> + grub_memcpy(dest, buffers[i].buf, csize);
> + first = 0;
> +  } else
> + grub_crypto_xor (dest, dest, 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 csize, void *buf)
> +{
> +  struct raid56_buffer *buffers;
> +  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_OUT_OF_MEMORY;
> +  grub_uint64_t i, failed_devices;
> +
> +  buffers = grub_zalloc (sizeof(*buffers) * nstripes);
> +  if (!buffers)
> +goto cleanup;
> +
> +  for (i = 0; i < nstripes; i++)
> +{
> +  buffers[i].buf = grub_zalloc (csize);
> +  if (!buffers[i].buf)
> + goto cleanup;
> +}
> +
> +  for (failed_devices = 0, i = 0; i < nstripes; i++)
> +{
> +  struct grub_btrfs_chunk_stripe *stripe;
> +  grub_disk_addr_t paddr;
> +  grub_device_t dev;
> +  grub_err_t err;
> +
> +  /* The struct grub_btrfs_chunk_stripe array lives behind struct
> +  grub_btrfs_chunk_item. */
> +  stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1) + 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);
> +
> +  dev = find_device (data, stripe->device_id);
> +  if (!dev)
> + {
> +   buffers[i].data_is_valid = 0;

You do not need this and...

> +   grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " FAILED (dev ID 
> %"
> + PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
> +   failed_devices++;
> +   continue;
> + }
> +
> +  err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
> + paddr & (GRUB_DISK_SECTOR_SIZE - 1),
> + csize, buffers[i].buf);
> +  if (err == 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;

...this. grub_zalloc() above did work for you.

> +   grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T
> + " FAILED (dev ID %" PRIxGRUB_UINT64_T ")\n", i,

s/FAILED/READ FAILED/

Otherwise Reviewed-by: Daniel Kiper 

Daniel

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

[PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Goffredo Baroncelli]

From: Goffredo Baroncelli 

Add support for recovery for a RAID 5 btrfs profile. In addition
it is added some code as preparatory work for RAID 6 recovery code.

Signed-off-by: Goffredo Baroncelli 
---
 grub-core/fs/btrfs.c | 161 +--
 1 file changed, 156 insertions(+), 5 deletions(-)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index ea97f0502..b277f2904 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -665,6 +666,140 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
 return err;
 }
 
+struct raid56_buffer {
+  void *buf;
+  int  data_is_valid;
+};
+
+static void
+rebuild_raid5 (char *dest, struct raid56_buffer *buffers,
+  grub_uint64_t nstripes, grub_uint64_t csize)
+{
+  grub_uint64_t i;
+  int first;
+
+  for(i = 0; buffers[i].data_is_valid && i < nstripes; i++);
+
+  if (i == nstripes)
+{
+  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n");
+  return;
+}
+
+  grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T "\n", 
i);
+
+  for (i = 0, first = 1; i < nstripes; i++)
+{
+  if (!buffers[i].data_is_valid)
+   continue;
+
+  if (first) {
+   grub_memcpy(dest, buffers[i].buf, csize);
+   first = 0;
+  } else
+   grub_crypto_xor (dest, dest, 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 csize, void *buf)
+{
+  struct raid56_buffer *buffers;
+  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_OUT_OF_MEMORY;
+  grub_uint64_t i, failed_devices;
+
+  buffers = grub_zalloc (sizeof(*buffers) * nstripes);
+  if (!buffers)
+goto cleanup;
+
+  for (i = 0; i < nstripes; i++)
+{
+  buffers[i].buf = grub_zalloc (csize);
+  if (!buffers[i].buf)
+   goto cleanup;
+}
+
+  for (failed_devices = 0, i = 0; i < nstripes; i++)
+{
+  struct grub_btrfs_chunk_stripe *stripe;
+  grub_disk_addr_t paddr;
+  grub_device_t dev;
+  grub_err_t err;
+
+  /* The struct grub_btrfs_chunk_stripe array lives behind struct
+grub_btrfs_chunk_item. */
+  stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1) + 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);
+
+  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);
+ failed_devices++;
+ continue;
+   }
+
+  err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
+   paddr & (GRUB_DISK_SECTOR_SIZE - 1),
+   csize, buffers[i].buf);
+  if (err == 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++;
+   }
+}
+
+  if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
+{
+  grub_dprintf ("btrfs",
+   "not enough disks for RAID 5: 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 RAID 5: total %"
+ PRIuGRUB_UINT64_T ", missing %" PRIuGRUB_UINT64_T "\n",
+ nstripes, failed_devices);
+
+  /* We have enough disks. So, rebuild the data. */
+  if (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5)
+rebuild_raid5 (buf, buffers, nstripes, csize);
+  else
+grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
+
+  ret = GRUB_ERR_NONE;
+ cleanup:
+  if (buffers)
+for (i = 0; i < nstripes; i++)
+   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)
@@ 

Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Daniel Kiper]

On Wed, Oct 17, 2018 at 09:03:58PM +0200, Goffredo Baroncelli wrote:
> On 17/10/2018 16.14, Daniel Kiper wrote:
> > On Thu, Oct 11, 2018 at 08:51:01PM +0200, Goffredo Baroncelli wrote:
> >> From: Goffredo Baroncelli 
> >>
> >> Add support for recovery for a RAID 5 btrfs profile. In addition
> >> it is added some code as preparatory work for RAID 6 recovery code.
> >>
> >> Signed-off-by: Goffredo Baroncelli 
> >> ---
> [...]
>
> >> +
> >> +  for (failed_devices = 0, i = 0; i < nstripes; i++)
> >> +{
> >> +  struct grub_btrfs_chunk_stripe *stripe;
> >> +  grub_disk_addr_t paddr;
> >> +  grub_device_t dev;
> >> +  grub_err_t err;
> >> +
> >> +  /* after the struct grub_btrfs_chunk_item, there is an array of
> >> + struct grub_btrfs_chunk_stripe */
> >
> > /* Struct grub_btrfs_chunk_stripe lives behind struct 
> > grub_btrfs_chunk_item. */
>
> What about
>
> /* The struct grub_btrfs_chunk_stripe array lives behind struct 
> grub_btrfs_chunk_item. */

Works for me.

> [...]
>
> >> @@ -921,17 +1061,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)
> >
> > Why not "if (is_raid56)"? I asked about that earlier. Please change
> > this if and of course code below. It will be much easier to read. And
> > you do not need curly brackets for for loop after else.
>
> Frankly speaking I don't see any problem having a if (!...). However I
> update the code as your request, hoping to speedup this patch set

OK, it works. However, if you have "else" below then I think that it is
more natural to drop "!" here. If you would not have else I would not
complain. Well, because it would not make sense to do so... :-)))

Daniel

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

Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Goffredo Baroncelli]

On 17/10/2018 16.14, Daniel Kiper wrote:
> On Thu, Oct 11, 2018 at 08:51:01PM +0200, Goffredo Baroncelli wrote:
>> From: Goffredo Baroncelli 
>>
>> Add support for recovery for a RAID 5 btrfs profile. In addition
>> it is added some code as preparatory work for RAID 6 recovery code.
>>
>> Signed-off-by: Goffredo Baroncelli 
>> ---
[...]

>> +
>> +  for (failed_devices = 0, i = 0; i < nstripes; i++)
>> +{
>> +  struct grub_btrfs_chunk_stripe *stripe;
>> +  grub_disk_addr_t paddr;
>> +  grub_device_t dev;
>> +  grub_err_t err;
>> +
>> +  /* after the struct grub_btrfs_chunk_item, there is an array of
>> + struct grub_btrfs_chunk_stripe */
> 
> /* Struct grub_btrfs_chunk_stripe lives behind struct grub_btrfs_chunk_item. 
> */

What about 

/* The struct grub_btrfs_chunk_stripe array lives behind struct 
grub_btrfs_chunk_item. */

[...]

>> @@ -921,17 +1061,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)
> 
> Why not "if (is_raid56)"? I asked about that earlier. Please change
> this if and of course code below. It will be much easier to read. And
> you do not need curly brackets for for loop after else.

Frankly speaking I don't see any problem having a if (!...). However I update 
the code as your request, hoping to speedup this patch set

[...]


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

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

Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Daniel Kiper]

On Thu, Oct 11, 2018 at 08:51:01PM +0200, Goffredo Baroncelli wrote:
> From: Goffredo Baroncelli 
>
> Add support for recovery for a RAID 5 btrfs profile. In addition
> it is added some code as preparatory work for RAID 6 recovery code.
>
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 162 +--
>  1 file changed, 157 insertions(+), 5 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 899dc32b7..d066d54cc 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -29,6 +29,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -665,6 +666,141 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
>  return err;
>  }
>
> +struct raid56_buffer {
> +  void *buf;
> +  int  data_is_valid;
> +};
> +
> +static void
> +rebuild_raid5 (char *dest, struct raid56_buffer *buffers,
> +grub_uint64_t nstripes, grub_uint64_t csize)
> +{
> +  grub_uint64_t i;
> +  int first;
> +
> +  for(i = 0; buffers[i].data_is_valid && i < nstripes; i++);
> +
> +  if (i == nstripes)
> +{
> +  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are 
> OK\n");
> +  return;
> +}
> +
> +  grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T 
> "\n", i);
> +
> +  for (i = 0, first = 1; i < nstripes; i++)
> +{
> +  if (!buffers[i].data_is_valid)
> + continue;
> +
> +  if (first) {
> + grub_memcpy(dest, buffers[i].buf, csize);
> + first = 0;
> +  } else
> + grub_crypto_xor (dest, dest, buffers[i].buf, csize);
> +

Please drop this empty line.

> +}
> +}
> +
> +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 csize, void *buf)
> +{
> +  struct raid56_buffer *buffers;
> +  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_OUT_OF_MEMORY;
> +  grub_uint64_t i, failed_devices;
> +
> +  buffers = grub_zalloc (sizeof(*buffers) * nstripes);
> +  if (!buffers)
> +goto cleanup;
> +
> +  for (i = 0; i < nstripes; i++)
> +{
> +  buffers[i].buf = grub_zalloc (csize);
> +  if (!buffers[i].buf)
> + goto cleanup;
> +}
> +
> +  for (failed_devices = 0, i = 0; i < nstripes; i++)
> +{
> +  struct grub_btrfs_chunk_stripe *stripe;
> +  grub_disk_addr_t paddr;
> +  grub_device_t dev;
> +  grub_err_t err;
> +
> +  /* after the struct grub_btrfs_chunk_item, there is an array of
> + struct grub_btrfs_chunk_stripe */

/* Struct grub_btrfs_chunk_stripe lives behind struct grub_btrfs_chunk_item. */

> +  stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1) + 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);
> +
> +  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);
> +   failed_devices++;
> +   continue;
> + }
> +
> +  err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
> + paddr & (GRUB_DISK_SECTOR_SIZE - 1),
> + csize, buffers[i].buf);
> +  if (err == GRUB_ERR_NONE)
> + {
> +   buffers[i].data_is_valid = 1;
> +   grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " Ok (dev ID %"

s/Ok/OK/

> + 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++;
> + }
> +}
> +
> +  if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
> +{
> +  grub_dprintf ("btrfs",
> + "not enough disks for RAID 5: 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 RAID 5 rebuilding: total %"

s/enough disks for RAID 5 rebuilding/enough disks for RAID 5/

> +   PRIuGRUB_UINT64_T ", missing %" PRIuGRUB_UINT64_T "\n",
> +   nstripes, failed_devices);
> +
> +  /* if these are enough, try to rebuild the data */

/* We have enough disks. So, 

[PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Goffredo Baroncelli]

From: Goffredo Baroncelli 

Add support for recovery for a RAID 5 btrfs profile. In addition
it is added some code as preparatory work for RAID 6 recovery code.

Signed-off-by: Goffredo Baroncelli 
---
 grub-core/fs/btrfs.c | 162 +--
 1 file changed, 157 insertions(+), 5 deletions(-)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 899dc32b7..d066d54cc 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -665,6 +666,141 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
 return err;
 }
 
+struct raid56_buffer {
+  void *buf;
+  int  data_is_valid;
+};
+
+static void
+rebuild_raid5 (char *dest, struct raid56_buffer *buffers,
+  grub_uint64_t nstripes, grub_uint64_t csize)
+{
+  grub_uint64_t i;
+  int first;
+
+  for(i = 0; buffers[i].data_is_valid && i < nstripes; i++);
+
+  if (i == nstripes)
+{
+  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n");
+  return;
+}
+
+  grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T "\n", 
i);
+
+  for (i = 0, first = 1; i < nstripes; i++)
+{
+  if (!buffers[i].data_is_valid)
+   continue;
+
+  if (first) {
+   grub_memcpy(dest, buffers[i].buf, csize);
+   first = 0;
+  } else
+   grub_crypto_xor (dest, dest, 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 csize, void *buf)
+{
+  struct raid56_buffer *buffers;
+  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_OUT_OF_MEMORY;
+  grub_uint64_t i, failed_devices;
+
+  buffers = grub_zalloc (sizeof(*buffers) * nstripes);
+  if (!buffers)
+goto cleanup;
+
+  for (i = 0; i < nstripes; i++)
+{
+  buffers[i].buf = grub_zalloc (csize);
+  if (!buffers[i].buf)
+   goto cleanup;
+}
+
+  for (failed_devices = 0, i = 0; i < nstripes; i++)
+{
+  struct grub_btrfs_chunk_stripe *stripe;
+  grub_disk_addr_t paddr;
+  grub_device_t dev;
+  grub_err_t err;
+
+  /* after the struct grub_btrfs_chunk_item, there is an array of
+ struct grub_btrfs_chunk_stripe */
+  stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1) + 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);
+
+  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);
+ failed_devices++;
+ continue;
+   }
+
+  err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
+   paddr & (GRUB_DISK_SECTOR_SIZE - 1),
+   csize, buffers[i].buf);
+  if (err == 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++;
+   }
+}
+
+  if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
+{
+  grub_dprintf ("btrfs",
+   "not enough disks for RAID 5: 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 RAID 5 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 (buf, buffers, nstripes, csize);
+  else
+grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
+
+  ret = GRUB_ERR_NONE;
+ cleanup:
+  if (buffers)
+for (i = 0; i < nstripes; i++)
+   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 

Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Goffredo Baroncelli]

On 09/10/2018 20.20, Daniel Kiper wrote:
> On Thu, Sep 27, 2018 at 08:35:02PM +0200, Goffredo Baroncelli wrote:
>> From: Goffredo Baroncelli 
>>
>> Add support for recovery for a RAID 5 btrfs profile. In addition
>> it is added some code as preparatory work for RAID 6 recovery code.
>>
>> Signed-off-by: Goffredo Baroncelli 
>> ---
>>  grub-core/fs/btrfs.c | 160 +--
>>  1 file changed, 155 insertions(+), 5 deletions(-)
>>
>> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
>> index 554f350c5..db8df0eea 100644
>> --- a/grub-core/fs/btrfs.c
>> +++ b/grub-core/fs/btrfs.c
>> @@ -29,6 +29,7 @@
[...]

>> +
>> +  stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1) + i;
> 
> I think that "chunk + 1" requires short comment why...

This is a quite common pattern (a struct followed by an array of struct). 
However I added a short comment like

  /* after the struct grub_btrfs_chunk_item, there is an array of 
 struct grub_btrfs_chunk_stripe */
  stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1) + i;


[...]
>> -for (i = 0; i < redundancy; i++)
>> +if (!is_raid56)
> 
> Why not "if (is_raid56)"? This looks more natural here.

I think that it was due to the fact than before it was only the not "raid5/6" 
profiles.
Then it was added the "raid5/6" profiles, so I added this case after.

What I dislike is not the order, but the fact that in one case (not raid5), the 
same function is called several time until it found valid data. In the other 
case, it is called only the first time, then a completely different path is 
called
I am trying a different solution, but in any case the result is a bit ugly


> 
>> +  for (i = 0; i < redundancy; i++)
>> +{
>> +  err = btrfs_read_from_chunk (data, chunk, stripen,
>> +   stripe_offset,
>> +   i, /* redundancy */
>> +   csize, buf);
>> +  if (!err)
>> +break;
>> +  grub_errno = GRUB_ERR_NONE;
>> +}
>> +else
>>{
>>  err = btrfs_read_from_chunk (data, chunk, stripen,
>>   stripe_offset,
>> - i, /* redundancy */
>> + 0, /* no mirror */
>>   csize, buf);
>> -if (!err)
>> -  break;
>>  grub_errno = GRUB_ERR_NONE;
>> +if (err != GRUB_ERR_NONE)
> 
> Please be consistent and use "if (err)" here.
ok
> 
>> +  err = raid56_read_retry (data, chunk, stripe_offset,
>> +   csize, buf);
>>}
>> -if (i != redundancy)
>> +if (err == GRUB_ERR_NONE)
> 
> if (!err) please...
ok
> 
> Daniel
> 
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 


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

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

Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Daniel Kiper]

On Thu, Sep 27, 2018 at 08:35:02PM +0200, Goffredo Baroncelli wrote:
> From: Goffredo Baroncelli 
>
> Add support for recovery for a RAID 5 btrfs profile. In addition
> it is added some code as preparatory work for RAID 6 recovery code.
>
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 160 +--
>  1 file changed, 155 insertions(+), 5 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 554f350c5..db8df0eea 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -29,6 +29,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -665,6 +666,139 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
>  return err;
>  }
>
> +struct raid56_buffer {
> +  void *buf;
> +  int  data_is_valid;
> +};
> +
> +static void
> +rebuild_raid5 (char *dest, struct raid56_buffer *buffers,
> +grub_uint64_t nstripes, grub_uint64_t csize)
> +{
> +  grub_uint64_t i;
> +  int first;
> +
> +  for(i = 0; buffers[i].data_is_valid && i < nstripes; i++);
> +
> +  if (i == nstripes)
> +{
> +  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are 
> OK\n");
> +  return;
> +}
> +
> +  grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T 
> "\n", i);
> +
> +  for (i = 0, first = 1; i < nstripes; i++)
> +{
> +  if (!buffers[i].data_is_valid)
> + continue;
> +
> +  if (first) {
> + grub_memcpy(dest, buffers[i].buf, csize);
> + first = 0;
> +  } else
> + grub_crypto_xor (dest, dest, 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 csize, void *buf)
> +{
> +  struct raid56_buffer *buffers;
> +  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_OUT_OF_MEMORY;
> +  grub_uint64_t i, failed_devices;
> +
> +  buffers = grub_zalloc (sizeof(*buffers) * nstripes);
> +  if (!buffers)
> +goto cleanup;
> +
> +  for (i = 0; i < nstripes; i++)
> +{
> +  buffers[i].buf = grub_zalloc (csize);
> +  if (!buffers[i].buf)
> + goto cleanup;
> +}
> +
> +  for (failed_devices = 0, i = 0; i < nstripes; i++)
> +{
> +  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) + i;

I think that "chunk + 1" requires short comment why...

> +
> +  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);
> +
> +  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);
> +   failed_devices++;
> +   continue;
> + }
> +
> +  err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
> + paddr & (GRUB_DISK_SECTOR_SIZE - 1),
> + csize, buffers[i].buf);
> +  if (err == 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++;
> + }
> +}
> +
> +  if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
> +{
> +  grub_dprintf ("btrfs",
> + "not enough disks for RAID 5: 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 RAID 5 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 (buf, buffers, nstripes, csize);
> +  else
> +grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
> +
> +  ret = GRUB_ERR_NONE;
> + cleanup:
> +  if (buffers)
> +for (i = 0; i < nstripes; 

[PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Goffredo Baroncelli]

From: Goffredo Baroncelli 

Add support for recovery for a RAID 5 btrfs profile. In addition
it is added some code as preparatory work for RAID 6 recovery code.

Signed-off-by: Goffredo Baroncelli 
---
 grub-core/fs/btrfs.c | 160 +--
 1 file changed, 155 insertions(+), 5 deletions(-)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 554f350c5..db8df0eea 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -665,6 +666,139 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
 return err;
 }
 
+struct raid56_buffer {
+  void *buf;
+  int  data_is_valid;
+};
+
+static void
+rebuild_raid5 (char *dest, struct raid56_buffer *buffers,
+  grub_uint64_t nstripes, grub_uint64_t csize)
+{
+  grub_uint64_t i;
+  int first;
+
+  for(i = 0; buffers[i].data_is_valid && i < nstripes; i++);
+
+  if (i == nstripes)
+{
+  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n");
+  return;
+}
+
+  grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T "\n", 
i);
+
+  for (i = 0, first = 1; i < nstripes; i++)
+{
+  if (!buffers[i].data_is_valid)
+   continue;
+
+  if (first) {
+   grub_memcpy(dest, buffers[i].buf, csize);
+   first = 0;
+  } else
+   grub_crypto_xor (dest, dest, 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 csize, void *buf)
+{
+  struct raid56_buffer *buffers;
+  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_OUT_OF_MEMORY;
+  grub_uint64_t i, failed_devices;
+
+  buffers = grub_zalloc (sizeof(*buffers) * nstripes);
+  if (!buffers)
+goto cleanup;
+
+  for (i = 0; i < nstripes; i++)
+{
+  buffers[i].buf = grub_zalloc (csize);
+  if (!buffers[i].buf)
+   goto cleanup;
+}
+
+  for (failed_devices = 0, i = 0; i < nstripes; i++)
+{
+  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) + 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);
+
+  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);
+ failed_devices++;
+ continue;
+   }
+
+  err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
+   paddr & (GRUB_DISK_SECTOR_SIZE - 1),
+   csize, buffers[i].buf);
+  if (err == 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++;
+   }
+}
+
+  if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
+{
+  grub_dprintf ("btrfs",
+   "not enough disks for RAID 5: 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 RAID 5 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 (buf, buffers, nstripes, csize);
+  else
+grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
+
+  ret = GRUB_ERR_NONE;
+ cleanup:
+  if (buffers)
+for (i = 0; i < nstripes; i++)
+   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)
@@ -742,6 +876,10 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
grub_disk_addr_t addr,
  

Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Daniel Kiper]

On Wed, Sep 26, 2018 at 09:55:57PM +0200, Goffredo Baroncelli wrote:
> On 25/09/2018 21.10, Daniel Kiper wrote:
> > On Wed, Sep 19, 2018 at 08:40:38PM +0200, Goffredo Baroncelli wrote:
> >> From: Goffredo Baroncelli 
> >>
> >> Add support for recovery for a RAID 5 btrfs profile. In addition
> >> it is added some code as preparatory work for RAID 6 recovery code.
> >>
> >> Signed-off-by: Goffredo Baroncelli 
> >> ---
> >>  grub-core/fs/btrfs.c | 169 +--
> >>  1 file changed, 164 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> >> index 5c1ebae77..55a7eeffc 100644
> >> --- a/grub-core/fs/btrfs.c
> >> +++ b/grub-core/fs/btrfs.c
> >> @@ -29,6 +29,7 @@
> >>  #include 
> >>  #include 
> >>  #include 
> >> +#include 
> >>
> >>  GRUB_MOD_LICENSE ("GPLv3+");
> >>
> >> @@ -665,6 +666,148 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
> >>  return err;
> >>  }
> >>
> >> +struct raid56_buffer {
> >> +  void *buf;
> >> +  int  data_is_valid;
> >> +};
> >> +
> >> +static void
> >> +rebuild_raid5 (char *dest, struct raid56_buffer *buffers,
> >> + grub_uint64_t nstripes, grub_uint64_t csize)
> >> +{
> >> +  grub_uint64_t i;
> >> +  int first;
> >> +
> >> +  i = 0;
> >> +  while (buffers[i].data_is_valid && i < nstripes)
> >> +++i;
> >
> > for (i = 0; buffers[i].data_is_valid && i < nstripes; i++);
> >
> >> +  if (i == nstripes)
> >> +{
> >> +  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are 
> >> OK\n");
> >> +  return;
> >> +}
> >> +
> >> +  grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T 
> >> "\n",
> >> +  i);
> >
> > One line here please.
> >
> >> +  for (i = 0, first = 1; i < nstripes; i++)
> >> +{
> >> +  if (!buffers[i].data_is_valid)
> >> +  continue;
> >> +
> >> +  if (first) {
> >> +  grub_memcpy(dest, buffers[i].buf, csize);
> >> +  first = 0;
> >> +  } else
> >> +  grub_crypto_xor (dest, dest, buffers[i].buf, csize);
> >> +
> >> +}
> >
> > Hmmm... I think that this function can be simpler. You can drop first
> > while/for and "if (i == nstripes)". Then here:
> >
> > if (first) {
> >   grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n");
> >
> > Am I right?
>
> Ehm.. no. The "if" is an internal check to avoid BUG. rebuild_raid5() should 
> be called only if some disk is missed.
> To perform this control, the code checks if all buffers are valid. Otherwise 
> there is an internal BUG.

Something is wrong here. I think that the code checks if it is an invalid
buffer. If there is not then GRUB complains. Right? However, it looks
that I misread the code and made a mistake here. So, please ignore
this change. Though please change while() with for() at the beginning.

Daniel

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

[PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Goffredo Baroncelli]

On 25/09/2018 21.10, Daniel Kiper wrote:
> On Wed, Sep 19, 2018 at 08:40:38PM +0200, Goffredo Baroncelli wrote:
>> From: Goffredo Baroncelli 
>>
>> Add support for recovery for a RAID 5 btrfs profile. In addition
>> it is added some code as preparatory work for RAID 6 recovery code.
>>
>> Signed-off-by: Goffredo Baroncelli 
>> ---
>>  grub-core/fs/btrfs.c | 169 +--
>>  1 file changed, 164 insertions(+), 5 deletions(-)
>>
>> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
>> index 5c1ebae77..55a7eeffc 100644
>> --- a/grub-core/fs/btrfs.c
>> +++ b/grub-core/fs/btrfs.c
>> @@ -29,6 +29,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  GRUB_MOD_LICENSE ("GPLv3+");
>>
>> @@ -665,6 +666,148 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
>>  return err;
>>  }
>>
>> +struct raid56_buffer {
>> +  void *buf;
>> +  int  data_is_valid;
>> +};
>> +
>> +static void
>> +rebuild_raid5 (char *dest, struct raid56_buffer *buffers,
>> +   grub_uint64_t nstripes, grub_uint64_t csize)
>> +{
>> +  grub_uint64_t i;
>> +  int first;
>> +
>> +  i = 0;
>> +  while (buffers[i].data_is_valid && i < nstripes)
>> +++i;
> 
> for (i = 0; buffers[i].data_is_valid && i < nstripes; i++);
> 
>> +  if (i == nstripes)
>> +{
>> +  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are 
>> OK\n");
>> +  return;
>> +}
>> +
>> +  grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T 
>> "\n",
>> +i);
> 
> One line here please.
> 
>> +  for (i = 0, first = 1; i < nstripes; i++)
>> +{
>> +  if (!buffers[i].data_is_valid)
>> +continue;
>> +
>> +  if (first) {
>> +grub_memcpy(dest, buffers[i].buf, csize);
>> +first = 0;
>> +  } else
>> +grub_crypto_xor (dest, dest, buffers[i].buf, csize);
>> +
>> +}
> 
> Hmmm... I think that this function can be simpler. You can drop first
> while/for and "if (i == nstripes)". Then here:
> 
> if (first) {
>   grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n");
> 
> Am I right?

Ehm.. no. The "if" is an internal check to avoid BUG. rebuild_raid5() should be 
called only if some disk is missed.
To perform this control, the code checks if all buffers are valid. Otherwise 
there is an internal BUG.

Checking "first" is a completely different test.

>> +}
>> +
>> +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 csize, void *buf)
>> +{
>> +  struct raid56_buffer *buffers;
>> +  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;
> 
> s/GRUB_ERR_NONE/GRUB_ERR_OUT_OF_MEMORY/ and then you can drop at
> least two relevant assigments and some curly brackets. Of course
> before cleanup label you have to add 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 (failed_devices = 0, i = 0; i < nstripes; i++)
>> +{
>> +  struct grub_btrfs_chunk_stripe *stripe;
>> +  grub_disk_addr_t paddr;
>> +  grub_device_t dev;
>> +  grub_err_t err2;
> 
> s/err2/err/?

Ok

> 
>> +
>> +  stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
>> +  stripe += i;
> 
> Why not stripe = ((struct grub_btrfs_chunk_stripe *) (chunk + 1)) + i;?

Make sense
> 
> Daniel
> 


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



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

Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Daniel Kiper]

On Wed, Sep 19, 2018 at 08:40:38PM +0200, Goffredo Baroncelli wrote:
> From: Goffredo Baroncelli 
>
> Add support for recovery for a RAID 5 btrfs profile. In addition
> it is added some code as preparatory work for RAID 6 recovery code.
>
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 169 +--
>  1 file changed, 164 insertions(+), 5 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 5c1ebae77..55a7eeffc 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -29,6 +29,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -665,6 +666,148 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
>  return err;
>  }
>
> +struct raid56_buffer {
> +  void *buf;
> +  int  data_is_valid;
> +};
> +
> +static void
> +rebuild_raid5 (char *dest, struct raid56_buffer *buffers,
> +grub_uint64_t nstripes, grub_uint64_t csize)
> +{
> +  grub_uint64_t i;
> +  int first;
> +
> +  i = 0;
> +  while (buffers[i].data_is_valid && i < nstripes)
> +++i;

for (i = 0; buffers[i].data_is_valid && i < nstripes; i++);

> +  if (i == nstripes)
> +{
> +  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are 
> OK\n");
> +  return;
> +}
> +
> +  grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T 
> "\n",
> + i);

One line here please.

> +  for (i = 0, first = 1; i < nstripes; i++)
> +{
> +  if (!buffers[i].data_is_valid)
> + continue;
> +
> +  if (first) {
> + grub_memcpy(dest, buffers[i].buf, csize);
> + first = 0;
> +  } else
> + grub_crypto_xor (dest, dest, buffers[i].buf, csize);
> +
> +}

Hmmm... I think that this function can be simpler. You can drop first
while/for and "if (i == nstripes)". Then here:

if (first) {
  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n");

Am I right?

> +}
> +
> +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 csize, void *buf)
> +{
> +  struct raid56_buffer *buffers;
> +  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;

s/GRUB_ERR_NONE/GRUB_ERR_OUT_OF_MEMORY/ and then you can drop at
least two relevant assigments and some curly brackets. Of course
before cleanup label you have to add 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 (failed_devices = 0, i = 0; i < nstripes; i++)
> +{
> +  struct grub_btrfs_chunk_stripe *stripe;
> +  grub_disk_addr_t paddr;
> +  grub_device_t dev;
> +  grub_err_t err2;

s/err2/err/?

> +
> +  stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
> +  stripe += i;

Why not stripe = ((struct grub_btrfs_chunk_stripe *) (chunk + 1)) + i;?

Daniel

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

[PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Goffredo Baroncelli]

From: Goffredo Baroncelli 

Add support for recovery for a RAID 5 btrfs profile. In addition
it is added some code as preparatory work for RAID 6 recovery code.

Signed-off-by: Goffredo Baroncelli 
---
 grub-core/fs/btrfs.c | 169 +--
 1 file changed, 164 insertions(+), 5 deletions(-)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 5c1ebae77..55a7eeffc 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -665,6 +666,148 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
 return err;
 }
 
+struct raid56_buffer {
+  void *buf;
+  int  data_is_valid;
+};
+
+static void
+rebuild_raid5 (char *dest, struct raid56_buffer *buffers,
+  grub_uint64_t nstripes, grub_uint64_t csize)
+{
+  grub_uint64_t i;
+  int first;
+
+  i = 0;
+  while (buffers[i].data_is_valid && i < nstripes)
+++i;
+
+  if (i == nstripes)
+{
+  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n");
+  return;
+}
+
+  grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T "\n",
+   i);
+
+  for (i = 0, first = 1; i < nstripes; i++)
+{
+  if (!buffers[i].data_is_valid)
+   continue;
+
+  if (first) {
+   grub_memcpy(dest, buffers[i].buf, csize);
+   first = 0;
+  } else
+   grub_crypto_xor (dest, dest, 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 csize, void *buf)
+{
+  struct raid56_buffer *buffers;
+  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 (failed_devices = 0, 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);
+
+  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);
+ failed_devices++;
+ 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++;
+   }
+}
+
+  if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
+{
+  grub_dprintf ("btrfs",
+   "not enough disks for RAID 5: 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 RAID 5 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 (buf, buffers, nstripes, csize);
+  else
+grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
+
+ cleanup:
+  if (buffers)
+for (i = 0; i < nstripes; i++)
+   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 

Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Daniel Kiper]

On Mon, Jun 18, 2018 at 08:12:22PM +0200, Goffredo Baroncelli wrote:
> On 06/14/2018 09:05 PM, Goffredo Baroncelli wrote:
> >>> +
> >>> +cleanup:
> >> Space before the label please.
> >> I have asked about earlier.
> > The line before the label is already a space; Am I missing something
>
> I think that now I understand: are you requiring a space before the
> label *on the same line* ?

Yep!

> I found this style in some grub code, but this is not used everywhere
> (ntfs.c and nilfs dont have the space, but xfs has it ).

I know but I prefer label with space before it.

Daniel

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

Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Daniel Kiper]

On Tue, Jun 19, 2018 at 07:39:54PM +0200, Goffredo Baroncelli wrote:
> Add support for recovery for a RAID 5 btrfs profile. In addition
> it is added some code as preparatory work for RAID 6 recovery code.
>
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 177 +--
>  1 file changed, 172 insertions(+), 5 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index fa5bf56e0..7f01e447a 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -29,6 +29,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -665,6 +666,154 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
>  return err;
>  }
>
> +struct raid56_buffer {
> +  void *buf;
> +  int  data_is_valid;
> +};
> +
> +static void
> +rebuild_raid5 (char *dest, struct raid56_buffer *buffers,
> +grub_uint64_t nstripes, grub_uint64_t csize)
> +{
> +  grub_uint64_t i;
> +  int first;
> +
> +  i = 0;
> +  while (buffers[i].data_is_valid && i < nstripes)
> +++i;
> +
> +  if (i == nstripes)
> +{
> +  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are 
> OK\n");
> +  return;
> +}
> +
> +  grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T 
> "\n",
> + i);
> +  first = 1;
> +  for (i = 0; i < nstripes; i++)

for (i = 0, first = 1; i < nstripes; i++)

> +{
> +  if (!buffers[i].data_is_valid)
> + continue;
> +
> +  if (first)
> + grub_memcpy(dest, buffers[i].buf, csize);
> +  else
> + grub_crypto_xor (dest, dest, buffers[i].buf, csize);
> +
> +  first = 0;

Oh, this seems wrong to me. I think that it should be done in that way.


  if (first) {
grub_memcpy(dest, buffers[i].buf, csize);
first = 0;
  } else
grub_crypto_xor (dest, dest, 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 csize, void *buf)
> +{
> +  struct raid56_buffer *buffers = NULL;

I do not think that you need NULL assignment here.

> +  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);
> +
> +  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);
> + }
> +}
> +
> +  for (failed_devices = i = 0; i < nstripes; i++)
> +if (!buffers[i].data_is_valid)
> +  ++failed_devices;

I think that you can count failed_devices in earlier loop.

> +  if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
> +{
> +  grub_dprintf ("btrfs",
> + "not enough disks for RAID 5: total %" PRIuGRUB_UINT64_T
> + ", missing %" PRIuGRUB_UINT64_T "\n",
> + nstripes, failed_devices);
> +  ret = GRUB_ERR_READ_ERROR;
> +  goto cleanup;
> +}
> +  else
> +{
> +  grub_dprintf 

[PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Goffredo Baroncelli]

Add support for recovery for a RAID 5 btrfs profile. In addition
it is added some code as preparatory work for RAID 6 recovery code.

Signed-off-by: Goffredo Baroncelli 
---
 grub-core/fs/btrfs.c | 177 +--
 1 file changed, 172 insertions(+), 5 deletions(-)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index fa5bf56e0..7f01e447a 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -665,6 +666,154 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
 return err;
 }
 
+struct raid56_buffer {
+  void *buf;
+  int  data_is_valid;
+};
+
+static void
+rebuild_raid5 (char *dest, struct raid56_buffer *buffers,
+  grub_uint64_t nstripes, grub_uint64_t csize)
+{
+  grub_uint64_t i;
+  int first;
+
+  i = 0;
+  while (buffers[i].data_is_valid && i < nstripes)
+++i;
+
+  if (i == nstripes)
+{
+  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n");
+  return;
+}
+
+  grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T "\n",
+   i);
+  first = 1;
+  for (i = 0; i < nstripes; i++)
+{
+  if (!buffers[i].data_is_valid)
+   continue;
+
+  if (first)
+   grub_memcpy(dest, buffers[i].buf, csize);
+  else
+   grub_crypto_xor (dest, dest, buffers[i].buf, csize);
+
+  first = 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 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);
+
+  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);
+   }
+}
+
+  for (failed_devices = 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 RAID 5: 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 RAID 5 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 (buf, buffers, nstripes, 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 

Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Goffredo Baroncelli]

On 06/14/2018 09:05 PM, Goffredo Baroncelli wrote:
>>> +
>>> +cleanup:
>> Space before the label please.
>> I have asked about earlier.
> The line before the label is already a space; Am I missing something 

I think that now I understand: are you requiring a space before the label *on 
the same line* ?
I found this style in some grub code, but this is not used everywhere (ntfs.c 
and nilfs dont have the space, but xfs has it ).

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

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

Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Goffredo Baroncelli]

On 06/14/2018 03:03 PM, Daniel Kiper wrote:
> On Sun, Jun 03, 2018 at 08:53:46PM +0200, Goffredo Baroncelli wrote:
>> Add support for recovery fo a RAID 5 btrfs profile. In addition
> 
> s/fo /for /
> 
>> it is added some code as preparatory work for RAID 6 recovery code.
>>
>> Signed-off-by: Goffredo Baroncelli 
>> ---
>>  grub-core/fs/btrfs.c | 180 +--
>>  1 file changed, 175 insertions(+), 5 deletions(-)
>>
>> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
>> index 9cdbfe792..c8f034641 100644
>> --- a/grub-core/fs/btrfs.c
>> +++ b/grub-core/fs/btrfs.c
>> @@ -29,6 +29,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  GRUB_MOD_LICENSE ("GPLv3+");
>>
>> @@ -666,6 +667,157 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
>>  return err;
>>  }
>>
>> +struct raid56_buffer {
>> +  void *buf;
>> +  int  data_is_valid;
>> +};
>> +
>> +static void
>> +rebuild_raid5 (char *dest, struct raid56_buffer *buffers,
>> +   grub_uint64_t nstripes, grub_uint64_t csize)
>> +{
>> +  grub_uint64_t i;
> 
> grub_uint64_t i = 0;
> int first = 1;
> 
>> +  int first;
>> +
>> +  i = 0;
> 
> Then you can drop this assignment.

I prefer to tie the assignment to the place where it is used.

> 
>> +  while (buffers[i].data_is_valid && i < nstripes)
>> +++i;
>> +
>> +  if (i == nstripes)
>> +{
>> +  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are 
>> OK\n");
>> +  return;
>> +}
>> +
>> +  grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T 
>> "\n",
>> +i);
> 
> This can be in one line.
> 
>> +  first = 1;
> 
> And you can drop this assignment too.
> 
>> +  for (i = 0; i < nstripes; i++)
>> +{
>> +  if (!buffers[i].data_is_valid)
>> +continue;
>> +
>> +  if (first)
>> +grub_memcpy(dest, buffers[i].buf, csize);
>> +  else
>> +grub_crypto_xor (dest, dest, buffers[i].buf, csize);
> 
> I am not sure why at first you use grub_memcpy() and
> then move to grub_crypto_xor(). Could you explain this?
> Why do not use grub_crypto_xor() in all cases?

This avoid to require that dest has to be initialized to zero.

> 
>> +
>> +  first = 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 csize, void *buf)
>> +{
>> +
> 
> Please drop this empty line. I have asked about that earlier.
> 
>> +  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);
> 
> How often this function is called? Maybe you should consider
> doing memory allocation for this function only once and free
> it at btrfs module unload.

This is only needed in case of recovery. Which should happen no too often. 
Usually, this function is never called.
> 
>> +  if (!buffers)
>> +{
>> +  ret = GRUB_ERR_OUT_OF_MEMORY;
>> +  goto cleanup;
>> +}
>> +
>> +  for (i = 0; i < nstripes; i++)
>> +{
>> +  buffers[i].buf = grub_zalloc (csize);
> 
> Ditto.
> 
>> +  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);
>> +
>> +  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 

Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Daniel Kiper]

On Sun, Jun 03, 2018 at 08:53:46PM +0200, Goffredo Baroncelli wrote:
> Add support for recovery fo a RAID 5 btrfs profile. In addition

s/fo /for /

> it is added some code as preparatory work for RAID 6 recovery code.
>
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 180 +--
>  1 file changed, 175 insertions(+), 5 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 9cdbfe792..c8f034641 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -29,6 +29,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -666,6 +667,157 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
>  return err;
>  }
>
> +struct raid56_buffer {
> +  void *buf;
> +  int  data_is_valid;
> +};
> +
> +static void
> +rebuild_raid5 (char *dest, struct raid56_buffer *buffers,
> +grub_uint64_t nstripes, grub_uint64_t csize)
> +{
> +  grub_uint64_t i;

grub_uint64_t i = 0;
int first = 1;

> +  int first;
> +
> +  i = 0;

Then you can drop this assignment.

> +  while (buffers[i].data_is_valid && i < nstripes)
> +++i;
> +
> +  if (i == nstripes)
> +{
> +  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are 
> OK\n");
> +  return;
> +}
> +
> +  grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T 
> "\n",
> + i);

This can be in one line.

> +  first = 1;

And you can drop this assignment too.

> +  for (i = 0; i < nstripes; i++)
> +{
> +  if (!buffers[i].data_is_valid)
> + continue;
> +
> +  if (first)
> + grub_memcpy(dest, buffers[i].buf, csize);
> +  else
> + grub_crypto_xor (dest, dest, buffers[i].buf, csize);

I am not sure why at first you use grub_memcpy() and
then move to grub_crypto_xor(). Could you explain this?
Why do not use grub_crypto_xor() in all cases?

> +
> +  first = 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 csize, void *buf)
> +{
> +

Please drop this empty line. I have asked about that earlier.

> +  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);

How often this function is called? Maybe you should consider
doing memory allocation for this function only once and free
it at btrfs module unload.

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

Ditto.

> +  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);
> +
> +  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++)

for (failed_devices = 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 RAID 5: total %" PRIuGRUB_UINT64_T
> + 

[PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Goffredo Baroncelli]

Add support for recovery fo a RAID 5 btrfs profile. In addition
it is added some code as preparatory work for RAID 6 recovery code.

Signed-off-by: Goffredo Baroncelli 
---
 grub-core/fs/btrfs.c | 180 +--
 1 file changed, 175 insertions(+), 5 deletions(-)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 9cdbfe792..c8f034641 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -666,6 +667,157 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
 return err;
 }
 
+struct raid56_buffer {
+  void *buf;
+  int  data_is_valid;
+};
+
+static void
+rebuild_raid5 (char *dest, struct raid56_buffer *buffers,
+  grub_uint64_t nstripes, grub_uint64_t csize)
+{
+  grub_uint64_t i;
+  int first;
+
+  i = 0;
+  while (buffers[i].data_is_valid && i < nstripes)
+++i;
+
+  if (i == nstripes)
+{
+  grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n");
+  return;
+}
+
+  grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T "\n",
+   i);
+  first = 1;
+  for (i = 0; i < nstripes; i++)
+{
+  if (!buffers[i].data_is_valid)
+   continue;
+
+  if (first)
+   grub_memcpy(dest, buffers[i].buf, csize);
+  else
+   grub_crypto_xor (dest, dest, buffers[i].buf, csize);
+
+  first = 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 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);
+
+  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 RAID 5: 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 RAID 5 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 (buf, buffers, nstripes, 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 

Re: [PATCH 7/9] btrfs: add support for recovery for a RAID 5 btrfs profiles.

[Goffredo Baroncelli]

On 05/30/2018 01:30 PM, Daniel Kiper wrote:
> On Wed, May 16, 2018 at 08:48:17PM +0200, Goffredo Baroncelli wrote:
>> Signed-off-by: Goffredo Baroncelli 
>> ---
>>  grub-core/fs/btrfs.c | 174 ++-
>>  1 file changed, 170 insertions(+), 4 deletions(-)
>>
>> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
>> index 63651928b..5fcaad86f 100644
>> --- a/grub-core/fs/btrfs.c
>> +++ b/grub-core/fs/btrfs.c
>> @@ -29,6 +29,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  GRUB_MOD_LICENSE ("GPLv3+");
>>
>> @@ -666,6 +667,150 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
>>  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);
>> +
>> +  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;
> 
> What will happen if more than one stripe is broken?
See below
> 
>> +}
>> +
>> +  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);
> 
> Ditto?
See below
> 
>> +}
>> +}
>> +
>> +  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
> 
> s/raid5/RAID5/ Please fix this and the rest of messages in the other patches.
OK
> 
>> +", missing %" PRIuGRUB_UINT64_T "\n",
>> +nstripes, failed_devices);
>> +  ret = GRUB_ERR_READ_ERROR;
>> +  goto cleanup;
> 
> Ahhh... Here it is! Perfect!
Right !
> 
>> +}
>> +  else
>> +{
>> +  grub_dprintf ("btrfs",
>> +"enough disks for raid5/6 rebuilding: total %"
> 
> s#raid5/6#RAID5# This is the patch for RAID 5. Right?
> Please do not mix RAID 5 changes with RAID 6 changes.
ok
> 
>> +PRIuGRUB_UINT64_T ", 

Re: [PATCH 7/9] btrfs: add support for recovery for a RAID 5 btrfs profiles.

[Daniel Kiper]

On Wed, May 16, 2018 at 08:48:17PM +0200, Goffredo Baroncelli wrote:
> Signed-off-by: Goffredo Baroncelli 
> ---
>  grub-core/fs/btrfs.c | 174 ++-
>  1 file changed, 170 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 63651928b..5fcaad86f 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -29,6 +29,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -666,6 +667,150 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
>  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);
> +
> +  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;

What will happen if more than one stripe is broken?

> + }
> +
> +  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);

Ditto?

> + }
> +}
> +
> +  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

s/raid5/RAID5/ Please fix this and the rest of messages in the other patches.

> + ", missing %" PRIuGRUB_UINT64_T "\n",
> + nstripes, failed_devices);
> +  ret = GRUB_ERR_READ_ERROR;
> +  goto cleanup;

Ahhh... Here it is! Perfect!

> +}
> +  else
> +{
> +  grub_dprintf ("btrfs",
> +"enough disks for raid5/6 rebuilding: total %"

s#raid5/6#RAID5# This is the patch for RAID 5. Right?
Please do not mix RAID 5 changes with RAID 6 changes.

> + 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)
> +{
> +  

[PATCH 7/9] btrfs: add support for recovery for a RAID 5 btrfs profiles.

[Goffredo Baroncelli]

Signed-off-by: Goffredo Baroncelli 
---
 grub-core/fs/btrfs.c | 174 ++-
 1 file changed, 170 insertions(+), 4 deletions(-)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 63651928b..5fcaad86f 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -666,6 +667,150 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
 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);
+
+  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)