Re: [PATCH v6 2/2] btrfs: Add zstd support to grub btrfs

[Daniel Kiper]

On Mon, Nov 19, 2018 at 03:22:51PM +0100, Daniel Kiper wrote:
> On Thu, Nov 15, 2018 at 02:36:03PM -0800, Nick Terrell wrote:
> > - Adds zstd support to the btrfs module.
> > - Adds a test case for btrfs zstd support.
> > - Changes top_srcdir to srcdir in the btrfs module's lzo include
> >   following comments from Daniel Kiper about the zstd include.
> >
> > Tested on Ubuntu-18.04 with a btrfs /boot partition with and without zstd
> > compression. A test case was also added to the test suite that fails before
> > the patch, and passes after.
> >
> > Signed-off-by: Nick Terrell 
>
> Reviewed-by: Daniel Kiper 
>
> If there are no objections I will apply this patch series in a week or so.

Errr... I have just realized that there are two many spaces just at the
beginning of most lines. There should be 2 instead of 4. Please take
a look at currently existing functions. Anyway, I can fix this before
committing. However, if you could repost whole patch series that would
be much easier for me.

Daniel

Re: [PATCH v6 2/2] btrfs: Add zstd support to grub btrfs

[Daniel Kiper]

On Thu, Nov 15, 2018 at 02:36:03PM -0800, Nick Terrell wrote:
> - Adds zstd support to the btrfs module.
> - Adds a test case for btrfs zstd support.
> - Changes top_srcdir to srcdir in the btrfs module's lzo include
>   following comments from Daniel Kiper about the zstd include.
>
> Tested on Ubuntu-18.04 with a btrfs /boot partition with and without zstd
> compression. A test case was also added to the test suite that fails before
> the patch, and passes after.
>
> Signed-off-by: Nick Terrell 

Reviewed-by: Daniel Kiper 

If there are no objections I will apply this patch series in a week or so.

Daniel

Re: [PATCH 1/9] btrfs: Add support for reading a filesystem with a RAID 5 or RAID 6 profile.

[Daniel Kiper]

On Tue, Oct 09, 2018 at 07:51:01PM +0200, Daniel Kiper wrote:
> On Thu, Sep 27, 2018 at 08:34:56PM +0200, Goffredo Baroncelli wrote:
> > From: Goffredo Baroncelli 
> >
> > Signed-off-by: Goffredo Baroncelli 
> 
> Code LGTM. Though comment begs improvement. I will send you updated
> comment for approval shortly.

Below you can find updated patch. Please check I have not messed up something.

Daniel

>From ecefb12a10d39bdd09e1d2b8fbbcbdb1b35274f8 Mon Sep 17 00:00:00 2001
From: Goffredo Baroncelli 
Date: Thu, 27 Sep 2018 20:34:56 +0200
Subject: [PATCH 1/1] btrfs: Add support for reading a filesystem with a RAID
 5 or RAID 6 profile.

Signed-off-by: Goffredo Baroncelli 
Signed-off-by: Daniel Kiper 
---
 grub-core/fs/btrfs.c |   73 ++
 1 file changed, 73 insertions(+)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index be19544..933a57d 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -119,6 +119,8 @@ struct grub_btrfs_chunk_item
 #define GRUB_BTRFS_CHUNK_TYPE_RAID1 0x10
 #define GRUB_BTRFS_CHUNK_TYPE_DUPLICATED0x20
 #define GRUB_BTRFS_CHUNK_TYPE_RAID100x40
+#define GRUB_BTRFS_CHUNK_TYPE_RAID5 0x80
+#define GRUB_BTRFS_CHUNK_TYPE_RAID6 0x100
   grub_uint8_t dummy2[0xc];
   grub_uint16_t nstripes;
   grub_uint16_t nsubstripes;
@@ -766,6 +768,77 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
grub_disk_addr_t addr,
  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;
+   }
+
+ /*
+  * RAID 6 layout consists of several stripes spread over
+  * the disks, e.g.:
+  *
+  *   Disk_0  Disk_1  Disk_2  Disk_3
+  * A0  B0  P0  Q0
+  * Q1  A1  B1  P1
+  * P2  Q2  A2  B2
+  *
+  * Note: placement of the parities depend on row number.
+  *
+  * Pay attention that the btrfs terminology may differ from
+  * terminology used in other RAID implementations, e.g. LVM,
+  * dm or md. The main difference is that btrfs calls contiguous
+  * block of data on a given disk, e.g. A0, stripe instead of 
chunk.
+  *
+  * The variables listed below have following meaning:
+  *   - stripe_nr is the stripe number excluding the parities
+  * (A0 = 0, B0 = 1, A1 = 2, B1 = 3, etc.),
+  *   - high is the row number (0 for A0...Q0, 1 for Q1...P1, 
etc.),
+  *   - stripen is the disk number in a row (0 for A0, Q1, P2,
+  * 1 for B0, A1, Q2, etc.),
+  *   - off is the logical address to read,
+  *   - chunk_stripe_length is the size of a stripe (typically 64 
KiB),
+  *   - nstripes is the number of disks in a row,
+  *   - low is the offset of the data inside a stripe,
+  *   - stripe_offset is the data offset in an array,
+  *   - csize is the "potential" data to read; it will be reduced
+  * to size if the latter is smaller,
+  *   - nparities is the number of parities (1 for RAID 5, 2 for
+  * RAID 6); used only in RAID 5/6 code.
+  */
+ stripe_nr = grub_divmod64 (off, chunk_stripe_length, );
+
+ /*
+  * stripen is computed without the parities
+  * (0 for A0, A1, A2, 1 for B0, B1, B2, etc.).
+  */
+ high = grub_divmod64 (stripe_nr, nstripes - nparities, );
+
+ /*
+  * The stripes are spread over the disks. Every each row their
+  * positions are shifted by 1 place. So, the real disks number
+  * change. Hence, we have to take current row number modulo
+  * nstripes into account (0 for A0, 1 for A1, 2 for A2, etc.).
+  */
+ grub_divmod64 (high + stripen, nstripes, );
+
+ stripe_offset = low + chunk_stripe_length * high;
+ csize = chunk_stripe_length - low;
+
+ break;
+   }
  default:
grub_dprintf ("btrfs", "unsupported RAID\n");
return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
-- 
1.7.10.4

Re: [PATCH 9/9] btrfs: Add RAID 6 recovery for a btrfs filesystem.

[Daniel Kiper]

On Wed, Sep 26, 2018 at 09:56:07PM +0200, Goffredo Baroncelli wrote:
> On 25/09/2018 21.20, Daniel Kiper wrote:
> > On Wed, Sep 19, 2018 at 08:40:40PM +0200, Goffredo Baroncelli wrote:
> >> From: Goffredo Baroncelli 
> >>
> []
> >>   *  - stripe_offset is the disk offset,
> >>   *  - csize is the "potential" data to read. It will be reduced to
> >>   *size if the latter is smaller.
> >> + *  - parities_pos is the position of the parity inside a row (
> >
> > s/inside/in/>
> >> + *2 for P1, 3 for P2...)
>
> +  *  - nparities is the number of parities (1 for RAID5, 2 for 
> RAID6);
> +  *used only in RAID5/6 code.
>
> >>   */
> >>  block_nr = grub_divmod64 (off, chunk_stripe_length, );
> >>
> >> @@ -1030,6 +1069,9 @@ grub_btrfs_read_logical (struct grub_btrfs_data 
> >> *data, grub_disk_addr_t addr,
> >>   */
> >>  grub_divmod64 (high + stripen, nstripes, );
> >>
> >> +grub_divmod64 (high + nstripes - nparities, nstripes,
> >> +   _pos);
> >
> > I think that this math requires a bit of explanation in the comment
> > before grub_divmod64(). Especially I am interested in why high +
> > nstripes - nparities works as expected.
>
>
> What about
>
> /*
>  * parities_pos is equal to "(high - nparities) % nstripes" (see the diagram 
> above).
>  * However "high - nparities" might be negative (eg when high == 0) leading 
> to an
>  * incorrect computation.
>  * Instead "high + nstripes - nparities" is always positive and in modulo 
> nstripes is
>  * equal to "(high - nparities) % nstripes
>  */

LGTM.

Daniel

Re: [PATCH 7/9] btrfs: Add support for recovery for a RAID 5 btrfs profiles.

[Daniel Kiper]

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 
> >>
> >> 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 
> >> ---
> >>  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 
> >>  #include 
> >>  #include 
> >> +#include 
> >>
> >>  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

Re: [PATCH 4/9] btrfs: Avoid a rescan for a device which was already not found.

[Daniel Kiper]

On Wed, Sep 26, 2018 at 09:55:54PM +0200, Goffredo Baroncelli wrote:
> On 25/09/2018 19.29, Daniel Kiper wrote:
> > On Wed, Sep 19, 2018 at 08:40:35PM +0200, Goffredo Baroncelli wrote:
> >> From: Goffredo Baroncelli 
> >>
> >> If a device is not found, do not return immediately but
> >> record this failure by storing NULL in data->devices_attached[].
> >
> > Still the same question: Where the store happens in the code?
> > I cannot find it in the patch below. This have to be clarified.
> >
> > Daniel
>
>
> What about the following commit description
> -
> Change the behavior of find_device(): before the patch, a read of a
> missed device might trigger a rescan. However, it is never recorded

s/might/may/

> that a device is missed, so each single read of a missed device might
> triggers a rescan.  It is the caller who decides if a rescan is
> performed in case of a missed device. And it does quite often, without
> considering if in the past a devices was already found as "missed"
> This behavior causes a lot of unneeded rescan, causing a huge slowdown
> in case of a missed device.
>
> After the patch, the "missed device" information is stored in the
> cache (as a NULL value). A rescan is triggered only if no information

What do you mean by "cache"? ctx.dev_found? If yes please use latter
instead of former. Or both together if it makes sense.

> at all is found in the cache. This means that only the first time a
> read of a missed device triggers a rescan.
>
> The change in the code is done removing "return NULL" when the disk is
> not found. So it is always executed the code which stores in the cache

cache?

> the value returned by grub_device_iterate(): NULL if the device is
> missed, or a valid data otherwise.
> -

Otherwise it is much better than earlier one.

Daniel

Re: [PATCH 1/9] btrfs: Add support for reading a filesystem with a RAID 5 or RAID 6 profile.

[Daniel Kiper]

On Wed, Sep 26, 2018 at 10:40:32PM +0200, Goffredo Baroncelli wrote:
> On 25/09/2018 17.31, Daniel Kiper wrote:
> > On Wed, Sep 19, 2018 at 08:40:32PM +0200, Goffredo Baroncelli wrote:
> >> From: Goffredo Baroncelli 
> >>
> >> Signed-off-by: Goffredo Baroncelli 
> >> ---
> >>  grub-core/fs/btrfs.c | 66 
> >>  1 file changed, 66 insertions(+)
> >>
> >> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> >> index be195448d..56c42746d 100644
> >> --- a/grub-core/fs/btrfs.c
> >> +++ b/grub-core/fs/btrfs.c
> >> @@ -119,6 +119,8 @@ struct grub_btrfs_chunk_item
> >>  #define GRUB_BTRFS_CHUNK_TYPE_RAID1 0x10
> >>  #define GRUB_BTRFS_CHUNK_TYPE_DUPLICATED0x20
> >>  #define GRUB_BTRFS_CHUNK_TYPE_RAID100x40
> >> +#define GRUB_BTRFS_CHUNK_TYPE_RAID5 0x80
> >> +#define GRUB_BTRFS_CHUNK_TYPE_RAID6 0x100
> >>grub_uint8_t dummy2[0xc];
> >>grub_uint16_t nstripes;
> >>grub_uint16_t nsubstripes;
> >> @@ -764,6 +766,70 @@ 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, block_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;
> >> +  }
> >> +
> >> +/*
> >> + * A RAID 6 layout consists of several blocks spread on the disks.
> >> + * The raid terminology is used to call all the blocks of a row
> >> + * "stripe". Unfortunately the BTRFS terminology confuses block
> >
> > Stripe is data set or parity (parity stripe) on one disk. Block has
> > different meaning. Please stick to btrfs terminology and say it clearly
> > in the comment. And even add a link to btrfs wiki page to ease reading.
> >
> > I think about this one:
> >   
> > https://btrfs.wiki.kernel.org/index.php/Manpage/mkfs.btrfs#BLOCK_GROUPS.2C_CHUNKS.2C_RAID
> >
> >> + * and stripe.
> >
> > I do not think so. Or at least not so much...
>
> Trust me, generally speaking stripe is the "row" in the disks (without the 
> parity); looking at the ext3 man page:
>
> 
>stride=stride-size
>   Configure  the  filesystem  for  a  RAID  array with
>   stride-size filesystem blocks. This is the number of
>   blocks  read or written to disk before moving to the
>   next disk, which is sometimes  referred  to  as  the
>   chunk   size.   This  mostly  affects  placement  of
>   filesystem metadata like bitmaps at mke2fs  time  to
>   avoid  placing them on a single disk, which can hurt
>   performance.  It may also be used by the block 
> allo???
>   cator.
>
>stripe_width=stripe-width
>   Configure  the  filesystem  for  a  RAID  array with
>   stripe-width filesystem blocks per stripe.  This  is
>   typically  stride-size * N, where N is the number of
>   data-bearing disks in the  RAID  (e.g.  for  RAID  5
>   there is one parity disk, so N will be the number of
>   disks in the array minus 1).  This allows the  block
>   allocator to prevent read-modify-write of the parity
>   in a RAID stripe if possible when the data is  
> writ???
>   ten.
>
> 
> Looking at the RAID5 wikipedia page, it seems that the term "stripe"
> is coherent with the ext3 man page.

Ugh... It looks that I have messe

Re: [PATCH 3/3] btrfs: Add zstd support to btrfs

[Daniel Kiper]

On Mon, Aug 27, 2018 at 06:36:54PM -0700, Nick Terrell wrote:
> Adds zstd support to the btrfs module. I'm not sure that my changes to the
> Makefiles are correct, please let me know if I need to do something
> differently.
>
> Tested on Ubuntu-18.04 with a btrfs /boot partition with and without zstd
> compression. A test case was also added to the test suite that fails before
> the patch, and passes after.
>
> Signed-off-by: Nick Terrell 
> ---
>  Makefile.util.def|  8 -
>  grub-core/Makefile.core.def  | 10 --
>  grub-core/fs/btrfs.c | 85 
> +++-
>  tests/btrfs_test.in  |  1 +
>  tests/util/grub-fs-tester.in |  4 +--
>  5 files changed, 102 insertions(+), 6 deletions(-)
>
> diff --git a/Makefile.util.def b/Makefile.util.def
> index 3180ac880..b987dc637 100644
> --- a/Makefile.util.def
> +++ b/Makefile.util.def
> @@ -54,7 +54,7 @@ library = {
>  library = {
>name = libgrubmods.a;
>cflags = '-fno-builtin -Wno-undef';
> -  cppflags = '-I$(top_srcdir)/grub-core/lib/minilzo 
> -I$(srcdir)/grub-core/lib/xzembed -DMINILZO_HAVE_CONFIG_H';
> +  cppflags = '-I$(top_srcdir)/grub-core/lib/minilzo 
> -I$(srcdir)/grub-core/lib/xzembed -I$(top_srcdir)/grub-core/lib/zstd 
> -DMINILZO_HAVE_CONFIG_H';
>
>common_nodist = grub_script.tab.c;
>common_nodist = grub_script.yy.c;
> @@ -165,6 +165,12 @@ library = {
>common = grub-core/lib/xzembed/xz_dec_bcj.c;
>common = grub-core/lib/xzembed/xz_dec_lzma2.c;
>common = grub-core/lib/xzembed/xz_dec_stream.c;
> +  common = grub-core/lib/zstd/zstd_common.c;
> +  common = grub-core/lib/zstd/huf_decompress.c;
> +  common = grub-core/lib/zstd/fse_decompress.c;
> +  common = grub-core/lib/zstd/entropy_common.c;
> +  common = grub-core/lib/zstd/decompress.c;
> +  common = grub-core/lib/zstd/xxhash.c;
>  };
>
>  program = {
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 9590e87d9..c24bf9147 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -404,7 +404,7 @@ image = {
>i386_pc = boot/i386/pc/boot.S;
>
>cppflags = '-DHYBRID_BOOT=1';
> -
> +

Please do not introduce such noise...

>i386_pc_ldflags = '$(TARGET_IMG_LDFLAGS)';
>i386_pc_ldflags = '$(TARGET_IMG_BASE_LDOPT),0x7C00';
>
> @@ -1264,8 +1264,14 @@ module = {
>name = btrfs;
>common = fs/btrfs.c;
>common = lib/crc.c;
> +  common = lib/zstd/zstd_common.c;
> +  common = lib/zstd/huf_decompress.c;
> +  common = lib/zstd/fse_decompress.c;
> +  common = lib/zstd/entropy_common.c;
> +  common = lib/zstd/decompress.c;
> +  common = lib/zstd/xxhash.c;
>cflags = '$(CFLAGS_POSIX) -Wno-undef';
> -  cppflags = '-I$(srcdir)/lib/posix_wrap -I$(srcdir)/lib/minilzo 
> -DMINILZO_HAVE_CONFIG_H';
> +  cppflags = '-I$(srcdir)/lib/posix_wrap -I$(srcdir)/lib/minilzo 
> -I$(srcdir)/lib/zstd -DMINILZO_HAVE_CONFIG_H';
>  };
>
>  module = {
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index be195448d..89ed4884e 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -27,6 +27,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>
> @@ -45,6 +46,9 @@ GRUB_MOD_LICENSE ("GPLv3+");
>  #define GRUB_BTRFS_LZO_BLOCK_MAX_CSIZE (GRUB_BTRFS_LZO_BLOCK_SIZE + \
>(GRUB_BTRFS_LZO_BLOCK_SIZE / 16) + 64 + 3)
>
> +#define ZSTD_BTRFS_MAX_WINDOWLOG 17
> +#define ZSTD_BTRFS_MAX_INPUT (1 << ZSTD_BTRFS_MAX_WINDOWLOG)

Please align stuff properly, i.e.

#define ZSTD_BTRFS_MAX_WINDOWLOG 17
#define ZSTD_BTRFS_MAX_INPUT (1 << ZSTD_BTRFS_MAX_WINDOWLOG)

> +
>  typedef grub_uint8_t grub_btrfs_checksum_t[0x20];
>  typedef grub_uint16_t grub_btrfs_uuid_t[8];
>
> @@ -212,6 +216,7 @@ struct grub_btrfs_extent_data
>  #define GRUB_BTRFS_COMPRESSION_NONE 0
>  #define GRUB_BTRFS_COMPRESSION_ZLIB 1
>  #define GRUB_BTRFS_COMPRESSION_LZO  2
> +#define GRUB_BTRFS_COMPRESSION_ZSTD  3

Ditto.

>  #define GRUB_BTRFS_OBJECT_ID_CHUNK 0x100
>
> @@ -912,6 +917,70 @@ grub_btrfs_read_inode (struct grub_btrfs_data *data,
>return grub_btrfs_read_logical (data, elemaddr, inode, sizeof (*inode), 0);
>  }
>
> +static grub_ssize_t
> +grub_btrfs_zstd_decompress(char *ibuf, grub_size_t isize, grub_off_t off,
> +   char *obuf, grub_size_t osize)
> +{
> + void *allocated = NULL;
> + char *otmpbuf = obuf;
> + grub_size_t otmpsize = osize;
> + void *wmem = NULL;
> + const grub_size_t wmem_size = ZSTD_DCtxWorkspaceBound ();
> + ZSTD_DCtx *dctx;
> + grub_size_t zstd_ret;
> + grub_ssize_t ret = -1;
> +
> + /* Zstd will fail if it can't fit the entire output in the destination
> +  * buffer, so if osize isn't large enough, allocate a temporary buffer.
> +  */
> + if (otmpsize < ZSTD_BTRFS_MAX_INPUT) {
> + allocated = grub_malloc (ZSTD_BTRFS_MAX_INPUT);
> + if (!allocated) {
> + grub_dprintf ("zstd", "outtmpbuf 

Re: [RFC] Add support for BTRFS raid5/6 to GRUB

[Daniel Kiper]

On Tue, Apr 17, 2018 at 09:57:40PM +0200, Goffredo Baroncelli wrote:
> Hi All,
>
> Below you can find a patch to add support for accessing files from
> grub in a RAID5/6 btrfs filesystem. This is a RFC because it is
> missing the support for recovery (i.e. if some devices are missed). In
> the next days (weeks ?) I will extend this patch to support also this
> case.
>
> Comments are welcome.

More or less LGTM. Just a nitpick below... I am happy to take full blown
patch into GRUB if it is ready.

> BR
> G.Baroncelli
>
>
> ---
>
> commit 8c80a1b7c913faf50f95c5c76b4666ed17685666
> Author: Goffredo Baroncelli 
> Date:   Tue Apr 17 21:40:31 2018 +0200
>
> Add initial support for btrfs raid5/6 chunk
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index be195448d..4c5632acb 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -119,6 +119,8 @@ struct grub_btrfs_chunk_item
>  #define GRUB_BTRFS_CHUNK_TYPE_RAID1 0x10
>  #define GRUB_BTRFS_CHUNK_TYPE_DUPLICATED0x20
>  #define GRUB_BTRFS_CHUNK_TYPE_RAID100x40
> +#define GRUB_BTRFS_CHUNK_TYPE_RAID5 0x80
> +#define GRUB_BTRFS_CHUNK_TYPE_RAID60x100
>grub_uint8_t dummy2[0xc];
>grub_uint16_t nstripes;
>grub_uint16_t nsubstripes;
> @@ -764,6 +766,39 @@ 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;
> +   grub_uint64_t parity_pos;
> +   grub_uint64_t stripe_nr, high;
> +   grub_uint64_t 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;
> + }
> +
> +   stripe_nr = grub_divmod64 (off, chunk_stripe_length, );
> +
> +   high = grub_divmod64 (stripe_nr, nstripes - nparities, );
> +   grub_divmod64 (high+nstripes-nparities, nstripes, _pos);
> +   grub_divmod64 (parity_pos+nparities+stripen, nstripes, );

Missing spaces around "+" and "-".

Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html