[PATCH] jbd2: Add a comment for incorrect tag size

[PATCH] jbd2: Add a comment for incorrect tag size

[Wang Jianjian]

journal_tag_t has already counted the checksum size, however, for
compatibility reason, we don't fix this bug and keep it as is.

Signed-off-by: Wang Jianjian <wangjianjian0@foxmail.com>
---
 fs/jbd2/journal.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index b6c114c11b97..b5e614818e8b 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -2698,6 +2698,10 @@ size_t journal_tag_bytes(journal_t *journal)
 
 	sz = sizeof(journal_block_tag_t);
 
+	/*
+	 * journal_block_tag_t has already counted checksum size
+	 * but for compatibility reason, we keep it as is.
+	 */
 	if (jbd2_has_feature_csum2(journal))
 		sz += sizeof(__u16);
 
-- 
2.34.3

Re: [PATCH] jbd2: Add a comment for incorrect tag size

[Theodore Ts'o]

On Thu, Apr 04, 2024 at 09:36:54PM +0800, Wang Jianjian wrote:
> journal_tag_t has already counted the checksum size, however, for
> compatibility reason, we don't fix this bug and keep it as is.
> 
> Signed-off-by: Wang Jianjian <wangjianjian0@foxmail.com>

The csum_v2 layout had a number of problems, which was documented in
commit db9ee220361de:

    jbd2: fix descriptor block size handling errors with journal_csum
    
    It turns out that there are some serious problems with the on-disk
    format of journal checksum v2.  The foremost is that the function to
    calculate descriptor tag size returns sizes that are too big.  This
    causes alignment issues on some architectures and is compounded by the
    fact that some parts of jbd2 use the structure size (incorrectly) to
    determine the presence of a 64bit journal instead of checking the
    feature flags.
    
    Therefore, introduce journal checksum v3, which enlarges the
    descriptor block tag format to allow for full 32-bit checksums of
    journal blocks, fix the journal tag function to return the correct
    sizes, and fix the jbd2 recovery code to use feature flags to
    determine 64bitness.
    
    Add a few function helpers so we don't have to open-code quite so
    many pieces.
    
    Switching to a 16-byte block size was found to increase journal size
    overhead by a maximum of 0.1%, to convert a 32-bit journal with no
    checksumming to a 32-bit journal with checksum v3 enabled.

We switched to using csum_v3 in 2014, in Linux v3.17.  So most recent
LTS kernel which used the v2 csum format was Linux v3.14 which EOL'ed
in 2016 --- a full eight years ago.  So it's probably not worth adding
the comment at this point.

In fact, what we might want to consider is yanking support for the
CSUM_v2 and CSUM_V1 at this point, since *all* currently supported LTS
kernels (4.19, 5.4, 5.10, 5.15, 6.1 and 6.6) will be using CSUM_V3.
It's not actually *that* much code, but what's there is a bit hard to
understand since it very much relies on how the v2 and v3 data
strutures line up with each other, and the fact that the jbd2
structures are stored on disk in big_endian. 

						- Ted

Re: [PATCH] jbd2: Add a comment for incorrect tag size

[wangjianjian (C)]

On 2024/6/20 7:36, Theodore Ts'o wrote:
> bd2: fix descriptor block size handling errors with journal_csum
 > in 2016 --- a full eight years ago.  So it's probably not worth adding
the comment at this point.

Hi Ted,
Thanks for detailed information, but is it better to put it in document 
in case any other one confuse about this when read code.
-- 
Regards

Re: [PATCH] jbd2: Add a comment for incorrect tag size

[Theodore Ts'o]

On Thu, Jun 20, 2024 at 11:19:30AM +0800, wangjianjian (C) wrote:
> On 2024/6/20 7:36, Theodore Ts'o wrote:
> > bd2: fix descriptor block size handling errors with journal_csum
> > in 2016 --- a full eight years ago.  So it's probably not worth adding
> the comment at this point.
> 
> Thanks for detailed information, but is it better to put it in document in
> case any other one confuse about this when read code.

The comment would probably make things more confusing, since there's a
much larger context involving the fact that csum_v2 is an obsolete
format that in practice is never used.  Sure, we could make the
comment even more verbose, but perhaps it would be better to simply
completely remove csum_v1 and csum_v2 from the code.  That will
probably make the code even more simpler to read.

Cheers,

						- Ted

Re: [PATCH] jbd2: Add a comment for incorrect tag size

[wangjianjian (C)]

On 2024/6/27 21:31, Theodore Ts'o wrote:
> On Thu, Jun 20, 2024 at 11:19:30AM +0800, wangjianjian (C) wrote:
>> On 2024/6/20 7:36, Theodore Ts'o wrote:
>>> bd2: fix descriptor block size handling errors with journal_csum
>>> in 2016 --- a full eight years ago.  So it's probably not worth adding
>> the comment at this point.
>>
>> Thanks for detailed information, but is it better to put it in document in
>> case any other one confuse about this when read code.
> 
> The comment would probably make things more confusing, since there's a
> much larger context involving the fact that csum_v2 is an obsolete
> format that in practice is never used.  Sure, we could make the
> comment even more verbose, but perhaps it would be better to simply
> completely remove csum_v1 and csum_v2 from the code.  That will
> probably make the code even more simpler to read.

Agree with that, if it is ok, I can submit a patch do that.
> 
> Cheers,
> 
> 						- Ted
> 
-- 
Regards