Re: [PATCH v2] dm: Preserve the order of REQ_PREFLUSH writes

[Damien Le Moal <dlemoal@kernel.org>]

On 9/11/25 02:27, Bart Van Assche wrote:
> The dm core splits REQ_PREFLUSH bios that have data into two bios.
> First, a REQ_PREFLUSH bio with no data is submitted to all underlying
> dm devices. Next, the REQ_PREFLUSH flag is cleared and the same bio is
> resubmitted. This approach is essential if there are multiple underlying
> devices to provide correct REQ_PREFLUSH semantics.

The same can be said if there is a single underlying device.

> Splitting a bio into an empty flush bio and a non-flush data bio is
> not necessary if there is only a single underlying device. Hence this
> patch that does not split REQ_PREFLUSH bios if there is only one
> underlying device.

Why is it not necessary ? You are not giving any justification/explanation of
that here, which makes your patch impossible to review.

I suspect it is because this will be handled by the flush machinery during the
submit_bio() to the underlying device ? But if that is the case, why is it a
problem with multiple devices ? The same block layer flush machinery will handle
that preflush write for all devices.

> 
> This patch preserves the order of REQ_PREFLUSH writes if there is only
> one underlying device and if one or more write bios have been queued
> past the REQ_PREFLUSH bio before the REQ_PREFLUSH bio is processed.
> 
> Cc: Mike Snitzer <snitzer@kernel.org>
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> 
> Changes compared to v1:
>  - Made the patch description more detailed.
>  - Removed the reference to write pipelining from the patch description.
> 
>  drivers/md/dm.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 66dd5f6ce778..d0791eef21f7 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -490,9 +490,13 @@ u64 dm_start_time_ns_from_clone(struct bio *bio)
>  }
>  EXPORT_SYMBOL_GPL(dm_start_time_ns_from_clone);
>  
> -static inline bool bio_is_flush_with_data(struct bio *bio)
> +static inline bool bio_is_flush_with_data(struct mapped_device *md,
> +					  struct bio *bio)
>  {
> -	return ((bio->bi_opf & REQ_PREFLUSH) && bio->bi_iter.bi_size);
> +	guard(rcu)();
> +
> +	return bio->bi_opf & REQ_PREFLUSH && bio->bi_iter.bi_size &&
> +	       ((struct dm_table *)rcu_dereference(md->map))->num_targets > 1;
>  }
>  
>  static inline unsigned int dm_io_sectors(struct dm_io *io, struct bio *bio)
> @@ -501,7 +505,7 @@ static inline unsigned int dm_io_sectors(struct dm_io *io, struct bio *bio)
>  	 * If REQ_PREFLUSH set, don't account payload, it will be
>  	 * submitted (and accounted) after this flush completes.
>  	 */
> -	if (bio_is_flush_with_data(bio))
> +	if (bio_is_flush_with_data(io->md, bio))
>  		return 0;
>  	if (unlikely(dm_io_flagged(io, DM_IO_WAS_SPLIT)))
>  		return io->sectors;
> @@ -976,7 +980,7 @@ static void __dm_io_complete(struct dm_io *io, bool first_stage)
>  	if (requeued)
>  		return;
>  
> -	if (bio_is_flush_with_data(bio)) {
> +	if (bio_is_flush_with_data(md, bio)) {
>  		/*
>  		 * Preflush done for flush with data, reissue
>  		 * without REQ_PREFLUSH.
> @@ -1715,7 +1719,7 @@ static void dm_queue_poll_io(struct bio *bio, struct dm_io *io)
>  }
>  
>  /*
> - * Select the correct strategy for processing a non-flush bio.
> + * Select the correct strategy for processing a bio.
>   */
>  static blk_status_t __split_and_process_bio(struct clone_info *ci)
>  {
> @@ -1996,7 +2000,7 @@ static void dm_split_and_process_bio(struct mapped_device *md,
>  	}
>  	init_clone_info(&ci, io, map, bio, is_abnormal);
>  
> -	if (bio->bi_opf & REQ_PREFLUSH) {
> +	if (bio->bi_opf & REQ_PREFLUSH && ci.map->num_targets > 1) {
>  		__send_empty_flush(&ci);
>  		/* dm_io_complete submits any data associated with flush */
>  		goto out;


-- 
Damien Le Moal
Western Digital Research