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

[Jan Kara <jack@suse.cz>]

On Thu 13-11-25 18:06:28, 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 iomap_dio_bio_iter itself tracks if a write
> might need metadata updates based on the iomap type and flags, and
> then have a condition based on that to use the FUA flag.
> 
> Note that this also moves the REQ_OP_WRITE assignment to the end of
> the branch to improve readability a bit.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/iomap/direct-io.c | 78 +++++++++++++++++++++++++++-----------------
>  1 file changed, 48 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index 765ab6dd6637..fb2d83f640ef 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -287,23 +287,6 @@ static int iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio,
>  	return 0;
>  }
>  
> -/*
> - * Use a FUA write if we need datasync semantics and this is a pure data I/O
> - * that doesn't require any metadata updates (including after I/O completion
> - * such as unwritten extent conversion) and the underlying device either
> - * doesn't have a volatile write cache or supports FUA.
> - * This allows us to avoid cache flushes on I/O completion.
> - */
> -static inline bool iomap_dio_can_use_fua(const struct iomap *iomap,
> -		struct iomap_dio *dio)
> -{
> -	if (iomap->flags & (IOMAP_F_SHARED | IOMAP_F_DIRTY))
> -		return false;
> -	if (!(dio->flags & IOMAP_DIO_WRITE_THROUGH))
> -		return false;
> -	return !bdev_write_cache(iomap->bdev) || bdev_fua(iomap->bdev);
> -}
> -
>  static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
>  {
>  	const struct iomap *iomap = &iter->iomap;
> @@ -332,7 +315,24 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
>  		return -EINVAL;
>  
>  	if (dio->flags & IOMAP_DIO_WRITE) {
> -		bio_opf |= REQ_OP_WRITE;
> +		bool need_completion_work = true;
> +
> +		switch (iomap->type) {
> +		case IOMAP_MAPPED:
> +			/*
> +			 * Directly mapped I/O does not inherently need to do
> +			 * work at I/O completion time.  But there are various
> +			 * cases below where this will get set again.
> +			 */
> +			need_completion_work = false;
> +			break;
> +		case IOMAP_UNWRITTEN:
> +			dio->flags |= IOMAP_DIO_UNWRITTEN;
> +			need_zeroout = true;
> +			break;
> +		default:
> +			break;
> +		}
>  
>  		if (iomap->flags & IOMAP_F_ATOMIC_BIO) {
>  			/*
> @@ -345,22 +345,40 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
>  			bio_opf |= REQ_ATOMIC;
>  		}
>  
> -		if (iomap->type == IOMAP_UNWRITTEN) {
> -			dio->flags |= IOMAP_DIO_UNWRITTEN;
> -			need_zeroout = true;
> -		}
> -
> -		if (iomap->flags & IOMAP_F_SHARED)
> +		if (iomap->flags & IOMAP_F_SHARED) {
> +			/*
> +			 * Unsharing of needs to update metadata at I/O
> +			 * completion time.
> +			 */
> +			need_completion_work = true;
>  			dio->flags |= IOMAP_DIO_COW;
> +		}
>  
> -		if (iomap->flags & IOMAP_F_NEW)
> +		if (iomap->flags & IOMAP_F_NEW) {
> +			/*
> +			 * Newly allocated blocks might need recording in
> +			 * metadata at I/O completion time.
> +			 */
> +			need_completion_work = true;
>  			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 (!need_completion_work &&
> +			    !(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;
> +		}
> +		bio_opf |= REQ_OP_WRITE;
>  	} else {
>  		bio_opf |= REQ_OP_READ;
>  	}
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR