public inbox for [email protected]
 help / color / mirror / Atom feed
From: "Darrick J. Wong" <[email protected]>
To: Jens Axboe <[email protected]>
Cc: [email protected], [email protected], [email protected],
	[email protected], [email protected]
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: <[email protected]>

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 <[email protected]>
> Signed-off-by: Jens Axboe <[email protected]>
> ---
>  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 <[email protected]>

--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
> 

  reply	other threads:[~2023-07-21 16:29 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-07-21 16:16 [PATCHSET v5 0/9] Improve async iomap DIO performance Jens Axboe
2023-07-21 16:16 ` [PATCH 1/9] iomap: cleanup up iomap_dio_bio_end_io() Jens Axboe
2023-07-21 16:16 ` [PATCH 2/9] iomap: add IOMAP_DIO_INLINE_COMP Jens Axboe
2023-07-21 16:16 ` [PATCH 3/9] iomap: treat a write through cache the same as FUA Jens Axboe
2023-07-21 16:25   ` Darrick J. Wong [this message]
2023-07-21 16:27     ` Jens Axboe
2023-07-21 16:47       ` Darrick J. Wong
2023-07-21 16:52         ` Jens Axboe
2023-07-21 16:16 ` [PATCH 4/9] iomap: completed polled IO inline Jens Axboe
2023-07-21 16:16 ` [PATCH 5/9] iomap: only set iocb->private for polled bio Jens Axboe
2023-07-21 16:16 ` [PATCH 6/9] fs: add IOCB flags related to passing back dio completions Jens Axboe
2023-07-21 16:28   ` Darrick J. Wong
2023-07-21 16:30     ` Jens Axboe
2023-07-21 16:43       ` Jens Axboe
2023-07-21 16:16 ` [PATCH 7/9] io_uring/rw: add write support for IOCB_DIO_CALLER_COMP Jens Axboe
2023-07-21 16:29   ` Darrick J. Wong
2023-07-21 16:16 ` [PATCH 8/9] iomap: support IOCB_DIO_CALLER_COMP Jens Axboe
2023-07-21 16:16 ` [PATCH 9/9] iomap: use an unsigned type for IOMAP_DIO_* defines Jens Axboe
2023-07-21 16:29   ` Darrick J. Wong
2023-07-24 16:36   ` Christoph Hellwig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230721162553.GS11352@frogsfrogsfrogs \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox