Re: [PATCH 3/5] iomap: rework REQ_FUA selection

[Jan Kara <jack@suse.cz>]

On Wed 12-11-25 08:21:27, Christoph Hellwig wrote:
> The way how iomap_dio_can_use_fua and the caller is structured is
> a bit confusing, as the main guarding condition is hidden in the
> helper, and the secondary conditions are split between caller and
> callee.
> 
> Refactor the code, so that there is a main IOMAP_DIO_WRITE_THROUGH
> guard in iomap_dio_bio_iter, which is directly tied to clearing it
> when not supported, and a helper that just checks if the I/O is a
> pure overwrite.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

One nit below but feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

> -static inline bool iomap_dio_can_use_fua(const struct iomap *iomap,
> -		struct iomap_dio *dio)
> +static inline bool iomap_dio_is_overwrite(const struct iomap *iomap)
>  {
> -	if (iomap->flags & (IOMAP_F_SHARED | IOMAP_F_DIRTY))
> +	if (iomap->type != IOMAP_MAPPED)
>  		return false;
> -	if (!(dio->flags & IOMAP_DIO_WRITE_THROUGH))
> -		return false;
> -	return !bdev_write_cache(iomap->bdev) || bdev_fua(iomap->bdev);
> +	return !(iomap->flags & (IOMAP_F_NEW | IOMAP_F_SHARED));
>  }

I'm a bit puzzled why did you leave IOMAP_F_DIRTY check in the caller.
Because that means we need metadata update to make the extent stable as the
comment before this function states...

								Honza

>  static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
> @@ -355,12 +349,22 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
>  
>  		if (iomap->flags & IOMAP_F_NEW)
>  			need_zeroout = true;
> -		else if (iomap->type == IOMAP_MAPPED &&
> -			 iomap_dio_can_use_fua(iomap, dio))
> -			bio_opf |= REQ_FUA;
>  
> -		if (!(bio_opf & REQ_FUA))
> -			dio->flags &= ~IOMAP_DIO_WRITE_THROUGH;
> +		/*
> +		 * Use a FUA write if we need datasync semantics and this is a
> +		 * pure overwrite that doesn't require any metadata updates.
> +		 *
> +		 * This allows us to avoid cache flushes on I/O completion.
> +		 */
> +		if (dio->flags & IOMAP_DIO_WRITE_THROUGH) {
> +			if (iomap_dio_is_overwrite(iomap) &&
> +			    !(iomap->flags & IOMAP_F_DIRTY) &&
> +			    (!bdev_write_cache(iomap->bdev) ||
> +			     bdev_fua(iomap->bdev)))
> +				bio_opf |= REQ_FUA;
> +			else
> +				dio->flags &= ~IOMAP_DIO_WRITE_THROUGH;
> +		}
>  	} else {
>  		bio_opf |= REQ_OP_READ;
>  	}
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR