Re: [PATCH 02/19] buffer: allow a buffer_head to point at memory outside the page cache

On Sat 01-08-26 18:00:46, Chao Shi wrote:
> jbd2 builds a temporary buffer_head to write out the frozen copy of a
> metadata block, and that copy lives in slab memory.  Today jbd2 points the
> temporary buffer at the slab folio backing it.  A slab folio's ->mapping is
> not an address_space, so anything that follows bh->b_folio->mapping there
> gets garbage rather than NULL; mark_buffer_write_io_error() does exactly
> that, and we are about to start calling it on this buffer.
> 
> Rather than teach every such helper about slab folios, allow bh->b_folio to
> be NULL and let b_data point straight at the memory.  Code that needs the
> folio has to check.  There are two places in this file:
> 
>  - __bh_submit() adds the data by virtual address using
>    bio_add_virt_nofail(), and skips the cgroup accounting: a buffer that is
>    not in the page cache has no owning folio to attribute writeback to.
> 
>  - buffer_set_crypto_ctx() returns early.  fscrypt has no interest in a
>    buffer that is not part of a file mapping, which is why it already
>    returns when folio_mapping() comes back NULL.
> 
> Nothing sets b_folio to NULL yet, so this patch is a no-op on its own.
> 
> This is deliberately not a general capability.  Buffers over highmem have
> no permanent kernel virtual address, which is why folio_set_bh() records a
> folio and an offset instead of an address.  A folio-less buffer_head is
> only valid over memory that is always mapped, and must not be passed to
> bh_offset().

There are much more things you cannot do with a bh that doesn't have valid
b_folio - touch_buffer(), end_buffer_async_read(), ... and many more. But
that's a bussiness of the code that sets up such temporary bhs. I agree
that setting b_folio to NULL will if nothing else lead to much more obvious
failures than when we accidentally get slab folio. So I'd prefer we update
the description a bit in this direction but otherwise feel free to add:

Reviewed-by: Jan Kara <[email protected]>

								Honza

> 
> Suggested-by: Matthew Wilcox (Oracle) <[email protected]>
> Signed-off-by: Chao Shi <[email protected]>
> ---
>  fs/buffer.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index be8b57a635cd..04fcc34e4fa6 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1099,12 +1099,16 @@ EXPORT_SYMBOL(__bforget);
>  static void buffer_set_crypto_ctx(struct bio *bio, const struct buffer_head *bh,
>  				  gfp_t gfp_mask)
>  {
> -	const struct address_space *mapping = folio_mapping(bh->b_folio);
> +	const struct address_space *mapping;
>  
>  	/*
>  	 * The ext4 journal (jbd2) can submit a buffer_head it directly created
> -	 * for a non-pagecache page.  fscrypt doesn't care about these.
> +	 * for memory that is not in the page cache at all.  fscrypt doesn't
> +	 * care about these.
>  	 */
> +	if (!bh->b_folio)
> +		return;
> +	mapping = folio_mapping(bh->b_folio);
>  	if (!mapping)
>  		return;
>  	fscrypt_set_bio_crypt_ctx(bio, mapping->host,
> @@ -1142,7 +1146,11 @@ static void __bh_submit(struct buffer_head *bh, blk_opf_t opf,
>  	bio->bi_iter.bi_sector = bh->b_blocknr * (bh->b_size >> 9);
>  	bio->bi_write_hint = write_hint;
>  
> -	bio_add_folio_nofail(bio, bh->b_folio, bh->b_size, bh_offset(bh));
> +	if (bh->b_folio)
> +		bio_add_folio_nofail(bio, bh->b_folio, bh->b_size,
> +				     bh_offset(bh));
> +	else
> +		bio_add_virt_nofail(bio, bh->b_data, bh->b_size);
>  
>  	bio->bi_end_io = end_bio;
>  	bio->bi_private = bh;
> @@ -1152,7 +1160,8 @@ static void __bh_submit(struct buffer_head *bh, blk_opf_t opf,
>  
>  	if (wbc) {
>  		wbc_init_bio(wbc, bio);
> -		wbc_account_cgroup_owner(wbc, bh->b_folio, bh->b_size);
> +		if (bh->b_folio)
> +			wbc_account_cgroup_owner(wbc, bh->b_folio, bh->b_size);
>  	}
>  
>  	blk_crypto_submit_bio(bio);
> -- 
> 2.43.0
> 
-- 
Jan Kara <[email protected]>
SUSE Labs, CR