Re: [PATCH v11 22/40] btrfs: split ordered extent when bio is sent

[Josef Bacik <josef@toxicpanda.com>]

On 12/21/20 10:49 PM, Naohiro Aota wrote:
> For a zone append write, the device decides the location the data is
> written to. Therefore we cannot ensure that two bios are written
> consecutively on the device. In order to ensure that a ordered extent maps
> to a contiguous region on disk, we need to maintain a "one bio == one
> ordered extent" rule.
> 
> This commit implements the splitting of an ordered extent and extent map
> on bio submission to adhere to the rule.
> 
> [testbot] made extract_ordered_extent static
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> ---
>   fs/btrfs/inode.c        | 89 +++++++++++++++++++++++++++++++++++++++++
>   fs/btrfs/ordered-data.c | 76 +++++++++++++++++++++++++++++++++++
>   fs/btrfs/ordered-data.h |  2 +
>   3 files changed, 167 insertions(+)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 37782b4cfd28..15e0c7714c7f 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2217,6 +2217,86 @@ static blk_status_t btrfs_submit_bio_start(struct inode *inode, struct bio *bio,
>   	return btrfs_csum_one_bio(BTRFS_I(inode), bio, 0, 0);
>   }
>   
> +static int extract_ordered_extent(struct inode *inode, struct bio *bio,
> +				  loff_t file_offset)
> +{
> +	struct btrfs_ordered_extent *ordered;
> +	struct extent_map *em = NULL, *em_new = NULL;
> +	struct extent_map_tree *em_tree = &BTRFS_I(inode)->extent_tree;
> +	u64 start = (u64)bio->bi_iter.bi_sector << SECTOR_SHIFT;
> +	u64 len = bio->bi_iter.bi_size;
> +	u64 end = start + len;
> +	u64 ordered_end;
> +	u64 pre, post;
> +	int ret = 0;
> +
> +	ordered = btrfs_lookup_ordered_extent(BTRFS_I(inode), file_offset);
> +	if (WARN_ON_ONCE(!ordered))
> +		return -EIO;
> +
> +	/* No need to split */
> +	if (ordered->disk_num_bytes == len)
> +		goto out;
> +
> +	/* We cannot split once end_bio'd ordered extent */
> +	if (WARN_ON_ONCE(ordered->bytes_left != ordered->disk_num_bytes)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* We cannot split a compressed ordered extent */
> +	if (WARN_ON_ONCE(ordered->disk_num_bytes != ordered->num_bytes)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* We cannot split a waited ordered extent */
> +	if (WARN_ON_ONCE(wq_has_sleeper(&ordered->wait))) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	ordered_end = ordered->disk_bytenr + ordered->disk_num_bytes;
> +	/* bio must be in one ordered extent */
> +	if (WARN_ON_ONCE(start < ordered->disk_bytenr || end > ordered_end)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* Checksum list should be empty */
> +	if (WARN_ON_ONCE(!list_empty(&ordered->list))) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	pre = start - ordered->disk_bytenr;
> +	post = ordered_end - end;
> +
> +	btrfs_split_ordered_extent(ordered, pre, post);
> +
> +	read_lock(&em_tree->lock);
> +	em = lookup_extent_mapping(em_tree, ordered->file_offset, len);
> +	if (!em) {
> +		read_unlock(&em_tree->lock);
> +		ret = -EIO;
> +		goto out;
> +	}
> +	read_unlock(&em_tree->lock);
> +
> +	ASSERT(!test_bit(EXTENT_FLAG_COMPRESSED, &em->flags));
> +	em_new = create_io_em(BTRFS_I(inode), em->start + pre, len,
> +			      em->start + pre, em->block_start + pre, len,
> +			      len, len, BTRFS_COMPRESS_NONE,
> +			      BTRFS_ORDERED_REGULAR);
> +	free_extent_map(em_new);
> +
> +out:
> +	free_extent_map(em);
> +	btrfs_put_ordered_extent(ordered);
> +
> +	return ret;
> +}
> +
>   /*
>    * extent_io.c submission hook. This does the right thing for csum calculation
>    * on write, or reading the csums from the tree before a read.
> @@ -2252,6 +2332,15 @@ blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
>   	if (btrfs_is_free_space_inode(BTRFS_I(inode)))
>   		metadata = BTRFS_WQ_ENDIO_FREE_SPACE;
>   
> +	if (bio_op(bio) == REQ_OP_ZONE_APPEND) {
> +		struct page *page = bio_first_bvec_all(bio)->bv_page;
> +		loff_t file_offset = page_offset(page);
> +
> +		ret = extract_ordered_extent(inode, bio, file_offset);
> +		if (ret)
> +			goto out;
> +	}
> +
>   	if (btrfs_op(bio) != BTRFS_MAP_WRITE) {
>   		ret = btrfs_bio_wq_end_io(fs_info, bio, metadata);
>   		if (ret)
> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
> index 79d366a36223..4f8f48e7a482 100644
> --- a/fs/btrfs/ordered-data.c
> +++ b/fs/btrfs/ordered-data.c
> @@ -898,6 +898,82 @@ void btrfs_lock_and_flush_ordered_range(struct btrfs_inode *inode, u64 start,
>   	}
>   }
>   
> +static void clone_ordered_extent(struct btrfs_ordered_extent *ordered, u64 pos,
> +				 u64 len)
> +{
> +	struct inode *inode = ordered->inode;
> +	u64 file_offset = ordered->file_offset + pos;
> +	u64 disk_bytenr = ordered->disk_bytenr + pos;
> +	u64 num_bytes = len;
> +	u64 disk_num_bytes = len;
> +	int type;
> +	unsigned long flags_masked =
> +		ordered->flags & ~(1 << BTRFS_ORDERED_DIRECT);
> +	int compress_type = ordered->compress_type;
> +	unsigned long weight;
> +
> +	weight = hweight_long(flags_masked);
> +	WARN_ON_ONCE(weight > 1);
> +	if (!weight)
> +		type = 0;
> +	else
> +		type = __ffs(flags_masked);
> +
> +	if (test_bit(BTRFS_ORDERED_COMPRESSED, &ordered->flags)) {
> +		WARN_ON_ONCE(1);
> +		btrfs_add_ordered_extent_compress(BTRFS_I(inode), file_offset,
> +						  disk_bytenr, num_bytes,
> +						  disk_num_bytes, type,
> +						  compress_type);
> +	} else if (test_bit(BTRFS_ORDERED_DIRECT, &ordered->flags)) {
> +		btrfs_add_ordered_extent_dio(BTRFS_I(inode), file_offset,
> +					     disk_bytenr, num_bytes,
> +					     disk_num_bytes, type);
> +	} else {
> +		btrfs_add_ordered_extent(BTRFS_I(inode), file_offset,
> +					 disk_bytenr, num_bytes, disk_num_bytes,
> +					 type);
> +	}
> +}

You're completely ignoring errors here which isn't ok.  Thanks,

Josef