| Message ID | 20230720181310.71589-4-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Improve async iomap DIO performance | expand |
On Thu, Jul 20, 2023 at 12:13:05PM -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. I would have preferred IOMAP_DIO_WRITE_THROUGH, STABLE_WRITES is a flag we use in file systems and the page cache for cases where the page can't be touched before writeback has completed, e.g. QUEUE_FLAG_STABLE_WRITES and SB_I_STABLE_WRITES. Otherwise this looks good to me: Reviewed-by: Christoph Hellwig <hch@lst.de>
On 7/21/23 12:15?AM, Christoph Hellwig wrote: > On Thu, Jul 20, 2023 at 12:13:05PM -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. > > I would have preferred IOMAP_DIO_WRITE_THROUGH, STABLE_WRITES is a flag > we use in file systems and the page cache for cases where the page > can't be touched before writeback has completed, e.g. > QUEUE_FLAG_STABLE_WRITES and SB_I_STABLE_WRITES. Good point, it does confuse terminology with stable pages for writes. I'll change it to WRITE_THROUGH, that is more descriptive for this case.
On Fri, Jul 21, 2023 at 08:04:19AM -0600, Jens Axboe wrote: > On 7/21/23 12:15?AM, Christoph Hellwig wrote: > > On Thu, Jul 20, 2023 at 12:13:05PM -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. > > > > I would have preferred IOMAP_DIO_WRITE_THROUGH, STABLE_WRITES is a flag > > we use in file systems and the page cache for cases where the page > > can't be touched before writeback has completed, e.g. > > QUEUE_FLAG_STABLE_WRITES and SB_I_STABLE_WRITES. > > Good point, it does confuse terminology with stable pages for writes. > I'll change it to WRITE_THROUGH, that is more descriptive for this case. +1 for the name change. With IOMAP_DIO_WRITE_THROUGH, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D Separately: At some point, the definition for IOMAP_DIO_DIRTY needs to grow a type annotation: #define IOMAP_DIO_DIRTY (1U << 31) due (apparently) triggering UBSAN because "1" on its own is a signed constant. If this series goes through my tree then I'll add a trivial patch fixing all of this ... unless you'd rather do it yourself as a patch 9? --D > -- > Jens Axboe >
On 7/21/23 9:55?AM, Darrick J. Wong wrote: > On Fri, Jul 21, 2023 at 08:04:19AM -0600, Jens Axboe wrote: >> On 7/21/23 12:15?AM, Christoph Hellwig wrote: >>> On Thu, Jul 20, 2023 at 12:13:05PM -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. >>> >>> I would have preferred IOMAP_DIO_WRITE_THROUGH, STABLE_WRITES is a flag >>> we use in file systems and the page cache for cases where the page >>> can't be touched before writeback has completed, e.g. >>> QUEUE_FLAG_STABLE_WRITES and SB_I_STABLE_WRITES. >> >> Good point, it does confuse terminology with stable pages for writes. >> I'll change it to WRITE_THROUGH, that is more descriptive for this case. > > +1 for the name change. > > With IOMAP_DIO_WRITE_THROUGH, > Reviewed-by: Darrick J. Wong <djwong@kernel.org> Thanks, I did make that change. > Separately: At some point, the definition for IOMAP_DIO_DIRTY needs to > grow a type annotation: > > #define IOMAP_DIO_DIRTY (1U << 31) > > due (apparently) triggering UBSAN because "1" on its own is a signed > constant. If this series goes through my tree then I'll add a trivial > patch fixing all of this ... unless you'd rather do it yourself as a > patch 9? Ah yes. I can add a patch for that and send out a v5. Will run the usual testing on it with that patch added, then ship it out. Risk of conflict with io_uring changes is pretty small, so would be fine to stage through your tree.
diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c index c654612b24e5..9f97d0d03724 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_STABLE_WRITE (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 STABLE_WRITE 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_STABLE_WRITE; 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_STABLE_WRITE) && + (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 STABLE_WRITE 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_STABLE_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 * cache on IO completion. Clear the sync flag for this case. */ - if (dio->flags & IOMAP_DIO_WRITE_FUA) + if (dio->flags & IOMAP_DIO_STABLE_WRITE) dio->flags &= ~IOMAP_DIO_NEED_SYNC; WRITE_ONCE(iocb->private, dio->submit.poll_bio);
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. Signed-off-by: Jens Axboe <axboe@kernel.dk> --- fs/iomap/direct-io.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)