Re: [PATCH 1/2] Btrfs: factor out extent dropping code from hole punch handler

[Nikolay Borisov <nborisov@suse.com>]

On 27.06.19 г. 20:00 ч., fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Move the code that is responsible for dropping extents in a range out of
> btrfs_punch_hole() into a new helper function, btrfs_punch_hole_range(),
> so that later it can be used by the reflinking (extent cloning and dedup)
> code to fix a ENOSPC bug.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/file.c | 308 ++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 166 insertions(+), 142 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 1c7533db16b0..393a6d23b6b0 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -2448,27 +2448,171 @@ static int btrfs_punch_hole_lock_range(struct inode *inode,
>  	return 0;
>  }
>  
> +/*
> + * The respective range must have been previously locked, as well as the inode.
> + * The end offset is inclusive (last byte of the range).
> + */
> +static int btrfs_punch_hole_range(struct inode *inode, struct btrfs_path *path,
> +				  const u64 start, const u64 end,
> +				  struct btrfs_trans_handle **trans_out)

I'm not a big fan of the way a lower function starts a transaction which
is then passed to the caller. So while it fixes a real bug in the next
patch it isn't really pushing the code in the right direction. I see
that this transaction is bound to whether no_hole is enabled or not so
it's not just a matter of lifting it up to the caller. And there's also
the while loop which commits it and starts a new one. So yeah, it
doesn't seem like there's a significantly better way of doing that now
but IMO we need to think of cleaning that up later.


<snip>