* [PATCH V5] Add support for BTRFS raid5/6 to GRUB @ 2018-06-03 18:53 Goffredo Baroncelli 2018-06-03 18:53 ` [PATCH 1/9] btrfs: Add support for reading a filesystem with a RAID 5 or RAID 6 profile Goffredo Baroncelli ` (8 more replies) 0 siblings, 9 replies; 33+ messages in thread From: Goffredo Baroncelli @ 2018-06-03 18:53 UTC (permalink / raw) To: grub-devel 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. Changelog v1: initial support for btrfs raid5/6. No recovery allowed v2: full support for btrfs raid5/6. Recovery allowed v3: some minor cleanup suggested by Daniel Kiper; reusing the original raid6 recovery code of grub v4: Several spell fix; better description of the RAID layout in btrfs, and the variables which describes the stripe positioning; split the patch #5 in two (#5 and #6) v5: Several spell fix; improved code comment in patch #1, small clean up in the code BR G.Baroncelli -- gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/9] btrfs: Add support for reading a filesystem with a RAID 5 or RAID 6 profile. 2018-06-03 18:53 [PATCH V5] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli @ 2018-06-03 18:53 ` Goffredo Baroncelli 2018-06-14 11:17 ` Daniel Kiper 2018-06-03 18:53 ` [PATCH 2/9] btrfs: Add helper to check the btrfs header Goffredo Baroncelli ` (7 subsequent siblings) 8 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-06-03 18:53 UTC (permalink / raw) To: grub-devel; +Cc: Goffredo Baroncelli Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it> --- 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_DUPLICATED 0x20 #define GRUB_BTRFS_CHUNK_TYPE_RAID10 0x40 +#define GRUB_BTRFS_CHUNK_TYPE_RAID5 0x80 +#define GRUB_BTRFS_CHUNK_TYPE_RAID6 0x100 grub_uint8_t dummy2[0xc]; grub_uint16_t nstripes; grub_uint16_t nsubstripes; @@ -764,6 +766,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 + * + * 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, ...), + * - high is the row number (0 for A1...Q1, 1 for Q2..P2, ...), + * - stripen is the column number (or disk number), + * - off is the logical address to read (from the beginning of + * the chunk space), + * - 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, + * - 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, &low); + + /* + * stripen is evaluated without considering + * the parities (0 for A1, A2, A3... 1 for B1, B2...). + */ + high = grub_divmod64 (stripe_nr, nstripes - nparities, &stripen); + + /* + * 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, &stripen); + + stripe_offset = low + chunk_stripe_length * high; + csize = chunk_stripe_length - low; + break; } default: -- 2.17.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 1/9] btrfs: Add support for reading a filesystem with a RAID 5 or RAID 6 profile. 2018-06-03 18:53 ` [PATCH 1/9] btrfs: Add support for reading a filesystem with a RAID 5 or RAID 6 profile Goffredo Baroncelli @ 2018-06-14 11:17 ` Daniel Kiper 2018-06-14 18:55 ` Goffredo Baroncelli 0 siblings, 1 reply; 33+ messages in thread From: Daniel Kiper @ 2018-06-14 11:17 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Goffredo Baroncelli On Sun, Jun 03, 2018 at 08:53:40PM +0200, Goffredo Baroncelli wrote: > Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it> > --- > 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_DUPLICATED 0x20 > #define GRUB_BTRFS_CHUNK_TYPE_RAID10 0x40 > +#define GRUB_BTRFS_CHUNK_TYPE_RAID5 0x80 > +#define GRUB_BTRFS_CHUNK_TYPE_RAID6 0x100 > grub_uint8_t dummy2[0xc]; > grub_uint16_t nstripes; > grub_uint16_t nsubstripes; > @@ -764,6 +766,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, &low); OK. > + /* > + * stripen is evaluated without considering > + * the parities (0 for A1, A2, A3... 1 for B1, B2...). > + */ > + high = grub_divmod64 (stripe_nr, nstripes - nparities, &stripen); 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, &stripen); OK. > + stripe_offset = low + chunk_stripe_length * high; Hmmm... I am confused. What does it mean? Daniel ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/9] btrfs: Add support for reading a filesystem with a RAID 5 or RAID 6 profile. 2018-06-14 11:17 ` Daniel Kiper @ 2018-06-14 18:55 ` Goffredo Baroncelli 2018-06-19 17:30 ` Goffredo Baroncelli 0 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-06-14 18:55 UTC (permalink / raw) To: grub-devel 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 <kreijack@inwind.it> >> --- >> 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_DUPLICATED 0x20 >> #define GRUB_BTRFS_CHUNK_TYPE_RAID10 0x40 >> +#define GRUB_BTRFS_CHUNK_TYPE_RAID5 0x80 >> +#define GRUB_BTRFS_CHUNK_TYPE_RAID6 0x100 >> grub_uint8_t dummy2[0xc]; >> grub_uint16_t nstripes; >> grub_uint16_t nsubstripes; >> @@ -764,6 +766,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, &low); > > OK. > >> + /* >> + * stripen is evaluated without considering >> + * the parities (0 for A1, A2, A3... 1 for B1, B2...). >> + */ >> + high = grub_divmod64 (stripe_nr, nstripes - nparities, &stripen); > > 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, &stripen); > > 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 <kreijackATinwind.it> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/9] btrfs: Add support for reading a filesystem with a RAID 5 or RAID 6 profile. 2018-06-14 18:55 ` Goffredo Baroncelli @ 2018-06-19 17:30 ` Goffredo Baroncelli 0 siblings, 0 replies; 33+ messages in thread From: Goffredo Baroncelli @ 2018-06-19 17:30 UTC (permalink / raw) To: grub-devel On 06/14/2018 08:55 PM, Goffredo Baroncelli wrote: >>> + * - 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. > After some thoughts, I will update this description as: - * - stripe_offset is the offset from the beginning of the chunk - * disks physical address, + * - stripe_offset is the disk offset from the beginning + * of the disk chunk mapping start, -- gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 2/9] btrfs: Add helper to check the btrfs header. 2018-06-03 18:53 [PATCH V5] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli 2018-06-03 18:53 ` [PATCH 1/9] btrfs: Add support for reading a filesystem with a RAID 5 or RAID 6 profile Goffredo Baroncelli @ 2018-06-03 18:53 ` Goffredo Baroncelli 2018-06-14 11:25 ` Daniel Kiper 2018-06-03 18:53 ` [PATCH 3/9] btrfs: Move the error logging from find_device() to its caller Goffredo Baroncelli ` (6 subsequent siblings) 8 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-06-03 18:53 UTC (permalink / raw) To: grub-devel; +Cc: Goffredo Baroncelli 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 because I am not sure if this can change something in the error handling of the currently existing code. Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it> --- grub-core/fs/btrfs.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c index 4d418859b..669c49301 100644 --- a/grub-core/fs/btrfs.c +++ b/grub-core/fs/btrfs.c @@ -77,7 +77,8 @@ struct btrfs_header { grub_btrfs_checksum_t checksum; grub_btrfs_uuid_t uuid; - grub_uint8_t dummy[0x30]; + grub_uint64_t bytenr; + grub_uint8_t dummy[0x28]; grub_uint32_t nitems; grub_uint8_t level; } GRUB_PACKED; @@ -286,6 +287,25 @@ free_iterator (struct grub_btrfs_leaf_descriptor *desc) grub_free (desc->data); } +static grub_err_t +check_btrfs_header (struct grub_btrfs_data *data, struct btrfs_header *header, + grub_disk_addr_t addr) +{ + if (grub_le_to_cpu64 (header->bytenr) != addr) + { + grub_dprintf ("btrfs", "btrfs_header.bytenr is not equal node addr\n"); + return grub_error (GRUB_ERR_BAD_FS, + "header bytenr is not equal node addr"); + } + if (grub_memcmp (data->sblock.uuid, header->uuid, sizeof(grub_btrfs_uuid_t))) + { + grub_dprintf ("btrfs", "btrfs_header.uuid doesn't match sblock uuid\n"); + return grub_error (GRUB_ERR_BAD_FS, + "header uuid doesn't match sblock uuid"); + } + return GRUB_ERR_NONE; +} + static grub_err_t save_ref (struct grub_btrfs_leaf_descriptor *desc, grub_disk_addr_t addr, unsigned i, unsigned m, int l) @@ -341,6 +361,7 @@ next (struct grub_btrfs_data *data, err = grub_btrfs_read_logical (data, grub_le_to_cpu64 (node.addr), &head, sizeof (head), 0); + check_btrfs_header (data, &head, grub_le_to_cpu64 (node.addr)); if (err) return -err; @@ -402,6 +423,7 @@ lower_bound (struct grub_btrfs_data *data, /* FIXME: preread few nodes into buffer. */ err = grub_btrfs_read_logical (data, addr, &head, sizeof (head), recursion_depth + 1); + check_btrfs_header (data, &head, addr); if (err) return err; addr += sizeof (head); -- 2.17.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 2/9] btrfs: Add helper to check the btrfs header. 2018-06-03 18:53 ` [PATCH 2/9] btrfs: Add helper to check the btrfs header Goffredo Baroncelli @ 2018-06-14 11:25 ` Daniel Kiper 0 siblings, 0 replies; 33+ messages in thread From: Daniel Kiper @ 2018-06-14 11:25 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Goffredo Baroncelli 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 3/9] btrfs: Move the error logging from find_device() to its caller. 2018-06-03 18:53 [PATCH V5] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli 2018-06-03 18:53 ` [PATCH 1/9] btrfs: Add support for reading a filesystem with a RAID 5 or RAID 6 profile Goffredo Baroncelli 2018-06-03 18:53 ` [PATCH 2/9] btrfs: Add helper to check the btrfs header Goffredo Baroncelli @ 2018-06-03 18:53 ` Goffredo Baroncelli 2018-06-14 11:28 ` Daniel Kiper 2018-06-03 18:53 ` [PATCH 5/9] btrfs: Move logging code in grub_btrfs_read_logical() Goffredo Baroncelli ` (5 subsequent siblings) 8 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-06-03 18:53 UTC (permalink / raw) To: grub-devel; +Cc: Goffredo Baroncelli This is a preparatory patch. The caller knows better if this error is fatal or not, i.e. another disk is available or not. Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it> --- grub-core/fs/btrfs.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c index 669c49301..9b3a22772 100644 --- a/grub-core/fs/btrfs.c +++ b/grub-core/fs/btrfs.c @@ -604,9 +604,6 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t id, int do_rescan) grub_device_iterate (find_device_iter, &ctx); if (!ctx.dev_found) { - grub_error (GRUB_ERR_BAD_FS, - N_("couldn't find a necessary member device " - "of multi-device filesystem")); return NULL; } data->n_devices_attached++; @@ -902,6 +899,9 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, dev = find_device (data, stripe->device_id, j); if (!dev) { + grub_dprintf ("btrfs", + "couldn't find a necessary member device " + "of multi-device filesystem\n"); err = grub_errno; grub_errno = GRUB_ERR_NONE; continue; -- 2.17.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 3/9] btrfs: Move the error logging from find_device() to its caller. 2018-06-03 18:53 ` [PATCH 3/9] btrfs: Move the error logging from find_device() to its caller Goffredo Baroncelli @ 2018-06-14 11:28 ` Daniel Kiper 0 siblings, 0 replies; 33+ messages in thread From: Daniel Kiper @ 2018-06-14 11:28 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Goffredo Baroncelli 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 5/9] btrfs: Move logging code in grub_btrfs_read_logical() 2018-06-03 18:53 [PATCH V5] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli ` (2 preceding siblings ...) 2018-06-03 18:53 ` [PATCH 3/9] btrfs: Move the error logging from find_device() to its caller Goffredo Baroncelli @ 2018-06-03 18:53 ` Goffredo Baroncelli 2018-06-14 11:52 ` Daniel Kiper 2018-06-03 18:53 ` [PATCH 6/9] btrfs: Refactor the code that read from disk Goffredo Baroncelli ` (4 subsequent siblings) 8 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-06-03 18:53 UTC (permalink / raw) To: grub-devel; +Cc: Goffredo Baroncelli 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. This is a preparatory patch: in the next one it will be possible to refactor the code inside the for(;;) in an another function. Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it> --- 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", + grub_le_to_cpu64 (key->offset), + grub_le_to_cpu64 (chunk->size), + grub_le_to_cpu16 (chunk->nstripes), + grub_le_to_cpu16 (chunk->nsubstripes), + grub_le_to_cpu64 (chunk->stripe_length)); + grub_dprintf ("btrfs", "reading laddr 0x%" PRIxGRUB_UINT64_T "\n", + addr); + for (i = 0; i < redundancy; i++) { struct grub_btrfs_chunk_stripe *stripe; @@ -879,20 +891,11 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; - grub_dprintf ("btrfs", "chunk 0x%" PRIxGRUB_UINT64_T - "+0x%" PRIxGRUB_UINT64_T - " (%d stripes (%d substripes) of %" - PRIxGRUB_UINT64_T ") stripe %" PRIxGRUB_UINT64_T + grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T " maps to 0x%" PRIxGRUB_UINT64_T "\n", - grub_le_to_cpu64 (key->offset), - grub_le_to_cpu64 (chunk->size), - grub_le_to_cpu16 (chunk->nstripes), - grub_le_to_cpu16 (chunk->nsubstripes), - grub_le_to_cpu64 (chunk->stripe_length), stripen, stripe->offset); grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T - " for laddr 0x%" PRIxGRUB_UINT64_T "\n", paddr, - addr); + "\n", paddr); dev = find_device (data, stripe->device_id); if (!dev) -- 2.17.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 5/9] btrfs: Move logging code in grub_btrfs_read_logical() 2018-06-03 18:53 ` [PATCH 5/9] btrfs: Move logging code in grub_btrfs_read_logical() Goffredo Baroncelli @ 2018-06-14 11:52 ` Daniel Kiper 0 siblings, 0 replies; 33+ messages in thread From: Daniel Kiper @ 2018-06-14 11:52 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Goffredo Baroncelli 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 <kreijack@inwind.it> > --- > 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 6/9] btrfs: Refactor the code that read from disk 2018-06-03 18:53 [PATCH V5] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli ` (3 preceding siblings ...) 2018-06-03 18:53 ` [PATCH 5/9] btrfs: Move logging code in grub_btrfs_read_logical() Goffredo Baroncelli @ 2018-06-03 18:53 ` Goffredo Baroncelli 2018-06-14 12:38 ` Daniel Kiper 2018-06-03 18:53 ` [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles Goffredo Baroncelli ` (3 subsequent siblings) 8 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-06-03 18:53 UTC (permalink / raw) To: grub-devel; +Cc: Goffredo Baroncelli Move the code in charge to read the data from disk in a separate function. This helps to separate the error handling logic (which depend by 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 <kreijack@inwind.it> --- 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) +{ + + struct grub_btrfs_chunk_stripe *stripe; + grub_disk_addr_t paddr; + grub_device_t dev; + grub_err_t err; + + stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); + /* Right now the redundancy handling is easy. + With RAID5-like it will be more difficult. */ + stripe += stripen + redundancy; + + paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; + + grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T + " maps to 0x%" PRIxGRUB_UINT64_T "\n", + stripen, stripe->offset); + grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T "\n", paddr); + + dev = find_device (data, stripe->device_id); + if (!dev) + { + grub_dprintf ("btrfs", + "couldn't find a necessary member device " + "of multi-device filesystem\n"); + grub_errno = GRUB_ERR_NONE; + return GRUB_ERR_READ_ERROR; + } + + err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, + paddr & (GRUB_DISK_SECTOR_SIZE - 1), + csize, buf); + return err; +} + static grub_err_t grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, void *buf, grub_size_t size, int recursion_depth) @@ -638,7 +679,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, grub_err_t err = 0; struct grub_btrfs_key key_out; int challoc = 0; - grub_device_t dev; struct grub_btrfs_key key_in; grub_size_t chsize; grub_disk_addr_t chaddr; @@ -881,36 +921,10 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, for (i = 0; i < redundancy; i++) { - struct grub_btrfs_chunk_stripe *stripe; - grub_disk_addr_t paddr; - - stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); - /* Right now the redundancy handling is easy. - With RAID5-like it will be more difficult. */ - stripe += stripen + i; - - paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; - - grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T - " maps to 0x%" PRIxGRUB_UINT64_T "\n", - stripen, stripe->offset); - grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T - "\n", paddr); - - dev = find_device (data, stripe->device_id); - if (!dev) - { - grub_dprintf ("btrfs", - "couldn't find a necessary member device " - "of multi-device filesystem\n"); - err = grub_errno; - grub_errno = GRUB_ERR_NONE; - continue; - } - - err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, - paddr & (GRUB_DISK_SECTOR_SIZE - 1), - csize, buf); + err = btrfs_read_from_chunk (data, chunk, stripen, + stripe_offset, + i, /* redundancy */ + csize, buf); if (!err) break; grub_errno = GRUB_ERR_NONE; -- 2.17.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 6/9] btrfs: Refactor the code that read from disk 2018-06-03 18:53 ` [PATCH 6/9] btrfs: Refactor the code that read from disk Goffredo Baroncelli @ 2018-06-14 12:38 ` Daniel Kiper 0 siblings, 0 replies; 33+ messages in thread From: Daniel Kiper @ 2018-06-14 12:38 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Goffredo Baroncelli 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 <kreijack@inwind.it> > --- > 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles. 2018-06-03 18:53 [PATCH V5] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli ` (4 preceding siblings ...) 2018-06-03 18:53 ` [PATCH 6/9] btrfs: Refactor the code that read from disk Goffredo Baroncelli @ 2018-06-03 18:53 ` Goffredo Baroncelli 2018-06-14 13:03 ` Daniel Kiper 2018-06-03 18:53 ` [PATCH 8/9] btrfs: Make more generic the code for RAID 6 rebuilding Goffredo Baroncelli ` (2 subsequent siblings) 8 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-06-03 18:53 UTC (permalink / raw) To: grub-devel; +Cc: 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 <kreijack@inwind.it> --- 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 <minilzo.h> #include <grub/i18n.h> #include <grub/btrfs.h> +#include <grub/crypto.h> 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 grub_btrfs_data *data, grub_disk_addr_t addr, void *buf, grub_size_t size, int recursion_depth) @@ -743,6 +895,10 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, grub_uint16_t nstripes; unsigned redundancy = 1; unsigned i, j; + int is_raid56; + + is_raid56 = !!(grub_le_to_cpu64 (chunk->type) & + GRUB_BTRFS_CHUNK_TYPE_RAID5); if (grub_le_to_cpu64 (chunk->size) <= off) { @@ -919,17 +1075,31 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, grub_dprintf ("btrfs", "reading laddr 0x%" PRIxGRUB_UINT64_T "\n", addr); - for (i = 0; i < redundancy; i++) + if (!is_raid56) + { + for (i = 0; i < redundancy; i++) + { + err = btrfs_read_from_chunk (data, chunk, stripen, + stripe_offset, + i, /* redundancy */ + csize, buf); + if (!err) + 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) + err = raid56_read_retry (data, chunk, stripe_offset, + csize, buf); } - if (i != redundancy) + if (err == GRUB_ERR_NONE) break; } if (err) -- 2.17.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles. 2018-06-03 18:53 ` [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles Goffredo Baroncelli @ 2018-06-14 13:03 ` Daniel Kiper 2018-06-14 19:05 ` Goffredo Baroncelli 0 siblings, 1 reply; 33+ messages in thread From: Daniel Kiper @ 2018-06-14 13:03 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Goffredo Baroncelli 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 <kreijack@inwind.it> > --- > 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 <minilzo.h> > #include <grub/i18n.h> > #include <grub/btrfs.h> > +#include <grub/crypto.h> > > 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 > + ", 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: Space before the label please. I have asked about earlier. > + Please drop this empty line. Daniel ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles. 2018-06-14 13:03 ` Daniel Kiper @ 2018-06-14 19:05 ` Goffredo Baroncelli 2018-06-18 18:12 ` Goffredo Baroncelli 0 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-06-14 19:05 UTC (permalink / raw) To: Daniel Kiper, The development of GNU GRUB 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 <kreijack@inwind.it> >> --- >> 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 <minilzo.h> >> #include <grub/i18n.h> >> #include <grub/btrfs.h> >> +#include <grub/crypto.h> >> >> 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 ")\n", i, >> + stripe->device_id); >> + } >> + } >> + >> + failed_devices = 0; >> + for (i = 0; i < nstripes; i++) > > for (failed_devices = i = 0; i < nstripes; i++) Nice > >> + 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: > > Space before the label please. > I have asked about earlier. The line before the label is already a space; Am I missing something ? > >> + > > Please drop this empty line. > > Daniel > -- gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles. 2018-06-14 19:05 ` Goffredo Baroncelli @ 2018-06-18 18:12 ` Goffredo Baroncelli 2018-09-03 12:32 ` Daniel Kiper 0 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-06-18 18:12 UTC (permalink / raw) To: Daniel Kiper, The development of GNU GRUB 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 <kreijackATinwind.it> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles. 2018-06-18 18:12 ` Goffredo Baroncelli @ 2018-09-03 12:32 ` Daniel Kiper 0 siblings, 0 replies; 33+ messages in thread From: Daniel Kiper @ 2018-09-03 12:32 UTC (permalink / raw) To: Goffredo Baroncelli; +Cc: Daniel Kiper, The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 8/9] btrfs: Make more generic the code for RAID 6 rebuilding 2018-06-03 18:53 [PATCH V5] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli ` (5 preceding siblings ...) 2018-06-03 18:53 ` [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles Goffredo Baroncelli @ 2018-06-03 18:53 ` Goffredo Baroncelli 2018-06-03 18:53 ` [PATCH 9/9] btrfs: Add RAID 6 recovery for a btrfs filesystem Goffredo Baroncelli 2018-06-14 13:21 ` [PATCH V5] Add support for BTRFS raid5/6 to GRUB Daniel Kiper 8 siblings, 0 replies; 33+ messages in thread From: Goffredo Baroncelli @ 2018-06-03 18:53 UTC (permalink / raw) To: grub-devel; +Cc: Goffredo Baroncelli The original code which handles the recovery of a RAID 6 disks array assumes that all reads are multiple of 1 << GRUB_DISK_SECTOR_BITS and it assumes that all the I/O is done via the struct grub_diskfilter_segment. This is not true for the btrfs code. In order to reuse the native grub_raid6_recover() code, it is modified to not call grub_diskfilter_read_node() directly, but to call an handler passed as an argument. Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it> --- grub-core/disk/raid6_recover.c | 52 ++++++++++++++++++++++------------ include/grub/diskfilter.h | 9 ++++++ 2 files changed, 43 insertions(+), 18 deletions(-) diff --git a/grub-core/disk/raid6_recover.c b/grub-core/disk/raid6_recover.c index aa674f6ca..0cf691ddf 100644 --- a/grub-core/disk/raid6_recover.c +++ b/grub-core/disk/raid6_recover.c @@ -74,14 +74,26 @@ mod_255 (unsigned x) } static grub_err_t -grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p, - char *buf, grub_disk_addr_t sector, grub_size_t size) +raid6_recover_read_node (void *data, int disknr, + grub_uint64_t sector, + void *buf, grub_size_t size) +{ + struct grub_diskfilter_segment *array = data; + + return grub_diskfilter_read_node (&array->nodes[disknr], + (grub_disk_addr_t)sector, + size >> GRUB_DISK_SECTOR_BITS, buf); +} + +grub_err_t +grub_raid6_recover_gen (void *data, grub_uint64_t nstripes, int disknr, int p, + char *buf, grub_uint64_t sector, grub_size_t size, + int layout, raid_recover_read_t read_func) { int i, q, pos; int bad1 = -1, bad2 = -1; char *pbuf = 0, *qbuf = 0; - size <<= GRUB_DISK_SECTOR_BITS; pbuf = grub_zalloc (size); if (!pbuf) goto quit; @@ -91,17 +103,17 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p, goto quit; q = p + 1; - if (q == (int) array->node_count) + if (q == (int) nstripes) q = 0; pos = q + 1; - if (pos == (int) array->node_count) + if (pos == (int) nstripes) pos = 0; - for (i = 0; i < (int) array->node_count - 2; i++) + for (i = 0; i < (int) nstripes - 2; i++) { int c; - if (array->layout & GRUB_RAID_LAYOUT_MUL_FROM_POS) + if (layout & GRUB_RAID_LAYOUT_MUL_FROM_POS) c = pos; else c = i; @@ -109,8 +121,7 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p, bad1 = c; else { - if (! grub_diskfilter_read_node (&array->nodes[pos], sector, - size >> GRUB_DISK_SECTOR_BITS, buf)) + if (!read_func(data, pos, sector, buf, size)) { grub_crypto_xor (pbuf, pbuf, buf, size); grub_raid_block_mulx (c, buf, size); @@ -128,7 +139,7 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p, } pos++; - if (pos == (int) array->node_count) + if (pos == (int) nstripes) pos = 0; } @@ -139,16 +150,14 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p, if (bad2 < 0) { /* One bad device */ - if ((! grub_diskfilter_read_node (&array->nodes[p], sector, - size >> GRUB_DISK_SECTOR_BITS, buf))) + if (!read_func(data, p, sector, buf, size)) { grub_crypto_xor (buf, buf, pbuf, size); goto quit; } grub_errno = GRUB_ERR_NONE; - if (grub_diskfilter_read_node (&array->nodes[q], sector, - size >> GRUB_DISK_SECTOR_BITS, buf)) + if (read_func(data, q, sector, buf, size)) goto quit; grub_crypto_xor (buf, buf, qbuf, size); @@ -160,14 +169,12 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p, /* Two bad devices */ unsigned c; - if (grub_diskfilter_read_node (&array->nodes[p], sector, - size >> GRUB_DISK_SECTOR_BITS, buf)) + if (read_func(data, p, sector, buf, size)) goto quit; grub_crypto_xor (pbuf, pbuf, buf, size); - if (grub_diskfilter_read_node (&array->nodes[q], sector, - size >> GRUB_DISK_SECTOR_BITS, buf)) + if (read_func(data, q, sector, buf, size)) goto quit; grub_crypto_xor (qbuf, qbuf, buf, size); @@ -190,6 +197,15 @@ quit: return grub_errno; } +static grub_err_t +grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p, + char *buf, grub_disk_addr_t sector, grub_size_t size) +{ + return grub_raid6_recover_gen (array, array->node_count, disknr, p, buf, + sector, size << GRUB_DISK_SECTOR_BITS, + array->layout, raid6_recover_read_node); +} + GRUB_MOD_INIT(raid6rec) { grub_raid6_init_table (); diff --git a/include/grub/diskfilter.h b/include/grub/diskfilter.h index d89273c1b..8deb1a8c3 100644 --- a/include/grub/diskfilter.h +++ b/include/grub/diskfilter.h @@ -189,6 +189,15 @@ typedef grub_err_t (*grub_raid6_recover_func_t) (struct grub_diskfilter_segment extern grub_raid5_recover_func_t grub_raid5_recover_func; extern grub_raid6_recover_func_t grub_raid6_recover_func; +typedef grub_err_t (* raid_recover_read_t)(void *data, int disk_nr, + grub_uint64_t addr, void *dest, + grub_size_t size); + +extern grub_err_t +grub_raid6_recover_gen (void *data, grub_uint64_t nstripes, int disknr, int p, + char *buf, grub_uint64_t sector, grub_size_t size, + int layout, raid_recover_read_t read_func); + grub_err_t grub_diskfilter_vg_register (struct grub_diskfilter_vg *vg); grub_err_t -- 2.17.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 9/9] btrfs: Add RAID 6 recovery for a btrfs filesystem. 2018-06-03 18:53 [PATCH V5] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli ` (6 preceding siblings ...) 2018-06-03 18:53 ` [PATCH 8/9] btrfs: Make more generic the code for RAID 6 rebuilding Goffredo Baroncelli @ 2018-06-03 18:53 ` Goffredo Baroncelli 2018-06-14 13:11 ` Daniel Kiper 2018-06-14 13:21 ` [PATCH V5] Add support for BTRFS raid5/6 to GRUB Daniel Kiper 8 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-06-03 18:53 UTC (permalink / raw) To: grub-devel; +Cc: Goffredo Baroncelli 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 <kreijack@inwind.it> --- 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 <grub/i18n.h> #include <grub/btrfs.h> #include <grub/crypto.h> +#include <grub/diskfilter.h> 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; +} + +static void +rebuild_raid6 (struct raid56_buffer *buffers, grub_uint64_t nstripes, + grub_uint64_t csize, grub_uint64_t parities_pos, void *dest, + grub_uint64_t stripen) + +{ + grub_raid6_recover_gen (buffers, nstripes, stripen, parities_pos, + dest, 0, csize, 0, raid6_recover_read_buffer); +} + 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) + grub_uint64_t stripe_offset, grub_uint64_t stripen, + grub_uint64_t csize, void *buf, grub_uint64_t parities_pos) { struct raid56_buffer *buffers = NULL; @@ -791,6 +816,15 @@ raid56_read_retry (struct grub_btrfs_data *data, ret = GRUB_ERR_READ_ERROR; goto cleanup; } + else if (failed_devices > 2 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID6)) + { + grub_dprintf ("btrfs", + "not enough disks for raid6: total %" PRIuGRUB_UINT64_T + ", missing %" PRIuGRUB_UINT64_T "\n", + nstripes, failed_devices); + ret = GRUB_ERR_READ_ERROR; + goto cleanup; + } else { grub_dprintf ("btrfs", @@ -803,7 +837,7 @@ raid56_read_retry (struct grub_btrfs_data *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"); + rebuild_raid6 (buffers, nstripes, csize, parities_pos, buf, stripen); cleanup: @@ -896,9 +930,11 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, unsigned redundancy = 1; unsigned i, j; int is_raid56; + grub_uint64_t parities_pos = 0; - is_raid56 = !!(grub_le_to_cpu64 (chunk->type) & - GRUB_BTRFS_CHUNK_TYPE_RAID5); + is_raid56 = !!(grub_le_to_cpu64 (chunk->type) & + (GRUB_BTRFS_CHUNK_TYPE_RAID5 | + GRUB_BTRFS_CHUNK_TYPE_RAID6)); if (grub_le_to_cpu64 (chunk->size) <= off) { @@ -1043,6 +1079,8 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, * 2 for A3....). The math is performed modulo number of disks. */ grub_divmod64 (high + stripen, nstripes, &stripen); + grub_divmod64 (high + nstripes - nparities, nstripes, + &parities_pos); stripe_offset = low + chunk_stripe_length * high; csize = chunk_stripe_length - low; @@ -1097,7 +1135,7 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, grub_errno = GRUB_ERR_NONE; if (err != GRUB_ERR_NONE) err = raid56_read_retry (data, chunk, stripe_offset, - csize, buf); + stripen, csize, buf, parities_pos); } if (err == GRUB_ERR_NONE) break; -- 2.17.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 9/9] btrfs: Add RAID 6 recovery for a btrfs filesystem. 2018-06-03 18:53 ` [PATCH 9/9] btrfs: Add RAID 6 recovery for a btrfs filesystem Goffredo Baroncelli @ 2018-06-14 13:11 ` Daniel Kiper 0 siblings, 0 replies; 33+ messages in thread From: Daniel Kiper @ 2018-06-14 13:11 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Goffredo Baroncelli 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 <kreijack@inwind.it> > --- > 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 <grub/i18n.h> > #include <grub/btrfs.h> > #include <grub/crypto.h> > +#include <grub/diskfilter.h> > > 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH V5] Add support for BTRFS raid5/6 to GRUB 2018-06-03 18:53 [PATCH V5] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli ` (7 preceding siblings ...) 2018-06-03 18:53 ` [PATCH 9/9] btrfs: Add RAID 6 recovery for a btrfs filesystem Goffredo Baroncelli @ 2018-06-14 13:21 ` Daniel Kiper 2018-06-14 18:06 ` Goffredo Baroncelli 8 siblings, 1 reply; 33+ messages in thread From: Daniel Kiper @ 2018-06-14 13:21 UTC (permalink / raw) To: kreijack; +Cc: grub-devel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH V5] Add support for BTRFS raid5/6 to GRUB 2018-06-14 13:21 ` [PATCH V5] Add support for BTRFS raid5/6 to GRUB Daniel Kiper @ 2018-06-14 18:06 ` Goffredo Baroncelli 0 siblings, 0 replies; 33+ messages in thread From: Goffredo Baroncelli @ 2018-06-14 18:06 UTC (permalink / raw) To: Daniel Kiper; +Cc: grub-devel 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 <kreijackATinwind.it> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH V11] Add support for BTRFS raid5/6 to GRUB @ 2018-10-22 17:29 Goffredo Baroncelli 2018-10-22 17:29 ` [PATCH 6/9] btrfs: Refactor the code that read from disk Goffredo Baroncelli 0 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-10-22 17:29 UTC (permalink / raw) To: grub-devel; +Cc: Daniel Kiper, linux-btrfs 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. Changelog v1: initial support for btrfs raid5/6. No recovery allowed v2: full support for btrfs raid5/6. Recovery allowed v3: some minor cleanup suggested by Daniel Kiper; reusing the original raid6 recovery code of grub v4: Several spell fix; better description of the RAID layout in btrfs, and the variables which describes the stripe positioning; split the patch #5 in two (#5 and #6) v5: Several spell fix; improved code comment in patch #1, small clean up in the code v6: Small cleanup; improved the wording in the RAID6 layout description; in the function raid6_recover_read_buffer() avoid a unnecessary memcpy in case of invalid data; v7: - patch 2,3,5,6,8 received an Review-by Daniel, and were unchanged from the last time (only minor cleanup in the commit description requested by Daniel) - patch 7 received some small update rearranging a for(), and some bracket around if() - patch 4, received an update message which explains better why NULL is stored in data->devices_attached[] - patch 9, received a blank line to separate better a code line from a previous comment. A description of 'parities_pos' was added - patch 1, received a major update about the variable meaning description in the comment. However I suspect that we need some further review to reach a fully agreement about this text. NB: the update are relate only to comments v8: - patch 2,5,6,8 received an Review-by Daniel, and were unchanged from the last time (only minor cleanup in the commit description requested by Daniel) - patch 1 received some adjustement to the variables description due to the different terminology between BTRFS and other RAID implementatio. Added a description for the "nparities" variable. - patch 3 removed some unnecessary curly brackets (change request by Daniel) - patch 4 received an improved commit description about why and how the function find_device() is changed - patch 7 received an update which transforms a i = 0; while(i..) i++; in for( i = 0..... ; i++); - patch 9 received an update to the comment v9: - patch 1: update comments - patch 4: update commit messages - patch 7: added a comment about accessing an array of structs after another structs; changed if(err == GRUB_ERR_NONE) in if(!err) changed if(err != GRUB_ERR_NONE) in if(err) v10:- patch 1: update comments (replace might with can) - patch 4: add a Signed off by Daniel - patch 7: drop an empty line; changed some text in grub_dprintf; reversed the logic of an if (if(!is_raid56) A else B -> if(is_raid5) B else A); add a space between the function name and the '(' - patch 9: update the wording in the comment; s/raid6/RAID 6/ in grub_dprintf() v11:- patch 7: removed redundant buffers[i].data_is_valid = 0; (2x); updated log messages (READ FAILED instead of FAILED) BR G.Baroncelli ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 6/9] btrfs: Refactor the code that read from disk 2018-10-22 17:29 [PATCH V11] " Goffredo Baroncelli @ 2018-10-22 17:29 ` Goffredo Baroncelli 0 siblings, 0 replies; 33+ messages in thread From: Goffredo Baroncelli @ 2018-10-22 17:29 UTC (permalink / raw) To: grub-devel; +Cc: Daniel Kiper, linux-btrfs, Goffredo Baroncelli From: Goffredo Baroncelli <kreijack@inwind.it> Move the code in charge to read the data from disk into a separate function. This helps to separate the error handling logic (which depends on 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 <kreijack@inwind.it> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> --- grub-core/fs/btrfs.c | 75 ++++++++++++++++++++++++++------------------ 1 file changed, 44 insertions(+), 31 deletions(-) diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c index dde0edd03..ea97f0502 100644 --- a/grub-core/fs/btrfs.c +++ b/grub-core/fs/btrfs.c @@ -625,6 +625,46 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t id) return ctx.dev_found; } +static grub_err_t +btrfs_read_from_chunk (struct grub_btrfs_data *data, + struct grub_btrfs_chunk_item *chunk, + grub_uint64_t stripen, grub_uint64_t stripe_offset, + int redundancy, grub_uint64_t csize, + void *buf) +{ + struct grub_btrfs_chunk_stripe *stripe; + grub_disk_addr_t paddr; + grub_device_t dev; + grub_err_t err; + + stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); + /* Right now the redundancy handling is easy. + With RAID5-like it will be more difficult. */ + stripe += stripen + redundancy; + + paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; + + grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T + " maps to 0x%" PRIxGRUB_UINT64_T + ". Reading paddr 0x%" PRIxGRUB_UINT64_T "\n", + stripen, stripe->offset, paddr); + + dev = find_device (data, stripe->device_id); + if (!dev) + { + grub_dprintf ("btrfs", + "couldn't find a necessary member device " + "of multi-device filesystem\n"); + grub_errno = GRUB_ERR_NONE; + return GRUB_ERR_READ_ERROR; + } + + err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, + paddr & (GRUB_DISK_SECTOR_SIZE - 1), + csize, buf); + return err; +} + static grub_err_t grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, void *buf, grub_size_t size, int recursion_depth) @@ -638,7 +678,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, grub_err_t err = 0; struct grub_btrfs_key key_out; int challoc = 0; - grub_device_t dev; struct grub_btrfs_key key_in; grub_size_t chsize; grub_disk_addr_t chaddr; @@ -884,36 +923,10 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, for (i = 0; i < redundancy; i++) { - struct grub_btrfs_chunk_stripe *stripe; - grub_disk_addr_t paddr; - - stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); - /* Right now the redundancy handling is easy. - With RAID5-like it will be more difficult. */ - stripe += stripen + i; - - paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; - - grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T - " maps to 0x%" PRIxGRUB_UINT64_T "\n", - stripen, stripe->offset); - grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T - "\n", paddr); - - dev = find_device (data, stripe->device_id); - if (!dev) - { - grub_dprintf ("btrfs", - "couldn't find a necessary member device " - "of multi-device filesystem\n"); - err = grub_errno; - grub_errno = GRUB_ERR_NONE; - continue; - } - - err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, - paddr & (GRUB_DISK_SECTOR_SIZE - 1), - csize, buf); + err = btrfs_read_from_chunk (data, chunk, stripen, + stripe_offset, + i, /* redundancy */ + csize, buf); if (!err) break; grub_errno = GRUB_ERR_NONE; -- 2.19.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH V10] Add support for BTRFS raid5/6 to GRUB @ 2018-10-18 17:55 Goffredo Baroncelli 2018-10-18 17:55 ` [PATCH 6/9] btrfs: Refactor the code that read from disk Goffredo Baroncelli 0 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-10-18 17:55 UTC (permalink / raw) To: grub-devel; +Cc: Daniel Kiper, linux-btrfs 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. Changelog v1: initial support for btrfs raid5/6. No recovery allowed v2: full support for btrfs raid5/6. Recovery allowed v3: some minor cleanup suggested by Daniel Kiper; reusing the original raid6 recovery code of grub v4: Several spell fix; better description of the RAID layout in btrfs, and the variables which describes the stripe positioning; split the patch #5 in two (#5 and #6) v5: Several spell fix; improved code comment in patch #1, small clean up in the code v6: Small cleanup; improved the wording in the RAID6 layout description; in the function raid6_recover_read_buffer() avoid a unnecessary memcpy in case of invalid data; v7: - patch 2,3,5,6,8 received an Review-by Daniel, and were unchanged from the last time (only minor cleanup in the commit description requested by Daniel) - patch 7 received some small update rearranging a for(), and some bracket around if() - patch 4, received an update message which explains better why NULL is stored in data->devices_attached[] - patch 9, received a blank line to separate better a code line from a previous comment. A description of 'parities_pos' was added - patch 1, received a major update about the variable meaning description in the comment. However I suspect that we need some further review to reach a fully agreement about this text. NB: the update are relate only to comments v8: - patch 2,5,6,8 received an Review-by Daniel, and were unchanged from the last time (only minor cleanup in the commit description requested by Daniel) - patch 1 received some adjustement to the variables description due to the different terminology between BTRFS and other RAID implementatio. Added a description for the "nparities" variable. - patch 3 removed some unnecessary curly brackets (change request by Daniel) - patch 4 received an improved commit description about why and how the function find_device() is changed - patch 7 received an update which transforms a i = 0; while(i..) i++; in for( i = 0..... ; i++); - patch 9 received an update to the comment v9: - patch 1: update comments - patch 4: update commit messages - patch 7: added a comment about accessing an array of structs after another structs; changed if(err == GRUB_ERR_NONE) in if(!err) changed if(err != GRUB_ERR_NONE) in if(err) v10:- patch 1: update comments (replace might with can) - patch 4: add a Signed off by Daniel - patch 7: drop an empty line; changed some text in grub_dprintf; reversed the logic of an if (if(!is_raid56) A else B -> if(is_raid5) B else A); add a space between the function name and the '(' - patch 9: update the wording in the comment; s/raid6/RAID 6/ in grub_dprintf() BR G.Baroncelli -- gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 6/9] btrfs: Refactor the code that read from disk 2018-10-18 17:55 [PATCH V10] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli @ 2018-10-18 17:55 ` Goffredo Baroncelli 0 siblings, 0 replies; 33+ messages in thread From: Goffredo Baroncelli @ 2018-10-18 17:55 UTC (permalink / raw) To: grub-devel; +Cc: Daniel Kiper, linux-btrfs, Goffredo Baroncelli From: Goffredo Baroncelli <kreijack@inwind.it> Move the code in charge to read the data from disk into a separate function. This helps to separate the error handling logic (which depends on 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 <kreijack@inwind.it> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> --- grub-core/fs/btrfs.c | 75 ++++++++++++++++++++++++++------------------ 1 file changed, 44 insertions(+), 31 deletions(-) diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c index dde0edd03..ea97f0502 100644 --- a/grub-core/fs/btrfs.c +++ b/grub-core/fs/btrfs.c @@ -625,6 +625,46 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t id) return ctx.dev_found; } +static grub_err_t +btrfs_read_from_chunk (struct grub_btrfs_data *data, + struct grub_btrfs_chunk_item *chunk, + grub_uint64_t stripen, grub_uint64_t stripe_offset, + int redundancy, grub_uint64_t csize, + void *buf) +{ + struct grub_btrfs_chunk_stripe *stripe; + grub_disk_addr_t paddr; + grub_device_t dev; + grub_err_t err; + + stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); + /* Right now the redundancy handling is easy. + With RAID5-like it will be more difficult. */ + stripe += stripen + redundancy; + + paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; + + grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T + " maps to 0x%" PRIxGRUB_UINT64_T + ". Reading paddr 0x%" PRIxGRUB_UINT64_T "\n", + stripen, stripe->offset, paddr); + + dev = find_device (data, stripe->device_id); + if (!dev) + { + grub_dprintf ("btrfs", + "couldn't find a necessary member device " + "of multi-device filesystem\n"); + grub_errno = GRUB_ERR_NONE; + return GRUB_ERR_READ_ERROR; + } + + err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, + paddr & (GRUB_DISK_SECTOR_SIZE - 1), + csize, buf); + return err; +} + static grub_err_t grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, void *buf, grub_size_t size, int recursion_depth) @@ -638,7 +678,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, grub_err_t err = 0; struct grub_btrfs_key key_out; int challoc = 0; - grub_device_t dev; struct grub_btrfs_key key_in; grub_size_t chsize; grub_disk_addr_t chaddr; @@ -884,36 +923,10 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, for (i = 0; i < redundancy; i++) { - struct grub_btrfs_chunk_stripe *stripe; - grub_disk_addr_t paddr; - - stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); - /* Right now the redundancy handling is easy. - With RAID5-like it will be more difficult. */ - stripe += stripen + i; - - paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; - - grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T - " maps to 0x%" PRIxGRUB_UINT64_T "\n", - stripen, stripe->offset); - grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T - "\n", paddr); - - dev = find_device (data, stripe->device_id); - if (!dev) - { - grub_dprintf ("btrfs", - "couldn't find a necessary member device " - "of multi-device filesystem\n"); - err = grub_errno; - grub_errno = GRUB_ERR_NONE; - continue; - } - - err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, - paddr & (GRUB_DISK_SECTOR_SIZE - 1), - csize, buf); + err = btrfs_read_from_chunk (data, chunk, stripen, + stripe_offset, + i, /* redundancy */ + csize, buf); if (!err) break; grub_errno = GRUB_ERR_NONE; -- 2.19.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH V9] Add support for BTRFS raid5/6 to GRUB @ 2018-10-11 18:50 Goffredo Baroncelli 2018-10-11 18:51 ` [PATCH 6/9] btrfs: Refactor the code that read from disk Goffredo Baroncelli 0 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-10-11 18:50 UTC (permalink / raw) To: grub-devel; +Cc: Daniel Kiper, linux-btrfs 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. Changelog v1: initial support for btrfs raid5/6. No recovery allowed v2: full support for btrfs raid5/6. Recovery allowed v3: some minor cleanup suggested by Daniel Kiper; reusing the original raid6 recovery code of grub v4: Several spell fix; better description of the RAID layout in btrfs, and the variables which describes the stripe positioning; split the patch #5 in two (#5 and #6) v5: Several spell fix; improved code comment in patch #1, small clean up in the code v6: Small cleanup; improved the wording in the RAID6 layout description; in the function raid6_recover_read_buffer() avoid a unnecessary memcpy in case of invalid data; v7: - patch 2,3,5,6,8 received an Review-by Daniel, and were unchanged from the last time (only minor cleanup in the commit description requested by Daniel) - patch 7 received some small update rearranging a for(), and some bracket around if() - patch 4, received an update message which explains better why NULL is stored in data->devices_attached[] - patch 9, received a blank line to separate better a code line from a previous comment. A description of 'parities_pos' was added - patch 1, received a major update about the variable meaning description in the comment. However I suspect that we need some further review to reach a fully agreement about this text. NB: the update are relate only to comments v8: - patch 2,5,6,8 received an Review-by Daniel, and were unchanged from the last time (only minor cleanup in the commit description requested by Daniel) - patch 1 received some adjustement to the variables description due to the different terminology between BTRFS and other RAID implementatio. Added a description for the "nparities" variable. - patch 3 removed some unnecessary curly brackets (change request by Daniel) - patch 4 received an improved commit description about why and how the function find_device() is changed - patch 7 received an update which transforms a i = 0; while(i..) i++; in for( i = 0..... ; i++); - patch 9 received an update to the comment v9: - patch 1: update comments - patch 4: update commit messages - patch 7: added a comment about accessing an array of structs after another structs; changed if(err == GRUB_ERR_NONE) in if(!err) changed if(err != GRUB_ERR_NONE) in if(err) BR G.Baroncelli -- gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 6/9] btrfs: Refactor the code that read from disk 2018-10-11 18:50 [PATCH V9] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli @ 2018-10-11 18:51 ` Goffredo Baroncelli 0 siblings, 0 replies; 33+ messages in thread From: Goffredo Baroncelli @ 2018-10-11 18:51 UTC (permalink / raw) To: grub-devel; +Cc: Daniel Kiper, linux-btrfs, Goffredo Baroncelli From: Goffredo Baroncelli <kreijack@inwind.it> Move the code in charge to read the data from disk into a separate function. This helps to separate the error handling logic (which depends on 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 <kreijack@inwind.it> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> --- grub-core/fs/btrfs.c | 75 ++++++++++++++++++++++++++------------------ 1 file changed, 44 insertions(+), 31 deletions(-) diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c index a82211ccc..899dc32b7 100644 --- a/grub-core/fs/btrfs.c +++ b/grub-core/fs/btrfs.c @@ -625,6 +625,46 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t id) return ctx.dev_found; } +static grub_err_t +btrfs_read_from_chunk (struct grub_btrfs_data *data, + struct grub_btrfs_chunk_item *chunk, + grub_uint64_t stripen, grub_uint64_t stripe_offset, + int redundancy, grub_uint64_t csize, + void *buf) +{ + struct grub_btrfs_chunk_stripe *stripe; + grub_disk_addr_t paddr; + grub_device_t dev; + grub_err_t err; + + stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); + /* Right now the redundancy handling is easy. + With RAID5-like it will be more difficult. */ + stripe += stripen + redundancy; + + paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; + + grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T + " maps to 0x%" PRIxGRUB_UINT64_T + ". Reading paddr 0x%" PRIxGRUB_UINT64_T "\n", + stripen, stripe->offset, paddr); + + dev = find_device (data, stripe->device_id); + if (!dev) + { + grub_dprintf ("btrfs", + "couldn't find a necessary member device " + "of multi-device filesystem\n"); + grub_errno = GRUB_ERR_NONE; + return GRUB_ERR_READ_ERROR; + } + + err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, + paddr & (GRUB_DISK_SECTOR_SIZE - 1), + csize, buf); + return err; +} + static grub_err_t grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, void *buf, grub_size_t size, int recursion_depth) @@ -638,7 +678,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, grub_err_t err = 0; struct grub_btrfs_key key_out; int challoc = 0; - grub_device_t dev; struct grub_btrfs_key key_in; grub_size_t chsize; grub_disk_addr_t chaddr; @@ -884,36 +923,10 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, for (i = 0; i < redundancy; i++) { - struct grub_btrfs_chunk_stripe *stripe; - grub_disk_addr_t paddr; - - stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); - /* Right now the redundancy handling is easy. - With RAID5-like it will be more difficult. */ - stripe += stripen + i; - - paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; - - grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T - " maps to 0x%" PRIxGRUB_UINT64_T "\n", - stripen, stripe->offset); - grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T - "\n", paddr); - - dev = find_device (data, stripe->device_id); - if (!dev) - { - grub_dprintf ("btrfs", - "couldn't find a necessary member device " - "of multi-device filesystem\n"); - err = grub_errno; - grub_errno = GRUB_ERR_NONE; - continue; - } - - err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, - paddr & (GRUB_DISK_SECTOR_SIZE - 1), - csize, buf); + err = btrfs_read_from_chunk (data, chunk, stripen, + stripe_offset, + i, /* redundancy */ + csize, buf); if (!err) break; grub_errno = GRUB_ERR_NONE; -- 2.19.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH V8] Add support for BTRFS raid5/6 to GRUB @ 2018-09-27 18:34 Goffredo Baroncelli 2018-09-27 18:35 ` [PATCH 6/9] btrfs: Refactor the code that read from disk Goffredo Baroncelli 0 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-09-27 18:34 UTC (permalink / raw) To: grub-devel; +Cc: Daniel Kiper, linux-btrfs i 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. Changelog v1: initial support for btrfs raid5/6. No recovery allowed v2: full support for btrfs raid5/6. Recovery allowed v3: some minor cleanup suggested by Daniel Kiper; reusing the original raid6 recovery code of grub v4: Several spell fix; better description of the RAID layout in btrfs, and the variables which describes the stripe positioning; split the patch #5 in two (#5 and #6) v5: Several spell fix; improved code comment in patch #1, small clean up in the code v6: Small cleanup; improved the wording in the RAID6 layout description; in the function raid6_recover_read_buffer() avoid a unnecessary memcpy in case of invalid data; v7: - patch 2,3,5,6,8 received an Review-by Daniel, and were unchanged from the last time (only minor cleanup in the commit description requested by Daniel) - patch 7 received some small update rearranging a for(), and some bracket around if() - patch 4, received an update message which explains better why NULL is stored in data->devices_attached[] - patch 9, received a blank line to separate better a code line from a previous comment. A description of 'parities_pos' was added - patch 1, received a major update about the variable meaning description in the comment. However I suspect that we need some further review to reach a fully agreement about this text. NB: the update are relate only to comments v8: - patch 2,5,6,8 received an Review-by Daniel, and were unchanged from the last time (only minor cleanup in the commit description requested by Daniel) - patch 1 received some adjustement to the variables description due to the different terminology between BTRFS and other RAID implementatio. Added a description for the "nparities" variable. - patch 3 removed some unnecessary curly brackets (change request by Daniel) - patch 4 received an improved commit description about why and how the function find_device() is changed - patch 7 received an update which transforms a i = 0; while(i..) i++; in for( i = 0..... ; i++); - patch 9 received an update to the comment BR G.Baroncelli -- gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 6/9] btrfs: Refactor the code that read from disk 2018-09-27 18:34 [PATCH V8] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli @ 2018-09-27 18:35 ` Goffredo Baroncelli 0 siblings, 0 replies; 33+ messages in thread From: Goffredo Baroncelli @ 2018-09-27 18:35 UTC (permalink / raw) To: grub-devel; +Cc: Daniel Kiper, linux-btrfs, Goffredo Baroncelli From: Goffredo Baroncelli <kreijack@inwind.it> Move the code in charge to read the data from disk into a separate function. This helps to separate the error handling logic (which depends on 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 <kreijack@inwind.it> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> --- grub-core/fs/btrfs.c | 75 ++++++++++++++++++++++++++------------------ 1 file changed, 44 insertions(+), 31 deletions(-) diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c index 179a2da9d..554f350c5 100644 --- a/grub-core/fs/btrfs.c +++ b/grub-core/fs/btrfs.c @@ -625,6 +625,46 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t id) return ctx.dev_found; } +static grub_err_t +btrfs_read_from_chunk (struct grub_btrfs_data *data, + struct grub_btrfs_chunk_item *chunk, + grub_uint64_t stripen, grub_uint64_t stripe_offset, + int redundancy, grub_uint64_t csize, + void *buf) +{ + struct grub_btrfs_chunk_stripe *stripe; + grub_disk_addr_t paddr; + grub_device_t dev; + grub_err_t err; + + stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); + /* Right now the redundancy handling is easy. + With RAID5-like it will be more difficult. */ + stripe += stripen + redundancy; + + paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; + + grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T + " maps to 0x%" PRIxGRUB_UINT64_T + ". Reading paddr 0x%" PRIxGRUB_UINT64_T "\n", + stripen, stripe->offset, paddr); + + dev = find_device (data, stripe->device_id); + if (!dev) + { + grub_dprintf ("btrfs", + "couldn't find a necessary member device " + "of multi-device filesystem\n"); + grub_errno = GRUB_ERR_NONE; + return GRUB_ERR_READ_ERROR; + } + + err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, + paddr & (GRUB_DISK_SECTOR_SIZE - 1), + csize, buf); + return err; +} + static grub_err_t grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, void *buf, grub_size_t size, int recursion_depth) @@ -638,7 +678,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, grub_err_t err = 0; struct grub_btrfs_key key_out; int challoc = 0; - grub_device_t dev; struct grub_btrfs_key key_in; grub_size_t chsize; grub_disk_addr_t chaddr; @@ -885,36 +924,10 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, for (i = 0; i < redundancy; i++) { - struct grub_btrfs_chunk_stripe *stripe; - grub_disk_addr_t paddr; - - stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); - /* Right now the redundancy handling is easy. - With RAID5-like it will be more difficult. */ - stripe += stripen + i; - - paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; - - grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T - " maps to 0x%" PRIxGRUB_UINT64_T "\n", - stripen, stripe->offset); - grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T - "\n", paddr); - - dev = find_device (data, stripe->device_id); - if (!dev) - { - grub_dprintf ("btrfs", - "couldn't find a necessary member device " - "of multi-device filesystem\n"); - err = grub_errno; - grub_errno = GRUB_ERR_NONE; - continue; - } - - err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, - paddr & (GRUB_DISK_SECTOR_SIZE - 1), - csize, buf); + err = btrfs_read_from_chunk (data, chunk, stripen, + stripe_offset, + i, /* redundancy */ + csize, buf); if (!err) break; grub_errno = GRUB_ERR_NONE; -- 2.19.0 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH V7] Add support for BTRFS raid5/6 to GRUB @ 2018-09-19 18:40 Goffredo Baroncelli 2018-09-19 18:40 ` [PATCH 6/9] btrfs: Refactor the code that read from disk Goffredo Baroncelli 0 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-09-19 18:40 UTC (permalink / raw) To: grub-devel; +Cc: Daniel Kiper, linux-btrfs 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. Changelog v1: initial support for btrfs raid5/6. No recovery allowed v2: full support for btrfs raid5/6. Recovery allowed v3: some minor cleanup suggested by Daniel Kiper; reusing the original raid6 recovery code of grub v4: Several spell fix; better description of the RAID layout in btrfs, and the variables which describes the stripe positioning; split the patch #5 in two (#5 and #6) v5: Several spell fix; improved code comment in patch #1, small clean up in the code v6: Small cleanup; improved the wording in the RAID6 layout description; in the function raid6_recover_read_buffer() avoid a unnecessary memcpy in case of invalid data; v7: - patch 2,3,5,6,8 received an Review-by Daniel, and were unchanged from the last time (only minor cleanup in the commit description requested by Daniel) - patch 7 received some small update rearranging a for(), and some bracket around if() - patch 4, received an update message which explains better why NULL is stored in data->devices_attached[] - patch 9, received a blank line to separate better a code line from a previous comment. A description of 'parities_pos' was added - patch 1, received a major update about the variable meaning description in the comment. However I suspect that we need some further review to reach a fully agreement about this text. NB: the update are relate only to comments BR G.Baroncelli -- gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 6/9] btrfs: Refactor the code that read from disk 2018-09-19 18:40 [PATCH V7] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli @ 2018-09-19 18:40 ` Goffredo Baroncelli 0 siblings, 0 replies; 33+ messages in thread From: Goffredo Baroncelli @ 2018-09-19 18:40 UTC (permalink / raw) To: grub-devel; +Cc: Daniel Kiper, linux-btrfs, Goffredo Baroncelli From: Goffredo Baroncelli <kreijack@inwind.it> Move the code in charge to read the data from disk into a separate function. This helps to separate the error handling logic (which depends on 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 <kreijack@inwind.it> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> --- grub-core/fs/btrfs.c | 75 ++++++++++++++++++++++++++------------------ 1 file changed, 44 insertions(+), 31 deletions(-) diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c index ee134c167..5c1ebae77 100644 --- a/grub-core/fs/btrfs.c +++ b/grub-core/fs/btrfs.c @@ -625,6 +625,46 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t id) return ctx.dev_found; } +static grub_err_t +btrfs_read_from_chunk (struct grub_btrfs_data *data, + struct grub_btrfs_chunk_item *chunk, + grub_uint64_t stripen, grub_uint64_t stripe_offset, + int redundancy, grub_uint64_t csize, + void *buf) +{ + struct grub_btrfs_chunk_stripe *stripe; + grub_disk_addr_t paddr; + grub_device_t dev; + grub_err_t err; + + stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); + /* Right now the redundancy handling is easy. + With RAID5-like it will be more difficult. */ + stripe += stripen + redundancy; + + paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; + + grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T + " maps to 0x%" PRIxGRUB_UINT64_T + ". Reading paddr 0x%" PRIxGRUB_UINT64_T "\n", + stripen, stripe->offset, paddr); + + dev = find_device (data, stripe->device_id); + if (!dev) + { + grub_dprintf ("btrfs", + "couldn't find a necessary member device " + "of multi-device filesystem\n"); + grub_errno = GRUB_ERR_NONE; + return GRUB_ERR_READ_ERROR; + } + + err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, + paddr & (GRUB_DISK_SECTOR_SIZE - 1), + csize, buf); + return err; +} + static grub_err_t grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, void *buf, grub_size_t size, int recursion_depth) @@ -638,7 +678,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, grub_err_t err = 0; struct grub_btrfs_key key_out; int challoc = 0; - grub_device_t dev; struct grub_btrfs_key key_in; grub_size_t chsize; grub_disk_addr_t chaddr; @@ -877,36 +916,10 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, for (i = 0; i < redundancy; i++) { - struct grub_btrfs_chunk_stripe *stripe; - grub_disk_addr_t paddr; - - stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); - /* Right now the redundancy handling is easy. - With RAID5-like it will be more difficult. */ - stripe += stripen + i; - - paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; - - grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T - " maps to 0x%" PRIxGRUB_UINT64_T "\n", - stripen, stripe->offset); - grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T - "\n", paddr); - - dev = find_device (data, stripe->device_id); - if (!dev) - { - grub_dprintf ("btrfs", - "couldn't find a necessary member device " - "of multi-device filesystem\n"); - err = grub_errno; - grub_errno = GRUB_ERR_NONE; - continue; - } - - err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, - paddr & (GRUB_DISK_SECTOR_SIZE - 1), - csize, buf); + err = btrfs_read_from_chunk (data, chunk, stripen, + stripe_offset, + i, /* redundancy */ + csize, buf); if (!err) break; grub_errno = GRUB_ERR_NONE; -- 2.19.0 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH V6] Add support for BTRFS raid5/6 to GRUB @ 2018-06-19 17:39 Goffredo Baroncelli 2018-06-19 17:39 ` [PATCH 6/9] btrfs: Refactor the code that read from disk Goffredo Baroncelli 0 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-06-19 17:39 UTC (permalink / raw) To: grub-devel 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. Changelog v1: initial support for btrfs raid5/6. No recovery allowed v2: full support for btrfs raid5/6. Recovery allowed v3: some minor cleanup suggested by Daniel Kiper; reusing the original raid6 recovery code of grub v4: Several spell fix; better description of the RAID layout in btrfs, and the variables which describes the stripe positioning; split the patch #5 in two (#5 and #6) v5: Several spell fix; improved code comment in patch #1, small clean up in the code v6: Small cleanup; improved the wording in the RAID6 layout description; in the function raid6_recover_read_buffer() avoid a unnecessary memcpy in case of invalid data; ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 6/9] btrfs: Refactor the code that read from disk 2018-06-19 17:39 [PATCH V6] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli @ 2018-06-19 17:39 ` Goffredo Baroncelli 2018-07-12 14:10 ` Daniel Kiper 0 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-06-19 17:39 UTC (permalink / raw) To: grub-devel; +Cc: Goffredo Baroncelli Move the code in charge to read the data from disk into a separate function. This helps to separate the error handling logic (which depend on 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 <kreijack@inwind.it> --- grub-core/fs/btrfs.c | 75 ++++++++++++++++++++++++++------------------ 1 file changed, 44 insertions(+), 31 deletions(-) diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c index 15ed59151..fa5bf56e0 100644 --- a/grub-core/fs/btrfs.c +++ b/grub-core/fs/btrfs.c @@ -625,6 +625,46 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t id) return ctx.dev_found; } +static grub_err_t +btrfs_read_from_chunk (struct grub_btrfs_data *data, + struct grub_btrfs_chunk_item *chunk, + grub_uint64_t stripen, grub_uint64_t stripe_offset, + int redundancy, grub_uint64_t csize, + void *buf) +{ + struct grub_btrfs_chunk_stripe *stripe; + grub_disk_addr_t paddr; + grub_device_t dev; + grub_err_t err; + + stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); + /* Right now the redundancy handling is easy. + With RAID5-like it will be more difficult. */ + stripe += stripen + redundancy; + + paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; + + grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T + " maps to 0x%" PRIxGRUB_UINT64_T + ". Reading paddr 0x%" PRIxGRUB_UINT64_T "\n", + stripen, stripe->offset, paddr); + + dev = find_device (data, stripe->device_id); + if (!dev) + { + grub_dprintf ("btrfs", + "couldn't find a necessary member device " + "of multi-device filesystem\n"); + grub_errno = GRUB_ERR_NONE; + return GRUB_ERR_READ_ERROR; + } + + err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, + paddr & (GRUB_DISK_SECTOR_SIZE - 1), + csize, buf); + return err; +} + static grub_err_t grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, void *buf, grub_size_t size, int recursion_depth) @@ -638,7 +678,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, grub_err_t err = 0; struct grub_btrfs_key key_out; int challoc = 0; - grub_device_t dev; struct grub_btrfs_key key_in; grub_size_t chsize; grub_disk_addr_t chaddr; @@ -881,36 +920,10 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, for (i = 0; i < redundancy; i++) { - struct grub_btrfs_chunk_stripe *stripe; - grub_disk_addr_t paddr; - - stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); - /* Right now the redundancy handling is easy. - With RAID5-like it will be more difficult. */ - stripe += stripen + i; - - paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; - - grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T - " maps to 0x%" PRIxGRUB_UINT64_T "\n", - stripen, stripe->offset); - grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T - "\n", paddr); - - dev = find_device (data, stripe->device_id); - if (!dev) - { - grub_dprintf ("btrfs", - "couldn't find a necessary member device " - "of multi-device filesystem\n"); - err = grub_errno; - grub_errno = GRUB_ERR_NONE; - continue; - } - - err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, - paddr & (GRUB_DISK_SECTOR_SIZE - 1), - csize, buf); + err = btrfs_read_from_chunk (data, chunk, stripen, + stripe_offset, + i, /* redundancy */ + csize, buf); if (!err) break; grub_errno = GRUB_ERR_NONE; -- 2.18.0.rc2 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 6/9] btrfs: Refactor the code that read from disk 2018-06-19 17:39 ` [PATCH 6/9] btrfs: Refactor the code that read from disk Goffredo Baroncelli @ 2018-07-12 14:10 ` Daniel Kiper 0 siblings, 0 replies; 33+ messages in thread From: Daniel Kiper @ 2018-07-12 14:10 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Goffredo Baroncelli On Tue, Jun 19, 2018 at 07:39:53PM +0200, Goffredo Baroncelli wrote: > Move the code in charge to read the data from disk into a separate > function. This helps to separate the error handling logic (which depend on s/depend/depends/ > 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 <kreijack@inwind.it> Otherwise, Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> Daniel ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH V4] Add support for BTRFS raid5/6 to GRUB @ 2018-05-16 18:48 Goffredo Baroncelli 2018-05-16 18:48 ` [PATCH 6/9] btrfs: refactor the code that read from disk Goffredo Baroncelli 0 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-05-16 18:48 UTC (permalink / raw) To: grub-devel 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 (for me) both with all the disks, and with some disks missing. I checked the crc32 calculated from grub and from linux and these matched. Finally I checked if the support for md raid6 still works properly, and it does (with all the drives and with up to 2 drives missing) Comments are welcome. Changelog v1: initial support for btrfs raid5/6. No recovery allowed v2: full support for btrfs raid5/6. Recovery allowed v3: some minor cleanup suggested by Daniel Kiper; reusing the original raid6 recovery code of grub v4: Several spell fix; better description of the RAID layout in btrfs, and the variables which describes the stripe positioning; split the patch #5 in two (#5 and #6) BR G.Baroncelli-- gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 6/9] btrfs: refactor the code that read from disk 2018-05-16 18:48 [PATCH V4] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli @ 2018-05-16 18:48 ` Goffredo Baroncelli 2018-05-30 11:07 ` Daniel Kiper 0 siblings, 1 reply; 33+ messages in thread From: Goffredo Baroncelli @ 2018-05-16 18:48 UTC (permalink / raw) To: grub-devel; +Cc: Goffredo Baroncelli This is a preparatory patch, to help the adding of the RAID 5/6 recovery code. In case of availability of all disks, this code is good for all the RAID profiles. However in case of failure, the error handling is quite different. Refactoring this code increases the general readability. Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it> --- grub-core/fs/btrfs.c | 85 +++++++++++++++++++++++++------------------- 1 file changed, 49 insertions(+), 36 deletions(-) diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c index 51f300829..63651928b 100644 --- a/grub-core/fs/btrfs.c +++ b/grub-core/fs/btrfs.c @@ -625,6 +625,47 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t id) return ctx.dev_found; } +static grub_err_t +btrfs_read_from_chunk (struct grub_btrfs_data *data, + struct grub_btrfs_chunk_item *chunk, + grub_uint64_t stripen, grub_uint64_t stripe_offset, + int redundancy, grub_uint64_t csize, + void *buf) +{ + + struct grub_btrfs_chunk_stripe *stripe; + grub_disk_addr_t paddr; + grub_device_t dev; + grub_err_t err; + + stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); + /* Right now the redundancy handling is easy. + With RAID5-like it will be more difficult. */ + stripe += stripen + redundancy; + + paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; + + grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T + " maps to 0x%" PRIxGRUB_UINT64_T "\n", + stripen, stripe->offset); + grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T "\n", paddr); + + dev = find_device (data, stripe->device_id); + if (!dev) + { + grub_dprintf ("btrfs", + "couldn't find a necessary member device " + "of multi-device filesystem\n"); + grub_errno = GRUB_ERR_NONE; + return GRUB_ERR_READ_ERROR; + } + + err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, + paddr & (GRUB_DISK_SECTOR_SIZE - 1), + csize, buf); + return err; +} + static grub_err_t grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, void *buf, grub_size_t size, int recursion_depth) @@ -638,7 +679,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, grub_err_t err = 0; struct grub_btrfs_key key_out; int challoc = 0; - grub_device_t dev; struct grub_btrfs_key key_in; grub_size_t chsize; grub_disk_addr_t chaddr; @@ -879,42 +919,15 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, for (i = 0; i < redundancy; i++) { - struct grub_btrfs_chunk_stripe *stripe; - grub_disk_addr_t paddr; - - stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); - /* Right now the redundancy handling is easy. - With RAID5-like it will be more difficult. */ - stripe += stripen + i; - - paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; - - grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T - " maps to 0x%" PRIxGRUB_UINT64_T "\n", - stripen, stripe->offset); - grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T - "\n", paddr); - - dev = find_device (data, stripe->device_id); - if (!dev) - { - grub_dprintf ("btrfs", - "couldn't find a necessary member device " - "of multi-device filesystem\n"); - err = grub_errno; - grub_errno = GRUB_ERR_NONE; - continue; - } - - err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, - paddr & (GRUB_DISK_SECTOR_SIZE - 1), - csize, buf); - if (!err) - break; - grub_errno = GRUB_ERR_NONE; + err = btrfs_read_from_chunk (data, chunk, stripen, + stripe_offset, + i, /* redundancy */ + csize, buf); + if (err == GRUB_ERR_NONE) + break; } - if (i != redundancy) - break; + if (err == GRUB_ERR_NONE) + break; } if (err) return grub_errno = err; -- 2.17.0 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 6/9] btrfs: refactor the code that read from disk 2018-05-16 18:48 ` [PATCH 6/9] btrfs: refactor the code that read from disk Goffredo Baroncelli @ 2018-05-30 11:07 ` Daniel Kiper 2018-06-01 18:50 ` Goffredo Baroncelli 0 siblings, 1 reply; 33+ messages in thread From: Daniel Kiper @ 2018-05-30 11:07 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Goffredo Baroncelli On Wed, May 16, 2018 at 08:48:16PM +0200, Goffredo Baroncelli wrote: > This is a preparatory patch, to help the adding of the RAID 5/6 recovery Please move "This is a preparatory patch" sentence below... > code. In case of availability of all disks, this code is good for all the > RAID profiles. However in case of failure, the error handling is quite > different. Refactoring this code increases the general readability. s/readability/readability too/? You can put "This is a preparatory patch" in separate line here. Same for the other patches. > Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it> > --- > grub-core/fs/btrfs.c | 85 +++++++++++++++++++++++++------------------- > 1 file changed, 49 insertions(+), 36 deletions(-) > > diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c > index 51f300829..63651928b 100644 > --- a/grub-core/fs/btrfs.c > +++ b/grub-core/fs/btrfs.c > @@ -625,6 +625,47 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t id) > return ctx.dev_found; > } > > +static grub_err_t > +btrfs_read_from_chunk (struct grub_btrfs_data *data, > + struct grub_btrfs_chunk_item *chunk, > + grub_uint64_t stripen, grub_uint64_t stripe_offset, > + int redundancy, grub_uint64_t csize, > + void *buf) > +{ > + > + struct grub_btrfs_chunk_stripe *stripe; > + grub_disk_addr_t paddr; > + grub_device_t dev; > + grub_err_t err; > + > + stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); > + /* Right now the redundancy handling is easy. > + With RAID5-like it will be more difficult. */ > + stripe += stripen + redundancy; > + > + paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; > + > + grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T > + " maps to 0x%" PRIxGRUB_UINT64_T "\n", > + stripen, stripe->offset); > + grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T "\n", paddr); > + > + dev = find_device (data, stripe->device_id); > + if (!dev) > + { > + grub_dprintf ("btrfs", > + "couldn't find a necessary member device " > + "of multi-device filesystem\n"); > + grub_errno = GRUB_ERR_NONE; > + return GRUB_ERR_READ_ERROR; > + } > + > + err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, > + paddr & (GRUB_DISK_SECTOR_SIZE - 1), > + csize, buf); > + return err; > +} > + > static grub_err_t > grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, > void *buf, grub_size_t size, int recursion_depth) > @@ -638,7 +679,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, > grub_err_t err = 0; > struct grub_btrfs_key key_out; > int challoc = 0; > - grub_device_t dev; > struct grub_btrfs_key key_in; > grub_size_t chsize; > grub_disk_addr_t chaddr; > @@ -879,42 +919,15 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, > > for (i = 0; i < redundancy; i++) > { > - struct grub_btrfs_chunk_stripe *stripe; > - grub_disk_addr_t paddr; > - > - stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); > - /* Right now the redundancy handling is easy. > - With RAID5-like it will be more difficult. */ > - stripe += stripen + i; > - > - paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; > - > - grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T > - " maps to 0x%" PRIxGRUB_UINT64_T "\n", > - stripen, stripe->offset); > - grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T > - "\n", paddr); > - > - dev = find_device (data, stripe->device_id); > - if (!dev) > - { > - grub_dprintf ("btrfs", > - "couldn't find a necessary member device " > - "of multi-device filesystem\n"); > - err = grub_errno; > - grub_errno = GRUB_ERR_NONE; > - continue; > - } > - > - err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, > - paddr & (GRUB_DISK_SECTOR_SIZE - 1), > - csize, buf); > - if (!err) > - break; > - grub_errno = GRUB_ERR_NONE; If you drop this line then you change behavior of this function. I have a feeling that this should stay as is. At least for now. If you need this change then probably it should be a part of the other patch. > + err = btrfs_read_from_chunk (data, chunk, stripen, > + stripe_offset, > + i, /* redundancy */ > + csize, buf); > + if (err == GRUB_ERR_NONE) > + break; > } > - if (i != redundancy) > - break; > + if (err == GRUB_ERR_NONE) > + break; Ditto. Daniel ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 6/9] btrfs: refactor the code that read from disk 2018-05-30 11:07 ` Daniel Kiper @ 2018-06-01 18:50 ` Goffredo Baroncelli 0 siblings, 0 replies; 33+ messages in thread From: Goffredo Baroncelli @ 2018-06-01 18:50 UTC (permalink / raw) To: Daniel Kiper, The development of GNU GRUB On 05/30/2018 01:07 PM, Daniel Kiper wrote: > On Wed, May 16, 2018 at 08:48:16PM +0200, Goffredo Baroncelli wrote: >> This is a preparatory patch, to help the adding of the RAID 5/6 recovery > > Please move "This is a preparatory patch" sentence below... > >> code. In case of availability of all disks, this code is good for all the >> RAID profiles. However in case of failure, the error handling is quite >> different. Refactoring this code increases the general readability. > > s/readability/readability too/? > > You can put "This is a preparatory patch" in separate line here. > Same for the other patches. What about ---- btrfs: Refactor the code that read from disk Move the code in charge to read the data from disk in a separate function. This helps to separate the error handling logic (which depend by 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 <kreijack@inwind.it> >> --- >> grub-core/fs/btrfs.c | 85 +++++++++++++++++++++++++------------------- >> 1 file changed, 49 insertions(+), 36 deletions(-) >> >> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c >> index 51f300829..63651928b 100644 >> --- a/grub-core/fs/btrfs.c >> +++ b/grub-core/fs/btrfs.c >> @@ -625,6 +625,47 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t id) >> return ctx.dev_found; >> } >> >> +static grub_err_t >> +btrfs_read_from_chunk (struct grub_btrfs_data *data, >> + struct grub_btrfs_chunk_item *chunk, >> + grub_uint64_t stripen, grub_uint64_t stripe_offset, >> + int redundancy, grub_uint64_t csize, >> + void *buf) >> +{ >> + >> + struct grub_btrfs_chunk_stripe *stripe; >> + grub_disk_addr_t paddr; >> + grub_device_t dev; >> + grub_err_t err; >> + >> + stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); >> + /* Right now the redundancy handling is easy. >> + With RAID5-like it will be more difficult. */ >> + stripe += stripen + redundancy; >> + >> + paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; >> + >> + grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T >> + " maps to 0x%" PRIxGRUB_UINT64_T "\n", >> + stripen, stripe->offset); >> + grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T "\n", paddr); >> + >> + dev = find_device (data, stripe->device_id); >> + if (!dev) >> + { >> + grub_dprintf ("btrfs", >> + "couldn't find a necessary member device " >> + "of multi-device filesystem\n"); >> + grub_errno = GRUB_ERR_NONE; >> + return GRUB_ERR_READ_ERROR; >> + } >> + >> + err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, >> + paddr & (GRUB_DISK_SECTOR_SIZE - 1), >> + csize, buf); >> + return err; >> +} >> + >> static grub_err_t >> grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, >> void *buf, grub_size_t size, int recursion_depth) >> @@ -638,7 +679,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, >> grub_err_t err = 0; >> struct grub_btrfs_key key_out; >> int challoc = 0; >> - grub_device_t dev; >> struct grub_btrfs_key key_in; >> grub_size_t chsize; >> grub_disk_addr_t chaddr; >> @@ -879,42 +919,15 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, >> >> for (i = 0; i < redundancy; i++) >> { >> - struct grub_btrfs_chunk_stripe *stripe; >> - grub_disk_addr_t paddr; >> - >> - stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); >> - /* Right now the redundancy handling is easy. >> - With RAID5-like it will be more difficult. */ >> - stripe += stripen + i; >> - >> - paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; >> - >> - grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T >> - " maps to 0x%" PRIxGRUB_UINT64_T "\n", >> - stripen, stripe->offset); >> - grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T >> - "\n", paddr); >> - >> - dev = find_device (data, stripe->device_id); >> - if (!dev) >> - { >> - grub_dprintf ("btrfs", >> - "couldn't find a necessary member device " >> - "of multi-device filesystem\n"); >> - err = grub_errno; >> - grub_errno = GRUB_ERR_NONE; >> - continue; >> - } >> - >> - err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, >> - paddr & (GRUB_DISK_SECTOR_SIZE - 1), >> - csize, buf); >> - if (!err) >> - break; >> - grub_errno = GRUB_ERR_NONE; > > If you drop this line then you change behavior of this function. > I have a feeling that this should stay as is. At least for now. > If you need this change then probably it should be a part of the > other patch. OK > >> + err = btrfs_read_from_chunk (data, chunk, stripen, >> + stripe_offset, >> + i, /* redundancy */ >> + csize, buf); >> + if (err == GRUB_ERR_NONE) >> + break; >> } >> - if (i != redundancy) >> - break; >> + if (err == GRUB_ERR_NONE) >> + break; OK > > Ditto. > > Daniel > -- gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2018-10-22 17:31 UTC | newest] Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-03 18:53 [PATCH V5] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli 2018-06-03 18:53 ` [PATCH 1/9] btrfs: Add support for reading a filesystem with a RAID 5 or RAID 6 profile Goffredo Baroncelli 2018-06-14 11:17 ` Daniel Kiper 2018-06-14 18:55 ` Goffredo Baroncelli 2018-06-19 17:30 ` Goffredo Baroncelli 2018-06-03 18:53 ` [PATCH 2/9] btrfs: Add helper to check the btrfs header Goffredo Baroncelli 2018-06-14 11:25 ` Daniel Kiper 2018-06-03 18:53 ` [PATCH 3/9] btrfs: Move the error logging from find_device() to its caller Goffredo Baroncelli 2018-06-14 11:28 ` Daniel Kiper 2018-06-03 18:53 ` [PATCH 5/9] btrfs: Move logging code in grub_btrfs_read_logical() Goffredo Baroncelli 2018-06-14 11:52 ` Daniel Kiper 2018-06-03 18:53 ` [PATCH 6/9] btrfs: Refactor the code that read from disk Goffredo Baroncelli 2018-06-14 12:38 ` Daniel Kiper 2018-06-03 18:53 ` [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles Goffredo Baroncelli 2018-06-14 13:03 ` Daniel Kiper 2018-06-14 19:05 ` Goffredo Baroncelli 2018-06-18 18:12 ` Goffredo Baroncelli 2018-09-03 12:32 ` Daniel Kiper 2018-06-03 18:53 ` [PATCH 8/9] btrfs: Make more generic the code for RAID 6 rebuilding Goffredo Baroncelli 2018-06-03 18:53 ` [PATCH 9/9] btrfs: Add RAID 6 recovery for a btrfs filesystem Goffredo Baroncelli 2018-06-14 13:11 ` Daniel Kiper 2018-06-14 13:21 ` [PATCH V5] Add support for BTRFS raid5/6 to GRUB Daniel Kiper 2018-06-14 18:06 ` Goffredo Baroncelli -- strict thread matches above, loose matches on Subject: below -- 2018-10-22 17:29 [PATCH V11] " Goffredo Baroncelli 2018-10-22 17:29 ` [PATCH 6/9] btrfs: Refactor the code that read from disk Goffredo Baroncelli 2018-10-18 17:55 [PATCH V10] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli 2018-10-18 17:55 ` [PATCH 6/9] btrfs: Refactor the code that read from disk Goffredo Baroncelli 2018-10-11 18:50 [PATCH V9] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli 2018-10-11 18:51 ` [PATCH 6/9] btrfs: Refactor the code that read from disk Goffredo Baroncelli 2018-09-27 18:34 [PATCH V8] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli 2018-09-27 18:35 ` [PATCH 6/9] btrfs: Refactor the code that read from disk Goffredo Baroncelli 2018-09-19 18:40 [PATCH V7] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli 2018-09-19 18:40 ` [PATCH 6/9] btrfs: Refactor the code that read from disk Goffredo Baroncelli 2018-06-19 17:39 [PATCH V6] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli 2018-06-19 17:39 ` [PATCH 6/9] btrfs: Refactor the code that read from disk Goffredo Baroncelli 2018-07-12 14:10 ` Daniel Kiper 2018-05-16 18:48 [PATCH V4] Add support for BTRFS raid5/6 to GRUB Goffredo Baroncelli 2018-05-16 18:48 ` [PATCH 6/9] btrfs: refactor the code that read from disk Goffredo Baroncelli 2018-05-30 11:07 ` Daniel Kiper 2018-06-01 18:50 ` Goffredo Baroncelli
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.