From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1fOp8L-00031b-CB for mharc-grub-devel@gnu.org; Fri, 01 Jun 2018 14:50:41 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37466) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fOp8F-0002yL-HZ for grub-devel@gnu.org; Fri, 01 Jun 2018 14:50:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fOp8C-000307-28 for grub-devel@gnu.org; Fri, 01 Jun 2018 14:50:35 -0400 Received: from smtp-17-i6.italiaonline.it ([213.209.14.17]:46053 helo=libero.it) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fOp8B-0002z5-OZ for grub-devel@gnu.org; Fri, 01 Jun 2018 14:50:32 -0400 Received: from venice.bhome ([94.38.2.87]) by smtp-17.iol.local with SMTP id Op89f7hLEb701Op8AfHY1X; Fri, 01 Jun 2018 20:50:30 +0200 x-libjamoibt: 1601 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=inwind.it; s=s2014; t=1527879030; bh=VNXvQVKLcEZIyRH1OoqoCwgbGwi5eO4HoHilx9aR6Hw=; h=From:Subject:Reply-To:To:References:Date:In-Reply-To; b=ZN01KVEn3mU7mQvfL7M51RrnzhPiqM6HAEpxZxKGw+EWogMZL8iiYFpQ5ECtFyEk2 0cTgL6yeFKnRZrN+hNA8QdePGHSmAsOMda68X4Eu8NmEpnTDA2gTIq2ouj3Zd7NFwo keK6D9sChhrOwUl05kVw06bKq63W4ulR8DtHRNKS0Zdeb1KWTUmVmT4hE6JTAfP/9K bCHzz9SG3hbfWRZbs9cZtU4fpJeCHvxLPFsvMaWZP6tdTujdltkeq+bJ5R5f7gyJKI OgC8ADZhOhJIDNKAv6DK24B2xryIQdUgR/cERAI4iwNzknAhFu+oVoyQ/5ZUYOaT+W 5F7lVETIMjN2g== X-CNFS-Analysis: v=2.2 cv=K+tSJ2eI c=1 sm=1 tr=0 a=zXjt9gNCPfuEQr1Uad4NEg==:117 a=zXjt9gNCPfuEQr1Uad4NEg==:17 a=IkcTkHD0fZMA:10 a=wBLhZXOqOWTpmkMa_5wA:9 a=QEXdDO2ut3YA:10 From: Goffredo Baroncelli Subject: Re: [PATCH 6/9] btrfs: refactor the code that read from disk Reply-To: kreijack@inwind.it To: Daniel Kiper , The development of GNU GRUB References: <20180516184819.23736-1-kreijack@inwind.it> <20180516184819.23736-7-kreijack@inwind.it> <20180530110705.GF27874@router-fw-old.local.net-space.pl> Message-ID: <50bf26de-f4b8-5bef-393c-daafda880676@inwind.it> Date: Fri, 1 Jun 2018 20:50:29 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <20180530110705.GF27874@router-fw-old.local.net-space.pl> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-CMAE-Envelope: MS4wfNPIIrdyThB9+4U3NBafNCo3y5YiioGNxfJSFL7PdvufLubPKT8LE8sHSZRTdiosPUZncr1m/leQAvUcHTR6v6c5dkku4SKow22IIAWyBbCG6J7O2Ime yRU53HLUF6APEYSePxsXR9x2iKdtZd1q5JjI5Y6YnKdBbx6I9V/agiUX8fXMyYZC7W+A1PDTjOgupg== X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [fuzzy] X-Received-From: 213.209.14.17 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: Fri, 01 Jun 2018 18:50:37 -0000 On 05/30/2018 01:07 PM, Daniel Kiper wrote: > 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. 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. ---- > >> 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. OK > >> + 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 > > Ditto. > > Daniel > -- gpg @keyserver.linux.it: Goffredo Baroncelli Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5