| Message ID | 20230721161650.319414-4-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Improve async iomap DIO performance | expand |
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 >
On 7/21/23 10:25?AM, Darrick J. Wong wrote: >> @@ -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..." > >> @@ -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> If you're queueing up this series, could you just make those two edits while applying? I don't want to spam resend with just a comment change, at least if I can avoid it...
On Fri, Jul 21, 2023 at 10:27:16AM -0600, Jens Axboe wrote: > On 7/21/23 10:25?AM, Darrick J. Wong wrote: > >> @@ -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..." > > > >> @@ -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> > > If you're queueing up this series, could you just make those two edits > while applying? I don't want to spam resend with just a comment change, > at least if I can avoid it... How about pushing the updated branch, tagging it with the cover letter as the message, and sending me a pull request? Linus has been very receptive to preserving cover letters this way. --D > -- > Jens Axboe >
On 7/21/23 10:47 AM, Darrick J. Wong wrote: > On Fri, Jul 21, 2023 at 10:27:16AM -0600, Jens Axboe wrote: >> On 7/21/23 10:25?AM, Darrick J. Wong wrote: >>>> @@ -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..." >>> >>>> @@ -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> >> >> If you're queueing up this series, could you just make those two edits >> while applying? I don't want to spam resend with just a comment change, >> at least if I can avoid it... > > How about pushing the updated branch, tagging it with the cover letter > as the message, and sending me a pull request? Linus has been very > receptive to preserving cover letters this way. OK, will do.
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 + * 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 * 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);