Re: [PATCH 01/11] btrfs: add function to create and return an ordered extent

[Naohiro Aota <Naohiro.Aota@wdc.com>]

On Fri, Mar 24, 2023 at 10:31:57AM +0800, Christoph Hellwig wrote:
> From: Boris Burkov <boris@bur.io>
> 
> Currently, btrfs_add_ordered_extent allocates a new ordered extent, adds
> it to the rb_tree, but doesn't return a referenced pointer to the
> caller. There are cases where it is useful for the creator of a new
> ordered_extent to hang on to such a pointer, so add a new function
> btrfs_alloc_ordered_extent which is the same as
> btrfs_add_ordered_extent, except it takes an additional reference count
> and returns a pointer to the ordered_extent. Implement
> btrfs_add_ordered_extent as btrfs_alloc_ordered_extent followed by
> dropping the new reference and handling the IS_ERR case.
> 
> The type of flags in btrfs_alloc_ordered_extent and
> btrfs_add_ordered_extent is changed from unsigned int to unsigned long
> so it's unified with the other ordered extent functions.
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Boris Burkov <boris@bur.io>
> Signed-off-by: David Sterba <dsterba@suse.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/btrfs/ordered-data.c | 46 +++++++++++++++++++++++++++++++++--------
>  fs/btrfs/ordered-data.h |  5 +++++
>  2 files changed, 42 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
> index 6c24b69e2d0a37..83a51c692406ab 100644
> --- a/fs/btrfs/ordered-data.c
> +++ b/fs/btrfs/ordered-data.c
> @@ -160,14 +160,16 @@ static inline struct rb_node *tree_search(struct btrfs_ordered_inode_tree *tree,
>   * @compress_type:   Compression algorithm used for data.
>   *
>   * Most of these parameters correspond to &struct btrfs_file_extent_item. The
> - * tree is given a single reference on the ordered extent that was inserted.
> + * tree is given a single reference on the ordered extent that was inserted, and
> + * the returned pointer is given a second reference.
>   *
> - * Return: 0 or -ENOMEM.
> + * Return: the new ordered extent or ERR_PTR(-ENOMEM).
>   */
> -int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset,
> -			     u64 num_bytes, u64 ram_bytes, u64 disk_bytenr,
> -			     u64 disk_num_bytes, u64 offset, unsigned flags,
> -			     int compress_type)
> +struct btrfs_ordered_extent *btrfs_alloc_ordered_extent(
> +			struct btrfs_inode *inode, u64 file_offset,
> +			u64 num_bytes, u64 ram_bytes, u64 disk_bytenr,
> +			u64 disk_num_bytes, u64 offset, unsigned long flags,
> +			int compress_type)

I'm sorry to write a comment on this late, but isn't the function name
confusing?  As I suggested a function only to allocate and initialize a
btrfs_ordered_extent in the previous mail [1], I first thought this
function is something like that.

[1] https://lore.kernel.org/linux-btrfs/20230323083608.m2ut2whbk2smjjpu@naota-xeon/

But, both btrfs_alloc_ordered_extent() and btrfs_add_ordered_extent() "add
an ordered extent to the per-inode tree." The difference is that
btrfs_alloc_ordered_extent() returns the created ordered extent to the
caller taking a reference for them...

However, I can't think of a different name, so it might be OK...

Other than that,

Reviewed-by: Naohiro Aota <naohiro.aota@wdc.com>

>  {
>  	struct btrfs_root *root = inode->root;
>  	struct btrfs_fs_info *fs_info = root->fs_info;
> @@ -181,7 +183,7 @@ int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset,
>  		/* For nocow write, we can release the qgroup rsv right now */
>  		ret = btrfs_qgroup_free_data(inode, NULL, file_offset, num_bytes);
>  		if (ret < 0)
> -			return ret;
> +			return ERR_PTR(ret);
>  		ret = 0;
>  	} else {
>  		/*
> @@ -190,11 +192,11 @@ int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset,
>  		 */
>  		ret = btrfs_qgroup_release_data(inode, file_offset, num_bytes);
>  		if (ret < 0)
> -			return ret;
> +			return ERR_PTR(ret);
>  	}
>  	entry = kmem_cache_zalloc(btrfs_ordered_extent_cache, GFP_NOFS);
>  	if (!entry)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  
>  	entry->file_offset = file_offset;
>  	entry->num_bytes = num_bytes;
> @@ -256,6 +258,32 @@ int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset,
>  	btrfs_mod_outstanding_extents(inode, 1);
>  	spin_unlock(&inode->lock);
>  
> +	/* One ref for the returned entry to match semantics of lookup. */
> +	refcount_inc(&entry->refs);
> +
> +	return entry;
> +}
> +
> +/*
> + * Add a new btrfs_ordered_extent for the range, but drop the reference instead
> + * of returning it to the caller.
> + */
> +int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset,
> +			     u64 num_bytes, u64 ram_bytes, u64 disk_bytenr,
> +			     u64 disk_num_bytes, u64 offset, unsigned flags,
> +			     int compress_type)
> +{
> +	struct btrfs_ordered_extent *ordered;
> +
> +	ordered = btrfs_alloc_ordered_extent(inode, file_offset, num_bytes,
> +					     ram_bytes, disk_bytenr,
> +					     disk_num_bytes, offset, flags,
> +					     compress_type);
> +
> +	if (IS_ERR(ordered))
> +		return PTR_ERR(ordered);
> +	btrfs_put_ordered_extent(ordered);
> +
>  	return 0;
>  }
>  
> diff --git a/fs/btrfs/ordered-data.h b/fs/btrfs/ordered-data.h
> index eb40cb39f842e6..c00a5a3f060fa2 100644
> --- a/fs/btrfs/ordered-data.h
> +++ b/fs/btrfs/ordered-data.h
> @@ -178,6 +178,11 @@ void btrfs_mark_ordered_io_finished(struct btrfs_inode *inode,
>  bool btrfs_dec_test_ordered_pending(struct btrfs_inode *inode,
>  				    struct btrfs_ordered_extent **cached,
>  				    u64 file_offset, u64 io_size);
> +struct btrfs_ordered_extent *btrfs_alloc_ordered_extent(
> +			struct btrfs_inode *inode, u64 file_offset,
> +			u64 num_bytes, u64 ram_bytes, u64 disk_bytenr,
> +			u64 disk_num_bytes, u64 offset, unsigned long flags,
> +			int compress_type);
>  int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset,
>  			     u64 num_bytes, u64 ram_bytes, u64 disk_bytenr,
>  			     u64 disk_num_bytes, u64 offset, unsigned flags,
> -- 
> 2.39.2
>