Re: [RFC PATCH v4 3/4] ext4: data=journal: fixes for ext4_page_mkwrite()

[Jan Kara <jack@suse.cz>]

On Mon 28-09-20 16:41:02, Mauricio Faria de Oliveira wrote:
> These are two fixes for data journalling required by
> the next patch, discovered while testing it.
> 
> First, the optimization to return early if all buffers
> are mapped is not appropriate for the next patch:
> 
> The inode _must_ be added to the transaction's list in
> data=journal mode (so to write-protect pages on commit)
> thus we cannot return early there.
> 
> Second, once that optimization to reduce transactions
> was disabled for data=journal mode, more transactions
> happened, and occasionally hit this warning message:
> 'JBD2: Spotted dirty metadata buffer'.
> 
> Reason is, block_page_mkwrite() will set_buffer_dirty()
> before do_journal_get_write_access() that is there to
> prevent it. This issue was masked by the optimization.
> 
> So, on data=journal use __block_write_begin() instead.
> This also requires page locking and len recalculation.
> (see block_page_mkwrite() for implementation details.)
> 
> Finally, as Jan noted there is little sharing between
> data=journal and other modes in ext4_page_mkwrite().
> 
> However, a prototype of ext4_journalled_page_mkwrite()
> showed there still would be lots of duplicated lines
> (tens of) that didn't seem worth it.
> 
> Thus this patch ends up with an ugly goto to skip all
> non-data journalling code (to avoid long indentations,
> but that can be changed..) in the beginning, and just
> a conditional in the transaction section.
> 
> Well, we skip a common part to data journalling which
> is the page truncated check, but we do it again after
> ext4_journal_start() when we re-acquire the page lock
> (so not to acquire the page lock twice needlessly for
> data journalling.)
> 
> Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
> Suggested-by: Jan Kara <jack@suse.cz>

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

							Honza

> ---
>  fs/ext4/inode.c | 51 ++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 44 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index bf596467c234..ac153e340a6f 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5977,9 +5977,17 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
>  	if (err)
>  		goto out_ret;
>  
> +	/*
> +	 * On data journalling we skip straight to the transaction handle:
> +	 * there's no delalloc; page truncated will be checked later; the
> +	 * early return w/ all buffers mapped (calculates size/len) can't
> +	 * be used; and there's no dioread_nolock, so only ext4_get_block.
> +	 */
> +	if (ext4_should_journal_data(inode))
> +		goto retry_alloc;
> +
>  	/* Delalloc case is easy... */
>  	if (test_opt(inode->i_sb, DELALLOC) &&
> -	    !ext4_should_journal_data(inode) &&
>  	    !ext4_nonda_switch(inode->i_sb)) {
>  		do {
>  			err = block_page_mkwrite(vma, vmf,
> @@ -6005,6 +6013,9 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
>  	/*
>  	 * Return if we have all the buffers mapped. This avoids the need to do
>  	 * journal_start/journal_stop which can block and take a long time
> +	 *
> +	 * This cannot be done for data journalling, as we have to add the
> +	 * inode to the transaction's list to writeprotect pages on commit.
>  	 */
>  	if (page_has_buffers(page)) {
>  		if (!ext4_walk_page_buffers(NULL, page_buffers(page),
> @@ -6029,16 +6040,42 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
>  		ret = VM_FAULT_SIGBUS;
>  		goto out;
>  	}
> -	err = block_page_mkwrite(vma, vmf, get_block);
> -	if (!err && ext4_should_journal_data(inode)) {
> -		if (ext4_walk_page_buffers(handle, page_buffers(page), 0,
> -			  PAGE_SIZE, NULL, do_journal_get_write_access)) {
> +	/*
> +	 * Data journalling can't use block_page_mkwrite() because it
> +	 * will set_buffer_dirty() before do_journal_get_write_access()
> +	 * thus might hit warning messages for dirty metadata buffers.
> +	 */
> +	if (!ext4_should_journal_data(inode)) {
> +		err = block_page_mkwrite(vma, vmf, get_block);
> +	} else {
> +		lock_page(page);
> +		size = i_size_read(inode);
> +		/* Page got truncated from under us? */
> +		if (page->mapping != mapping || page_offset(page) > size) {
>  			unlock_page(page);
> -			ret = VM_FAULT_SIGBUS;
> +			ret = VM_FAULT_NOPAGE;
>  			ext4_journal_stop(handle);
>  			goto out;
>  		}
> -		ext4_set_inode_state(inode, EXT4_STATE_JDATA);
> +
> +		if (page->index == size >> PAGE_SHIFT)
> +			len = size & ~PAGE_MASK;
> +		else
> +			len = PAGE_SIZE;
> +
> +		err = __block_write_begin(page, 0, len, ext4_get_block);
> +		if (!err) {
> +			if (ext4_walk_page_buffers(handle, page_buffers(page),
> +					0, len, NULL, do_journal_get_write_access)) {
> +				unlock_page(page);
> +				ret = VM_FAULT_SIGBUS;
> +				ext4_journal_stop(handle);
> +				goto out;
> +			}
> +			ext4_set_inode_state(inode, EXT4_STATE_JDATA);
> +		} else {
> +			unlock_page(page);
> +		}
>  	}
>  	ext4_journal_stop(handle);
>  	if (err == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR