On 06/14/2018 03:03 PM, Daniel Kiper wrote: > On Sun, Jun 03, 2018 at 08:53:46PM +0200, Goffredo Baroncelli wrote: >> Add support for recovery fo a RAID 5 btrfs profile. In addition > > s/fo /for / > >> it is added some code as preparatory work for RAID 6 recovery code. >> >> Signed-off-by: Goffredo Baroncelli <kreij...@inwind.it> >> --- >> grub-core/fs/btrfs.c | 180 +++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 175 insertions(+), 5 deletions(-) >> >> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c >> index 9cdbfe792..c8f034641 100644 >> --- a/grub-core/fs/btrfs.c >> +++ b/grub-core/fs/btrfs.c >> @@ -29,6 +29,7 @@ >> #include <minilzo.h> >> #include <grub/i18n.h> >> #include <grub/btrfs.h> >> +#include <grub/crypto.h> >> >> GRUB_MOD_LICENSE ("GPLv3+"); >> >> @@ -666,6 +667,157 @@ btrfs_read_from_chunk (struct grub_btrfs_data *data, >> return err; >> } >> >> +struct raid56_buffer { >> + void *buf; >> + int data_is_valid; >> +}; >> + >> +static void >> +rebuild_raid5 (char *dest, struct raid56_buffer *buffers, >> + grub_uint64_t nstripes, grub_uint64_t csize) >> +{ >> + grub_uint64_t i; > > grub_uint64_t i = 0; > int first = 1; > >> + int first; >> + >> + i = 0; > > Then you can drop this assignment.
I prefer to tie the assignment to the place where it is used. > >> + while (buffers[i].data_is_valid && i < nstripes) >> + ++i; >> + >> + if (i == nstripes) >> + { >> + grub_dprintf ("btrfs", "called rebuild_raid5(), but all disks are >> OK\n"); >> + return; >> + } >> + >> + grub_dprintf ("btrfs", "rebuilding RAID 5 stripe #%" PRIuGRUB_UINT64_T >> "\n", >> + i); > > This can be in one line. > >> + first = 1; > > And you can drop this assignment too. > >> + for (i = 0; i < nstripes; i++) >> + { >> + if (!buffers[i].data_is_valid) >> + continue; >> + >> + if (first) >> + grub_memcpy(dest, buffers[i].buf, csize); >> + else >> + grub_crypto_xor (dest, dest, buffers[i].buf, csize); > > I am not sure why at first you use grub_memcpy() and > then move to grub_crypto_xor(). Could you explain this? > Why do not use grub_crypto_xor() in all cases? This avoid to require that dest has to be initialized to zero. > >> + >> + first = 0; >> + } >> +} >> + >> +static grub_err_t >> +raid56_read_retry (struct grub_btrfs_data *data, >> + struct grub_btrfs_chunk_item *chunk, >> + grub_uint64_t stripe_offset, >> + grub_uint64_t csize, void *buf) >> +{ >> + > > Please drop this empty line. I have asked about that earlier. > >> + struct raid56_buffer *buffers = NULL; >> + grub_uint64_t nstripes = grub_le_to_cpu16 (chunk->nstripes); >> + grub_uint64_t chunk_type = grub_le_to_cpu64 (chunk->type); >> + grub_err_t ret = GRUB_ERR_NONE; >> + grub_uint64_t i, failed_devices; >> + >> + buffers = grub_zalloc (sizeof(*buffers) * nstripes); > > How often this function is called? Maybe you should consider > doing memory allocation for this function only once and free > it at btrfs module unload. This is only needed in case of recovery. Which should happen no too often. Usually, this function is never called. > >> + if (!buffers) >> + { >> + ret = GRUB_ERR_OUT_OF_MEMORY; >> + goto cleanup; >> + } >> + >> + for (i = 0; i < nstripes; i++) >> + { >> + buffers[i].buf = grub_zalloc (csize); > > Ditto. > >> + if (!buffers[i].buf) >> + { >> + ret = GRUB_ERR_OUT_OF_MEMORY; >> + goto cleanup; >> + } >> + } >> + >> + for (i = 0; i < nstripes; i++) >> + { >> + struct grub_btrfs_chunk_stripe *stripe; >> + grub_disk_addr_t paddr; >> + grub_device_t dev; >> + grub_err_t err2; >> + >> + stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); >> + stripe += i; >> + >> + paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; >> + grub_dprintf ("btrfs", "reading paddr %" PRIxGRUB_UINT64_T >> + " from stripe ID %" PRIxGRUB_UINT64_T "\n", paddr, >> + stripe->device_id); >> + >> + dev = find_device (data, stripe->device_id); >> + if (!dev) >> + { >> + buffers[i].data_is_valid = 0; >> + grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " FAILED (dev ID >> %" >> + PRIxGRUB_UINT64_T ")\n", i, stripe->device_id); >> + continue; >> + } >> + >> + err2 = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, >> + paddr & (GRUB_DISK_SECTOR_SIZE - 1), >> + csize, buffers[i].buf); >> + if (err2 == GRUB_ERR_NONE) >> + { >> + buffers[i].data_is_valid = 1; >> + grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T " Ok (dev ID %" >> + PRIxGRUB_UINT64_T ")\n", i, stripe->device_id); >> + } >> + else >> + { >> + buffers[i].data_is_valid = 0; >> + grub_dprintf ("btrfs", "stripe %" PRIuGRUB_UINT64_T >> + " FAILED (dev ID %" PRIxGRUB_UINT64_T ")\n", i, >> + stripe->device_id); >> + } >> + } >> + >> + failed_devices = 0; >> + for (i = 0; i < nstripes; i++) > > for (failed_devices = i = 0; i < nstripes; i++) Nice > >> + if (!buffers[i].data_is_valid) >> + ++failed_devices; >> + if (failed_devices > 1 && (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5)) >> + { >> + grub_dprintf ("btrfs", >> + "not enough disks for RAID 5: total %" PRIuGRUB_UINT64_T >> + ", missing %" PRIuGRUB_UINT64_T "\n", >> + nstripes, failed_devices); >> + ret = GRUB_ERR_READ_ERROR; >> + goto cleanup; >> + } >> + else >> + { >> + grub_dprintf ("btrfs", >> + "enough disks for RAID 5 rebuilding: total %" >> + PRIuGRUB_UINT64_T ", missing %" PRIuGRUB_UINT64_T "\n", >> + nstripes, failed_devices); >> + } >> + >> + /* if these are enough, try to rebuild the data */ >> + if (chunk_type & GRUB_BTRFS_CHUNK_TYPE_RAID5) >> + rebuild_raid5 (buf, buffers, nstripes, csize); >> + else >> + grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n"); >> + >> +cleanup: > > Space before the label please. > I have asked about earlier. The line before the label is already a space; Am I missing something ? > >> + > > Please drop this empty line. > > Daniel > -- gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel