Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.
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 1/9] btrfs: Add support for reading a filesystem with a RAID 5 or RAID 6 profile.
On 06/14/2018 01:17 PM, Daniel Kiper wrote:
> On Sun, Jun 03, 2018 at 08:53:40PM +0200, Goffredo Baroncelli wrote:
>> Signed-off-by: Goffredo Baroncelli
>> ---
>> grub-core/fs/btrfs.c | 70
>> 1 file changed, 70 insertions(+)
>>
>> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
>> index be195448d..4d418859b 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_DUPLICATED0x20
>> #define GRUB_BTRFS_CHUNK_TYPE_RAID100x40
>> +#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,74 @@ 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;
>> +}
>> +
>> + /*
>> + * Below is an example of a RAID 6 layout and the meaning of the
>> + * variables. The same applies to RAID 5. The only differences is
>> + * that there is only one parity disk instead of two.
>> + *
>> + * A RAID 6 layout consists of several stripes spread
>> + * on the disks, following a layout like the one below
>> + *
>> + * Disk1 Disk2 Disk3 Ddisk4
>
> Numbering seems confusing to me. I think that it should be
>Disk0 Disk1 Disk2 Disk3
>
>> + *
>> + *A1 B1 P1 Q1
>> + *Q2 A2 B2 P2
>> + *P3 Q3 A3 B3
>> + * [...]
>> + *
>> + * Note that the placement of the parities depends on row index.
>> + * In the code below:
>> + * - stripe_nr is the stripe number not considering the parities
>> + *(A1=0, B1=1, A2 = 2, B2 = 3, ...),
>
> Please be consistent. A1 = 0, B1 = 1, A2 = 2, B2 = 3, ...
>
>> + * - high is the row number (0 for A1...Q1, 1 for Q2..P2, ...),
>
> Ditto. Please always use "..." not "..".
>
>> + * - stripen is the column number (or disk number),
>
> AIUI starting from 0. Right? If yes then I think that
> drawing above requires disks/columns renumbering.
right
>
>> + * - off is the logical address to read (from the beginning of
>> + *the chunk space),
>
> s/chunk space/chunk/?
>
>> + * - chunk_stripe_length is the size of a stripe (typically 64k),
>> + * - nstripes is the number of disks,
>> + * - low is the offset of the data inside a stripe,
>> + * - stripe_offset is the offset from the beginning of the chunk
>> + *disks physical address,
>
> I am not sure that I understand. Could clarify this?
- stripe_offset is the offset (in bytes) from the beginning of the chunk
portion
stored on disk.
You can think "stripe_offset" as the "row" in the drawing, but measured in
bytes.
>
>> + * - csize is the "potential" data to read. It will be reduced to
>> + *size if the latter is smaller.
>> + */
>> + stripe_nr = grub_divmod64 (off, chunk_stripe_length, );
>
> OK.
>
>> + /*
>> + * stripen is evaluated without considering
>> + * the parities (0 for A1, A2, A3... 1 for B1, B2...).
>> + */
>> + high = grub_divmod64 (stripe_nr, nstripes - nparities, );
>
> OK.
>
>> + /*
>> + * stripen now considers also the parities (0 for A1, 1 for A2,
>> + * 2 for A3). The math is performed modulo number of disks.
>> + */
>> + grub_divmod64 (high + stripen, nstripes, );
>
> OK.
>
>> + stripe_offset = low + chunk_stripe_length * high;
>
> Hmmm... I am confused. What does it mean?
>
> 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
[PATCH v4] mbi: use per segment a separate relocator chunk
--
Alexander Boettcher
Genode Labs
http://www.genode-labs.com - http://www.genode.org
Genode Labs GmbH - Amtsgericht Dresden - HRB 28424 - Sitz Dresden
Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth
>From b63e9760de70f83e4d8dd2e06d766e2de0135ff9 Mon Sep 17 00:00:00 2001
From: Alexander Boettcher
Date: Tue, 12 Jun 2018 20:04:09 +0200
Subject: [PATCH] mbi: use per segment a separate relocator chunk
Instead of setting up a all comprising relocator chunk for all segments,
use per segment a separate relocator chunk.
Currently, if the ELF is non-relocatable, a single relocator chunk will
comprise memory (between the segments) which gets overridden by the relst()
invocation of the movers code in grub_relocator16/32/64_boot().
The overridden memory may contain reserved ranges like VGA memory or ACPI
tables, which may lead to crashes or at least to strange boot behaviour.
Signed-off-by: Alexander Boettcher
---
grub-core/loader/multiboot_elfxx.c | 63 --
1 file changed, 40 insertions(+), 23 deletions(-)
diff --git a/grub-core/loader/multiboot_elfxx.c b/grub-core/loader/multiboot_elfxx.c
index 67daf59..ae36d9d 100644
--- a/grub-core/loader/multiboot_elfxx.c
+++ b/grub-core/loader/multiboot_elfxx.c
@@ -57,9 +57,9 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
char *phdr_base;
grub_err_t err;
grub_relocator_chunk_t ch;
- grub_uint32_t load_offset, load_size;
+ grub_uint32_t load_offset = 0, load_size;
int i;
- void *source;
+ void *source = NULL;
if (ehdr->e_ident[EI_MAG0] != ELFMAG0
|| ehdr->e_ident[EI_MAG1] != ELFMAG1
@@ -97,10 +97,15 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
return grub_error (GRUB_ERR_BAD_OS, "segment crosses 4 GiB border");
#endif
- load_size = highest_load - mld->link_base_addr;
-
if (mld->relocatable)
{
+ load_size = highest_load - mld->link_base_addr;
+
+ grub_dprintf ("multiboot_loader", "align=0x%lx, preference=0x%x, "
+ "load_size=0x%x, avoid_efi_boot_services=%d\n",
+ (long) mld->align, mld->preference, load_size,
+ mld->avoid_efi_boot_services);
+
if (load_size > mld->max_addr || mld->min_addr > mld->max_addr - load_size)
return grub_error (GRUB_ERR_BAD_OS, "invalid min/max address and/or load size");
@@ -108,27 +113,22 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
mld->min_addr, mld->max_addr - load_size,
load_size, mld->align ? mld->align : 1,
mld->preference, mld->avoid_efi_boot_services);
-}
- else
-err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT (relocator), ,
- mld->link_base_addr, load_size);
- if (err)
-{
- grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS image\n");
- return err;
-}
-
- mld->load_base_addr = get_physical_target_address (ch);
- source = get_virtual_current_address (ch);
+ if (err)
+{
+ grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS image\n");
+ return err;
+}
- grub_dprintf ("multiboot_loader", "link_base_addr=0x%x, load_base_addr=0x%x, "
- "load_size=0x%x, relocatable=%d\n", mld->link_base_addr,
- mld->load_base_addr, load_size, mld->relocatable);
+ mld->load_base_addr = get_physical_target_address (ch);
+ source = get_virtual_current_address (ch);
+}
+ else
+mld->load_base_addr = mld->link_base_addr;
- if (mld->relocatable)
-grub_dprintf ("multiboot_loader", "align=0x%lx, preference=0x%x, avoid_efi_boot_services=%d\n",
- (long) mld->align, mld->preference, mld->avoid_efi_boot_services);
+ grub_dprintf ("multiboot_loader", "relocatable=%d, link_base_addr=0x%x, "
+ "load_base_addr=0x%x\n", relocatable,
+ mld->link_base_addr, mld->load_base_addr);
/* Load every loadable segment in memory. */
for (i = 0; i < ehdr->e_phnum; i++)
@@ -139,7 +139,24 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx, memsz=0x%lx, vaddr=0x%lx\n",
i, (long) phdr(i)->p_paddr, (long) phdr(i)->p_memsz, (long) phdr(i)->p_vaddr);
- load_offset = phdr(i)->p_paddr - mld->link_base_addr;
+ if (mld->relocatable)
+ {
+ load_offset = phdr(i)->p_paddr - mld->link_base_addr;
+ grub_dprintf ("multiboot_loader", "segment %d: load_offset=0x%x\n", i, load_offset);
+ }
+ else
+ {
+ err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT (relocator), ,
+ phdr(i)->p_paddr, phdr(i)->p_memsz);
+
+ if (err)
+ {
+ grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS image\n");
+ return err;
+ }
+
+ source = get_virtual_current_address (ch);
+ }
if (phdr(i)->p_filesz != 0)
{
--
2.7.4
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Fwd: [PATCH 4/9] btrfs: Avoid a rescan for a device which was already not found.
On 06/14/2018 01:45 PM, Daniel Kiper wrote:
> On Mon, Jun 04, 2018 at 09:26:06PM +0200, Goffredo Baroncelli wrote:
>> Resend this patch because I am not sure that all received it.
>
> It looks that #4 is a bit unfortunate for you and/or patch series... :-)))
Yes, but I have another suspect: the patch #4 never changed, and I know that
gmail "collapse" the emails when these are equal...
>
>> BR
>> G.Baroncelli
>>
>>
>> Forwarded Message
>> Subject: [PATCH 4/9] btrfs: Avoid a rescan for a device which was already
>> not found.
>> Date: Sun, 3 Jun 2018 20:53:43 +0200
>> From: Goffredo Baroncelli
>> To: grub-devel@gnu.org
>> CC: Goffredo Baroncelli
>>
>> If a device is not found, record this failure by storing NULL in
>> data->devices_attached[]. This way we avoid unnecessary devices rescan,
>
> Hmmm... Could you point me out where this store happens below?
Se below
[...]
>>for (i = 0; i < data->n_devices_attached; i++)
>> if (id == data->devices_attached[i].id)
>>return data->devices_attached[i].dev;
>> - if (do_rescan)
>> -grub_device_iterate (find_device_iter, );
>> - if (!ctx.dev_found)
>> -{
>> - return NULL;
>> -}
The check above, performs a function exit if ctx.dev_found is NULL. Removing
this check allows to store the NULL in the array.
In case of another iteration we know that the device is missing without doing a
rescam
>> +
>> + grub_device_iterate (find_device_iter, );
>> +
>>data->n_devices_attached++;
>>if (data->n_devices_attached > data->n_devices_allocated)
>> {
>> @@ -617,7 +614,8 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t
>> id, int do_rescan)
>> * sizeof (data->devices_attached[0]));
>>if (!data->devices_attached)
>> {
>> - grub_device_close (ctx.dev_found);
>> + if (ctx.dev_found)
>> +grub_device_close (ctx.dev_found);
>>data->devices_attached = tmp;
>>return NULL;
>> }
>> @@ -896,7 +894,7 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data,
>> grub_disk_addr_t addr,
>>" for laddr 0x%" PRIxGRUB_UINT64_T "\n", paddr,
>>addr);
>>
>> -dev = find_device (data, stripe->device_id, j);
>> +dev = find_device (data, stripe->device_id);
>> if (!dev)
>>{
>> grub_dprintf ("btrfs",
>> @@ -973,7 +971,8 @@ grub_btrfs_unmount (struct grub_btrfs_data *data)
>>unsigned i;
>>/* The device 0 is closed one layer upper. */
>>for (i = 1; i < data->n_devices_attached; i++)
>> -grub_device_close (data->devices_attached[i].dev);
>> +if (data->devices_attached[i].dev)
>> +grub_device_close (data->devices_attached[i].dev);
>>grub_free (data->devices_attached);
>>grub_free (data->extent);
>>grub_free (data);
>> --
>> 2.17.1
>>
>>
>>
>> ___
>> 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 V5] Add support for BTRFS raid5/6 to GRUB
On 06/14/2018 03:21 PM, Daniel Kiper wrote: > Hi Goffredo, > > On Sun, Jun 03, 2018 at 08:53:39PM +0200, 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 5 patches, are preparatory ones. >> >> The 7th patch implements the raid5 recovery for btrfs (i.e. handling the >> disappearing of 1 disk). >> The 8th 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 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 drives and with up to 2 drives >> missing) >> >> Comments are welcome. > > In general I am happy that you are doing this work. However, I have just > realized that in some cases you are agreeing with my comments and then > you do not incorporate the changes which I was asking for. So, I would > be more happy if you instead of saying OK just do requested changes. This is not good. I apologize, If this happened it was a my mistake. When I put OK, this means that I agree with your reviews and I incorporate them. Now I am reviewing your old comments > Otherwise you lose your and my time. Hence, I would like ask you to > check carefully all my comments for v4 and v5 (at least), apply all > requested changes with which you agree and then post v6. > > Sorry for being blunt. > > 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 0/2] efi/fdt: set #address-cells/#size-cells on empty tree
On Thu, Jun 14, 2018 at 03:38:13PM +0200, Daniel Kiper wrote: > On Mon, Jun 11, 2018 at 05:24:57PM +0100, Leif Lindholm wrote: > > Set #address-cells and #size-cells properties (to 2) for ARM*/UEFI > > systems when creating an empty DT at boot time. This resolves an issue > > seen in the wild with kexec on certain 64-bit ARM systems. > > > > First part is moving out the prop_entry_size macro from lib/fdt.c > > and make it available in (with the required name > > change). > > > > Second part is adding the two properties to the empty tree. > > In general LGTM. Though I would prefer "?" instead of "if" in patch #2. > Or at least drop curly braces. Anyway, if you are OK with proposed > changes I can incorporate them before push. I thought the added additional bits made the ternary a bit painful, but I'd be happy for you to get rid of the braces (TianoCore coding style is making me use those more heavily :) / Leif ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/2] efi/fdt: set #address-cells/#size-cells on empty tree
On Mon, Jun 11, 2018 at 05:24:57PM +0100, Leif Lindholm wrote: > Set #address-cells and #size-cells properties (to 2) for ARM*/UEFI > systems when creating an empty DT at boot time. This resolves an issue > seen in the wild with kexec on certain 64-bit ARM systems. > > First part is moving out the prop_entry_size macro from lib/fdt.c > and make it available in (with the required name > change). > > Second part is adding the two properties to the empty tree. In general LGTM. Though I would prefer "?" instead of "if" in patch #2. Or at least drop curly braces. Anyway, if you are OK with proposed changes I can incorporate them before push. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH V5] Add support for BTRFS raid5/6 to GRUB
Hi Goffredo, On Sun, Jun 03, 2018 at 08:53:39PM +0200, 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 5 patches, are preparatory ones. > > The 7th patch implements the raid5 recovery for btrfs (i.e. handling the > disappearing of 1 disk). > The 8th 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 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 drives and with up to 2 drives > missing) > > Comments are welcome. In general I am happy that you are doing this work. However, I have just realized that in some cases you are agreeing with my comments and then you do not incorporate the changes which I was asking for. So, I would be more happy if you instead of saying OK just do requested changes. Otherwise you lose your and my time. Hence, I would like ask you to check carefully all my comments for v4 and v5 (at least), apply all requested changes with which you agree and then post v6. Sorry for being blunt. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 9/9] btrfs: Add RAID 6 recovery for a btrfs filesystem.
On Sun, Jun 03, 2018 at 08:53:48PM +0200, Goffredo Baroncelli wrote:
> Add the RAID 6 recovery, in order to use a RAID 6 filesystem even if some
> disks (up to two) are missing. This code use the md RAID 6 code already
> present in grub.
>
> Signed-off-by: Goffredo Baroncelli
> ---
> grub-core/fs/btrfs.c | 50 ++--
> 1 file changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index c8f034641..0b6557ce3 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -30,6 +30,7 @@
> #include
> #include
> #include
> +#include
>
> GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -706,11 +707,35 @@ rebuild_raid5 (char *dest, struct raid56_buffer
> *buffers,
> }
> }
>
> +static grub_err_t
> +raid6_recover_read_buffer (void *data, int disk_nr,
> +grub_uint64_t addr __attribute__ ((unused)),
> +void *dest, grub_size_t size)
> +{
> +struct raid56_buffer *buffers = data;
> +
> +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;
if (!buffers[disk_nr].data_is_valid)
return GRUB_ERR_READ_ERROR;
grub_memcpy(dest, buffers[disk_nr].buf, size);
return GRUB_ERR_NONE;
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.
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
> +
Re: [PATCH 6/9] btrfs: Refactor the code that read from disk
On Sun, Jun 03, 2018 at 08:53:45PM +0200, Goffredo Baroncelli wrote:
> Move the code in charge to read the data from disk in a separate
s/in a/into a/
> function. This helps to separate the error handling logic (which depend by
s/depend by/depends on/ Please fix this here and in other patches too.
> the different raid profiles) from the read from disk logic.
> Refactoring this code increases the general readability too.
>
> This is a preparatory patch, to help the adding of the RAID 5/6 recovery
> code.
>
> Signed-off-by: Goffredo Baroncelli
> ---
> grub-core/fs/btrfs.c | 76 ++--
> 1 file changed, 45 insertions(+), 31 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index e2baed665..9cdbfe792 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)
> +{
> +
Please drop this empty line.
> +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);
Could you put this into one grub_dprintf() call?
> +
> +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;
Why grub_errno = GRUB_ERR_NONE and then return GRUB_ERR_READ_ERROR?
If you do things like that I think that you should add a few words
of comment before such code. Otherwise it is confusing.
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 5/9] btrfs: Move logging code in grub_btrfs_read_logical()
On Sun, Jun 03, 2018 at 08:53:44PM +0200, Goffredo Baroncelli wrote:
> A portion of the logging code is moved outside of internal for(;;). The part
> that is left inside is the one which depends by the internal for(;;) index.
s/depends by/depends on/
> This is a preparatory patch: in the next one it will be possible to refactor
s/: in the next one it will be possible to/. The next one will/
> the code inside the for(;;) in an another function.
s/in/into/
> Signed-off-by: Goffredo Baroncelli
> ---
> grub-core/fs/btrfs.c | 25 ++---
> 1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index b64b692f8..e2baed665 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -867,6 +867,18 @@ 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",
s/") \n"/")\n"/
Otherwise LGTM.
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Fwd: [PATCH 4/9] btrfs: Avoid a rescan for a device which was already not found.
On Mon, Jun 04, 2018 at 09:26:06PM +0200, Goffredo Baroncelli wrote:
> Resend this patch because I am not sure that all received it.
It looks that #4 is a bit unfortunate for you and/or patch series... :-)))
> BR
> G.Baroncelli
>
>
> Forwarded Message
> Subject: [PATCH 4/9] btrfs: Avoid a rescan for a device which was already not
> found.
> Date: Sun, 3 Jun 2018 20:53:43 +0200
> From: Goffredo Baroncelli
> To: grub-devel@gnu.org
> CC: Goffredo Baroncelli
>
> If a device is not found, record this failure by storing NULL in
> data->devices_attached[]. This way we avoid unnecessary devices rescan,
Hmmm... Could you point me out where this store happens below?
Daniel
> and speedup the reads in case of a degraded array.
>
> Signed-off-by: Goffredo Baroncelli
> ---
> grub-core/fs/btrfs.c | 19 +--
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 9b3a22772..b64b692f8 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -588,7 +588,7 @@ find_device_iter (const char *name, void *data)
> }
>
> static grub_device_t
> -find_device (struct grub_btrfs_data *data, grub_uint64_t id, int do_rescan)
> +find_device (struct grub_btrfs_data *data, grub_uint64_t id)
> {
>struct find_device_ctx ctx = {
> .data = data,
> @@ -600,12 +600,9 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t
> id, int do_rescan)
>for (i = 0; i < data->n_devices_attached; i++)
> if (id == data->devices_attached[i].id)
>return data->devices_attached[i].dev;
> - if (do_rescan)
> -grub_device_iterate (find_device_iter, );
> - if (!ctx.dev_found)
> -{
> - return NULL;
> -}
> +
> + grub_device_iterate (find_device_iter, );
> +
>data->n_devices_attached++;
>if (data->n_devices_attached > data->n_devices_allocated)
> {
> @@ -617,7 +614,8 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t
> id, int do_rescan)
> * sizeof (data->devices_attached[0]));
>if (!data->devices_attached)
> {
> - grub_device_close (ctx.dev_found);
> + if (ctx.dev_found)
> + grub_device_close (ctx.dev_found);
> data->devices_attached = tmp;
> return NULL;
> }
> @@ -896,7 +894,7 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data,
> grub_disk_addr_t addr,
> " for laddr 0x%" PRIxGRUB_UINT64_T "\n", paddr,
> addr);
>
> - dev = find_device (data, stripe->device_id, j);
> + dev = find_device (data, stripe->device_id);
> if (!dev)
> {
> grub_dprintf ("btrfs",
> @@ -973,7 +971,8 @@ grub_btrfs_unmount (struct grub_btrfs_data *data)
>unsigned i;
>/* The device 0 is closed one layer upper. */
>for (i = 1; i < data->n_devices_attached; i++)
> -grub_device_close (data->devices_attached[i].dev);
> +if (data->devices_attached[i].dev)
> +grub_device_close (data->devices_attached[i].dev);
>grub_free (data->devices_attached);
>grub_free (data->extent);
>grub_free (data);
> --
> 2.17.1
>
>
>
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 3/9] btrfs: Move the error logging from find_device() to its caller.
On Sun, Jun 03, 2018 at 08:53:42PM +0200, Goffredo Baroncelli wrote: > This is a preparatory patch. The caller knows better if this > error is fatal or not, i.e. another disk is available or not. Please make first sentence last one in separate line. The same applies to other patches. Otherwise LGTM. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/9] btrfs: Add helper to check the btrfs header.
On Sun, Jun 03, 2018 at 08:53:41PM +0200, Goffredo Baroncelli wrote: > This helper will be used in a few places to help the debugging. As > conservative approach, in case of error it is only logged. This is s/, in case of error it/ the error/ > because I am not sure if this can change something in the error > handling of the currently existing code. s/code/code or not/ Otherwise LGTM. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/9] btrfs: Add support for reading a filesystem with a RAID 5 or RAID 6 profile.
On Sun, Jun 03, 2018 at 08:53:40PM +0200, Goffredo Baroncelli wrote:
> Signed-off-by: Goffredo Baroncelli
> ---
> grub-core/fs/btrfs.c | 70
> 1 file changed, 70 insertions(+)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index be195448d..4d418859b 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_DUPLICATED0x20
> #define GRUB_BTRFS_CHUNK_TYPE_RAID100x40
> +#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,74 @@ 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;
> + }
> +
> + /*
> +* Below is an example of a RAID 6 layout and the meaning of the
> +* variables. The same applies to RAID 5. The only differences is
> +* that there is only one parity disk instead of two.
> +*
> +* A RAID 6 layout consists of several stripes spread
> +* on the disks, following a layout like the one below
> +*
> +* Disk1 Disk2 Disk3 Ddisk4
Numbering seems confusing to me. I think that it should be
Disk0 Disk1 Disk2 Disk3
> +*
> +*A1 B1 P1 Q1
> +*Q2 A2 B2 P2
> +*P3 Q3 A3 B3
> +* [...]
> +*
> +* Note that the placement of the parities depends on row index.
> +* In the code below:
> +* - stripe_nr is the stripe number not considering the parities
> +*(A1=0, B1=1, A2 = 2, B2 = 3, ...),
Please be consistent. A1 = 0, B1 = 1, A2 = 2, B2 = 3, ...
> +* - high is the row number (0 for A1...Q1, 1 for Q2..P2, ...),
Ditto. Please always use "..." not "..".
> +* - stripen is the column number (or disk number),
AIUI starting from 0. Right? If yes then I think that
drawing above requires disks/columns renumbering.
> +* - off is the logical address to read (from the beginning of
> +*the chunk space),
s/chunk space/chunk/?
> +* - chunk_stripe_length is the size of a stripe (typically 64k),
> +* - nstripes is the number of disks,
> +* - low is the offset of the data inside a stripe,
> +* - stripe_offset is the offset from the beginning of the chunk
> +*disks physical address,
I am not sure that I understand. Could clarify this?
> +* - csize is the "potential" data to read. It will be reduced to
> +*size if the latter is smaller.
> +*/
> + stripe_nr = grub_divmod64 (off, chunk_stripe_length, );
OK.
> + /*
> +* stripen is evaluated without considering
> +* the parities (0 for A1, A2, A3... 1 for B1, B2...).
> +*/
> + high = grub_divmod64 (stripe_nr, nstripes - nparities, );
OK.
> + /*
> +* stripen now considers also the parities (0 for A1, 1 for A2,
> +* 2 for A3). The math is performed modulo number of disks.
> +*/
> + grub_divmod64 (high + stripen, nstripes, );
OK.
> + stripe_offset = low + chunk_stripe_length * high;
Hmmm... I am confused. What does it mean?
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
