Re: [PATCH 10/26] dm: Use the block layer zone append emulation

[Mike Snitzer <snitzer@kernel.org>]

On Fri, Feb 02 2024 at  2:30P -0500,
Damien Le Moal <dlemoal@kernel.org> wrote:

> For targets requiring zone append operation emulation with regular
> writes (e.g. dm-crypt), we can use the block layer emulation provided by
> zone write plugging. Remove DM implemented zone append emulation and
> enable the block layer one.
> 
> This is done by setting the max_zone_append_sectors limit of the
> mapped device queue to 0 for mapped devices that have a target table
> that cannot support native zone append operations. These includes
> mixed zoned and non-zoned targets, or targets that explicitly requested
> emulation of zone append (e.g. dm-crypt). For these mapped devices, the
> new field emulate_zone_append is set to true. dm_split_and_process_bio()
> is modified to call blk_zone_write_plug_bio() for such device to let the
> block layer transform zone append operations into regular writes. This
> is done after ensuring that the submitted BIO is split if it straddles
> zone boundaries.
> 
> dm_revalidate_zones() is also modified to use the block layer provided
> function blk_revalidate_disk_zones() so that all zone resources needed
> for zone append emulation are allocated and initialized by the block
> layer without DM core needing to do anything. Since the device table is
> not yet live when dm_revalidate_zones() is executed, enabling the use of
> blk_revalidate_disk_zones() requires adding a pointer to the device
> table in struct mapped_device. This avoids errors in
> dm_blk_report_zones() trying to get the table with dm_get_live_table().
> The mapped device table pointer is set to the table passed as argument
> to dm_revalidate_zones() before calling blk_revalidate_disk_zones() and
> reset to NULL after this function returns to restore the live table
> handling for user call of report zones.
> 
> All the code related to zone append emulation is removed from
> dm-zone.c. This leads to simplifications of the functions __map_bio()
> and dm_zone_endio(). This later function now only needs to deal with
> completions of real zone append operations for targets that support it.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Love the overall improvement to the DM core code and the broader block
layer by switching to this bio-based ZWP approach.

Reviewed-by: Mike Snitzer <snitzer@kernel.org>

But one incremental suggestion inlined below.

> ---
>  drivers/md/dm-core.h |  11 +-
>  drivers/md/dm-zone.c | 470 ++++---------------------------------------
>  drivers/md/dm.c      |  44 ++--
>  drivers/md/dm.h      |   7 -
>  4 files changed, 68 insertions(+), 464 deletions(-)
> 

<snip>

> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 8dcabf84d866..92ce3b2eb4ae 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1419,25 +1419,12 @@ static void __map_bio(struct bio *clone)
>  		down(&md->swap_bios_semaphore);
>  	}
>  
> -	if (static_branch_unlikely(&zoned_enabled)) {
> -		/*
> -		 * Check if the IO needs a special mapping due to zone append
> -		 * emulation on zoned target. In this case, dm_zone_map_bio()
> -		 * calls the target map operation.
> -		 */
> -		if (unlikely(dm_emulate_zone_append(md)))
> -			r = dm_zone_map_bio(tio);
> -		else
> -			goto do_map;
> -	} else {
> -do_map:
> -		if (likely(ti->type->map == linear_map))
> -			r = linear_map(ti, clone);
> -		else if (ti->type->map == stripe_map)
> -			r = stripe_map(ti, clone);
> -		else
> -			r = ti->type->map(ti, clone);
> -	}
> +	if (likely(ti->type->map == linear_map))
> +		r = linear_map(ti, clone);
> +	else if (ti->type->map == stripe_map)
> +		r = stripe_map(ti, clone);
> +	else
> +		r = ti->type->map(ti, clone);
>  
>  	switch (r) {
>  	case DM_MAPIO_SUBMITTED:
> @@ -1774,19 +1761,33 @@ static void dm_split_and_process_bio(struct mapped_device *md,
>  	struct clone_info ci;
>  	struct dm_io *io;
>  	blk_status_t error = BLK_STS_OK;
> -	bool is_abnormal;
> +	bool is_abnormal, need_split;
>  
>  	is_abnormal = is_abnormal_io(bio);
> -	if (unlikely(is_abnormal)) {
> +	if (likely(!md->emulate_zone_append))
> +		need_split = is_abnormal;
> +	else
> +		need_split = is_abnormal || bio_straddle_zones(bio);
> +	if (unlikely(need_split)) {
>  		/*
>  		 * Use bio_split_to_limits() for abnormal IO (e.g. discard, etc)
>  		 * otherwise associated queue_limits won't be imposed.
> +		 * Also split the BIO for mapped devices needing zone append
> +		 * emulation to ensure that the BIO does not cross zone
> +		 * boundaries.
>  		 */
>  		bio = bio_split_to_limits(bio);
>  		if (!bio)
>  			return;
>  	}
>  
> +	/*
> +	 * Use the block layer zone write plugging for mapped devices that
> +	 * need zone append emulation (e.g. dm-crypt).
> +	 */
> +	if (md->emulate_zone_append && blk_zone_write_plug_bio(bio, 0))
> +		return;
> +
>  	/* Only support nowait for normal IO */
>  	if (unlikely(bio->bi_opf & REQ_NOWAIT) && !is_abnormal) {
>  		io = alloc_io(md, bio, GFP_NOWAIT);

Would prefer to see this incremental change included from the start:

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 92ce3b2eb4ae..1fd9bbf35db3 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1763,11 +1763,10 @@ static void dm_split_and_process_bio(struct mapped_device *md,
 	blk_status_t error = BLK_STS_OK;
 	bool is_abnormal, need_split;
 
-	is_abnormal = is_abnormal_io(bio);
-	if (likely(!md->emulate_zone_append))
-		need_split = is_abnormal;
-	else
+	need_split = is_abnormal = is_abnormal_io(bio);
+	if (static_branch_unlikely(&zoned_enabled) && unlikely(md->emulate_zone_append))
 		need_split = is_abnormal || bio_straddle_zones(bio);
+
 	if (unlikely(need_split)) {
 		/*
 		 * Use bio_split_to_limits() for abnormal IO (e.g. discard, etc)
@@ -1781,12 +1780,14 @@ static void dm_split_and_process_bio(struct mapped_device *md,
 			return;
 	}
 
-	/*
-	 * Use the block layer zone write plugging for mapped devices that
-	 * need zone append emulation (e.g. dm-crypt).
-	 */
-	if (md->emulate_zone_append && blk_zone_write_plug_bio(bio, 0))
-		return;
+	if (static_branch_unlikely(&zoned_enabled)) {
+		/*
+		 * Use the block layer zone write plugging for mapped devices that
+		 * need zone append emulation (e.g. dm-crypt).
+		 */
+		if (unlikely(md->emulate_zone_append) && blk_zone_write_plug_bio(bio, 0))
+			return;
+	}
 
 	/* Only support nowait for normal IO */
 	if (unlikely(bio->bi_opf & REQ_NOWAIT) && !is_abnormal) {