From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1fNywm-0008W9-7A for mharc-grub-devel@gnu.org; Wed, 30 May 2018 07:07:16 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37413) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fNywg-0008Sd-Ub for grub-devel@gnu.org; Wed, 30 May 2018 07:07:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fNywd-00020z-Ov for grub-devel@gnu.org; Wed, 30 May 2018 07:07:10 -0400 Received: from boksu.net-space.pl ([185.15.1.105]:42991) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:24) (Exim 4.71) (envelope-from ) id 1fNywd-00020I-DU for grub-devel@gnu.org; Wed, 30 May 2018 07:07:07 -0400 Received: (from localhost user: 'dkiper' uid#4000 fake: STDIN (dkiper@boksu.net-space.pl)) by router-fw-old.local.net-space.pl id S1841749AbeE3LHF (ORCPT ); Wed, 30 May 2018 13:07:05 +0200 Date: Wed, 30 May 2018 13:07:05 +0200 From: Daniel Kiper To: The development of GNU GRUB Cc: Goffredo Baroncelli Subject: Re: [PATCH 6/9] btrfs: refactor the code that read from disk Message-ID: <20180530110705.GF27874@router-fw-old.local.net-space.pl> References: <20180516184819.23736-1-kreijack@inwind.it> <20180516184819.23736-7-kreijack@inwind.it> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180516184819.23736-7-kreijack@inwind.it> User-Agent: Mutt/1.3.28i X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 185.15.1.105 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 30 May 2018 11:07:13 -0000 On Wed, May 16, 2018 at 08:48:16PM +0200, Goffredo Baroncelli wrote: > This is a preparatory patch, to help the adding of the RAID 5/6 recovery Please move "This is a preparatory patch" sentence below... > code. In case of availability of all disks, this code is good for all the > RAID profiles. However in case of failure, the error handling is quite > different. Refactoring this code increases the general readability. s/readability/readability too/? You can put "This is a preparatory patch" in separate line here. Same for the other patches. > Signed-off-by: Goffredo Baroncelli > --- > grub-core/fs/btrfs.c | 85 +++++++++++++++++++++++++------------------- > 1 file changed, 49 insertions(+), 36 deletions(-) > > diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c > index 51f300829..63651928b 100644 > --- a/grub-core/fs/btrfs.c > +++ b/grub-core/fs/btrfs.c > @@ -625,6 +625,47 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t id) > return ctx.dev_found; > } > > +static grub_err_t > +btrfs_read_from_chunk (struct grub_btrfs_data *data, > + struct grub_btrfs_chunk_item *chunk, > + grub_uint64_t stripen, grub_uint64_t stripe_offset, > + int redundancy, grub_uint64_t csize, > + void *buf) > +{ > + > + struct grub_btrfs_chunk_stripe *stripe; > + grub_disk_addr_t paddr; > + grub_device_t dev; > + grub_err_t err; > + > + stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); > + /* Right now the redundancy handling is easy. > + With RAID5-like it will be more difficult. */ > + stripe += stripen + redundancy; > + > + paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; > + > + grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T > + " maps to 0x%" PRIxGRUB_UINT64_T "\n", > + stripen, stripe->offset); > + grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T "\n", paddr); > + > + dev = find_device (data, stripe->device_id); > + if (!dev) > + { > + grub_dprintf ("btrfs", > + "couldn't find a necessary member device " > + "of multi-device filesystem\n"); > + grub_errno = GRUB_ERR_NONE; > + return GRUB_ERR_READ_ERROR; > + } > + > + err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, > + paddr & (GRUB_DISK_SECTOR_SIZE - 1), > + csize, buf); > + return err; > +} > + > static grub_err_t > grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, > void *buf, grub_size_t size, int recursion_depth) > @@ -638,7 +679,6 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, > grub_err_t err = 0; > struct grub_btrfs_key key_out; > int challoc = 0; > - grub_device_t dev; > struct grub_btrfs_key key_in; > grub_size_t chsize; > grub_disk_addr_t chaddr; > @@ -879,42 +919,15 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, grub_disk_addr_t addr, > > for (i = 0; i < redundancy; i++) > { > - struct grub_btrfs_chunk_stripe *stripe; > - grub_disk_addr_t paddr; > - > - stripe = (struct grub_btrfs_chunk_stripe *) (chunk + 1); > - /* Right now the redundancy handling is easy. > - With RAID5-like it will be more difficult. */ > - stripe += stripen + i; > - > - paddr = grub_le_to_cpu64 (stripe->offset) + stripe_offset; > - > - grub_dprintf ("btrfs", "stripe %" PRIxGRUB_UINT64_T > - " maps to 0x%" PRIxGRUB_UINT64_T "\n", > - stripen, stripe->offset); > - grub_dprintf ("btrfs", "reading paddr 0x%" PRIxGRUB_UINT64_T > - "\n", paddr); > - > - dev = find_device (data, stripe->device_id); > - if (!dev) > - { > - grub_dprintf ("btrfs", > - "couldn't find a necessary member device " > - "of multi-device filesystem\n"); > - err = grub_errno; > - grub_errno = GRUB_ERR_NONE; > - continue; > - } > - > - err = grub_disk_read (dev->disk, paddr >> GRUB_DISK_SECTOR_BITS, > - paddr & (GRUB_DISK_SECTOR_SIZE - 1), > - csize, buf); > - if (!err) > - break; > - grub_errno = GRUB_ERR_NONE; If you drop this line then you change behavior of this function. I have a feeling that this should stay as is. At least for now. If you need this change then probably it should be a part of the other patch. > + err = btrfs_read_from_chunk (data, chunk, stripen, > + stripe_offset, > + i, /* redundancy */ > + csize, buf); > + if (err == GRUB_ERR_NONE) > + break; > } > - if (i != redundancy) > - break; > + if (err == GRUB_ERR_NONE) > + break; Ditto. Daniel