Re: [PATCH 03/19] jbd2: point the shadow buffer at the frozen data directly

On Sat 01-08-26 18:00:47, Chao Shi wrote:
> When a metadata buffer has to be copied out before it can be journalled,
> jbd2_journal_write_metadata_buffer() writes jh->b_frozen_data rather than
> the page cache copy.  b_frozen_data is kmalloc()ed, so folio_set_bh() makes
> the shadow buffer point at a slab folio.
> 
> That is not something the buffer_head layer can reason about.  A slab folio
> overloads ->mapping, so a shadow buffer looks like it belongs to an
> address_space when it does not.  buffer_set_crypto_ctx() already has to use
> folio_mapping() to avoid tripping over this, and it is the reason
> mark_buffer_write_io_error() cannot be called on a shadow buffer today.
> 
> Point the shadow buffer at the frozen data itself instead: leave b_folio
> NULL and set b_data.  The previous patch taught fs/buffer.c to submit such
> a buffer.  The two commit-path checksum helpers are the only other users of
> the shadow buffer's contents, and they take the data directly rather than
> kmapping a folio that is already mapped.
> 
> Note that the shadow buffer must not be passed to bh_offset() while
> b_folio is NULL.  All four callers that can see one are handled here and in
> the previous patch.
> 
> Tested with ext4 mounted data=journal,journal_checksum on a metadata_csum
> filesystem, writing files whose every block begins with the JBD2 magic so
> that escaping forces the copy-out, then crashing with sysrq-b without
> unmounting and replaying the journal on the next mount.  Recovery
> completed, the file contents matched, e2fsck -fn was clean, and an
> instrumented build confirmed the b_folio == NULL path was taken.
> 
> Suggested-by: Matthew Wilcox (Oracle) <[email protected]>
> Signed-off-by: Chao Shi <[email protected]>

...

> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 09efa337649e..9e4cb04587b4 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -329,6 +329,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>  	struct buffer_head *new_bh;
>  	struct folio *new_folio;
>  	unsigned int new_offset;
> +	bool frozen = false;
>  	struct buffer_head *bh_in = jh2bh(jh_in);
>  	journal_t *journal = transaction->t_journal;
>  
> @@ -354,8 +355,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>  	 * we use that version of the data for the commit.
>  	 */
>  	if (jh_in->b_frozen_data) {
> -		new_folio = virt_to_folio(jh_in->b_frozen_data);
> -		new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
> +		frozen = true;
>  		do_escape = jbd2_data_needs_escaping(jh_in->b_frozen_data);
>  		if (do_escape)
>  			jbd2_data_do_escape(jh_in->b_frozen_data);
> @@ -400,13 +400,22 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>  		jh_in->b_frozen_triggers = jh_in->b_triggers;
>  
>  copy_done:
> -		new_folio = virt_to_folio(jh_in->b_frozen_data);
> -		new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
> +		frozen = true;
>  		jbd2_data_do_escape(jh_in->b_frozen_data);
>  	}
>  
>  escape_done:
> -	folio_set_bh(new_bh, new_folio, new_offset);
> +	if (frozen) {
> +		/*
> +		 * b_frozen_data is slab memory, not page cache.  Point the
> +		 * buffer at it directly rather than at a slab folio, whose
> +		 * ->mapping is not an address_space.
> +		 */
> +		new_bh->b_folio = NULL;
> +		new_bh->b_data = jh_in->b_frozen_data;
> +	} else {
> +		folio_set_bh(new_bh, new_folio, new_offset);
> +	}

Can we please simplify the flow in this function? folio_set_bh() is now
done only for a single branch in this function. You can move it there and
completely avoid new_folio, new_offset, and frozen variables (you just need
to set new_bh->b_data in the two branches where frozen data is used).

								Honza
-- 
Jan Kara <[email protected]>
SUSE Labs, CR