Message-ID: <e5b6c075-9edd-4729-8da8-65be0a48dea7@gmx.com>
Date: Wed, 19 Nov 2025 18:46:39 +1030
From: Qu Wenruo <quwenruo.btrfs@....com>
To: Daniel Vacek <neelx@...e.com>
Cc: Qu Wenruo <wqu@...e.com>, Chris Mason <clm@...com>,
 Josef Bacik <josef@...icpanda.com>, David Sterba <dsterba@...e.com>,
 linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 3/8] btrfs: add a bio argument to btrfs_csum_one_bio



在 2025/11/19 18:04, Daniel Vacek 写道:
[...]
>> Unless you mean the encrypted bio is not encapsulated by btrfs_bio, but
>> a vanilla bio.
> 

Now it's completely unrelated to the patchset, but I'm still very 
interested in the workflow of fscrypt.

Is there high level docs about it?

> That's the case. The bounce bio is created when you submit the
> original one. The data is encrypted by fscrypt, then the csum hook is
> called and the new bio submitted instead of the original one.

I guess by "submit" you didn't mean btrfs_submit_bio(), but something 
else right?
Or the plaintext bio will be submitted to the disks.

> Later
> the endio frees the new one and calls endio on the original bio.

But the encrypted bio still needs to be handled by btrfs_submit_bio(), 
as we still have a lot of extra works like splitting due to stripe 
boundary and duplicating to different mirrors.

The end io function of the encrypted bio won't be called until all of 
our btrfs specific work/IO is done.

> This
> means we don't have control over the bounce bio and cannot use it
> asynchronously at the moment. The csum needs to be finished directly
> in the hook.

Any code I can look into the "hook"?

I checked the ext4's code (althought it doesn't support csum), they just 
call fscrypt_encrypt_pagecache_blocks() on the plaintext folios, and 
submit the encrypted folios as the bio payload.

It's completely fine to just add those encrypted folios into a 
btrfs_bio, and submit as usual.

I believe we can handle it inside the endio function, end_bbio_data_write().

In fact, currently for write bios, we use an empty btrfs_bio::private, 
we can definitely allocate some extra structure for encrypted writes to 
track the plain text pages (mostly just the start/end file pos).


So my uneducated guess of the encyrpted write would be something like 
the following, mostly happenin in the function submit_extent_folio():

submit_extent_folio()
{
	/* setup bio_ctrl for encrypted, involving the
          * btrfs_bio::private.
	 * So that alloc_new_bio() will allocate a new
	 * private for encrypted write.
          */
	do {
		if (is_encrypted) {
			data_folio = fscrypt_encrypt();
			ret = bio_add_folio();
		} else {
			ret = bio_add_folio();
		}
		/* the remaining as usual */
	}
}

Then in the endio do the special handling:

end_bbio_data_write()
{
	if (!bbio->private) {
		/* The existing one. */
		end_bbio_data_write_plaintext();
	} else {
		/*
		 * Using bbio->private to grab the page cache folios
		 * and free the encrypted folios.
		 */
	}
	bio_put(&bbio->bio);
}

Thus that's why I didn't see the point to trace the original bbio inside 
btrfs_bio, and also didn't see why we have any problem doing async csum.

BTW, the same can be applied to data read, as we didn't utilize 
btrfs_bio::private either.

Or did I miss something?

Thanks,
Qu

> 
> Anyways, the hook changes are not upstreamed yet, so you can only see
> it on the mailing list. And that's also why this patch makes more
> sense to be sent later together with those changes.
> 
> --nX
> 
>> In that case you can not even submit it through btrfs_submit_bbio().
>>
>> Thanks,
>> Qu
>>
>>> We'll need to
>>> always compute the checksum synchronously for encrypted bios. In that
>>> case we don't need to store it in btrfs_bio::csum_bio at all. For the
>>> regular (unencrypted) case we can keep using the &bbio->bio.
>>>
>>> I'll drop the csum_bio for there is no use really.
>>>
>>> --nX
>>>
>>>> I thought it's more common to put the original plaintext into the
>>>> encryption specific structure, like what we did for compression.
>>>>
>>>> Thanks,
>>>> Qu
>>>>
>>>>>
>>>>> --nX
>>>>>
>>>>>> The storage layer doesn't need to bother the plaintext bio at all, they
>>>>>> just write the encrypted one to disk.
>>>>>>
>>>>>> And it's the dm-crypto tracking the plaintext bio <-> encrypted bio mapping.
>>>>>>
>>>>>>
>>>>>> So why we can not just create a new bio for the final csum caculation,
>>>>>> just like compression?
>>>>>>
>>>>>> Thanks,
>>>>>> Qu
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Josef Bacik <josef@...icpanda.com>
>>>>>>> Signed-off-by: Daniel Vacek <neelx@...e.com>
>>>>>>> ---
>>>>>>> Compared to v5 this needed to adapt to recent async csum changes.
>>>>>>> ---
>>>>>>>      fs/btrfs/bio.c       |  4 ++--
>>>>>>>      fs/btrfs/bio.h       |  1 +
>>>>>>>      fs/btrfs/file-item.c | 17 ++++++++---------
>>>>>>>      fs/btrfs/file-item.h |  2 +-
>>>>>>>      4 files changed, 12 insertions(+), 12 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
>>>>>>> index a73652b8724a..a69174b2b6b6 100644
>>>>>>> --- a/fs/btrfs/bio.c
>>>>>>> +++ b/fs/btrfs/bio.c
>>>>>>> @@ -542,9 +542,9 @@ static int btrfs_bio_csum(struct btrfs_bio *bbio)
>>>>>>>          if (bbio->bio.bi_opf & REQ_META)
>>>>>>>                  return btree_csum_one_bio(bbio);
>>>>>>>      #ifdef CONFIG_BTRFS_EXPERIMENTAL
>>>>>>> -     return btrfs_csum_one_bio(bbio, true);
>>>>>>> +     return btrfs_csum_one_bio(bbio, &bbio->bio, true);
>>>>>>>      #else
>>>>>>> -     return btrfs_csum_one_bio(bbio, false);
>>>>>>> +     return btrfs_csum_one_bio(bbio, &bbio->bio, false);
>>>>>>>      #endif
>>>>>>>      }
>>>>>>>
>>>>>>> diff --git a/fs/btrfs/bio.h b/fs/btrfs/bio.h
>>>>>>> index deaeea3becf4..c5a6c66d51a0 100644
>>>>>>> --- a/fs/btrfs/bio.h
>>>>>>> +++ b/fs/btrfs/bio.h
>>>>>>> @@ -58,6 +58,7 @@ struct btrfs_bio {
>>>>>>>                          struct btrfs_ordered_sum *sums;
>>>>>>>                          struct work_struct csum_work;
>>>>>>>                          struct completion csum_done;
>>>>>>> +                     struct bio *csum_bio;
>>>>>>>                          struct bvec_iter csum_saved_iter;
>>>>>>>                          u64 orig_physical;
>>>>>>>                  };
>>>>>>> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
>>>>>>> index 72be3ede0edf..474949074da8 100644
>>>>>>> --- a/fs/btrfs/file-item.c
>>>>>>> +++ b/fs/btrfs/file-item.c
>>>>>>> @@ -765,21 +765,19 @@ int btrfs_lookup_csums_bitmap(struct btrfs_root *root, struct btrfs_path *path,
>>>>>>>          return ret;
>>>>>>>      }
>>>>>>>
>>>>>>> -static void csum_one_bio(struct btrfs_bio *bbio, struct bvec_iter *src)
>>>>>>> +static void csum_one_bio(struct btrfs_bio *bbio, struct bio *bio, struct bvec_iter *iter)
>>>>>>>      {
>>>>>>>          struct btrfs_inode *inode = bbio->inode;
>>>>>>>          struct btrfs_fs_info *fs_info = inode->root->fs_info;
>>>>>>>          SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
>>>>>>> -     struct bio *bio = &bbio->bio;
>>>>>>>          struct btrfs_ordered_sum *sums = bbio->sums;
>>>>>>> -     struct bvec_iter iter = *src;
>>>>>>>          phys_addr_t paddr;
>>>>>>>          const u32 blocksize = fs_info->sectorsize;
>>>>>>>          int index = 0;
>>>>>>>
>>>>>>>          shash->tfm = fs_info->csum_shash;
>>>>>>>
>>>>>>> -     btrfs_bio_for_each_block(paddr, bio, &iter, blocksize) {
>>>>>>> +     btrfs_bio_for_each_block(paddr, bio, iter, blocksize) {
>>>>>>>                  btrfs_calculate_block_csum(fs_info, paddr, sums->sums + index);
>>>>>>>                  index += fs_info->csum_size;
>>>>>>>          }
>>>>>>> @@ -791,19 +789,18 @@ static void csum_one_bio_work(struct work_struct *work)
>>>>>>>
>>>>>>>          ASSERT(btrfs_op(&bbio->bio) == BTRFS_MAP_WRITE);
>>>>>>>          ASSERT(bbio->async_csum == true);
>>>>>>> -     csum_one_bio(bbio, &bbio->csum_saved_iter);
>>>>>>> +     csum_one_bio(bbio, bbio->csum_bio, &bbio->csum_saved_iter);
>>>>>>>          complete(&bbio->csum_done);
>>>>>>>      }
>>>>>>>
>>>>>>>      /*
>>>>>>>       * Calculate checksums of the data contained inside a bio.
>>>>>>>       */
>>>>>>> -int btrfs_csum_one_bio(struct btrfs_bio *bbio, bool async)
>>>>>>> +int btrfs_csum_one_bio(struct btrfs_bio *bbio, struct bio *bio, bool async)
>>>>>>>      {
>>>>>>>          struct btrfs_ordered_extent *ordered = bbio->ordered;
>>>>>>>          struct btrfs_inode *inode = bbio->inode;
>>>>>>>          struct btrfs_fs_info *fs_info = inode->root->fs_info;
>>>>>>> -     struct bio *bio = &bbio->bio;
>>>>>>>          struct btrfs_ordered_sum *sums;
>>>>>>>          unsigned nofs_flag;
>>>>>>>
>>>>>>> @@ -822,12 +819,14 @@ int btrfs_csum_one_bio(struct btrfs_bio *bbio, bool async)
>>>>>>>          btrfs_add_ordered_sum(ordered, sums);
>>>>>>>
>>>>>>>          if (!async) {
>>>>>>> -             csum_one_bio(bbio, &bbio->bio.bi_iter);
>>>>>>> +             struct bvec_iter iter = bio->bi_iter;
>>>>>>> +             csum_one_bio(bbio, bio, &iter);
>>>>>>>                  return 0;
>>>>>>>          }
>>>>>>>          init_completion(&bbio->csum_done);
>>>>>>>          bbio->async_csum = true;
>>>>>>> -     bbio->csum_saved_iter = bbio->bio.bi_iter;
>>>>>>> +     bbio->csum_bio = bio;
>>>>>>> +     bbio->csum_saved_iter = bio->bi_iter;
>>>>>>>          INIT_WORK(&bbio->csum_work, csum_one_bio_work);
>>>>>>>          schedule_work(&bbio->csum_work);
>>>>>>>          return 0;
>>>>>>> diff --git a/fs/btrfs/file-item.h b/fs/btrfs/file-item.h
>>>>>>> index 5645c5e3abdb..d16fd2144552 100644
>>>>>>> --- a/fs/btrfs/file-item.h
>>>>>>> +++ b/fs/btrfs/file-item.h
>>>>>>> @@ -64,7 +64,7 @@ int btrfs_lookup_file_extent(struct btrfs_trans_handle *trans,
>>>>>>>      int btrfs_csum_file_blocks(struct btrfs_trans_handle *trans,
>>>>>>>                             struct btrfs_root *root,
>>>>>>>                             struct btrfs_ordered_sum *sums);
>>>>>>> -int btrfs_csum_one_bio(struct btrfs_bio *bbio, bool async);
>>>>>>> +int btrfs_csum_one_bio(struct btrfs_bio *bbio, struct bio *bio, bool async);
>>>>>>>      int btrfs_alloc_dummy_sum(struct btrfs_bio *bbio);
>>>>>>>      int btrfs_lookup_csums_range(struct btrfs_root *root, u64 start, u64 end,
>>>>>>>                               struct list_head *list, int search_commit,
>>>>>>
>>>>
>>>
>>

Powered by blists - more mailing lists