Re: [PATCH v2 3/5] btrfs: simplify early error checking in btrfs_page_mkwrite()

[Qu Wenruo <wqu@suse.com>]

在 2025/5/14 21:08, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> We have this entangled error checks early at btrfs_page_mkwrite():
> 
> 1) Try to reserve delalloc space by calling btrfs_delalloc_reserve_space()
>     and storing the return value in the ret2 variable;
> 
> 2) If the reservation succeed, call file_update_time() and store the
>     return value in ret2 and also set the local variable 'reserved' to
>     true (1);
> 
> 3) Then do an error check on ret2 to see if any of the previous calls
>     failed and if so, jump either to the 'out' label or to the
>     'out_noreserve' label, depending on whether 'reserved' is true or
>     not.
> 
> This is unnecessarily complex. Instead change this to a simpler and
> more straighforward approach:
> 
> 1) Call btrfs_delalloc_reserve_space(), if that returns an error jump to
>     the 'out_noreserve' label;
> 
> 2) The call file_update_time() and if that returns an error jump to the
>     'out' label.
> 
> Like this there's less nested if statements, no need to use a local
> variable to track if space was reserved and if statements are used only
> to check errors.
> 
> Also move the call to extent_changeset_free() out of the 'out_noreserve'
> label and under the 'out' label  since the changeset is allocated only if
> the call to reserve delalloc space succeeded.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu> ---
>   fs/btrfs/file.c | 15 +++++++--------
>   1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index a2b1fc536fdd..9ecb9f3bd057 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1843,7 +1843,6 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>   	size_t fsize = folio_size(folio);
>   	vm_fault_t ret;
>   	int ret2;
> -	int reserved = 0;
>   	u64 reserved_space;
>   	u64 page_start;
>   	u64 page_end;
> @@ -1866,17 +1865,17 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>   	 */
>   	ret2 = btrfs_delalloc_reserve_space(BTRFS_I(inode), &data_reserved,
>   					    page_start, reserved_space);
> -	if (!ret2) {
> -		ret2 = file_update_time(vmf->vma->vm_file);
> -		reserved = 1;
> -	}
>   	if (ret2) {
>   		ret = vmf_error(ret2);
> -		if (reserved)
> -			goto out;
>   		goto out_noreserve;
>   	}
>   
> +	ret2 = file_update_time(vmf->vma->vm_file);
> +	if (ret2) {
> +		ret = vmf_error(ret2);
> +		goto out;
> +	}
> +
>   	/* Make the VM retry the fault. */
>   	ret = VM_FAULT_NOPAGE;
>   again:
> @@ -1972,9 +1971,9 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>   	btrfs_delalloc_release_extents(BTRFS_I(inode), fsize);
>   	btrfs_delalloc_release_space(BTRFS_I(inode), data_reserved, page_start,
>   				     reserved_space, true);
> +	extent_changeset_free(data_reserved);
>   out_noreserve:
>   	sb_end_pagefault(inode->i_sb);
> -	extent_changeset_free(data_reserved);
>   	return ret;
>   }
>