[PATCH 0/2] btrfs-progs: mkfs: --rootdir related fixes

[PATCH 0/2] btrfs-progs: mkfs: --rootdir related fixes

[Qu Wenruo]

I don't know if it's recent kernel tmpfs change or something else, but
I'm consistently get ino number smaller than 256 from my /tmp directory.

This behavior change exposed a new problem in mkfs.btrfs --rootdir, that
if some ino number (in the source directory, not in btrfs) is smaller
than 256, it can screw up the backref code.

As backref code is utilizing @owner to determine if a backref is data or
metadata.

And inode number smaller than 256 will make backref code to treat a data
backref as tree backref, and cause corruption.

Thankfully this should not happen that easily, only when --rootdir
points to a newly created fs.

Qu Wenruo (2):
  btrfs-progs: properly initialized extent generation for
    __btrfs_record_file_extent()
  btrfs-progs: avoid fs corruption if rootdir contains ino smaller than
    BTRFS_FIRST_FREE_OBJECTID

 kernel-shared/extent-tree.c | 8 +++++++-
 mkfs/rootdir.c              | 8 +++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

-- 
2.37.3

[PATCH 1/2] btrfs-progs: properly initialized extent generation for __btrfs_record_file_extent()

[Qu Wenruo]

[BUG]
When using mkfs.btrfs --rootdir option, the data extents generated will
has 0 as their generation in extent tree:

  # mkdir /tmp/rootdir
  # xfs_io -f -c "pwrite 0 16k" /tmp/rootdir
  # mkfs.btrfs -f --rootdir /tmp/rootdir $dev
  # btrfs ins dump-tree -t extent $dev
  btrfs-progs v5.19.1
  extent tree key (EXTENT_TREE ROOT_ITEM 0)
  leaf 30474240 items 13 free space 15536 generation 7 owner EXTENT_TREE
  leaf 30474240 flags 0x1(WRITTEN) backref revision 1
  fs uuid c1f05988-49f9-4dd4-8489-b90d60f522ee
  chunk uuid 40f81603-fe75-4f58-aa9e-e74e28df8523
	item 0 key (13631488 EXTENT_ITEM 16384) itemoff 16230 itemsize 53
		refs 1 gen 0 flags DATA <<< Generation is 0
  ...

[CAUSE]
In __btrfs_record_file_extent() we just set the extent generation to 0.

[FIX]
Use trans->transid to properly fill extent generation.

Now after mkfs, the first data extent backref looks like this:

	item 0 key (13631488 EXTENT_ITEM 16384) itemoff 16230 itemsize 53
		refs 1 gen 7 flags DATA
        ...

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 kernel-shared/extent-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel-shared/extent-tree.c b/kernel-shared/extent-tree.c
index 3a058a8698ee..92d5c521abe8 100644
--- a/kernel-shared/extent-tree.c
+++ b/kernel-shared/extent-tree.c
@@ -3582,7 +3582,7 @@ static int __btrfs_record_file_extent(struct btrfs_trans_handle *trans,
 					    struct btrfs_extent_item);
 
 			btrfs_set_extent_refs(leaf, ei, 0);
-			btrfs_set_extent_generation(leaf, ei, 0);
+			btrfs_set_extent_generation(leaf, ei, trans->transid);
 			btrfs_set_extent_flags(leaf, ei,
 					       BTRFS_EXTENT_FLAG_DATA);
 			btrfs_mark_buffer_dirty(leaf);
-- 
2.37.3

Re: [PATCH 1/2] btrfs-progs: properly initialized extent generation for __btrfs_record_file_extent()

[Anand Jain]

On 04/10/2022 15:43, Qu Wenruo wrote:
> [BUG]
> When using mkfs.btrfs --rootdir option, the data extents generated will
> has 0 as their generation in extent tree:
> 
>    # mkdir /tmp/rootdir
>    # xfs_io -f -c "pwrite 0 16k" /tmp/rootdir

This should be:

  # xfs_io -f -c "pwrite 0 16k" /tmp/rootdir/foobar


>    # mkfs.btrfs -f --rootdir /tmp/rootdir $dev
>    # btrfs ins dump-tree -t extent $dev
>    btrfs-progs v5.19.1
>    extent tree key (EXTENT_TREE ROOT_ITEM 0)
>    leaf 30474240 items 13 free space 15536 generation 7 owner EXTENT_TREE
>    leaf 30474240 flags 0x1(WRITTEN) backref revision 1
>    fs uuid c1f05988-49f9-4dd4-8489-b90d60f522ee
>    chunk uuid 40f81603-fe75-4f58-aa9e-e74e28df8523
> 	item 0 key (13631488 EXTENT_ITEM 16384) itemoff 16230 itemsize 53
> 		refs 1 gen 0 flags DATA <<< Generation is 0
>    ...
> 
> [CAUSE]
> In __btrfs_record_file_extent() we just set the extent generation to 0.
> 
> [FIX]
> Use trans->transid to properly fill extent generation.
> 
> Now after mkfs, the first data extent backref looks like this:
> 
> 	item 0 key (13631488 EXTENT_ITEM 16384) itemoff 16230 itemsize 53
> 		refs 1 gen 7 flags DATA
>          ...
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>


> ---
>   kernel-shared/extent-tree.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel-shared/extent-tree.c b/kernel-shared/extent-tree.c
> index 3a058a8698ee..92d5c521abe8 100644
> --- a/kernel-shared/extent-tree.c
> +++ b/kernel-shared/extent-tree.c
> @@ -3582,7 +3582,7 @@ static int __btrfs_record_file_extent(struct btrfs_trans_handle *trans,
>   					    struct btrfs_extent_item);
>   
>   			btrfs_set_extent_refs(leaf, ei, 0);
> -			btrfs_set_extent_generation(leaf, ei, 0);
> +			btrfs_set_extent_generation(leaf, ei, trans->transid);
>   			btrfs_set_extent_flags(leaf, ei,
>   					       BTRFS_EXTENT_FLAG_DATA);
>   			btrfs_mark_buffer_dirty(leaf);

Re: [PATCH 1/2] btrfs-progs: properly initialized extent generation for __btrfs_record_file_extent()

[David Sterba]

On Wed, Oct 05, 2022 at 05:39:24PM +0800, Anand Jain wrote:
> On 04/10/2022 15:43, Qu Wenruo wrote:
> > [BUG]
> > When using mkfs.btrfs --rootdir option, the data extents generated will
> > has 0 as their generation in extent tree:
> > 
> >    # mkdir /tmp/rootdir
> >    # xfs_io -f -c "pwrite 0 16k" /tmp/rootdir
> 
> This should be:
> 
>   # xfs_io -f -c "pwrite 0 16k" /tmp/rootdir/foobar

Updated, thanks.

[PATCH 2/2] btrfs-progs: avoid fs corruption if rootdir contains ino smaller than BTRFS_FIRST_FREE_OBJECTID

[Qu Wenruo]

[BUG]
When running mkfs tests on a newly rebooted minimal system, it can cause
mkfs/009 to fail.

The reproduce steps requires /tmp to has minimal files in the first
place.

  # mkdir /tmp/rootdir
  # xfs_io -f -c "pwrite 0 16k" /tmp/rootdir
  # mkfs.btrfs --rootdir /tmp/rootdir -f $dev
  # btrfs check $dev
  Opening filesystem to check...
  Checking filesystem on /dev/test/scratch1
  UUID: 6821b3db-f056-4c18-b797-32679dcd4272
  [1/7] checking root items
  [2/7] checking extents
  data backref 13631488 root 5 owner 170 offset 0 num_refs 0 not found in extent tree
  incorrect local backref count on 13631488 root 5 owner 170 offset 0 found 1 wanted 0 back 0x55ff6cd72260
  backref 13631488 root 5 not referenced back 0x55ff6cd4c1f0
  incorrect global backref count on 13631488 found 2 wanted 1
  backpointer mismatch on [13631488 16384]
  ERROR: errors found in extent allocation tree or chunk allocation

[CAUSE]
The extent tree has the following weird item:

	item 0 key (13631488 EXTENT_ITEM 16384) itemoff 16250 itemsize 33
		refs 1 gen 0 flags DATA
		tree block backref root FS_TREE

This is an extent item for data, thus it should not have an inline tree
backref.

Then checking the fs tree:

	item 0 key (170 INODE_ITEM 0) itemoff 16123 itemsize 160
		generation 7 transid 0 size 16384 nbytes 16384
		block group 0 mode 100600 links 1 uid 1000 gid 1000 rdev 0
		sequence 0 flags 0x0(none)
		atime 1664866393.0 (2022-10-04 14:53:13)
		ctime 1664863510.0 (2022-10-04 14:05:10)
		mtime 1664863455.0 (2022-10-04 14:04:15)
		otime 0.0 (1970-01-01 08:00:00)

There is an inode item before the root dir inode.

And that inode number 170 is causing the problem.

In traverse_directory(), we use the inode number reported from stat()
directly as btrfs inode number, and pass it to
btrfs_record_file_extent(), which finally calls btrfs_inc_extent_ref(),
with above 170 passed as @owner parameter.

But inside btrfs_inc_extent_ref() we use that @owner value to determine
if it's a data backref.
Since we got a smaller than BTRFS_FIRST_FREE_OBJECTID, btrfs treats it
as tree block, and cause the above problem.

[FIX]
As a quick fix, always add BTRFS_FIRST_FREE_OBJECTID to all inode number
directly grabbed from stat().

And add an ASSERT() in __btrfs_record_file_extent() to catch unexpected
objectid.

This is not a perfect solution, as the resulted fs will has a huge grap
in its inodes:

	item 0 key (256 INODE_ITEM 0) itemoff 16123 itemsize 160
	item 4 key (426 INODE_ITEM 0) itemoff 15883 itemsize 160

For a proper fix, we should allocate new btrfs inode numbers in a
sequential order, but that would be another series of patches.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 kernel-shared/extent-tree.c | 6 ++++++
 mkfs/rootdir.c              | 8 +++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/kernel-shared/extent-tree.c b/kernel-shared/extent-tree.c
index 92d5c521abe8..670eb66b9929 100644
--- a/kernel-shared/extent-tree.c
+++ b/kernel-shared/extent-tree.c
@@ -3529,6 +3529,12 @@ static int __btrfs_record_file_extent(struct btrfs_trans_handle *trans,
 	u64 extent_offset;
 	u64 num_bytes = *ret_num_bytes;
 
+	/*
+	 * @objectid should be an inode number, thus it should not be smaller
+	 * than BTRFS_FIRST_FREE_OBJECTID.
+	 */
+	ASSERT(objectid >= BTRFS_FIRST_FREE_OBJECTID);
+
 	/*
 	 * All supported file system should not use its 0 extent.
 	 * As it's for hole
diff --git a/mkfs/rootdir.c b/mkfs/rootdir.c
index e6a32e8bd3ba..6c2cc414d36c 100644
--- a/mkfs/rootdir.c
+++ b/mkfs/rootdir.c
@@ -533,7 +533,13 @@ static int traverse_directory(struct btrfs_trans_handle *trans,
 				goto fail;
 			}
 
-			cur_inum = st.st_ino;
+			/*
+			 * We can not directly use the reported ino number,
+			 * as there is a chance that the ino is smaller than
+			 * BTRFS_FIRST_FREE_OBJECTID, which will screw up
+			 * backref code.
+			 */
+			cur_inum = st.st_ino + BTRFS_FIRST_FREE_OBJECTID;
 			ret = add_directory_items(trans, root,
 						  cur_inum, parent_inum,
 						  cur_file->d_name,
-- 
2.37.3

Re: [PATCH 0/2] btrfs-progs: mkfs: --rootdir related fixes

[David Sterba]

On Tue, Oct 04, 2022 at 03:43:37PM +0800, Qu Wenruo wrote:
> I don't know if it's recent kernel tmpfs change or something else, but
> I'm consistently get ino number smaller than 256 from my /tmp directory.
> 
> This behavior change exposed a new problem in mkfs.btrfs --rootdir, that
> if some ino number (in the source directory, not in btrfs) is smaller
> than 256, it can screw up the backref code.
> 
> As backref code is utilizing @owner to determine if a backref is data or
> metadata.
> 
> And inode number smaller than 256 will make backref code to treat a data
> backref as tree backref, and cause corruption.
> 
> Thankfully this should not happen that easily, only when --rootdir
> points to a newly created fs.
> 
> Qu Wenruo (2):
>   btrfs-progs: properly initialized extent generation for
>     __btrfs_record_file_extent()
>   btrfs-progs: avoid fs corruption if rootdir contains ino smaller than
>     BTRFS_FIRST_FREE_OBJECTID

Added to devel, thanks.