Re: [PATCH 3/9] iomap: treat a write through cache the same as FUA

From: "Darrick J. Wong" <djwong@kernel.org>
To: Jens Axboe <axboe@kernel.dk>
Cc: io-uring@vger.kernel.org, linux-xfs@vger.kernel.org, hch@lst.de,
	andres@anarazel.de, david@fromorbit.com
Subject: Re: [PATCH 3/9] iomap: treat a write through cache the same as FUA
Date: Fri, 21 Jul 2023 09:25:53 -0700	[thread overview]
Message-ID: <20230721162553.GS11352@frogsfrogsfrogs> (raw)
In-Reply-To: <20230721161650.319414-4-axboe@kernel.dk>

On Fri, Jul 21, 2023 at 10:16:44AM -0600, Jens Axboe wrote:
> Whether we have a write back cache and are using FUA or don't have
> a write back cache at all is the same situation. Treat them the same.
> 
> This makes the IOMAP_DIO_WRITE_FUA name a bit misleading, as we have
> two cases that provide stable writes:
> 
> 1) Volatile write cache with FUA writes
> 2) Normal write without a volatile write cache
> 
> Rename that flag to IOMAP_DIO_STABLE_WRITE to make that clearer, and
> update some of the FUA comments as well.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  fs/iomap/direct-io.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index c654612b24e5..17b695b0e9d6 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -21,7 +21,7 @@
>   * iomap.h:
>   */
>  #define IOMAP_DIO_INLINE_COMP	(1 << 27)
> -#define IOMAP_DIO_WRITE_FUA	(1 << 28)
> +#define IOMAP_DIO_WRITE_THROUGH	(1 << 28)
>  #define IOMAP_DIO_NEED_SYNC	(1 << 29)
>  #define IOMAP_DIO_WRITE		(1 << 30)
>  #define IOMAP_DIO_DIRTY		(1 << 31)
> @@ -222,7 +222,7 @@ static void iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio,
>  /*
>   * Figure out the bio's operation flags from the dio request, the
>   * mapping, and whether or not we want FUA.  Note that we can end up
> - * clearing the WRITE_FUA flag in the dio request.
> + * clearing the WRITE_THROUGH flag in the dio request.
>   */
>  static inline blk_opf_t iomap_dio_bio_opflags(struct iomap_dio *dio,
>  		const struct iomap *iomap, bool use_fua)
> @@ -236,7 +236,7 @@ static inline blk_opf_t iomap_dio_bio_opflags(struct iomap_dio *dio,
>  	if (use_fua)
>  		opflags |= REQ_FUA;
>  	else
> -		dio->flags &= ~IOMAP_DIO_WRITE_FUA;
> +		dio->flags &= ~IOMAP_DIO_WRITE_THROUGH;
>  
>  	return opflags;
>  }
> @@ -276,11 +276,13 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
>  		 * Use a FUA write if we need datasync semantics, this is a pure
>  		 * data IO that doesn't require any metadata updates (including
>  		 * after IO completion such as unwritten extent conversion) and
> -		 * the underlying device supports FUA. This allows us to avoid
> -		 * cache flushes on IO completion.
> +		 * the underlying device either supports FUA or doesn't have
> +		 * a volatile write cache. This allows us to avoid cache flushes
> +		 * on IO completion.
>  		 */
>  		if (!(iomap->flags & (IOMAP_F_SHARED|IOMAP_F_DIRTY)) &&
> -		    (dio->flags & IOMAP_DIO_WRITE_FUA) && bdev_fua(iomap->bdev))
> +		    (dio->flags & IOMAP_DIO_WRITE_THROUGH) &&
> +		    (bdev_fua(iomap->bdev) || !bdev_write_cache(iomap->bdev)))
>  			use_fua = true;
>  	}
>  
> @@ -560,12 +562,15 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  
>  		       /*
>  			* For datasync only writes, we optimistically try
> -			* using FUA for this IO.  Any non-FUA write that
> -			* occurs will clear this flag, hence we know before
> -			* completion whether a cache flush is necessary.
> +			* using WRITE_THROUGH for this IO. Stable writes are

"...using WRITE_THROUGH for this IO.  This flag requires either FUA
writes through the device's write cache, or a normal write..."

> +			* either FUA with a write cache, or a normal write to
> +			* a device without a volatile write cache. For the
> +			* former, Any non-FUA write that occurs will clear this
> +			* flag, hence we know before completion whether a cache
> +			* flush is necessary.
>  			*/
>  			if (!(iocb->ki_flags & IOCB_SYNC))
> -				dio->flags |= IOMAP_DIO_WRITE_FUA;
> +				dio->flags |= IOMAP_DIO_WRITE_THROUGH;
>  		}
>  
>  		/*
> @@ -627,10 +632,10 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  		iomap_dio_set_error(dio, ret);
>  
>  	/*
> -	 * If all the writes we issued were FUA, we don't need to flush the
> +	 * If all the writes we issued were stable, we don't need to flush the

"If all the writes we issued were already written through to the media,
we don't need to flush..."

With those fixes,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  	 * cache on IO completion. Clear the sync flag for this case.
>  	 */
> -	if (dio->flags & IOMAP_DIO_WRITE_FUA)
> +	if (dio->flags & IOMAP_DIO_WRITE_THROUGH)
>  		dio->flags &= ~IOMAP_DIO_NEED_SYNC;
>  
>  	WRITE_ONCE(iocb->private, dio->submit.poll_bio);
> -- 
> 2.40.1
> 