Re: [PATCH 05/10] iomap: move common ioend code to ioend.c

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Dec 19, 2024 at 05:39:10PM +0000, Christoph Hellwig wrote:
> This code will be reused for direct I/O soon, so split it out of
> buffered-io.c.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

This appears to be a straight rearranagement, so
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/iomap/buffered-io.c | 135 +----------------------------------------
>  fs/iomap/internal.h    |   9 +++
>  fs/iomap/ioend.c       | 127 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 138 insertions(+), 133 deletions(-)
>  create mode 100644 fs/iomap/internal.h
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 0b68c9584a7f..06b90d859d74 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -12,17 +12,15 @@
>  #include <linux/buffer_head.h>
>  #include <linux/dax.h>
>  #include <linux/writeback.h>
> -#include <linux/list_sort.h>
>  #include <linux/swap.h>
>  #include <linux/bio.h>
>  #include <linux/sched/signal.h>
>  #include <linux/migrate.h>
> +#include "internal.h"
>  #include "trace.h"
>  
>  #include "../internal.h"
>  
> -#define IOEND_BATCH_SIZE	4096
> -
>  /*
>   * Structure allocated for each folio to track per-block uptodate, dirty state
>   * and I/O completions.
> @@ -40,9 +38,6 @@ struct iomap_folio_state {
>  	unsigned long		state[];
>  };
>  
> -struct bio_set iomap_ioend_bioset;
> -EXPORT_SYMBOL_GPL(iomap_ioend_bioset);
> -
>  static inline bool ifs_is_fully_uptodate(struct folio *folio,
>  		struct iomap_folio_state *ifs)
>  {
> @@ -1539,8 +1534,7 @@ static void iomap_finish_folio_write(struct inode *inode, struct folio *folio,
>   * state, release holds on bios, and finally free up memory.  Do not use the
>   * ioend after this.
>   */
> -static u32
> -iomap_finish_ioend_buffered(struct iomap_ioend *ioend)
> +u32 iomap_finish_ioend_buffered(struct iomap_ioend *ioend)
>  {
>  	struct inode *inode = ioend->io_inode;
>  	struct bio *bio = &ioend->io_bio;
> @@ -1567,123 +1561,6 @@ iomap_finish_ioend_buffered(struct iomap_ioend *ioend)
>  	return folio_count;
>  }
>  
> -static u32
> -iomap_finish_ioend(struct iomap_ioend *ioend, int error)
> -{
> -	if (ioend->io_parent) {
> -		struct bio *bio = &ioend->io_bio;
> -
> -		ioend = ioend->io_parent;
> -		bio_put(bio);
> -	}
> -
> -	if (error)
> -		cmpxchg(&ioend->io_error, 0, error);
> -
> -	if (!atomic_dec_and_test(&ioend->io_remaining))
> -		return 0;
> -	return iomap_finish_ioend_buffered(ioend);
> -}
> -
> -/*
> - * Ioend completion routine for merged bios. This can only be called from task
> - * contexts as merged ioends can be of unbound length. Hence we have to break up
> - * the writeback completions into manageable chunks to avoid long scheduler
> - * holdoffs. We aim to keep scheduler holdoffs down below 10ms so that we get
> - * good batch processing throughput without creating adverse scheduler latency
> - * conditions.
> - */
> -void
> -iomap_finish_ioends(struct iomap_ioend *ioend, int error)
> -{
> -	struct list_head tmp;
> -	u32 completions;
> -
> -	might_sleep();
> -
> -	list_replace_init(&ioend->io_list, &tmp);
> -	completions = iomap_finish_ioend(ioend, error);
> -
> -	while (!list_empty(&tmp)) {
> -		if (completions > IOEND_BATCH_SIZE * 8) {
> -			cond_resched();
> -			completions = 0;
> -		}
> -		ioend = list_first_entry(&tmp, struct iomap_ioend, io_list);
> -		list_del_init(&ioend->io_list);
> -		completions += iomap_finish_ioend(ioend, error);
> -	}
> -}
> -EXPORT_SYMBOL_GPL(iomap_finish_ioends);
> -
> -/*
> - * We can merge two adjacent ioends if they have the same set of work to do.
> - */
> -static bool
> -iomap_ioend_can_merge(struct iomap_ioend *ioend, struct iomap_ioend *next)
> -{
> -	if (ioend->io_bio.bi_status != next->io_bio.bi_status)
> -		return false;
> -	if (next->io_flags & IOMAP_IOEND_BOUNDARY)
> -		return false;
> -	if ((ioend->io_flags & IOMAP_IOEND_NOMERGE_FLAGS) !=
> -	    (next->io_flags & IOMAP_IOEND_NOMERGE_FLAGS))
> -		return false;
> -	if (ioend->io_offset + ioend->io_size != next->io_offset)
> -		return false;
> -	/*
> -	 * Do not merge physically discontiguous ioends. The filesystem
> -	 * completion functions will have to iterate the physical
> -	 * discontiguities even if we merge the ioends at a logical level, so
> -	 * we don't gain anything by merging physical discontiguities here.
> -	 *
> -	 * We cannot use bio->bi_iter.bi_sector here as it is modified during
> -	 * submission so does not point to the start sector of the bio at
> -	 * completion.
> -	 */
> -	if (ioend->io_sector + (ioend->io_size >> 9) != next->io_sector)
> -		return false;
> -	return true;
> -}
> -
> -void
> -iomap_ioend_try_merge(struct iomap_ioend *ioend, struct list_head *more_ioends)
> -{
> -	struct iomap_ioend *next;
> -
> -	INIT_LIST_HEAD(&ioend->io_list);
> -
> -	while ((next = list_first_entry_or_null(more_ioends, struct iomap_ioend,
> -			io_list))) {
> -		if (!iomap_ioend_can_merge(ioend, next))
> -			break;
> -		list_move_tail(&next->io_list, &ioend->io_list);
> -		ioend->io_size += next->io_size;
> -	}
> -}
> -EXPORT_SYMBOL_GPL(iomap_ioend_try_merge);
> -
> -static int
> -iomap_ioend_compare(void *priv, const struct list_head *a,
> -		const struct list_head *b)
> -{
> -	struct iomap_ioend *ia = container_of(a, struct iomap_ioend, io_list);
> -	struct iomap_ioend *ib = container_of(b, struct iomap_ioend, io_list);
> -
> -	if (ia->io_offset < ib->io_offset)
> -		return -1;
> -	if (ia->io_offset > ib->io_offset)
> -		return 1;
> -	return 0;
> -}
> -
> -void
> -iomap_sort_ioends(struct list_head *ioend_list)
> -{
> -	list_sort(NULL, ioend_list, iomap_ioend_compare);
> -}
> -EXPORT_SYMBOL_GPL(iomap_sort_ioends);
> -
>  static void iomap_writepage_end_bio(struct bio *bio)
>  {
>  	struct iomap_ioend *ioend = iomap_ioend_from_bio(bio);
> @@ -2033,11 +1910,3 @@ iomap_writepages(struct address_space *mapping, struct writeback_control *wbc,
>  	return iomap_submit_ioend(wpc, error);
>  }
>  EXPORT_SYMBOL_GPL(iomap_writepages);
> -
> -static int __init iomap_buffered_init(void)
> -{
> -	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
> -			   offsetof(struct iomap_ioend, io_bio),
> -			   BIOSET_NEED_BVECS);
> -}
> -fs_initcall(iomap_buffered_init);
> diff --git a/fs/iomap/internal.h b/fs/iomap/internal.h
> new file mode 100644
> index 000000000000..36d5c56e073e
> --- /dev/null
> +++ b/fs/iomap/internal.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _IOMAP_INTERNAL_H
> +#define _IOMAP_INTERNAL_H 1
> +
> +#define IOEND_BATCH_SIZE	4096
> +
> +u32 iomap_finish_ioend_buffered(struct iomap_ioend *ioend);
> +
> +#endif /* _IOMAP_INTERNAL_H */
> diff --git a/fs/iomap/ioend.c b/fs/iomap/ioend.c
> index 1b032323ee4e..b4f6dd9e319a 100644
> --- a/fs/iomap/ioend.c
> +++ b/fs/iomap/ioend.c
> @@ -3,6 +3,11 @@
>   * Copyright (c) 2024 Christoph Hellwig.
>   */
>  #include <linux/iomap.h>
> +#include <linux/list_sort.h>
> +#include "internal.h"
> +
> +struct bio_set iomap_ioend_bioset;
> +EXPORT_SYMBOL_GPL(iomap_ioend_bioset);
>  
>  struct iomap_ioend *iomap_init_ioend(struct inode *inode,
>  		struct bio *bio, loff_t file_offset, u16 ioend_flags)
> @@ -22,6 +27,120 @@ struct iomap_ioend *iomap_init_ioend(struct inode *inode,
>  }
>  EXPORT_SYMBOL_GPL(iomap_init_ioend);
>  
> +static u32 iomap_finish_ioend(struct iomap_ioend *ioend, int error)
> +{
> +	if (ioend->io_parent) {
> +		struct bio *bio = &ioend->io_bio;
> +
> +		ioend = ioend->io_parent;
> +		bio_put(bio);
> +	}
> +
> +	if (error)
> +		cmpxchg(&ioend->io_error, 0, error);
> +
> +	if (!atomic_dec_and_test(&ioend->io_remaining))
> +		return 0;
> +	return iomap_finish_ioend_buffered(ioend);
> +}
> +
> +/*
> + * Ioend completion routine for merged bios. This can only be called from task
> + * contexts as merged ioends can be of unbound length. Hence we have to break up
> + * the writeback completions into manageable chunks to avoid long scheduler
> + * holdoffs. We aim to keep scheduler holdoffs down below 10ms so that we get
> + * good batch processing throughput without creating adverse scheduler latency
> + * conditions.
> + */
> +void iomap_finish_ioends(struct iomap_ioend *ioend, int error)
> +{
> +	struct list_head tmp;
> +	u32 completions;
> +
> +	might_sleep();
> +
> +	list_replace_init(&ioend->io_list, &tmp);
> +	completions = iomap_finish_ioend(ioend, error);
> +
> +	while (!list_empty(&tmp)) {
> +		if (completions > IOEND_BATCH_SIZE * 8) {
> +			cond_resched();
> +			completions = 0;
> +		}
> +		ioend = list_first_entry(&tmp, struct iomap_ioend, io_list);
> +		list_del_init(&ioend->io_list);
> +		completions += iomap_finish_ioend(ioend, error);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(iomap_finish_ioends);
> +
> +/*
> + * We can merge two adjacent ioends if they have the same set of work to do.
> + */
> +static bool iomap_ioend_can_merge(struct iomap_ioend *ioend,
> +		struct iomap_ioend *next)
> +{
> +	if (ioend->io_bio.bi_status != next->io_bio.bi_status)
> +		return false;
> +	if (next->io_flags & IOMAP_IOEND_BOUNDARY)
> +		return false;
> +	if ((ioend->io_flags & IOMAP_IOEND_NOMERGE_FLAGS) !=
> +	    (next->io_flags & IOMAP_IOEND_NOMERGE_FLAGS))
> +		return false;
> +	if (ioend->io_offset + ioend->io_size != next->io_offset)
> +		return false;
> +	/*
> +	 * Do not merge physically discontiguous ioends. The filesystem
> +	 * completion functions will have to iterate the physical
> +	 * discontiguities even if we merge the ioends at a logical level, so
> +	 * we don't gain anything by merging physical discontiguities here.
> +	 *
> +	 * We cannot use bio->bi_iter.bi_sector here as it is modified during
> +	 * submission so does not point to the start sector of the bio at
> +	 * completion.
> +	 */
> +	if (ioend->io_sector + (ioend->io_size >> SECTOR_SHIFT) !=
> +	    next->io_sector)
> +		return false;
> +	return true;
> +}
> +
> +void iomap_ioend_try_merge(struct iomap_ioend *ioend,
> +		struct list_head *more_ioends)
> +{
> +	struct iomap_ioend *next;
> +
> +	INIT_LIST_HEAD(&ioend->io_list);
> +
> +	while ((next = list_first_entry_or_null(more_ioends, struct iomap_ioend,
> +			io_list))) {
> +		if (!iomap_ioend_can_merge(ioend, next))
> +			break;
> +		list_move_tail(&next->io_list, &ioend->io_list);
> +		ioend->io_size += next->io_size;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(iomap_ioend_try_merge);
> +
> +static int iomap_ioend_compare(void *priv, const struct list_head *a,
> +		const struct list_head *b)
> +{
> +	struct iomap_ioend *ia = container_of(a, struct iomap_ioend, io_list);
> +	struct iomap_ioend *ib = container_of(b, struct iomap_ioend, io_list);
> +
> +	if (ia->io_offset < ib->io_offset)
> +		return -1;
> +	if (ia->io_offset > ib->io_offset)
> +		return 1;
> +	return 0;
> +}
> +
> +void iomap_sort_ioends(struct list_head *ioend_list)
> +{
> +	list_sort(NULL, ioend_list, iomap_ioend_compare);
> +}
> +EXPORT_SYMBOL_GPL(iomap_sort_ioends);
> +
>  /*
>   * Split up to the first @max_len bytes from @ioend if the ioend covers more
>   * than @max_len bytes.
> @@ -84,3 +203,11 @@ struct iomap_ioend *iomap_split_ioend(struct iomap_ioend *ioend,
>  	return split_ioend;
>  }
>  EXPORT_SYMBOL_GPL(iomap_split_ioend);
> +
> +static int __init iomap_ioend_init(void)
> +{
> +	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
> +			   offsetof(struct iomap_ioend, io_bio),
> +			   BIOSET_NEED_BVECS);
> +}
> +fs_initcall(iomap_ioend_init);
> -- 
> 2.45.2
> 
>