mbox series

[U-BOOT,00/26] fs: btrfs: Re-implement btrfs support using the more widely used extent buffer base code

Message ID 20200422065009.69392-1-wqu@suse.com (mailing list archive)
Headers show
Series fs: btrfs: Re-implement btrfs support using the more widely used extent buffer base code | expand

Message

Qu Wenruo April 22, 2020, 6:49 a.m. UTC
The branch can be fetched from github:
https://github.com/adam900710/u-boot/tree/btrfs_rebuild

The current btrfs code in U-boot is using a creative way to read on-disk
data.
It's pretty simple, involving the least amount of code, but pretty
different from btrfs-progs nor kernel, making it pretty hard to sync
code between different projects.

This big patchset will rework the btrfs support, to use code mostly from
btrfs-progs, thus all existing btrfs developers will feel at home.

During the rework, the following new features are added:
- More hash algorithm support
  SHA256 and XXHASH support are added.
  BLAKE2 needs more backport, will happen in a separate patchset.

- The ability to read degraded RAID1
  Although we still only support one device btrfs, the new code base
  can choose from different copies already.
  As long as new device scan interface is provided, multi-device support
  is pretty simple.

- More correct handling of compressed extents with offset
  For compressed extent with offset, we should read the whole compressed
  extent, decompress them, then copy the referred part.

There are some more features incoming, with the new code base it would
be much easier to implement:
- Data checksum support
  The check would happen in read_extent_data(), btrfs-progs has the
  needed facility to locate data csum.

- BLAKE2 support
  Need BLAKE2 library cross ported.
  For btrfs it's just several lines of modification.

- Multi-device support along wit degraded RAID support
  We only need an interface to scan one device without opening it.
  The infrastructure is already provided in this patchset.

These new features would be submitted after the patchset get merged,
since the patchset is already large, I don't want to make it more scary.

Although this patchset look horribly large, most of them are code copy
from btrfs-progs.
E.g extent-cache.[ch], rbtree-utils.[ch], btrfs_btree.h.
And ctree.h has over 1000 lines copied just for various accessors.

While for disk-io.[ch] and volumes-io.[ch], they have some small
modifications to adapt the interface of U-boot.
E.g. btrfs_device::fd is replace with blkdev_desc and disk_partition_t.

The new code for U-boot are related to the following functions:
- btrfs_readlink()
- btrfs_lookup_path()
- btrfs_read_extent_inline()
- btrfs_read_extent_reg()
- lookup_data_extent()
- btrfs_file_read()

Thus only the following 5 patches need extra review attention:
- Patch 0017
- Patch 0018
- Patch 0022
- Patch 0023
- Patch 0024

Qu Wenruo (26):
  fs: btrfs: Sync btrfs_btree.h from kernel
  fs: btrfs: Add More checksum algorithm support to btrfs
  fs: btrfs: Cross-port btrfs_read_dev_super() from btrfs-progs
  fs: btrfs: Cross-port rbtree-utils from btrfs-progs
  fs: btrfs: Cross-port extent-cache.[ch] from btrfs-progs
  fs: btrfs: Cross-port extent-io.[ch] from btrfs-progs
  fs: btrfs: Cross port structure accessor into ctree.h
  fs: btrfs: Cross port volumes.[ch] from btrfs-progs
  fs: btrfs: Crossport read_tree_block() from btrfs-progs
  fs: btrfs: Rename struct btrfs_path to struct __btrfs_path
  fs: btrfs: Rename btrfs_root to __btrfs_root
  fs: btrfs: Cross port struct btrfs_root to ctree.h
  fs: btrfs: Crossport btrfs_search_slot() from btrfs-progs
  fs: btrfs: Crossport btrfs_read_sys_array() and
    btrfs_read_chunk_tree()
  fs: btrfs: Crossport open_ctree_fs_info()
  fs: btrfs: Rename path resolve related functions to avoid name
    conflicts
  fs: btrfs: Use btrfs_readlink() to implement __btrfs_readlink()
  fs: btrfs: Implement btrfs_lookup_path()
  fs: btrfs: Use btrfs_iter_dir() to replace btrfs_readdir()
  fs: btrfs: Use btrfs_lookup_path() to implement btrfs_exists() and
    btrfs_size()
  fs: btrfs: Rename btrfs_file_read() and its callees to avoid name
    conflicts
  fs: btrfs: Introduce btrfs_read_extent_inline() and
    btrfs_read_extent_reg()
  fs: btrfs: Introduce lookup_data_extent() for later use
  fs: btrfs: Implement btrfs_file_read()
  fs: btrfs: Cleanup the old implementation
  MAINTAINERS: Add btrfs mail list

 MAINTAINERS                         |    1 +
 fs/btrfs/Makefile                   |    5 +-
 fs/btrfs/btrfs.c                    |  311 +++---
 fs/btrfs/btrfs.h                    |   67 +-
 fs/btrfs/btrfs_tree.h               |  766 --------------
 fs/btrfs/chunk-map.c                |  177 ----
 fs/btrfs/common/rbtree-utils.c      |   83 ++
 fs/btrfs/common/rbtree-utils.h      |   53 +
 fs/btrfs/compat.h                   |   80 ++
 fs/btrfs/compression.c              |    2 +-
 fs/btrfs/crypto/hash.c              |   55 +
 fs/btrfs/crypto/hash.h              |   17 +
 fs/btrfs/ctree.c                    |  866 ++++++++++++----
 fs/btrfs/ctree.h                    | 1452 ++++++++++++++++++++++-----
 fs/btrfs/dir-item.c                 |  192 ++--
 fs/btrfs/disk-io.c                  | 1062 ++++++++++++++++++++
 fs/btrfs/disk-io.h                  |   50 +
 fs/btrfs/extent-cache.c             |  318 ++++++
 fs/btrfs/extent-cache.h             |  104 ++
 fs/btrfs/extent-io.c                |  845 ++++++++++++++--
 fs/btrfs/extent-io.h                |  164 +++
 fs/btrfs/hash.c                     |   38 -
 fs/btrfs/inode.c                    |  879 +++++++++++-----
 fs/btrfs/kernel-shared/btrfs_tree.h | 1333 ++++++++++++++++++++++++
 fs/btrfs/root-tree.c                |   47 +
 fs/btrfs/root.c                     |   92 --
 fs/btrfs/subvolume.c                |  130 ---
 fs/btrfs/super.c                    |  246 -----
 fs/btrfs/volumes.c                  | 1173 ++++++++++++++++++++++
 fs/btrfs/volumes.h                  |  204 ++++
 30 files changed, 8308 insertions(+), 2504 deletions(-)
 delete mode 100644 fs/btrfs/btrfs_tree.h
 delete mode 100644 fs/btrfs/chunk-map.c
 create mode 100644 fs/btrfs/common/rbtree-utils.c
 create mode 100644 fs/btrfs/common/rbtree-utils.h
 create mode 100644 fs/btrfs/compat.h
 create mode 100644 fs/btrfs/crypto/hash.c
 create mode 100644 fs/btrfs/crypto/hash.h
 create mode 100644 fs/btrfs/disk-io.c
 create mode 100644 fs/btrfs/disk-io.h
 create mode 100644 fs/btrfs/extent-cache.c
 create mode 100644 fs/btrfs/extent-cache.h
 create mode 100644 fs/btrfs/extent-io.h
 delete mode 100644 fs/btrfs/hash.c
 create mode 100644 fs/btrfs/kernel-shared/btrfs_tree.h
 create mode 100644 fs/btrfs/root-tree.c
 delete mode 100644 fs/btrfs/root.c
 delete mode 100644 fs/btrfs/subvolume.c
 delete mode 100644 fs/btrfs/super.c
 create mode 100644 fs/btrfs/volumes.c
 create mode 100644 fs/btrfs/volumes.h

Comments

Marek Behún April 22, 2020, 7:46 a.m. UTC | #1
On Wed, 22 Apr 2020 14:49:43 +0800
Qu Wenruo <wqu@suse.com> wrote:

Hi Qu,

> The current btrfs code in U-boot is using a creative way to read on-disk
> data.
> It's pretty simple, involving the least amount of code, but pretty
> different from btrfs-progs nor kernel, making it pretty hard to sync
> code between different projects.

do you think maybe btrfs-progs / kernel would be interested if I
tried to convert their code to this "simpler to use" implementation of
conversion functions?

> Thus only the following 5 patches need extra review attention:
> - Patch 0017
> - Patch 0018
> - Patch 0022
> - Patch 0023
> - Patch 0024

Anyway, this patch series does not apply cleanly on u-boot/master. I
tried with the first 3 patches and then gave up :(
Sorry about this but I will review and test if you send a series that
applies cleanly.

Marek
Marek Behún April 22, 2020, 7:52 a.m. UTC | #2
I am testing compilation from the github repository.

Another problem is that in the cleanup patch you have removed
btrfs_list_subvols functoin, which is used by cmd/btrfs.c to list
subvolumes. So Turris Mox and Turris Omnia with their default defconfig
do not compile now.

Marek
Qu Wenruo April 22, 2020, 7:52 a.m. UTC | #3
On 2020/4/22 下午3:46, Marek Behun wrote:
> On Wed, 22 Apr 2020 14:49:43 +0800
> Qu Wenruo <wqu@suse.com> wrote:
> 
> Hi Qu,
> 
>> The current btrfs code in U-boot is using a creative way to read on-disk
>> data.
>> It's pretty simple, involving the least amount of code, but pretty
>> different from btrfs-progs nor kernel, making it pretty hard to sync
>> code between different projects.
> 
> do you think maybe btrfs-progs / kernel would be interested if I
> tried to convert their code to this "simpler to use" implementation of
> conversion functions?

I don't think so, the problem is kernel and btrfs-progs all need to
modify extent buffer, which make the read time conversion become a
burden in write path.

> 
>> Thus only the following 5 patches need extra review attention:
>> - Patch 0017
>> - Patch 0018
>> - Patch 0022
>> - Patch 0023
>> - Patch 0024
> 
> Anyway, this patch series does not apply cleanly on u-boot/master. I
> tried with the first 3 patches and then gave up :(
> Sorry about this but I will review and test if you send a series that
> applies cleanly.

That's strange, the branch is originally based on an older master, as
you can find in the github.

I guess there are some more btrfs related code change since then, I'll
rebase them to latest master, and update the github repo then.

Thanks,
Qu

> 
> Marek
>
Qu Wenruo April 22, 2020, 7:56 a.m. UTC | #4
On 2020/4/22 下午3:52, Qu Wenruo wrote:
> 
> 
> On 2020/4/22 下午3:46, Marek Behun wrote:
>> On Wed, 22 Apr 2020 14:49:43 +0800
>> Qu Wenruo <wqu@suse.com> wrote:
>>
>> Hi Qu,
>>
>>> The current btrfs code in U-boot is using a creative way to read on-disk
>>> data.
>>> It's pretty simple, involving the least amount of code, but pretty
>>> different from btrfs-progs nor kernel, making it pretty hard to sync
>>> code between different projects.
>>
>> do you think maybe btrfs-progs / kernel would be interested if I
>> tried to convert their code to this "simpler to use" implementation of
>> conversion functions?
> 
> I don't think so, the problem is kernel and btrfs-progs all need to
> modify extent buffer, which make the read time conversion become a
> burden in write path.
> 
>>
>>> Thus only the following 5 patches need extra review attention:
>>> - Patch 0017
>>> - Patch 0018
>>> - Patch 0022
>>> - Patch 0023
>>> - Patch 0024
>>
>> Anyway, this patch series does not apply cleanly on u-boot/master. I
>> tried with the first 3 patches and then gave up :(
>> Sorry about this but I will review and test if you send a series that
>> applies cleanly.
> 
> That's strange, the branch is originally based on an older master, as
> you can find in the github.
> 
> I guess there are some more btrfs related code change since then, I'll
> rebase them to latest master, and update the github repo then.

Git repo updated.
You can fetch that branch.

Only two small conflicts during rebase, and since it compiles I haven't
re-done the full test, but it looks pretty OK.

Thanks,
Qu

> 
> Thanks,
> Qu
> 
>>
>> Marek
>>
>
Marek Behún April 22, 2020, 7:59 a.m. UTC | #5
Also there are some warnings when compiling for Mox and Omnia:




fs/btrfs/inode.c: In function ‘btrfs_lookup_path’:
fs/btrfs/inode.c:141:16: warning: ‘parent_root’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  141 |   key.objectid = parent_root;
      |   ~~~~~~~~~~~~~^~~~~~~~~~~~~
fs/btrfs/inode.c:127:7: note: ‘parent_root’ was declared here
  127 |   u64 parent_root;
      |       ^~~~~~~~~~~
  CC      cmd/nvedit.o
In file included from fs/btrfs/ctree.h:19,
                 from fs/btrfs/disk-io.h:7,
                 from fs/btrfs/disk-io.c:8:
fs/btrfs/disk-io.c: In function ‘btrfs_scan_fs_devices’:
fs/btrfs/disk-io.c:881:9: warning: format ‘%llu’ expects argument of type ‘long long unsigned int’, but argument 3 has type ‘lbaint_t’ {aka ‘long unsigned int’} [-Wformat=]
  881 |   error("superblock end %u is larger than device size %llu",
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  882 |     BTRFS_SUPER_INFO_SIZE + BTRFS_SUPER_INFO_OFFSET,
  883 |     part->size << desc->log2blksz);
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                |
      |                lbaint_t {aka long unsigned int}
fs/btrfs/compat.h:13:29: note: in definition of macro ‘error’
   13 | #define error(...) { printf(__VA_ARGS__); printf("\n"); }
      |                             ^~~~~~~~~~~
fs/btrfs/disk-io.c:881:58: note: format string is defined here
  881 |   error("superblock end %u is larger than device size %llu",
      |                                                       ~~~^
      |                                                          |
      |                                                          long long unsigned int
      |                                                       %lu






In file included from fs/btrfs/ctree.h:19,
                 from fs/btrfs/volumes.c:5:
fs/btrfs/volumes.c: In function ‘btrfs_check_chunk_valid’:
fs/btrfs/volumes.c:404:9: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘u32’ {aka ‘unsigned int’} [-Wformat=]
  404 |   error("invalid chunk item size, have %u expect [%zu, %lu)",
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
fs/btrfs/compat.h:13:29: note: in definition of macro ‘error’
   13 | #define error(...) { printf(__VA_ARGS__); printf("\n"); }
      |                             ^~~~~~~~~~~
fs/btrfs/volumes.c:404:58: note: format string is defined here
  404 |   error("invalid chunk item size, have %u expect [%zu, %lu)",
      |                                                        ~~^
      |                                                          |
      |                                                          long unsigned int
      |                                                        %u
Qu Wenruo April 22, 2020, 8:12 a.m. UTC | #6
On 2020/4/22 下午3:59, Marek Behun wrote:
> Also there are some warnings when compiling for Mox and Omnia:
> 
> 
> 
> 
> fs/btrfs/inode.c: In function ‘btrfs_lookup_path’:
> fs/btrfs/inode.c:141:16: warning: ‘parent_root’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   141 |   key.objectid = parent_root;
>       |   ~~~~~~~~~~~~~^~~~~~~~~~~~~
> fs/btrfs/inode.c:127:7: note: ‘parent_root’ was declared here
>   127 |   u64 parent_root;
>       |       ^~~~~~~~~~~
>   CC      cmd/nvedit.o
> In file included from fs/btrfs/ctree.h:19,
>                  from fs/btrfs/disk-io.h:7,
>                  from fs/btrfs/disk-io.c:8:
> fs/btrfs/disk-io.c: In function ‘btrfs_scan_fs_devices’:
> fs/btrfs/disk-io.c:881:9: warning: format ‘%llu’ expects argument of type ‘long long unsigned int’, but argument 3 has type ‘lbaint_t’ {aka ‘long unsigned int’} [-Wformat=]
>   881 |   error("superblock end %u is larger than device size %llu",
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   882 |     BTRFS_SUPER_INFO_SIZE + BTRFS_SUPER_INFO_OFFSET,
>   883 |     part->size << desc->log2blksz);
>       |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>       |                |
>       |                lbaint_t {aka long unsigned int}
> fs/btrfs/compat.h:13:29: note: in definition of macro ‘error’
>    13 | #define error(...) { printf(__VA_ARGS__); printf("\n"); }
>       |                             ^~~~~~~~~~~
> fs/btrfs/disk-io.c:881:58: note: format string is defined here
>   881 |   error("superblock end %u is larger than device size %llu",
>       |                                                       ~~~^
>       |                                                          |
>       |                                                          long long unsigned int
>       |                                                       %lu
> 
> 
> 
> 
> 
> 
> In file included from fs/btrfs/ctree.h:19,
>                  from fs/btrfs/volumes.c:5:
> fs/btrfs/volumes.c: In function ‘btrfs_check_chunk_valid’:
> fs/btrfs/volumes.c:404:9: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘u32’ {aka ‘unsigned int’} [-Wformat=]
>   404 |   error("invalid chunk item size, have %u expect [%zu, %lu)",
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> fs/btrfs/compat.h:13:29: note: in definition of macro ‘error’
>    13 | #define error(...) { printf(__VA_ARGS__); printf("\n"); }
>       |                             ^~~~~~~~~~~
> fs/btrfs/volumes.c:404:58: note: format string is defined here
>   404 |   error("invalid chunk item size, have %u expect [%zu, %lu)",
>       |                                                        ~~^
>       |                                                          |
>       |                                                          long unsigned int
>       |                                                        %u
> 
I also hit those with GCC 9.2, but in GCC 9.3 they just disappear, so
I'm not sure whether they are bugs from GCC or something else...

Thanks,
Qu
Marek Behún April 22, 2020, 8:13 a.m. UTC | #7
Just for informational purposes for others:

I tested compiling with and without this patches to see how much it
increases binary size.

		Omnia (armv7a)		Mox (aarch64)
with old btrfs	52900 (51.7 KB)		55456 (54.2 KB)
with new btrfs	76176 (74.4 KB)		75480 (73.7 KB)
diff		23276 (22.7 KB)		20024 (19.5 KB)
increase driver	   44%			   36%
increase u-boot	    3.46%		    2.81%

(I don't actually care much, this is not too much, this is something
that can maybe be reduced by using some optimizations which u-boot does
not support yet and also there should be enough storage for u-boot on
Mox and Omnia. But maybe someone else will care enough.)
Marek Behún April 22, 2020, 9:33 a.m. UTC | #8
On Wed, 22 Apr 2020 15:52:25 +0800
Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:

> > do you think maybe btrfs-progs / kernel would be interested if I
> > tried to convert their code to this "simpler to use" implementation of
> > conversion functions?  
> 
> I don't think so, the problem is kernel and btrfs-progs all need to
> modify extent buffer, which make the read time conversion become a
> burden in write path.

:(