[PATCH] btrfs: warn on tree blocks which are not nodesize aligned

[PATCH] btrfs: warn on tree blocks which are not nodesize aligned

[Qu Wenruo]

A long time ago, we have some metadata chunks which starts at sector
boundary but not aligned at nodesize boundary.

This led to some older fs which can have tree blocks only aligned to
sectorsize, but not nodesize.

Later btrfs check gained the ability to detect and warn about such tree
blocks, and kernel fixed the chunk allocation behavior, nowadays those
tree blocks should be pretty rare.

But in the future, if we want to migrate metadata to folio, we can not
have such tree blocks, as filemap_add_folio() requires the page index to
be aligned with the folio number of pages.
(AKA, such unaligned tree blocks can lead to VM_BUG_ON().)

So this patch adds extra warning for those unaligned tree blocks, as a
preparation for the future folio migration.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent_io.c | 8 ++++++++
 fs/btrfs/fs.h        | 7 +++++++
 2 files changed, 15 insertions(+)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 65b01ec5bab1..0aa27a212d1e 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3518,6 +3518,14 @@ static int check_eb_alignment(struct btrfs_fs_info *fs_info, u64 start)
 			  start, fs_info->nodesize);
 		return -EINVAL;
 	}
+	if (!IS_ALIGNED(start, fs_info->nodesize) &&
+	    !test_and_set_bit(BTRFS_FS_UNALIGNED_TREE_BLOCK,
+			      &fs_info->flags)) {
+		btrfs_warn(fs_info,
+		"tree block not nodesize aligned, start %llu nodesize %u",
+			      start, fs_info->nodesize);
+		btrfs_warn(fs_info, "this can be solved by a full metadata balance");
+	}
 	return 0;
 }
 
diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
index 203d2a267828..2de3961aee44 100644
--- a/fs/btrfs/fs.h
+++ b/fs/btrfs/fs.h
@@ -141,6 +141,13 @@ enum {
 	 */
 	BTRFS_FS_FEATURE_CHANGED,
 
+	/*
+	 * Indicate if we have tree block which is only aligned to sectorsize,
+	 * but not to nodesize.
+	 * This should be rare nowadays.
+	 */
+	BTRFS_FS_UNALIGNED_TREE_BLOCK,
+
 #if BITS_PER_LONG == 32
 	/* Indicate if we have error/warn message printed on 32bit systems */
 	BTRFS_FS_32BIT_ERROR,
-- 
2.41.0

Re: [PATCH] btrfs: warn on tree blocks which are not nodesize aligned

[Anand Jain]

On 24/7/23 14:09, Qu Wenruo wrote:
> A long time ago, we have some metadata chunks which starts at sector
> boundary but not aligned at nodesize boundary.
> 
> This led to some older fs which can have tree blocks only aligned to
> sectorsize, but not nodesize.
> 
> Later btrfs check gained the ability to detect and warn about such tree
> blocks, and kernel fixed the chunk allocation behavior, nowadays those
> tree blocks should be pretty rare.
> 
> But in the future, if we want to migrate metadata to folio, we can not
> have such tree blocks, as filemap_add_folio() requires the page index to
> be aligned with the folio number of pages.
> (AKA, such unaligned tree blocks can lead to VM_BUG_ON().)
> 
> So this patch adds extra warning for those unaligned tree blocks, as a
> preparation for the future folio migration.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/extent_io.c | 8 ++++++++
>   fs/btrfs/fs.h        | 7 +++++++
>   2 files changed, 15 insertions(+)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 65b01ec5bab1..0aa27a212d1e 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3518,6 +3518,14 @@ static int check_eb_alignment(struct btrfs_fs_info *fs_info, u64 start)
>   			  start, fs_info->nodesize);
>   		return -EINVAL;
>   	}
> +	if (!IS_ALIGNED(start, fs_info->nodesize) &&
> +	    !test_and_set_bit(BTRFS_FS_UNALIGNED_TREE_BLOCK,
> +			      &fs_info->flags)) {
> +		btrfs_warn(fs_info,
> +		"tree block not nodesize aligned, start %llu nodesize %u",
> +			      start, fs_info->nodesize);
> +		btrfs_warn(fs_info, "this can be solved by a full metadata balance");
> +	}

A btrfs_warn_rl() will be a safer option IMO.

Thanks.

>   	return 0;
>   }
>   
> diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
> index 203d2a267828..2de3961aee44 100644
> --- a/fs/btrfs/fs.h
> +++ b/fs/btrfs/fs.h
> @@ -141,6 +141,13 @@ enum {
>   	 */
>   	BTRFS_FS_FEATURE_CHANGED,
>   
> +	/*
> +	 * Indicate if we have tree block which is only aligned to sectorsize,
> +	 * but not to nodesize.
> +	 * This should be rare nowadays.
> +	 */
> +	BTRFS_FS_UNALIGNED_TREE_BLOCK,
> +
>   #if BITS_PER_LONG == 32
>   	/* Indicate if we have error/warn message printed on 32bit systems */
>   	BTRFS_FS_32BIT_ERROR,

Re: [PATCH] btrfs: warn on tree blocks which are not nodesize aligned

[Qu Wenruo]

On 2023/7/24 15:43, Anand Jain wrote:
> On 24/7/23 14:09, Qu Wenruo wrote:
>> A long time ago, we have some metadata chunks which starts at sector
>> boundary but not aligned at nodesize boundary.
>>
>> This led to some older fs which can have tree blocks only aligned to
>> sectorsize, but not nodesize.
>>
>> Later btrfs check gained the ability to detect and warn about such tree
>> blocks, and kernel fixed the chunk allocation behavior, nowadays those
>> tree blocks should be pretty rare.
>>
>> But in the future, if we want to migrate metadata to folio, we can not
>> have such tree blocks, as filemap_add_folio() requires the page index to
>> be aligned with the folio number of pages.
>> (AKA, such unaligned tree blocks can lead to VM_BUG_ON().)
>>
>> So this patch adds extra warning for those unaligned tree blocks, as a
>> preparation for the future folio migration.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/extent_io.c | 8 ++++++++
>>   fs/btrfs/fs.h        | 7 +++++++
>>   2 files changed, 15 insertions(+)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 65b01ec5bab1..0aa27a212d1e 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -3518,6 +3518,14 @@ static int check_eb_alignment(struct
>> btrfs_fs_info *fs_info, u64 start)
>>                 start, fs_info->nodesize);
>>           return -EINVAL;
>>       }
>> +    if (!IS_ALIGNED(start, fs_info->nodesize) &&
>> +        !test_and_set_bit(BTRFS_FS_UNALIGNED_TREE_BLOCK,
>> +                  &fs_info->flags)) {
>> +        btrfs_warn(fs_info,
>> +        "tree block not nodesize aligned, start %llu nodesize %u",
>> +                  start, fs_info->nodesize);
>> +        btrfs_warn(fs_info, "this can be solved by a full metadata
>> balance");
>> +    }
>
> A btrfs_warn_rl() will be a safer option IMO.

Not really, as this warning will only be output once, as we are doing
test_and_set_bit().

Thus I really want to all messages to be shown, including the solution
to fix it.

Thanks,
Qu
>
> Thanks.
>
>>       return 0;
>>   }
>> diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
>> index 203d2a267828..2de3961aee44 100644
>> --- a/fs/btrfs/fs.h
>> +++ b/fs/btrfs/fs.h
>> @@ -141,6 +141,13 @@ enum {
>>        */
>>       BTRFS_FS_FEATURE_CHANGED,
>> +    /*
>> +     * Indicate if we have tree block which is only aligned to
>> sectorsize,
>> +     * but not to nodesize.
>> +     * This should be rare nowadays.
>> +     */
>> +    BTRFS_FS_UNALIGNED_TREE_BLOCK,
>> +
>>   #if BITS_PER_LONG == 32
>>       /* Indicate if we have error/warn message printed on 32bit
>> systems */
>>       BTRFS_FS_32BIT_ERROR,
>

Re: [PATCH] btrfs: warn on tree blocks which are not nodesize aligned

[Christoph Hellwig]

A _warn level printk seems pretty excessive for something that was
perfectly legal and created by earlier implementations.

Re: [PATCH] btrfs: warn on tree blocks which are not nodesize aligned

[Anand Jain]

On 24/7/23 16:55, Qu Wenruo wrote:
> 
> 
> On 2023/7/24 15:43, Anand Jain wrote:
>> On 24/7/23 14:09, Qu Wenruo wrote:
>>> A long time ago, we have some metadata chunks which starts at sector
>>> boundary but not aligned at nodesize boundary.
>>>
>>> This led to some older fs which can have tree blocks only aligned to
>>> sectorsize, but not nodesize.
>>>
>>> Later btrfs check gained the ability to detect and warn about such tree
>>> blocks, and kernel fixed the chunk allocation behavior, nowadays those
>>> tree blocks should be pretty rare.
>>>
>>> But in the future, if we want to migrate metadata to folio, we can not
>>> have such tree blocks, as filemap_add_folio() requires the page index to
>>> be aligned with the folio number of pages.
>>> (AKA, such unaligned tree blocks can lead to VM_BUG_ON().)
>>>
>>> So this patch adds extra warning for those unaligned tree blocks, as a
>>> preparation for the future folio migration.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>   fs/btrfs/extent_io.c | 8 ++++++++
>>>   fs/btrfs/fs.h        | 7 +++++++
>>>   2 files changed, 15 insertions(+)
>>>
>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>> index 65b01ec5bab1..0aa27a212d1e 100644
>>> --- a/fs/btrfs/extent_io.c
>>> +++ b/fs/btrfs/extent_io.c
>>> @@ -3518,6 +3518,14 @@ static int check_eb_alignment(struct
>>> btrfs_fs_info *fs_info, u64 start)
>>>                 start, fs_info->nodesize);
>>>           return -EINVAL;
>>>       }
>>> +    if (!IS_ALIGNED(start, fs_info->nodesize) &&
>>> +        !test_and_set_bit(BTRFS_FS_UNALIGNED_TREE_BLOCK,
>>> +                  &fs_info->flags)) {
>>> +        btrfs_warn(fs_info,
>>> +        "tree block not nodesize aligned, start %llu nodesize %u",
>>> +                  start, fs_info->nodesize);
>>> +        btrfs_warn(fs_info, "this can be solved by a full metadata
>>> balance");
>>> +    }
>>
>> A btrfs_warn_rl() will be a safer option IMO.
> 
> Not really, as this warning will only be output once, as we are doing
> test_and_set_bit().

Oh. Right.

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

Re: [PATCH] btrfs: warn on tree blocks which are not nodesize aligned

[Qu Wenruo]

On 2023/7/24 22:12, Christoph Hellwig wrote:
> A _warn level printk seems pretty excessive for something that was
> perfectly legal and created by earlier implementations.
>
At least we have a valid workaround, and this partial aligned tree block
is already checked and warned by btrfs-check for a long long time.

Thanks,
Qu