Discussion:
[PATCH 6/9] btrfs: refactor the code that read from disk
Goffredo Baroncelli
2018-05-16 18:48:16 UTC
Permalink
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 <***@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
Goffredo Baroncelli
2018-05-16 18:48:13 UTC
Permalink
This is a preparatory patch. The callee knows better if this
error is fatal or not, i.e. another available disk.


Signed-off-by: Goffredo Baroncelli <***@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 123bfbfc1..fd4225d11 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++;
@@ -900,6 +897,9 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
dev = find_device (data, stripe->device_id, j);
if (!dev)
{
+ grub_dprintf ("btrfs",
+ "couldn't find a necessary member device "
+ "of multi-device filesystem\n");
err = grub_errno;
grub_errno = GRUB_ERR_NONE;
continue;
--
2.17.0
Daniel Kiper
2018-05-30 10:25:36 UTC
Permalink
Post by Goffredo Baroncelli
This is a preparatory patch. The callee knows better if this
s/callee/caller/
Post by Goffredo Baroncelli
error is fatal or not, i.e. another available disk.
s/i.e. another available disk./e.g. another disk is available or not./

And I think that you can reverse order of the sentences above.

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

Otherwise LGTM.

Daniel
Goffredo Baroncelli
2018-05-16 18:48:11 UTC
Permalink
Signed-off-by: Goffredo Baroncelli <***@inwind.it>
---
grub-core/fs/btrfs.c | 68 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 68 insertions(+)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index be195448d..394611cbb 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,72 @@ 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 an example of a RAID6 layout and the meaning of the
+ * variables. The same apply to RAID5. The only differences is that
+ * there is only one parity instead of two.
+ *
+ * A RAID6 6 layout consists in several stripes spread
+ * on the disks, following a layout like the one below
+ *
+ * Disk1 Disk2 Disk3 Ddisk4
+ *
+ * A1 B1 P1 Q1
+ * Q2 A2 B2 P2
+ * P3 Q3 A3 B3
+ * [...]
+ *
+ * Note that the placement of the parities depends 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 -> logical address to read (from the beginning of the
+ * chunk space)
+ * chunk_stripe_length -> size of a stripe (typically 64k)
+ * nstripes -> number of disks
+ * low -> offset of the data inside a stripe
+ *
+ */
+ stripe_nr = grub_divmod64 (off, chunk_stripe_length, &low);
+
+ /*
+ * In the line below stripen is evaluated without considering
+ * the parities (0 for A1, A2, A3... 1 for B1, B2...);
+ */
+ high = grub_divmod64 (stripe_nr, nstripes - nparities, &stripen);
+
+ /*
+ * In the line below stripen, now consider also the parities (0
+ * for A1, 1 for A2, 2 for A3....); the math is done "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.0
Daniel Kiper
2018-05-30 10:07:18 UTC
Permalink
Post by Goffredo Baroncelli
---
grub-core/fs/btrfs.c | 68 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index be195448d..394611cbb 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,72 @@ 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;
+ }
+ {
+ 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 an example of a RAID6 layout and the meaning of the
s/an example of a RAID6/is an example of a RAID 6/
Post by Goffredo Baroncelli
+ * variables. The same apply to RAID5. The only differences is that
s/The same apply to RAID5/The same applies to RAID 5/
Post by Goffredo Baroncelli
+ * there is only one parity instead of two.
s/parity/parity disk/
Post by Goffredo Baroncelli
+ *
+ * A RAID6 6 layout consists in several stripes spread
s/RAID6 6/RAID 6/
s/consists in/consists of/
Post by Goffredo Baroncelli
+ * on the disks, following a layout like the one below
s/on the disks/over the disks/
Post by Goffredo Baroncelli
+ *
+ * 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;
Please, please, please, do not abuse ";". Full stop is preffered at the
end of sentence. Please fix this in all patches.
Post by Goffredo Baroncelli
+ * stripe_nr -> is the stripe number not considering the parities
+ * (A1=0, B1=1, A2 = 2, B2 = 3, ...)
May I ask you to do this like that.

The variables below have following meaning:
- 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, ...),
...
- low is the offset of the data inside a stripe.
Post by Goffredo Baroncelli
+ * high -> is the row number (0 for A1...Q1, 1 for Q2..P2, ...)
+ * stripen -> is the column number (or disk number)
+ * off -> logical address to read (from the beginning of the
+ * chunk space)
What do you mean by "chunk"? Is it e.g. A1 + B1 region? Please make it
clear what do you mean by "chunk".
Post by Goffredo Baroncelli
+ * chunk_stripe_length -> size of a stripe (typically 64k)
+ * nstripes -> number of disks
+ * low -> offset of the data inside a stripe
It looks that stripe_offset and csize explanation is missing here.
Post by Goffredo Baroncelli
+ *
+ */
+ stripe_nr = grub_divmod64 (off, chunk_stripe_length, &low);
+
+ /*
+ * In the line below stripen is evaluated without considering
s/In the line below //
Post by Goffredo Baroncelli
+ * the parities (0 for A1, A2, A3... 1 for B1, B2...);
s/;/./
Post by Goffredo Baroncelli
+ */
+ high = grub_divmod64 (stripe_nr, nstripes - nparities, &stripen);
+
+ /*
+ * In the line below stripen, now consider also the parities (0
s/In the line below stripen, now/Now/
Post by Goffredo Baroncelli
+ * for A1, 1 for A2, 2 for A3....); the math is done "modulo"
s/; the/. The/
s/"modulo"/modulo/
Post by Goffredo Baroncelli
+ * number of disks
Full stop at the end of sentence please.

Daniel
Goffredo Baroncelli
2018-06-01 18:50:32 UTC
Permalink
Hi Daniel,

my comments below
[...]
Post by Daniel Kiper
Post by Goffredo Baroncelli
+ * off -> logical address to read (from the beginning of the
+ * chunk space)
What do you mean by "chunk"? Is it e.g. A1 + B1 region? Please make it
clear what do you mean by "chunk".
Chunk is a very pervasive concept in BTRFS. A reader who looks to a BTRFS
raid code, must know very well this concepts. I am not sure that GRUB code
is the right place to contain a full BTRFS chunk description.

Basically, the BTRFS logical space is mapped to the physical one via the
chunks (aka block group). Each chunk maps a range of the logical address to a
range in the disk(s). If more disks are involved, different profile
might be used (linear, raid0... raid5/6). E.g.: a chunk maps a logical
address (say 0.1GB) to a physical address (say 1GB..2GB of disk1, 3GB..to
4GB of disk2, in raid1 mode).
The chunks are a low layer. All the BTRFS metadata are in term of the
logical address (upper layer).

[...]
Post by Daniel Kiper
Post by Goffredo Baroncelli
+ /*
+ * In the line below stripen is evaluated without considering
s/In the line below //
Post by Goffredo Baroncelli
+ * the parities (0 for A1, A2, A3... 1 for B1, B2...);
s/;/./
Post by Goffredo Baroncelli
+ */
+ high = grub_divmod64 (stripe_nr, nstripes - nparities, &stripen);
+
+ /*
+ * In the line below stripen, now consider also the parities (0
s/In the line below stripen, now/Now/
I think that "stripen" must be re-added
Post by Daniel Kiper
Post by Goffredo Baroncelli
+ * for A1, 1 for A2, 2 for A3....); the math is done "modulo"
s/; the/. The/
s/"modulo"/modulo/
Post by Goffredo Baroncelli
+ * number of disks
Full stop at the end of sentence please.
Daniel
--
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
Goffredo Baroncelli
2018-05-16 18:48:15 UTC
Permalink
A portion of the logging code is moved outside a for(;;). The part that is
left inside is the one which depends by the 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.
---
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 4920dd3b1..51f300829 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -865,6 +865,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;
@@ -877,20 +889,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.0
Daniel Kiper
2018-05-30 10:55:15 UTC
Permalink
Post by Goffredo Baroncelli
A portion of the logging code is moved outside a for(;;). The part that is
s/outside a for/outside of for/
Post by Goffredo Baroncelli
left inside is the one which depends by the for(;;) index.
s/depends by the for/depends on the for/

By the way, there are 2 for(;;). You have to be clear which one.
Post by Goffredo Baroncelli
This is a preparatory patch: in the next one it will be possible to refactor
the code inside the for(;;) in an another function.
I think that you can drop these sentences. However, it seems to me that you
are no giving the reason why this move is needed for subsequent patches.
Please explain that in the commit message.

And missing SOB.

The code itself LGTM.

Daniel
Goffredo Baroncelli
2018-05-16 18:48:12 UTC
Permalink
This helper will be used in few places to help the debugging. As
conservative approach, in case of error it is only logged. This
because I am not sure if this can change something in the error
handling of the currently existing code.

Signed-off-by: Goffredo Baroncelli <***@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 394611cbb..123bfbfc1 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -77,7 +77,8 @@ struct btrfs_header
{
grub_btrfs_checksum_t checksum;
grub_btrfs_uuid_t uuid;
- grub_uint8_t dummy[0x30];
+ grub_uint64_t bytenr;
+ grub_uint8_t dummy[0x28];
grub_uint32_t nitems;
grub_uint8_t level;
} GRUB_PACKED;
@@ -286,6 +287,25 @@ free_iterator (struct grub_btrfs_leaf_descriptor *desc)
grub_free (desc->data);
}

+static grub_err_t
+check_btrfs_header (struct grub_btrfs_data *data, struct btrfs_header *header,
+ grub_disk_addr_t addr)
+{
+ if (grub_le_to_cpu64 (header->bytenr) != addr)
+ {
+ grub_dprintf ("btrfs", "btrfs_header.bytenr is not equal node addr\n");
+ return grub_error (GRUB_ERR_BAD_FS,
+ "header bytenr is not equal node addr");
+ }
+ if (grub_memcmp (data->sblock.uuid, header->uuid, sizeof(grub_btrfs_uuid_t)))
+ {
+ grub_dprintf ("btrfs", "btrfs_header.uuid doesn't match sblock uuid\n");
+ return grub_error (GRUB_ERR_BAD_FS,
+ "header uuid doesn't match sblock uuid");
+ }
+ return GRUB_ERR_NONE;
+}
+
static grub_err_t
save_ref (struct grub_btrfs_leaf_descriptor *desc,
grub_disk_addr_t addr, unsigned i, unsigned m, int l)
@@ -341,6 +361,7 @@ next (struct grub_btrfs_data *data,

err = grub_btrfs_read_logical (data, grub_le_to_cpu64 (node.addr),
&head, sizeof (head), 0);
+ check_btrfs_header (data, &head, grub_le_to_cpu64 (node.addr));
if (err)
return -err;

@@ -402,6 +423,7 @@ lower_bound (struct grub_btrfs_data *data,
/* FIXME: preread few nodes into buffer. */
err = grub_btrfs_read_logical (data, addr, &head, sizeof (head),
recursion_depth + 1);
+ check_btrfs_header (data, &head, addr);
if (err)
return err;
addr += sizeof (head);
--
2.17.0
Daniel Kiper
2018-05-30 10:12:36 UTC
Permalink
Post by Goffredo Baroncelli
This helper will be used in few places to help the debugging. As
s/in few/in a few/
Post by Goffredo Baroncelli
conservative approach, in case of error it is only logged. This
s/This/This is/

Otherwise LGTM.

Daniel
Goffredo Baroncelli
2018-05-16 18:48:19 UTC
Permalink
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 old md RAID 6 code already
present in grub.

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

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 5fcaad86f..3d71b954e 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+");

@@ -695,11 +696,35 @@ rebuild_raid5 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
csize);
}

+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, raid6_recover_read_buffer, 0);
+}
+
static grub_err_t
raid56_read_retry (struct grub_btrfs_data *data,
struct grub_btrfs_chunk_item *chunk,
grub_uint64_t stripe_offset, grub_uint64_t stripen,
- grub_uint64_t csize, void *buf)
+ grub_uint64_t csize, void *buf, grub_uint64_t parities_pos)
{

struct raid56_buffer *buffers = NULL;
@@ -780,6 +805,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",
@@ -796,7 +830,7 @@ raid56_read_retry (struct grub_btrfs_data *data,
}
else
{
- grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
+ rebuild_raid6 (buffers, nstripes, csize, parities_pos, buf, stripen);
}

cleanup:
@@ -891,8 +925,9 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
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)
{
@@ -1089,7 +1124,7 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
csize, buf);
if (err != GRUB_ERR_NONE)
err = raid56_read_retry (data, chunk, stripe_offset,
- stripen, csize, buf);
+ stripen, csize, buf, parities_pos);
}

if (err == GRUB_ERR_NONE)
--
2.17.0
Daniel Kiper
2018-05-30 12:15:48 UTC
Permalink
Post by 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 old md RAID 6 code already
s/the old md/the md/
Post by Goffredo Baroncelli
present in grub.
---
grub-core/fs/btrfs.c | 45 +++++++++++++++++++++++++++++++++++++++-----
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 5fcaad86f..3d71b954e 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+");
@@ -695,11 +696,35 @@ rebuild_raid5 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
csize);
}
+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);
Could we avoid this grub_memcpy() call somehow?
Post by Goffredo Baroncelli
+ 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, raid6_recover_read_buffer, 0);
+}
+
static grub_err_t
raid56_read_retry (struct grub_btrfs_data *data,
struct grub_btrfs_chunk_item *chunk,
grub_uint64_t stripe_offset, grub_uint64_t stripen,
- grub_uint64_t csize, void *buf)
+ grub_uint64_t csize, void *buf, grub_uint64_t parities_pos)
{
struct raid56_buffer *buffers = NULL;
@@ -780,6 +805,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",
@@ -796,7 +830,7 @@ raid56_read_retry (struct grub_btrfs_data *data,
}
else
{
- grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
+ rebuild_raid6 (buffers, nstripes, csize, parities_pos, buf, stripen);
}
@@ -891,8 +925,9 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
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|
Space before "|" please?
Post by Goffredo Baroncelli
+ GRUB_BTRFS_CHUNK_TYPE_RAID6));
if (grub_le_to_cpu64 (chunk->size) <= off)
{
@@ -1089,7 +1124,7 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
csize, buf);
if (err != GRUB_ERR_NONE)
err = raid56_read_retry (data, chunk, stripe_offset,
- stripen, csize, buf);
+ stripen, csize, buf, parities_pos);
I am not sure what is parities_pos and where it is initialized.
If it is a part of earlier patch but it is not used then I think
that it should be a part of this patch.

Daniel
Goffredo Baroncelli
2018-06-01 18:50:26 UTC
Permalink
Post by Daniel Kiper
Post by 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 old md RAID 6 code already
s/the old md/the md/
ok
Post by Daniel Kiper
Post by Goffredo Baroncelli
present in grub.
---
grub-core/fs/btrfs.c | 45 +++++++++++++++++++++++++++++++++++++++-----
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 5fcaad86f..3d71b954e 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+");
@@ -695,11 +696,35 @@ rebuild_raid5 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
csize);
}
+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);
Could we avoid this grub_memcpy() call somehow?
I would like; however this would mean that the handling of the raid 5 and raid 6 rebuilding code would be totally different. This has a big impact on the patches set and the related tests performed until now.
Are you sure that this effort will be really needed ?
Post by Daniel Kiper
Post by Goffredo Baroncelli
+ 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, raid6_recover_read_buffer, 0);
+}
+
static grub_err_t
raid56_read_retry (struct grub_btrfs_data *data,
struct grub_btrfs_chunk_item *chunk,
grub_uint64_t stripe_offset, grub_uint64_t stripen,
- grub_uint64_t csize, void *buf)
+ grub_uint64_t csize, void *buf, grub_uint64_t parities_pos)
{
struct raid56_buffer *buffers = NULL;
@@ -780,6 +805,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",
@@ -796,7 +830,7 @@ raid56_read_retry (struct grub_btrfs_data *data,
}
else
{
- grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
+ rebuild_raid6 (buffers, nstripes, csize, parities_pos, buf, stripen);
}
@@ -891,8 +925,9 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
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|
Space before "|" please?
OK
Post by Daniel Kiper
Post by Goffredo Baroncelli
+ GRUB_BTRFS_CHUNK_TYPE_RAID6));
if (grub_le_to_cpu64 (chunk->size) <= off)
{
@@ -1089,7 +1124,7 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
csize, buf);
if (err != GRUB_ERR_NONE)
err = raid56_read_retry (data, chunk, stripe_offset,
- stripen, csize, buf);
+ stripen, csize, buf, parities_pos);
I am not sure what is parities_pos and where it is initialized.
If it is a part of earlier patch but it is not used then I think
that it should be a part of this patch.
You are right
Post by Daniel Kiper
Daniel
I think that I implemented all your requests in the next patches set. The only exception is the removing the memcpy in the raid6 recovery path.
Now I am starting the test. when I will completed the test phase, I will resend the patches.

BR
G.Baroncelli
--
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
Goffredo Baroncelli
2018-05-16 18:48:17 UTC
Permalink
Signed-off-by: Goffredo Baroncelli <***@inwind.it>
---
grub-core/fs/btrfs.c | 174 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 170 insertions(+), 4 deletions(-)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 63651928b..5fcaad86f 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -29,6 +29,7 @@
#include <minilzo.h>
#include <grub/i18n.h>
#include <grub/btrfs.h>
+#include <grub/crypto.h>

GRUB_MOD_LICENSE ("GPLv3+");

@@ -666,6 +667,150 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
return err;
}

+struct raid56_buffer {
+ void *buf;
+ int data_is_valid;
+};
+
+static void
+rebuild_raid5 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
+ grub_uint64_t csize)
+{
+ grub_uint64_t target = 0, i;
+
+ while (buffers[target].data_is_valid && target < nstripes)
+ ++target;
+
+ if (target == nstripes)
+ {
+ grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n");
+ return;
+ }
+
+ grub_dprintf ("btrfs", "rebuilding raid5 stripe #%" PRIuGRUB_UINT64_T "\n",
+ target);
+ for (i = 0; i < nstripes; i++)
+ if (i != target)
+ grub_crypto_xor (buffers[target].buf, buffers[target].buf, buffers[i].buf,
+ csize);
+}
+
+static grub_err_t
+raid56_read_retry (struct grub_btrfs_data *data,
+ struct grub_btrfs_chunk_item *chunk,
+ grub_uint64_t stripe_offset, grub_uint64_t stripen,
+ grub_uint64_t csize, void *buf)
+{
+
+ struct raid56_buffer *buffers = NULL;
+ grub_uint64_t nstripes = grub_le_to_cpu16 (chunk->nstripes);
+ grub_uint64_t chunk_type = grub_le_to_cpu64 (chunk->type);
+ grub_err_t ret = GRUB_ERR_NONE;
+ grub_uint64_t i, failed_devices;
+
+ buffers = grub_zalloc (sizeof(*buffers) * nstripes);
+ if (!buffers)
+ {
+ ret = GRUB_ERR_OUT_OF_MEMORY;
+ goto cleanup;
+ }
+
+ for (i = 0; i < nstripes; i++)
+ {
+ buffers[i].buf = grub_zalloc (csize);
+ if (!buffers[i].buf)
+ {
+ ret = GRUB_ERR_OUT_OF_MEMORY;
+ goto cleanup;
+ }
+ }
+
+ for (i = 0; i < nstripes; i++)
+ {
+ struct grub_btrfs_chunk_stripe *stripe;
+ grub_disk_addr_t paddr;
+ grub_device_t dev;
+ grub_err_t err2;
+
+ stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
+ stripe += i;
+
+ paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
+ grub_dprintf ("btrfs", "reading paddr %" PRIxGRUB_UINT64_T
+ " from stripe ID %" PRIxGRUB_UINT64_T "\n", paddr,
+ stripe->device_id);
+
+ dev = find_device (data, stripe->device_id);
+ if (!dev)
+ {
+ buffers[i].data_is_valid = 0;
+ grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " FAILED (dev ID %"
+ PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
+ continue;
+ }
+
+ err2 = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
+ paddr & (GRUB_DISK_SECTOR_SIZE - 1),
+ csize, buffers[i].buf);
+ if (err2 == GRUB_ERR_NONE)
+ {
+ buffers[i].data_is_valid = 1;
+ grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " Ok (dev ID %"
+ PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
+ }
+ else
+ {
+ buffers[i].data_is_valid = 0;
+ grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T
+ " FAILED (dev ID %" PRIxGRUB_UINT64_T ")\n", i,
+ stripe->device_id);
+ }
+ }
+
+ failed_devices = 0;
+ for (i = 0; i < nstripes; i++)
+ if (!buffers[i].data_is_valid)
+ ++failed_devices;
+ if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
+ {
+ grub_dprintf ("btrfs",
+ "not enough disks for raid5: total %" PRIuGRUB_UINT64_T
+ ", missing %" PRIuGRUB_UINT64_T "\n",
+ nstripes, failed_devices);
+ ret = GRUB_ERR_READ_ERROR;
+ goto cleanup;
+ }
+ else
+ {
+ grub_dprintf ("btrfs",
+ "enough disks for raid5/6 rebuilding: total %"
+ PRIuGRUB_UINT64_T ", missing %" PRIuGRUB_UINT64_T "\n",
+ nstripes, failed_devices);
+ }
+
+ /* if these are enough, try to rebuild the data */
+ if (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5)
+ {
+ rebuild_raid5 (buffers, nstripes, csize);
+ grub_memcpy (buf, buffers[stripen].buf, csize);
+ }
+ else
+ {
+ grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
+ }
+
+cleanup:
+ if (buffers)
+ {
+ for (i = 0; i < nstripes; i++)
+ if (buffers[i].buf)
+ grub_free(buffers[i].buf);
+ grub_free(buffers);
+ }
+
+ return ret;
+}
+
static grub_err_t
grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
void *buf, grub_size_t size, int recursion_depth)
@@ -743,6 +888,11 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
grub_uint16_t nstripes;
unsigned redundancy = 1;
unsigned i, j;
+ int is_raid56;
+ grub_uint64_t parities_pos = 0;
+
+ is_raid56 = !!(grub_le_to_cpu64 (chunk->type) &
+ GRUB_BTRFS_CHUNK_TYPE_RAID5);

if (grub_le_to_cpu64 (chunk->size) <= off)
{
@@ -885,6 +1035,8 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
* 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;
@@ -917,15 +1069,29 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
grub_dprintf ("btrfs", "reading laddr 0x%" PRIxGRUB_UINT64_T "\n",
addr);

- for (i = 0; i < redundancy; i++)
+ if (!is_raid56)
+ {
+ for (i = 0; i < redundancy; i++)
+ {
+ err = btrfs_read_from_chunk (data, chunk, stripen,
+ stripe_offset,
+ i, /* redundancy */
+ csize, buf);
+ if (err == GRUB_ERR_NONE)
+ break;
+ }
+ }
+ else
{
err = btrfs_read_from_chunk (data, chunk, stripen,
stripe_offset,
- i, /* redundancy */
+ 0, /* no mirror */
csize, buf);
- if (err == GRUB_ERR_NONE)
- break;
+ if (err != GRUB_ERR_NONE)
+ err = raid56_read_retry (data, chunk, stripe_offset,
+ stripen, csize, buf);
}
+
if (err == GRUB_ERR_NONE)
break;
}
--
2.17.0
Daniel Kiper
2018-05-30 11:30:43 UTC
Permalink
Post by Goffredo Baroncelli
---
grub-core/fs/btrfs.c | 174 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 170 insertions(+), 4 deletions(-)
diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 63651928b..5fcaad86f 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -29,6 +29,7 @@
#include <minilzo.h>
#include <grub/i18n.h>
#include <grub/btrfs.h>
+#include <grub/crypto.h>
GRUB_MOD_LICENSE ("GPLv3+");
@@ -666,6 +667,150 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
return err;
}
+struct raid56_buffer {
+ void *buf;
+ int data_is_valid;
+};
+
+static void
+rebuild_raid5 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
+ grub_uint64_t csize)
+{
+ grub_uint64_t target = 0, i;
+
+ while (buffers[target].data_is_valid && target < nstripes)
+ ++target;
+
+ if (target == nstripes)
+ {
+ grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n");
+ return;
+ }
+
+ grub_dprintf ("btrfs", "rebuilding raid5 stripe #%" PRIuGRUB_UINT64_T "\n",
+ target);
+ for (i = 0; i < nstripes; i++)
+ if (i != target)
+ grub_crypto_xor (buffers[target].buf, buffers[target].buf, buffers[i].buf,
+ csize);
+}
+
+static grub_err_t
+raid56_read_retry (struct grub_btrfs_data *data,
+ struct grub_btrfs_chunk_item *chunk,
+ grub_uint64_t stripe_offset, grub_uint64_t stripen,
+ grub_uint64_t csize, void *buf)
+{
+
+ struct raid56_buffer *buffers = NULL;
+ grub_uint64_t nstripes = grub_le_to_cpu16 (chunk->nstripes);
+ grub_uint64_t chunk_type = grub_le_to_cpu64 (chunk->type);
+ grub_err_t ret = GRUB_ERR_NONE;
+ grub_uint64_t i, failed_devices;
+
+ buffers = grub_zalloc (sizeof(*buffers) * nstripes);
+ if (!buffers)
+ {
+ ret = GRUB_ERR_OUT_OF_MEMORY;
+ goto cleanup;
+ }
+
+ for (i = 0; i < nstripes; i++)
+ {
+ buffers[i].buf = grub_zalloc (csize);
+ if (!buffers[i].buf)
+ {
+ ret = GRUB_ERR_OUT_OF_MEMORY;
+ goto cleanup;
+ }
+ }
+
+ for (i = 0; i < nstripes; i++)
+ {
+ struct grub_btrfs_chunk_stripe *stripe;
+ grub_disk_addr_t paddr;
+ grub_device_t dev;
+ grub_err_t err2;
+
+ stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
+ stripe += i;
+
+ paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
+ grub_dprintf ("btrfs", "reading paddr %" PRIxGRUB_UINT64_T
+ " from stripe ID %" PRIxGRUB_UINT64_T "\n", paddr,
+ stripe->device_id);
+
+ dev = find_device (data, stripe->device_id);
+ if (!dev)
+ {
+ buffers[i].data_is_valid = 0;
+ grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " FAILED (dev ID %"
+ PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
+ continue;
What will happen if more than one stripe is broken?
Post by Goffredo Baroncelli
+ }
+
+ err2 = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
+ paddr & (GRUB_DISK_SECTOR_SIZE - 1),
+ csize, buffers[i].buf);
+ if (err2 == GRUB_ERR_NONE)
+ {
+ buffers[i].data_is_valid = 1;
+ grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " Ok (dev ID %"
+ PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
+ }
+ else
+ {
+ buffers[i].data_is_valid = 0;
+ grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T
+ " FAILED (dev ID %" PRIxGRUB_UINT64_T ")\n", i,
+ stripe->device_id);
Ditto?
Post by Goffredo Baroncelli
+ }
+ }
+
+ failed_devices = 0;
+ for (i = 0; i < nstripes; i++)
+ if (!buffers[i].data_is_valid)
+ ++failed_devices;
+ if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
+ {
+ grub_dprintf ("btrfs",
+ "not enough disks for raid5: total %" PRIuGRUB_UINT64_T
s/raid5/RAID5/ Please fix this and the rest of messages in the other patches.
Post by Goffredo Baroncelli
+ ", missing %" PRIuGRUB_UINT64_T "\n",
+ nstripes, failed_devices);
+ ret = GRUB_ERR_READ_ERROR;
+ goto cleanup;
Ahhh... Here it is! Perfect!
Post by Goffredo Baroncelli
+ }
+ else
+ {
+ grub_dprintf ("btrfs",
+ "enough disks for raid5/6 rebuilding: total %"
s#raid5/6#RAID5# This is the patch for RAID 5. Right?
Please do not mix RAID 5 changes with RAID 6 changes.
Post by Goffredo Baroncelli
+ PRIuGRUB_UINT64_T ", missing %" PRIuGRUB_UINT64_T "\n",
+ nstripes, failed_devices);
+ }
+
+ /* if these are enough, try to rebuild the data */
+ if (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5)
+ {
+ rebuild_raid5 (buffers, nstripes, csize);
+ grub_memcpy (buf, buffers[stripen].buf, csize);
Hmmm... Do we need this grub_memcpy()? Why rebuild_raid5() could
not fill buf directly? If it is not possible then I think that
grub_memcpy() should be called from rebuild_raid5().
Post by Goffredo Baroncelli
+ }
+ else
+ {
+ grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
+ }
Please drop this curly brackets.
Post by Goffredo Baroncelli
+
Space before the label please.
Post by Goffredo Baroncelli
+ 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 +888,11 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
grub_uint16_t nstripes;
unsigned redundancy = 1;
unsigned i, j;
+ int is_raid56;
+ grub_uint64_t parities_pos = 0;
+
+ is_raid56 = !!(grub_le_to_cpu64 (chunk->type) &
+ GRUB_BTRFS_CHUNK_TYPE_RAID5);
OK, I would leave this as is. However, I would mention
in the commit message that this patch also do some minor
preparatory work for RAID 6 support.
Post by Goffredo Baroncelli
if (grub_le_to_cpu64 (chunk->size) <= off)
{
@@ -885,6 +1035,8 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
* 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;
@@ -917,15 +1069,29 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
grub_dprintf ("btrfs", "reading laddr 0x%" PRIxGRUB_UINT64_T "\n",
addr);
- for (i = 0; i < redundancy; i++)
+ if (!is_raid56)
+ {
+ for (i = 0; i < redundancy; i++)
+ {
+ err = btrfs_read_from_chunk (data, chunk, stripen,
+ stripe_offset,
+ i, /* redundancy */
+ csize, buf);
+ if (err == GRUB_ERR_NONE)
+ break;
+ }
+ }
+ else
{
err = btrfs_read_from_chunk (data, chunk, stripen,
stripe_offset,
- i, /* redundancy */
+ 0, /* no mirror */
csize, buf);
I do not understand this change... Why?
Post by Goffredo Baroncelli
- if (err == GRUB_ERR_NONE)
- break;
+ if (err != GRUB_ERR_NONE)
+ err = raid56_read_retry (data, chunk, stripe_offset,
+ stripen, csize, buf);
I have a feeling that this change is somehow related to my comment
for patch #6.
Post by Goffredo Baroncelli
}
+
Drop this change please.
Post by Goffredo Baroncelli
if (err == GRUB_ERR_NONE)
break;
}
Daniel
Goffredo Baroncelli
2018-06-01 18:50:28 UTC
Permalink
Post by Daniel Kiper
Post by Goffredo Baroncelli
---
grub-core/fs/btrfs.c | 174 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 170 insertions(+), 4 deletions(-)
diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 63651928b..5fcaad86f 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -29,6 +29,7 @@
#include <minilzo.h>
#include <grub/i18n.h>
#include <grub/btrfs.h>
+#include <grub/crypto.h>
GRUB_MOD_LICENSE ("GPLv3+");
@@ -666,6 +667,150 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data,
return err;
}
+struct raid56_buffer {
+ void *buf;
+ int data_is_valid;
+};
+
+static void
+rebuild_raid5 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
+ grub_uint64_t csize)
+{
+ grub_uint64_t target = 0, i;
+
+ while (buffers[target].data_is_valid && target < nstripes)
+ ++target;
+
+ if (target == nstripes)
+ {
+ grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n");
+ return;
+ }
+
+ grub_dprintf ("btrfs", "rebuilding raid5 stripe #%" PRIuGRUB_UINT64_T "\n",
+ target);
+ for (i = 0; i < nstripes; i++)
+ if (i != target)
+ grub_crypto_xor (buffers[target].buf, buffers[target].buf, buffers[i].buf,
+ csize);
+}
+
+static grub_err_t
+raid56_read_retry (struct grub_btrfs_data *data,
+ struct grub_btrfs_chunk_item *chunk,
+ grub_uint64_t stripe_offset, grub_uint64_t stripen,
+ grub_uint64_t csize, void *buf)
+{
+
+ struct raid56_buffer *buffers = NULL;
+ grub_uint64_t nstripes = grub_le_to_cpu16 (chunk->nstripes);
+ grub_uint64_t chunk_type = grub_le_to_cpu64 (chunk->type);
+ grub_err_t ret = GRUB_ERR_NONE;
+ grub_uint64_t i, failed_devices;
+
+ buffers = grub_zalloc (sizeof(*buffers) * nstripes);
+ if (!buffers)
+ {
+ ret = GRUB_ERR_OUT_OF_MEMORY;
+ goto cleanup;
+ }
+
+ for (i = 0; i < nstripes; i++)
+ {
+ buffers[i].buf = grub_zalloc (csize);
+ if (!buffers[i].buf)
+ {
+ ret = GRUB_ERR_OUT_OF_MEMORY;
+ goto cleanup;
+ }
+ }
+
+ for (i = 0; i < nstripes; i++)
+ {
+ struct grub_btrfs_chunk_stripe *stripe;
+ grub_disk_addr_t paddr;
+ grub_device_t dev;
+ grub_err_t err2;
+
+ stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1);
+ stripe += i;
+
+ paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset;
+ grub_dprintf ("btrfs", "reading paddr %" PRIxGRUB_UINT64_T
+ " from stripe ID %" PRIxGRUB_UINT64_T "\n", paddr,
+ stripe->device_id);
+
+ dev = find_device (data, stripe->device_id);
+ if (!dev)
+ {
+ buffers[i].data_is_valid = 0;
+ grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " FAILED (dev ID %"
+ PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
+ continue;
What will happen if more than one stripe is broken?
See below
Post by Daniel Kiper
Post by Goffredo Baroncelli
+ }
+
+ err2 = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS,
+ paddr & (GRUB_DISK_SECTOR_SIZE - 1),
+ csize, buffers[i].buf);
+ if (err2 == GRUB_ERR_NONE)
+ {
+ buffers[i].data_is_valid = 1;
+ grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " Ok (dev ID %"
+ PRIxGRUB_UINT64_T ")\n", i, stripe->device_id);
+ }
+ else
+ {
+ buffers[i].data_is_valid = 0;
+ grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T
+ " FAILED (dev ID %" PRIxGRUB_UINT64_T ")\n", i,
+ stripe->device_id);
Ditto?
See below
Post by Daniel Kiper
Post by Goffredo Baroncelli
+ }
+ }
+
+ failed_devices = 0;
+ for (i = 0; i < nstripes; i++)
+ if (!buffers[i].data_is_valid)
+ ++failed_devices;
+ if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5))
+ {
+ grub_dprintf ("btrfs",
+ "not enough disks for raid5: total %" PRIuGRUB_UINT64_T
s/raid5/RAID5/ Please fix this and the rest of messages in the other patches.
OK
Post by Daniel Kiper
Post by Goffredo Baroncelli
+ ", missing %" PRIuGRUB_UINT64_T "\n",
+ nstripes, failed_devices);
+ ret = GRUB_ERR_READ_ERROR;
+ goto cleanup;
Ahhh... Here it is! Perfect!
Right !
Post by Daniel Kiper
Post by Goffredo Baroncelli
+ }
+ else
+ {
+ grub_dprintf ("btrfs",
+ "enough disks for raid5/6 rebuilding: total %"
s#raid5/6#RAID5# This is the patch for RAID 5. Right?
Please do not mix RAID 5 changes with RAID 6 changes.
ok
Post by Daniel Kiper
Post by Goffredo Baroncelli
+ PRIuGRUB_UINT64_T ", missing %" PRIuGRUB_UINT64_T "\n",
+ nstripes, failed_devices);
+ }
+
+ /* if these are enough, try to rebuild the data */
+ if (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5)
+ {
+ rebuild_raid5 (buffers, nstripes, csize);
+ grub_memcpy (buf, buffers[stripen].buf, csize);
Hmmm... Do we need this grub_memcpy()? Why rebuild_raid5() could
not fill buf directly? If it is not possible then I think that
grub_memcpy() should be called from rebuild_raid5().
I removed it
Post by Daniel Kiper
Post by Goffredo Baroncelli
+ }
+ else
+ {
+ grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
+ }
Please drop this curly brackets.
Ok,
Post by Daniel Kiper
Post by Goffredo Baroncelli
+
Space before the label please.
ok
Post by Daniel Kiper
Post by Goffredo Baroncelli
+ 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 +888,11 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
grub_uint16_t nstripes;
unsigned redundancy = 1;
unsigned i, j;
+ int is_raid56;
+ grub_uint64_t parities_pos = 0;
+
+ is_raid56 = !!(grub_le_to_cpu64 (chunk->type) &
+ GRUB_BTRFS_CHUNK_TYPE_RAID5);
OK, I would leave this as is. However, I would mention
in the commit message that this patch also do some minor
preparatory work for RAID 6 support.
OK
Post by Daniel Kiper
Post by Goffredo Baroncelli
if (grub_le_to_cpu64 (chunk->size) <= off)
{
@@ -885,6 +1035,8 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
* 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;
@@ -917,15 +1069,29 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
grub_dprintf ("btrfs", "reading laddr 0x%" PRIxGRUB_UINT64_T "\n",
addr);
- for (i = 0; i < redundancy; i++)
+ if (!is_raid56)
+ {
+ for (i = 0; i < redundancy; i++)
+ {
+ err = btrfs_read_from_chunk (data, chunk, stripen,
+ stripe_offset,
+ i, /* redundancy */
+ csize, buf);
+ if (err == GRUB_ERR_NONE)
+ break;
+ }
+ }
+ else
{
err = btrfs_read_from_chunk (data, chunk, stripen,
stripe_offset,
- i, /* redundancy */
+ 0, /* no mirror */
csize, buf);
I do not understand this change... Why?
In RAID1, RAID10 you have several disks from which you can read without further effort. In raid5 and 6, you must read from a specific disk, otherwise you need to rebuild the stripe. Is very different.
Pay attention that the diff output is very confusing, because the old code is shifted to left and diff think that it is a new code....
Post by Daniel Kiper
Post by Goffredo Baroncelli
- if (err == GRUB_ERR_NONE)
- break;
+ if (err != GRUB_ERR_NONE)
+ err = raid56_read_retry (data, chunk, stripe_offset,
+ stripen, csize, buf);
I have a feeling that this change is somehow related to my comment
for patch #6.
IS very different, the former is "exit from the retry-from-another-mirror loop if the read is ok", the latter is "in case of error rebuild the stripe". In the first case it is possible to check the iterator index; in the second one I can check only the exit code.
Post by Daniel Kiper
Post by Goffredo Baroncelli
}
+
Drop this change please.
ok
Post by Daniel Kiper
Post by Goffredo Baroncelli
if (err == GRUB_ERR_NONE)
break;
}
Daniel
--
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
Goffredo Baroncelli
2018-05-16 18:48:14 UTC
Permalink
If a device is not found, record this failure storing NULL in
data->devices_attached[]. This avoid un-necessary devices rescan,
speeding the reading in case of a degraded array.

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

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index fd4225d11..4920dd3b1 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -588,7 +588,7 @@ find_device_iter (const char *name, void *data)
}

static grub_device_t
-find_device (struct grub_btrfs_data *data, grub_uint64_t id, int do_rescan)
+find_device (struct grub_btrfs_data *data, grub_uint64_t id)
{
struct find_device_ctx ctx = {
.data = data,
@@ -600,12 +600,9 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t id, int do_rescan)
for (i = 0; i < data->n_devices_attached; i++)
if (id == data->devices_attached[i].id)
return data->devices_attached[i].dev;
- if (do_rescan)
- grub_device_iterate (find_device_iter, &ctx);
- if (!ctx.dev_found)
- {
- return NULL;
- }
+
+ grub_device_iterate (find_device_iter, &ctx);
+
data->n_devices_attached++;
if (data->n_devices_attached > data->n_devices_allocated)
{
@@ -617,7 +614,8 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t id, int do_rescan)
* sizeof (data->devices_attached[0]));
if (!data->devices_attached)
{
- grub_device_close (ctx.dev_found);
+ if (ctx.dev_found)
+ grub_device_close (ctx.dev_found);
data->devices_attached = tmp;
return NULL;
}
@@ -894,7 +892,7 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr,
" for laddr 0x%" PRIxGRUB_UINT64_T "\n", paddr,
addr);

- dev = find_device (data, stripe->device_id, j);
+ dev = find_device (data, stripe->device_id);
if (!dev)
{
grub_dprintf ("btrfs",
@@ -971,7 +969,8 @@ grub_btrfs_unmount (struct grub_btrfs_data *data)
unsigned i;
/* The device 0 is closed one layer upper. */
for (i = 1; i < data->n_devices_attached; i++)
- grub_device_close (data->devices_attached[i].dev);
+ if (data->devices_attached[i].dev)
+ grub_device_close (data->devices_attached[i].dev);
grub_free (data->devices_attached);
grub_free (data->extent);
grub_free (data);
--
2.17.0
Daniel Kiper
2018-05-30 10:43:51 UTC
Permalink
WRT the subject:
s/btrfs: avoiding a rescan for a device which was already not founded./
btrfs: Avoid a rescan for a device which was already not found/
Post by Goffredo Baroncelli
If a device is not found, record this failure storing NULL in
s/failure storing/failure by storing/
Post by Goffredo Baroncelli
data->devices_attached[]. This avoid un-necessary devices rescan,
speeding the reading in case of a degraded array.
Last sentence should sound more or less like that:

This way we avoid unnecessary devices rescan and speedup the reads
in case of degraded array.

Otherwise LGTM.

Daniel
Goffredo Baroncelli
2018-05-16 18:48:18 UTC
Permalink
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 argument.

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

diff --git a/grub-core/disk/raid6_recover.c b/grub-core/disk/raid6_recover.c
index aa674f6ca..10ee495e2 100644
--- a/grub-core/disk/raid6_recover.c
+++ b/grub-core/disk/raid6_recover.c
@@ -74,14 +74,25 @@ mod_255 (unsigned x)
}

static grub_err_t
-grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
- char *buf, grub_disk_addr_t sector, grub_size_t size)
+raid6_recover_read_node (void *data, int disk_nr,
+ grub_uint64_t sector,
+ void *buf, grub_size_t size)
+{
+ struct grub_diskfilter_segment *array = data;
+ return grub_diskfilter_read_node (&array->nodes[disk_nr],
+ (grub_disk_addr_t)sector,
+ size >> GRUB_DISK_SECTOR_BITS, buf);
+}
+
+grub_err_t
+grub_raid6_recover_gen (void *data, grub_uint64_t nstripes, int disknr, int p,
+ char *buf, grub_uint64_t sector, grub_size_t size,
+ raid_recover_read_t read_func, int layout)
{
int i, q, pos;
int bad1 = -1, bad2 = -1;
char *pbuf = 0, *qbuf = 0;

- size <<= GRUB_DISK_SECTOR_BITS;
pbuf = grub_zalloc (size);
if (!pbuf)
goto quit;
@@ -91,17 +102,17 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
goto quit;

q = p + 1;
- if (q == (int) array->node_count)
+ if (q == (int) nstripes)
q = 0;

pos = q + 1;
- if (pos == (int) array->node_count)
+ if (pos == (int) nstripes)
pos = 0;

- for (i = 0; i < (int) array->node_count - 2; i++)
+ for (i = 0; i < (int) nstripes - 2; i++)
{
int c;
- if (array->layout & GRUB_RAID_LAYOUT_MUL_FROM_POS)
+ if (layout & GRUB_RAID_LAYOUT_MUL_FROM_POS)
c = pos;
else
c = i;
@@ -109,8 +120,7 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
bad1 = c;
else
{
- if (! grub_diskfilter_read_node (&array->nodes[pos], sector,
- size >> GRUB_DISK_SECTOR_BITS, buf))
+ if (!read_func(data, pos, sector, buf, size))
{
grub_crypto_xor (pbuf, pbuf, buf, size);
grub_raid_block_mulx (c, buf, size);
@@ -128,7 +138,7 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
}

pos++;
- if (pos == (int) array->node_count)
+ if (pos == (int) nstripes)
pos = 0;
}

@@ -139,16 +149,14 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
if (bad2 < 0)
{
/* One bad device */
- if ((! grub_diskfilter_read_node (&array->nodes[p], sector,
- size >> GRUB_DISK_SECTOR_BITS, buf)))
+ if (!read_func(data, p, sector, buf, size))
{
grub_crypto_xor (buf, buf, pbuf, size);
goto quit;
}

grub_errno = GRUB_ERR_NONE;
- if (grub_diskfilter_read_node (&array->nodes[q], sector,
- size >> GRUB_DISK_SECTOR_BITS, buf))
+ if (read_func(data, q, sector, buf, size))
goto quit;

grub_crypto_xor (buf, buf, qbuf, size);
@@ -160,14 +168,12 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
/* Two bad devices */
unsigned c;

- if (grub_diskfilter_read_node (&array->nodes[p], sector,
- size >> GRUB_DISK_SECTOR_BITS, buf))
+ if (read_func(data, p, sector, buf, size))
goto quit;

grub_crypto_xor (pbuf, pbuf, buf, size);

- if (grub_diskfilter_read_node (&array->nodes[q], sector,
- size >> GRUB_DISK_SECTOR_BITS, buf))
+ if (read_func(data, q, sector, buf, size))
goto quit;

grub_crypto_xor (qbuf, qbuf, buf, size);
@@ -190,6 +196,17 @@ 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,
+ raid6_recover_read_node,
+ array->layout);
+}
+
GRUB_MOD_INIT(raid6rec)
{
grub_raid6_init_table ();
diff --git a/include/grub/diskfilter.h b/include/grub/diskfilter.h
index d89273c1b..911888e33 100644
--- a/include/grub/diskfilter.h
+++ b/include/grub/diskfilter.h
@@ -189,6 +189,15 @@ typedef grub_err_t (*grub_raid6_recover_func_t) (struct grub_diskfilter_segment
extern grub_raid5_recover_func_t grub_raid5_recover_func;
extern grub_raid6_recover_func_t grub_raid6_recover_func;

+typedef grub_err_t (* raid_recover_read_t)(void *data, int disk_nr,
+ grub_uint64_t addr, void *dest,
+ grub_size_t size);
+
+grub_err_t
+grub_raid6_recover_gen (void *data, grub_uint64_t nstripes, int disknr, int p,
+ char *buf, grub_uint64_t sector, grub_size_t size,
+ raid_recover_read_t read_func, int layout);
+
grub_err_t grub_diskfilter_vg_register (struct grub_diskfilter_vg *vg);

grub_err_t
--
2.17.0
Daniel Kiper
2018-05-30 12:05:56 UTC
Permalink
Post by 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 argument.
s/as argument/as an argument/
Post by Goffredo Baroncelli
---
grub-core/disk/raid6_recover.c | 53 ++++++++++++++++++++++------------
include/grub/diskfilter.h | 9 ++++++
2 files changed, 44 insertions(+), 18 deletions(-)
diff --git a/grub-core/disk/raid6_recover.c b/grub-core/disk/raid6_recover.c
index aa674f6ca..10ee495e2 100644
--- a/grub-core/disk/raid6_recover.c
+++ b/grub-core/disk/raid6_recover.c
@@ -74,14 +74,25 @@ mod_255 (unsigned x)
}
static grub_err_t
-grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
- char *buf, grub_disk_addr_t sector, grub_size_t size)
+raid6_recover_read_node (void *data, int disk_nr,
s/disk_nr/disknr/
Post by Goffredo Baroncelli
+ grub_uint64_t sector,
+ void *buf, grub_size_t size)
+{
+ struct grub_diskfilter_segment *array = data;
Please add one empty line here.
Post by Goffredo Baroncelli
+ return grub_diskfilter_read_node (&array->nodes[disk_nr],
+ (grub_disk_addr_t)sector,
+ size >> GRUB_DISK_SECTOR_BITS, buf);
+}
+
+grub_err_t
+grub_raid6_recover_gen (void *data, grub_uint64_t nstripes, int disknr, int p,
+ char *buf, grub_uint64_t sector, grub_size_t size,
+ raid_recover_read_t read_func, int layout)
Could you change the order of read_func and layout arguments?
I mean make read_func the last one and put the layout before it.
Post by Goffredo Baroncelli
{
int i, q, pos;
int bad1 = -1, bad2 = -1;
char *pbuf = 0, *qbuf = 0;
- size <<= GRUB_DISK_SECTOR_BITS;
pbuf = grub_zalloc (size);
if (!pbuf)
goto quit;
@@ -91,17 +102,17 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
goto quit;
q = p + 1;
- if (q == (int) array->node_count)
+ if (q == (int) nstripes)
q = 0;
pos = q + 1;
- if (pos == (int) array->node_count)
+ if (pos == (int) nstripes)
pos = 0;
- for (i = 0; i < (int) array->node_count - 2; i++)
+ for (i = 0; i < (int) nstripes - 2; i++)
{
int c;
- if (array->layout & GRUB_RAID_LAYOUT_MUL_FROM_POS)
+ if (layout & GRUB_RAID_LAYOUT_MUL_FROM_POS)
c = pos;
else
c = i;
@@ -109,8 +120,7 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
bad1 = c;
else
{
- if (! grub_diskfilter_read_node (&array->nodes[pos], sector,
- size >> GRUB_DISK_SECTOR_BITS, buf))
+ if (!read_func(data, pos, sector, buf, size))
{
grub_crypto_xor (pbuf, pbuf, buf, size);
grub_raid_block_mulx (c, buf, size);
@@ -128,7 +138,7 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
}
pos++;
- if (pos == (int) array->node_count)
+ if (pos == (int) nstripes)
pos = 0;
}
@@ -139,16 +149,14 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
if (bad2 < 0)
{
/* One bad device */
- if ((! grub_diskfilter_read_node (&array->nodes[p], sector,
- size >> GRUB_DISK_SECTOR_BITS, buf)))
+ if (!read_func(data, p, sector, buf, size))
{
grub_crypto_xor (buf, buf, pbuf, size);
goto quit;
}
grub_errno = GRUB_ERR_NONE;
- if (grub_diskfilter_read_node (&array->nodes[q], sector,
- size >> GRUB_DISK_SECTOR_BITS, buf))
+ if (read_func(data, q, sector, buf, size))
goto quit;
grub_crypto_xor (buf, buf, qbuf, size);
@@ -160,14 +168,12 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
/* Two bad devices */
unsigned c;
- if (grub_diskfilter_read_node (&array->nodes[p], sector,
- size >> GRUB_DISK_SECTOR_BITS, buf))
+ if (read_func(data, p, sector, buf, size))
goto quit;
grub_crypto_xor (pbuf, pbuf, buf, size);
- if (grub_diskfilter_read_node (&array->nodes[q], sector,
- size >> GRUB_DISK_SECTOR_BITS, buf))
+ if (read_func(data, q, sector, buf, size))
goto quit;
grub_crypto_xor (qbuf, qbuf, buf, size);
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)
+{
+
Drop this empty line.
Post by Goffredo Baroncelli
+ return grub_raid6_recover_gen (array, array->node_count, disknr, p, buf,
+ sector, size << GRUB_DISK_SECTOR_BITS,
+ raid6_recover_read_node,
+ array->layout);
+}
+
GRUB_MOD_INIT(raid6rec)
{
grub_raid6_init_table ();
diff --git a/include/grub/diskfilter.h b/include/grub/diskfilter.h
index d89273c1b..911888e33 100644
--- a/include/grub/diskfilter.h
+++ b/include/grub/diskfilter.h
@@ -189,6 +189,15 @@ typedef grub_err_t (*grub_raid6_recover_func_t) (struct grub_diskfilter_segment
extern grub_raid5_recover_func_t grub_raid5_recover_func;
extern grub_raid6_recover_func_t grub_raid6_recover_func;
+typedef grub_err_t (* raid_recover_read_t)(void *data, int disk_nr,
+ grub_uint64_t addr, void *dest,
+ grub_size_t size);
+
+grub_err_t
+grub_raid6_recover_gen (void *data, grub_uint64_t nstripes, int disknr, int p,
+ char *buf, grub_uint64_t sector, grub_size_t size,
+ raid_recover_read_t read_func, int layout);
Please add extern here.

Daniel
Goffredo Baroncelli
2018-06-01 18:50:31 UTC
Permalink
Post by Daniel Kiper
Post by 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 argument.
s/as argument/as an argument/
ok
Post by Daniel Kiper
Post by Goffredo Baroncelli
---
grub-core/disk/raid6_recover.c | 53 ++++++++++++++++++++++------------
include/grub/diskfilter.h | 9 ++++++
2 files changed, 44 insertions(+), 18 deletions(-)
diff --git a/grub-core/disk/raid6_recover.c b/grub-core/disk/raid6_recover.c
index aa674f6ca..10ee495e2 100644
--- a/grub-core/disk/raid6_recover.c
+++ b/grub-core/disk/raid6_recover.c
@@ -74,14 +74,25 @@ mod_255 (unsigned x)
}
static grub_err_t
-grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
- char *buf, grub_disk_addr_t sector, grub_size_t size)
+raid6_recover_read_node (void *data, int disk_nr,
s/disk_nr/disknr/
OK
Post by Daniel Kiper
Post by Goffredo Baroncelli
+ grub_uint64_t sector,
+ void *buf, grub_size_t size)
+{
+ struct grub_diskfilter_segment *array = data;
Please add one empty line here.
Post by Goffredo Baroncelli
+ return grub_diskfilter_read_node (&array->nodes[disk_nr],
+ (grub_disk_addr_t)sector,
+ size >> GRUB_DISK_SECTOR_BITS, buf);
+}
+
+grub_err_t
+grub_raid6_recover_gen (void *data, grub_uint64_t nstripes, int disknr, int p,
+ char *buf, grub_uint64_t sector, grub_size_t size,
+ raid_recover_read_t read_func, int layout)
Could you change the order of read_func and layout arguments?
I mean make read_func the last one and put the layout before it.
OK
Post by Daniel Kiper
Post by Goffredo Baroncelli
{
int i, q, pos;
int bad1 = -1, bad2 = -1;
char *pbuf = 0, *qbuf = 0;
- size <<= GRUB_DISK_SECTOR_BITS;
pbuf = grub_zalloc (size);
if (!pbuf)
goto quit;
@@ -91,17 +102,17 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
goto quit;
q = p + 1;
- if (q == (int) array->node_count)
+ if (q == (int) nstripes)
q = 0;
pos = q + 1;
- if (pos == (int) array->node_count)
+ if (pos == (int) nstripes)
pos = 0;
- for (i = 0; i < (int) array->node_count - 2; i++)
+ for (i = 0; i < (int) nstripes - 2; i++)
{
int c;
- if (array->layout & GRUB_RAID_LAYOUT_MUL_FROM_POS)
+ if (layout & GRUB_RAID_LAYOUT_MUL_FROM_POS)
c = pos;
else
c = i;
@@ -109,8 +120,7 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
bad1 = c;
else
{
- if (! grub_diskfilter_read_node (&array->nodes[pos], sector,
- size >> GRUB_DISK_SECTOR_BITS, buf))
+ if (!read_func(data, pos, sector, buf, size))
{
grub_crypto_xor (pbuf, pbuf, buf, size);
grub_raid_block_mulx (c, buf, size);
@@ -128,7 +138,7 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
}
pos++;
- if (pos == (int) array->node_count)
+ if (pos == (int) nstripes)
pos = 0;
}
@@ -139,16 +149,14 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
if (bad2 < 0)
{
/* One bad device */
- if ((! grub_diskfilter_read_node (&array->nodes[p], sector,
- size >> GRUB_DISK_SECTOR_BITS, buf)))
+ if (!read_func(data, p, sector, buf, size))
{
grub_crypto_xor (buf, buf, pbuf, size);
goto quit;
}
grub_errno = GRUB_ERR_NONE;
- if (grub_diskfilter_read_node (&array->nodes[q], sector,
- size >> GRUB_DISK_SECTOR_BITS, buf))
+ if (read_func(data, q, sector, buf, size))
goto quit;
grub_crypto_xor (buf, buf, qbuf, size);
@@ -160,14 +168,12 @@ grub_raid6_recover (struct grub_diskfilter_segment *array, int disknr, int p,
/* Two bad devices */
unsigned c;
- if (grub_diskfilter_read_node (&array->nodes[p], sector,
- size >> GRUB_DISK_SECTOR_BITS, buf))
+ if (read_func(data, p, sector, buf, size))
goto quit;
grub_crypto_xor (pbuf, pbuf, buf, size);
- if (grub_diskfilter_read_node (&array->nodes[q], sector,
- size >> GRUB_DISK_SECTOR_BITS, buf))
+ if (read_func(data, q, sector, buf, size))
goto quit;
grub_crypto_xor (qbuf, qbuf, buf, size);
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)
+{
+
Drop this empty line.
ok
Post by Daniel Kiper
Post by Goffredo Baroncelli
+ return grub_raid6_recover_gen (array, array->node_count, disknr, p, buf,
+ sector, size << GRUB_DISK_SECTOR_BITS,
+ raid6_recover_read_node,
+ array->layout);
+}
+
GRUB_MOD_INIT(raid6rec)
{
grub_raid6_init_table ();
diff --git a/include/grub/diskfilter.h b/include/grub/diskfilter.h
index d89273c1b..911888e33 100644
--- a/include/grub/diskfilter.h
+++ b/include/grub/diskfilter.h
@@ -189,6 +189,15 @@ typedef grub_err_t (*grub_raid6_recover_func_t) (struct grub_diskfilter_segment
extern grub_raid5_recover_func_t grub_raid5_recover_func;
extern grub_raid6_recover_func_t grub_raid6_recover_func;
+typedef grub_err_t (* raid_recover_read_t)(void *data, int disk_nr,
+ grub_uint64_t addr, void *dest,
+ grub_size_t size);
+
+grub_err_t
+grub_raid6_recover_gen (void *data, grub_uint64_t nstripes, int disknr, int p,
+ char *buf, grub_uint64_t sector, grub_size_t size,
+ raid_recover_read_t read_func, int layout);
Please add extern here.
ok
Post by Daniel Kiper
Daniel
--
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
Daniel Kiper
2018-05-30 11:07:05 UTC
Permalink
Post by Goffredo Baroncelli
This is a preparatory patch, to help the adding of the RAID 5/6 recovery
Please move "This is a preparatory patch" sentence below...
Post by Goffredo Baroncelli
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.
Post by Goffredo Baroncelli
---
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.
Post by Goffredo Baroncelli
+ 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
Goffredo Baroncelli
2018-06-01 18:50:29 UTC
Permalink
Post by Daniel Kiper
Post by Goffredo Baroncelli
This is a preparatory patch, to help the adding of the RAID 5/6 recovery
Please move "This is a preparatory patch" sentence below...
Post by Goffredo Baroncelli
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.

----
Post by Daniel Kiper
Post by Goffredo Baroncelli
---
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
Post by Daniel Kiper
Post by Goffredo Baroncelli
+ 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
Post by Daniel Kiper
Ditto.
Daniel
--
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
Continue reading on narkive:
Loading...