| Message ID | 20180919184040.22540-8-kreijack@libero.it (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/9] btrfs: Add support for reading a filesystem with a RAID 5 or RAID 6 profile. | expand |
On Wed, Sep 19, 2018 at 08:40:38PM +0200, Goffredo Baroncelli wrote: > From: Goffredo Baroncelli <kreijack@inwind.it> > > Add support for recovery for a RAID 5 btrfs profile. In addition > it is added some code as preparatory work for RAID 6 recovery code. > > Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it> > --- > grub-core/fs/btrfs.c | 169 +++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 164 insertions(+), 5 deletions(-) > > diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c > index 5c1ebae77..55a7eeffc 100644 > --- a/grub-core/fs/btrfs.c > +++ b/grub-core/fs/btrfs.c > @@ -29,6 +29,7 @@ > #include <minilzo.h> > #include <grub/i18n.h> > #include <grub/btrfs.h> > +#include <grub/crypto.h> > > GRUB_MOD_LICENSE ("GPLv3+"); > > @@ -665,6 +666,148 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data, > return err; > } > > +struct raid56_buffer { > + void *buf; > + int data_is_valid; > +}; > + > +static void > +rebuild_raid5 (char *dest, struct raid56_buffer *buffers, > + grub_uint64_t nstripes, grub_uint64_t csize) > +{ > + grub_uint64_t i; > + int first; > + > + i = 0; > + while (buffers[i].data_is_valid && i < nstripes) > + ++i; for (i = 0; buffers[i].data_is_valid && i < nstripes; i++); > + if (i == nstripes) > + { > + grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n"); > + return; > + } > + > + grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T "\n", > + i); One line here please. > + for (i = 0, first = 1; i < nstripes; i++) > + { > + if (!buffers[i].data_is_valid) > + continue; > + > + if (first) { > + grub_memcpy(dest, buffers[i].buf, csize); > + first = 0; > + } else > + grub_crypto_xor (dest, dest, buffers[i].buf, csize); > + > + } Hmmm... I think that this function can be simpler. You can drop first while/for and "if (i == nstripes)". Then here: if (first) { grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n"); Am I right? > +} > + > +static grub_err_t > +raid56_read_retry (struct grub_btrfs_data *data, > + struct grub_btrfs_chunk_item *chunk, > + grub_uint64_t stripe_offset, > + grub_uint64_t csize, void *buf) > +{ > + struct raid56_buffer *buffers; > + grub_uint64_t nstripes = grub_le_to_cpu16 (chunk->nstripes); > + grub_uint64_t chunk_type = grub_le_to_cpu64 (chunk->type); > + grub_err_t ret = GRUB_ERR_NONE; s/GRUB_ERR_NONE/GRUB_ERR_OUT_OF_MEMORY/ and then you can drop at least two relevant assigments and some curly brackets. Of course before cleanup label you have to add ret = GRUB_ERR_NONE. > + grub_uint64_t i, failed_devices; > + > + buffers = grub_zalloc (sizeof(*buffers) * nstripes); > + if (!buffers) > + { > + ret = GRUB_ERR_OUT_OF_MEMORY; > + goto cleanup; > + } > + > + for (i = 0; i < nstripes; i++) > + { > + buffers[i].buf = grub_zalloc (csize); > + if (!buffers[i].buf) > + { > + ret = GRUB_ERR_OUT_OF_MEMORY; > + goto cleanup; > + } > + } > + > + for (failed_devices = 0, i = 0; i < nstripes; i++) > + { > + struct grub_btrfs_chunk_stripe *stripe; > + grub_disk_addr_t paddr; > + grub_device_t dev; > + grub_err_t err2; s/err2/err/? > + > + stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); > + stripe += i; Why not stripe = ((struct grub_btrfs_chunk_stripe *) (chunk + 1)) + i;? Daniel
On 25/09/2018 21.10, Daniel Kiper wrote: > On Wed, Sep 19, 2018 at 08:40:38PM +0200, Goffredo Baroncelli wrote: >> From: Goffredo Baroncelli <kreijack@inwind.it> >> >> Add support for recovery for a RAID 5 btrfs profile. In addition >> it is added some code as preparatory work for RAID 6 recovery code. >> >> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it> >> --- >> grub-core/fs/btrfs.c | 169 +++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 164 insertions(+), 5 deletions(-) >> >> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c >> index 5c1ebae77..55a7eeffc 100644 >> --- a/grub-core/fs/btrfs.c >> +++ b/grub-core/fs/btrfs.c >> @@ -29,6 +29,7 @@ >> #include <minilzo.h> >> #include <grub/i18n.h> >> #include <grub/btrfs.h> >> +#include <grub/crypto.h> >> >> GRUB_MOD_LICENSE ("GPLv3+"); >> >> @@ -665,6 +666,148 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data, >> return err; >> } >> >> +struct raid56_buffer { >> + void *buf; >> + int data_is_valid; >> +}; >> + >> +static void >> +rebuild_raid5 (char *dest, struct raid56_buffer *buffers, >> + grub_uint64_t nstripes, grub_uint64_t csize) >> +{ >> + grub_uint64_t i; >> + int first; >> + >> + i = 0; >> + while (buffers[i].data_is_valid && i < nstripes) >> + ++i; > > for (i = 0; buffers[i].data_is_valid && i < nstripes; i++); > >> + if (i == nstripes) >> + { >> + grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n"); >> + return; >> + } >> + >> + grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T "\n", >> + i); > > One line here please. > >> + for (i = 0, first = 1; i < nstripes; i++) >> + { >> + if (!buffers[i].data_is_valid) >> + continue; >> + >> + if (first) { >> + grub_memcpy(dest, buffers[i].buf, csize); >> + first = 0; >> + } else >> + grub_crypto_xor (dest, dest, buffers[i].buf, csize); >> + >> + } > > Hmmm... I think that this function can be simpler. You can drop first > while/for and "if (i == nstripes)". Then here: > > if (first) { > grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n"); > > Am I right? Ehm.. no. The "if" is an internal check to avoid BUG. rebuild_raid5() should be called only if some disk is missed. To perform this control, the code checks if all buffers are valid. Otherwise there is an internal BUG. Checking "first" is a completely different test. >> +} >> + >> +static grub_err_t >> +raid56_read_retry (struct grub_btrfs_data *data, >> + struct grub_btrfs_chunk_item *chunk, >> + grub_uint64_t stripe_offset, >> + grub_uint64_t csize, void *buf) >> +{ >> + struct raid56_buffer *buffers; >> + grub_uint64_t nstripes = grub_le_to_cpu16 (chunk->nstripes); >> + grub_uint64_t chunk_type = grub_le_to_cpu64 (chunk->type); >> + grub_err_t ret = GRUB_ERR_NONE; > > s/GRUB_ERR_NONE/GRUB_ERR_OUT_OF_MEMORY/ and then you can drop at > least two relevant assigments and some curly brackets. Of course > before cleanup label you have to add ret = GRUB_ERR_NONE. > >> + grub_uint64_t i, failed_devices; >> + >> + buffers = grub_zalloc (sizeof(*buffers) * nstripes); >> + if (!buffers) >> + { >> + ret = GRUB_ERR_OUT_OF_MEMORY; >> + goto cleanup; >> + } >> + >> + for (i = 0; i < nstripes; i++) >> + { >> + buffers[i].buf = grub_zalloc (csize); >> + if (!buffers[i].buf) >> + { >> + ret = GRUB_ERR_OUT_OF_MEMORY; >> + goto cleanup; >> + } >> + } >> + >> + for (failed_devices = 0, i = 0; i < nstripes; i++) >> + { >> + struct grub_btrfs_chunk_stripe *stripe; >> + grub_disk_addr_t paddr; >> + grub_device_t dev; >> + grub_err_t err2; > > s/err2/err/? Ok > >> + >> + stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); >> + stripe += i; > > Why not stripe = ((struct grub_btrfs_chunk_stripe *) (chunk + 1)) + i;? Make sense > > Daniel >
On Wed, Sep 26, 2018 at 09:55:57PM +0200, Goffredo Baroncelli wrote: > On 25/09/2018 21.10, Daniel Kiper wrote: > > On Wed, Sep 19, 2018 at 08:40:38PM +0200, Goffredo Baroncelli wrote: > >> From: Goffredo Baroncelli <kreijack@inwind.it> > >> > >> Add support for recovery for a RAID 5 btrfs profile. In addition > >> it is added some code as preparatory work for RAID 6 recovery code. > >> > >> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it> > >> --- > >> grub-core/fs/btrfs.c | 169 +++++++++++++++++++++++++++++++++++++++++-- > >> 1 file changed, 164 insertions(+), 5 deletions(-) > >> > >> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c > >> index 5c1ebae77..55a7eeffc 100644 > >> --- a/grub-core/fs/btrfs.c > >> +++ b/grub-core/fs/btrfs.c > >> @@ -29,6 +29,7 @@ > >> #include <minilzo.h> > >> #include <grub/i18n.h> > >> #include <grub/btrfs.h> > >> +#include <grub/crypto.h> > >> > >> GRUB_MOD_LICENSE ("GPLv3+"); > >> > >> @@ -665,6 +666,148 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data, > >> return err; > >> } > >> > >> +struct raid56_buffer { > >> + void *buf; > >> + int data_is_valid; > >> +}; > >> + > >> +static void > >> +rebuild_raid5 (char *dest, struct raid56_buffer *buffers, > >> + grub_uint64_t nstripes, grub_uint64_t csize) > >> +{ > >> + grub_uint64_t i; > >> + int first; > >> + > >> + i = 0; > >> + while (buffers[i].data_is_valid && i < nstripes) > >> + ++i; > > > > for (i = 0; buffers[i].data_is_valid && i < nstripes; i++); > > > >> + if (i == nstripes) > >> + { > >> + grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n"); > >> + return; > >> + } > >> + > >> + grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T "\n", > >> + i); > > > > One line here please. > > > >> + for (i = 0, first = 1; i < nstripes; i++) > >> + { > >> + if (!buffers[i].data_is_valid) > >> + continue; > >> + > >> + if (first) { > >> + grub_memcpy(dest, buffers[i].buf, csize); > >> + first = 0; > >> + } else > >> + grub_crypto_xor (dest, dest, buffers[i].buf, csize); > >> + > >> + } > > > > Hmmm... I think that this function can be simpler. You can drop first > > while/for and "if (i == nstripes)". Then here: > > > > if (first) { > > grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n"); > > > > Am I right? > > Ehm.. no. The "if" is an internal check to avoid BUG. rebuild_raid5() should be called only if some disk is missed. > To perform this control, the code checks if all buffers are valid. Otherwise there is an internal BUG. Something is wrong here. I think that the code checks if it is an invalid buffer. If there is not then GRUB complains. Right? However, it looks that I misread the code and made a mistake here. So, please ignore this change. Though please change while() with for() at the beginning. Daniel
diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c index 5c1ebae77..55a7eeffc 100644 --- a/grub-core/fs/btrfs.c +++ b/grub-core/fs/btrfs.c @@ -29,6 +29,7 @@ #include <minilzo.h> #include <grub/i18n.h> #include <grub/btrfs.h> +#include <grub/crypto.h> GRUB_MOD_LICENSE ("GPLv3+"); @@ -665,6 +666,148 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data, return err; } +struct raid56_buffer { + void *buf; + int data_is_valid; +}; + +static void +rebuild_raid5 (char *dest, struct raid56_buffer *buffers, + grub_uint64_t nstripes, grub_uint64_t csize) +{ + grub_uint64_t i; + int first; + + i = 0; + while (buffers[i].data_is_valid && i < nstripes) + ++i; + + if (i == nstripes) + { + grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are OK\n"); + return; + } + + grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T "\n", + i); + + for (i = 0, first = 1; i < nstripes; i++) + { + if (!buffers[i].data_is_valid) + continue; + + if (first) { + grub_memcpy(dest, buffers[i].buf, csize); + first = 0; + } else + grub_crypto_xor (dest, dest, buffers[i].buf, csize); + + } +} + +static grub_err_t +raid56_read_retry (struct grub_btrfs_data *data, + struct grub_btrfs_chunk_item *chunk, + grub_uint64_t stripe_offset, + grub_uint64_t csize, void *buf) +{ + struct raid56_buffer *buffers; + grub_uint64_t nstripes = grub_le_to_cpu16 (chunk->nstripes); + grub_uint64_t chunk_type = grub_le_to_cpu64 (chunk->type); + grub_err_t ret = GRUB_ERR_NONE; + grub_uint64_t i, failed_devices; + + buffers = grub_zalloc (sizeof(*buffers) * nstripes); + if (!buffers) + { + ret = GRUB_ERR_OUT_OF_MEMORY; + goto cleanup; + } + + for (i = 0; i < nstripes; i++) + { + buffers[i].buf = grub_zalloc (csize); + if (!buffers[i].buf) + { + ret = GRUB_ERR_OUT_OF_MEMORY; + goto cleanup; + } + } + + for (failed_devices = 0, i = 0; i < nstripes; i++) + { + struct grub_btrfs_chunk_stripe *stripe; + grub_disk_addr_t paddr; + grub_device_t dev; + grub_err_t err2; + + stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); + stripe += i; + + paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; + grub_dprintf ("btrfs", "reading paddr %" PRIxGRUB_UINT64_T + " from stripe ID %" PRIxGRUB_UINT64_T "\n", paddr, + stripe->device_id); + + dev = find_device (data, stripe->device_id); + if (!dev) + { + buffers[i].data_is_valid = 0; + grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " FAILED (dev ID %" + PRIxGRUB_UINT64_T ")\n", i, stripe->device_id); + failed_devices++; + continue; + } + + err2 = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, + paddr & (GRUB_DISK_SECTOR_SIZE - 1), + csize, buffers[i].buf); + if (err2 == GRUB_ERR_NONE) + { + buffers[i].data_is_valid = 1; + grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " Ok (dev ID %" + PRIxGRUB_UINT64_T ")\n", i, stripe->device_id); + } + else + { + buffers[i].data_is_valid = 0; + grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T + " FAILED (dev ID %" PRIxGRUB_UINT64_T ")\n", i, + stripe->device_id); + failed_devices++; + } + } + + if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5)) + { + grub_dprintf ("btrfs", + "not enough disks for RAID 5: total %" PRIuGRUB_UINT64_T + ", missing %" PRIuGRUB_UINT64_T "\n", + nstripes, failed_devices); + ret = GRUB_ERR_READ_ERROR; + goto cleanup; + } + else + grub_dprintf ("btrfs", + "enough disks for RAID 5 rebuilding: total %" + PRIuGRUB_UINT64_T ", missing %" PRIuGRUB_UINT64_T "\n", + nstripes, failed_devices); + + /* if these are enough, try to rebuild the data */ + if (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5) + rebuild_raid5 (buf, buffers, nstripes, csize); + else + grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n"); + + cleanup: + if (buffers) + for (i = 0; i < nstripes; i++) + grub_free(buffers[i].buf); + grub_free(buffers); + + return ret; +} + static grub_err_t grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, void *buf, grub_size_t size, int recursion_depth) @@ -742,6 +885,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) { @@ -914,17 +1061,29 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, grub_dprintf ("btrfs", "reading laddr 0x%" PRIxGRUB_UINT64_T "\n", addr); - for (i = 0; i < redundancy; i++) + if (!is_raid56) + 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)