Re: [PATCH RFC] btrfs: allow extent buffer helpers to skip cross-page handling

[David Sterba <dsterba@suse.cz>]

On Fri, Nov 24, 2023 at 07:21:04AM +1030, Qu Wenruo wrote:
> 
> 
> On 2023/11/23 06:31, Qu Wenruo wrote:
> > 
> > 
> > On 2023/11/23 00:16, David Sterba wrote:
> >> On Thu, Nov 16, 2023 at 03:49:06PM +1030, Qu Wenruo wrote:
> >>> --- a/fs/btrfs/disk-io.c
> >>> +++ b/fs/btrfs/disk-io.c
> >>> @@ -80,8 +80,16 @@ static void csum_tree_block(struct extent_buffer 
> >>> *buf, u8 *result)
> >>>       char *kaddr;
> >>>       int i;
> >>>
> >>> +    memset(result, 0, BTRFS_CSUM_SIZE);
> >>>       shash->tfm = fs_info->csum_shash;
> >>>       crypto_shash_init(shash);
> >>> +
> >>> +    if (buf->addr) {
> >>> +        crypto_shash_digest(shash, buf->addr + 
> >>> offset_in_page(buf->start) + BTRFS_CSUM_SIZE,
> >>> +                    buf->len - BTRFS_CSUM_SIZE, result);
> >>> +        return;
> >>> +    }
> >>
> >> This duplicates the address and size
> >>> +
> >>>       kaddr = page_address(buf->pages[0]) + offset_in_page(buf->start);
> >>>       crypto_shash_update(shash, kaddr + BTRFS_CSUM_SIZE,
> >>>                   first_page_part - BTRFS_CSUM_SIZE);
> >>> @@ -90,7 +98,6 @@ static void csum_tree_block(struct extent_buffer 
> >>> *buf, u8 *result)
> >>>           kaddr = page_address(buf->pages[i]);
> >>>           crypto_shash_update(shash, kaddr, PAGE_SIZE);
> >>>       }
> >>> -    memset(result, 0, BTRFS_CSUM_SIZE);
> >>>       crypto_shash_final(shash, result);
> >>
> >> I'd like to have only one code doing the crypto_shash_ calls, so I'm
> >> suggesting this as the final code (the diff is not clear);
> > 
> > This looks good to me, mind to update it inside your branch?
> > 
> > Thanks,
> > Qu
> >>
> >>   74 static void csum_tree_block(struct extent_buffer *buf, u8 *result)
> >>   75 {
> >>   76         struct btrfs_fs_info *fs_info = buf->fs_info;
> >>   77         int num_pages;
> >>   78         u32 first_page_part;
> >>   79         SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
> >>   80         char *kaddr;
> >>   81         int i;
> >>   82
> >>   83         shash->tfm = fs_info->csum_shash;
> >>   84         crypto_shash_init(shash);
> >>   85
> >>   86         if (buf->addr) {
> >>   87                 /* Pages are contiguous, handle it as one big 
> >> page. */
> >>   88                 kaddr = buf->addr;
> >>   89                 first_page_part = fs_info->nodesize;
> >>   90                 num_pages = 1;
> >>   91         } else {
> >>   92                 kaddr = page_address(buf->pages[0]);
> >>   93                 first_page_part = min_t(u32, PAGE_SIZE, 
> >> fs_info->nodesize);
> >>   94                 num_pages = num_extent_pages(buf);
> >>   95         }
> >>   96         kaddr += offset_in_page(buf->start) + BTRFS_CSUM_SIZE;
> >>   97         first_page_part -= BTRFS_CSUM_SIZE;
> 
> This is decreasing the @first_page_part.
> 
> >>   98
> >>   99         crypto_shash_update(shash, kaddr + BTRFS_CSUM_SIZE,
> >> 100                             first_page_part - BTRFS_CSUM_SIZE);
> 
> Meanwhile we're reducing the size again, and I guess this is the problem 
> causing the test failure.

Yes that was it.

> Although my initial version is indeed doing its own size calculation, 
> the extra calculation is much simpler and does not affect the existing 
> path (thus a little safer).
> 
> I'm fine with either way.

I have some WIP that modifies the checksumming and there are already
like 4 ways how to pass the data, that's the reason I'd like to keep the
cases to minimum, here it was easy.