On 27.01.21 г. 15:57 ч., Michal Rostecki wrote:
> From: Michal Rostecki <mroste...@suse.com>
>
> Before this change, the btrfs_get_io_geometry() function was calling
> btrfs_get_chunk_map() to get the extent mapping, necessary for
> calculating the I/O geometry. It was using that extent mapping only
> internally and freeing the pointer after its execution.
>
> That resulted in calling btrfs_get_chunk_map() de facto twice by the
> __btrfs_map_block() function. It was calling btrfs_get_io_geometry()
> first and then calling btrfs_get_chunk_map() directly to get the extent
> mapping, used by the rest of the function.
>
> This change fixes that by passing the extent mapping to the
> btrfs_get_io_geometry() function as an argument.
>
> v2:
> When btrfs_get_chunk_map() returns an error in btrfs_submit_direct():
> - Use errno_to_blk_status(PTR_ERR(em)) as the status
> - Set em to NULL
>
> Signed-off-by: Michal Rostecki <mroste...@suse.com>
> ---
> fs/btrfs/inode.c | 38 +++++++++++++++++++++++++++++---------
> fs/btrfs/volumes.c | 39 ++++++++++++++++-----------------------
> fs/btrfs/volumes.h | 5 +++--
> 3 files changed, 48 insertions(+), 34 deletions(-)
So this adds more code but for what benefit? In your reply to Filipe you
said you didn't observe this being a performance-affecting change so
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 0dbe1aaa0b71..e2ee3a9c1140 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2183,9 +2183,10 @@ int btrfs_bio_fits_in_stripe(struct page *page, size_t
> size, struct bio *bio,
> struct inode *inode = page->mapping->host;
> struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> u64 logical = bio->bi_iter.bi_sector << 9;
> + struct extent_map *em;
> u64 length = 0;
> u64 map_length;
> - int ret;
> + int ret = 0;
> struct btrfs_io_geometry geom;
>
> if (bio_flags & EXTENT_BIO_COMPRESSED)
> @@ -2193,14 +2194,21 @@ int btrfs_bio_fits_in_stripe(struct page *page,
> size_t size, struct bio *bio,
>
> length = bio->bi_iter.bi_size;
> map_length = length;
> - ret = btrfs_get_io_geometry(fs_info, btrfs_op(bio), logical, map_length,
> - &geom);
> + em = btrfs_get_chunk_map(fs_info, logical, map_length);
> + if (IS_ERR(em))
> + return PTR_ERR(em);
> + ret = btrfs_get_io_geometry(fs_info, em, btrfs_op(bio), logical,
> + map_length, &geom);
> if (ret < 0)
> - return ret;
> + goto out;
>
> - if (geom.len < length + size)
> - return 1;
> - return 0;
> + if (geom.len < length + size) {
> + ret = 1;
> + goto out;
> + }
this could be simply
if (geom.len <length + size)
ret = 1;
Not need for the extra 'goto out'
> +out:
> + free_extent_map(em);
> + return ret;
> }
>
> /*
> @@ -7941,10 +7949,12 @@ static blk_qc_t btrfs_submit_direct(struct inode
> *inode, struct iomap *iomap,
> u64 submit_len;
> int clone_offset = 0;
> int clone_len;
> + int logical;
> int ret;
> blk_status_t status;
> struct btrfs_io_geometry geom;
> struct btrfs_dio_data *dio_data = iomap->private;
> + struct extent_map *em;
>
> dip = btrfs_create_dio_private(dio_bio, inode, file_offset);
> if (!dip) {
> @@ -7970,11 +7980,18 @@ static blk_qc_t btrfs_submit_direct(struct inode
> *inode, struct iomap *iomap,
> }
>
> start_sector = dio_bio->bi_iter.bi_sector;
> + logical = start_sector << 9;
> submit_len = dio_bio->bi_iter.bi_size;
>
> do {
> - ret = btrfs_get_io_geometry(fs_info, btrfs_op(dio_bio),
> - start_sector << 9, submit_len,
> + em = btrfs_get_chunk_map(fs_info, logical, submit_len);
> + if (IS_ERR(em)) {
> + status = errno_to_blk_status(PTR_ERR(em));
> + em = NULL;
> + goto out_err;
> + }
> + ret = btrfs_get_io_geometry(fs_info, em, btrfs_op(dio_bio),
> + logical, submit_len,
> &geom);
> if (ret) {
> status = errno_to_blk_status(ret);
> @@ -8030,12 +8047,15 @@ static blk_qc_t btrfs_submit_direct(struct inode
> *inode, struct iomap *iomap,
> clone_offset += clone_len;
> start_sector += clone_len >> 9;
> file_offset += clone_len;
> +
> + free_extent_map(em);
> } while (submit_len > 0);
> return BLK_QC_T_NONE;
>
> out_err:
> dip->dio_bio->bi_status = status;
> btrfs_dio_private_put(dip);
> + free_extent_map(em);
> return BLK_QC_T_NONE;
> }
For example in this function you increase complexity by having to deal
with free_extent_map as well so I'm not sure this is a net-win.
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index a8ec8539cd8d..4c753b17c0a2 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -5940,23 +5940,24 @@ static bool need_full_stripe(enum btrfs_map_op op)
> }
>
> /*
> - * btrfs_get_io_geometry - calculates the geomery of a particular (address,
> len)
> + * btrfs_get_io_geometry - calculates the geometry of a particular (address,
> len)
> * tuple. This information is used to calculate how big a
> * particular bio can get before it straddles a stripe.
> *
> - * @fs_info - the filesystem
> - * @logical - address that we want to figure out the geometry of
> - * @len - the length of IO we are going to perform, starting at
> @logical
> - * @op - type of operation - write or read
> - * @io_geom - pointer used to return values
> + * @fs_info: the filesystem
> + * @em: mapping containing the logical extent
> + * @op: type of operation - write or read
> + * @logical: address that we want to figure out the geometry of
> + * @len: the length of IO we are going to perform, starting at @logical
> + * @io_geom: pointer used to return values
> *
> * Returns < 0 in case a chunk for the given logical address cannot be found,
> * usually shouldn't happen unless @logical is corrupted, 0 otherwise.
> */
> -int btrfs_get_io_geometry(struct btrfs_fs_info *fs_info, enum btrfs_map_op
> op,
> - u64 logical, u64 len, struct btrfs_io_geometry *io_geom)
> +int btrfs_get_io_geometry(struct btrfs_fs_info *fs_info, struct extent_map
> *em,
> + enum btrfs_map_op op, u64 logical, u64 len,
> + struct btrfs_io_geometry *io_geom)
> {
> - struct extent_map *em;
> struct map_lookup *map;
> u64 offset;
> u64 stripe_offset;
> @@ -5964,14 +5965,9 @@ int btrfs_get_io_geometry(struct btrfs_fs_info
> *fs_info, enum btrfs_map_op op,
> u64 stripe_len;
> u64 raid56_full_stripe_start = (u64)-1;
> int data_stripes;
> - int ret = 0;
>
> ASSERT(op != BTRFS_MAP_DISCARD);
>
> - em = btrfs_get_chunk_map(fs_info, logical, len);
> - if (IS_ERR(em))
> - return PTR_ERR(em);
> -
> map = em->map_lookup;
> /* Offset of this logical address in the chunk */
> offset = logical - em->start;
> @@ -5985,8 +5981,7 @@ int btrfs_get_io_geometry(struct btrfs_fs_info
> *fs_info, enum btrfs_map_op op,
> btrfs_crit(fs_info,
> "stripe math has gone wrong, stripe_offset=%llu offset=%llu start=%llu
> logical=%llu stripe_len=%llu",
> stripe_offset, offset, em->start, logical, stripe_len);
> - ret = -EINVAL;
> - goto out;
> + return -EINVAL;
> }
>
> /* stripe_offset is the offset of this block in its stripe */
> @@ -6033,10 +6028,7 @@ int btrfs_get_io_geometry(struct btrfs_fs_info
> *fs_info, enum btrfs_map_op op,
> io_geom->stripe_offset = stripe_offset;
> io_geom->raid56_stripe_offset = raid56_full_stripe_start;
>
> -out:
> - /* once for us */
> - free_extent_map(em);
> - return ret;
> + return 0;
> }
Effectively, what's going on is you are pulling complexity from
btrfs_get_io_geometry and putting it into its 2 callers which is IMO bad.
So unless you can demonstrate this is indeed affecting performance I'd
be inclined to NAK it.
