| Message ID | 20241015090142.3189518-5-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block atomic writes for xfs | expand |
On Tue, Oct 15, 2024 at 09:01:39AM +0000, John Garry wrote: > Support direct I/O atomic writes by producing a single bio with REQ_ATOMIC > flag set. > > Initially FSes (XFS) should only support writing a single FS block > atomically. > > As with any atomic write, we should produce a single bio which covers the > complete write length. > > Signed-off-by: John Garry <john.g.garry@oracle.com> > --- > .../filesystems/iomap/operations.rst | 11 ++++++ > fs/iomap/direct-io.c | 38 +++++++++++++++++-- > fs/iomap/trace.h | 3 +- > include/linux/iomap.h | 1 + > 4 files changed, 48 insertions(+), 5 deletions(-) > > diff --git a/Documentation/filesystems/iomap/operations.rst b/Documentation/filesystems/iomap/operations.rst > index 8e6c721d2330..fb95e99ca1a0 100644 > --- a/Documentation/filesystems/iomap/operations.rst > +++ b/Documentation/filesystems/iomap/operations.rst > @@ -513,6 +513,17 @@ IOMAP_WRITE`` with any combination of the following enhancements: > if the mapping is unwritten and the filesystem cannot handle zeroing > the unaligned regions without exposing stale contents. > > + * ``IOMAP_ATOMIC``: This write is being issued with torn-write > + protection. Only a single bio can be created for the write, and the > + write must not be split into multiple I/O requests, i.e. flag > + REQ_ATOMIC must be set. > + The file range to write must be aligned to satisfy the requirements > + of both the filesystem and the underlying block device's atomic > + commit capabilities. > + If filesystem metadata updates are required (e.g. unwritten extent > + conversion or copy on write), all updates for the entire file range > + must be committed atomically as well. > + > Callers commonly hold ``i_rwsem`` in shared or exclusive mode before > calling this function. > > diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c > index f637aa0706a3..c968a0e2a60b 100644 > --- a/fs/iomap/direct-io.c > +++ b/fs/iomap/direct-io.c > @@ -271,7 +271,7 @@ static int iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio, > * 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) > + const struct iomap *iomap, bool use_fua, bool atomic) > { > blk_opf_t opflags = REQ_SYNC | REQ_IDLE; > > @@ -283,6 +283,8 @@ static inline blk_opf_t iomap_dio_bio_opflags(struct iomap_dio *dio, > opflags |= REQ_FUA; > else > dio->flags &= ~IOMAP_DIO_WRITE_THROUGH; > + if (atomic) > + opflags |= REQ_ATOMIC; > > return opflags; > } > @@ -293,7 +295,8 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter, > const struct iomap *iomap = &iter->iomap; > struct inode *inode = iter->inode; > unsigned int fs_block_size = i_blocksize(inode), pad; > - loff_t length = iomap_length(iter); > + const loff_t length = iomap_length(iter); > + bool atomic = iter->flags & IOMAP_ATOMIC; > loff_t pos = iter->pos; > blk_opf_t bio_opf; > struct bio *bio; > @@ -303,6 +306,9 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter, > size_t copied = 0; > size_t orig_count; > > + if (atomic && (length != fs_block_size)) Nit: no need for the inner braces here. > + if (atomic && n != length) { > + /* > + * This bio should have covered the complete length, > + * which it doesn't, so error. We may need to zero out > + * the tail (complete FS block), similar to when > + * bio_iov_iter_get_pages() returns an error, above. > + */ > + ret = -EINVAL; Do we want a WARN_ON_ONCE here because this is a condition that should be impossible to hit? Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On 15/10/2024 13:12, Christoph Hellwig wrote: > On Tue, Oct 15, 2024 at 09:01:39AM +0000, John Garry wrote: >> Support direct I/O atomic writes by producing a single bio with REQ_ATOMIC >> flag set. >> >> Initially FSes (XFS) should only support writing a single FS block >> atomically. >> >> As with any atomic write, we should produce a single bio which covers the >> complete write length. >> >> Signed-off-by: John Garry <john.g.garry@oracle.com> >> --- >> .../filesystems/iomap/operations.rst | 11 ++++++ >> fs/iomap/direct-io.c | 38 +++++++++++++++++-- >> fs/iomap/trace.h | 3 +- >> include/linux/iomap.h | 1 + >> 4 files changed, 48 insertions(+), 5 deletions(-) >> >> diff --git a/Documentation/filesystems/iomap/operations.rst b/Documentation/filesystems/iomap/operations.rst >> index 8e6c721d2330..fb95e99ca1a0 100644 >> --- a/Documentation/filesystems/iomap/operations.rst >> +++ b/Documentation/filesystems/iomap/operations.rst >> @@ -513,6 +513,17 @@ IOMAP_WRITE`` with any combination of the following enhancements: >> if the mapping is unwritten and the filesystem cannot handle zeroing >> the unaligned regions without exposing stale contents. >> >> + * ``IOMAP_ATOMIC``: This write is being issued with torn-write >> + protection. Only a single bio can be created for the write, and the >> + write must not be split into multiple I/O requests, i.e. flag >> + REQ_ATOMIC must be set. >> + The file range to write must be aligned to satisfy the requirements >> + of both the filesystem and the underlying block device's atomic >> + commit capabilities. >> + If filesystem metadata updates are required (e.g. unwritten extent >> + conversion or copy on write), all updates for the entire file range >> + must be committed atomically as well. >> + >> Callers commonly hold ``i_rwsem`` in shared or exclusive mode before >> calling this function. >> >> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c >> index f637aa0706a3..c968a0e2a60b 100644 >> --- a/fs/iomap/direct-io.c >> +++ b/fs/iomap/direct-io.c >> @@ -271,7 +271,7 @@ static int iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio, >> * 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) >> + const struct iomap *iomap, bool use_fua, bool atomic) >> { >> blk_opf_t opflags = REQ_SYNC | REQ_IDLE; >> >> @@ -283,6 +283,8 @@ static inline blk_opf_t iomap_dio_bio_opflags(struct iomap_dio *dio, >> opflags |= REQ_FUA; >> else >> dio->flags &= ~IOMAP_DIO_WRITE_THROUGH; >> + if (atomic) >> + opflags |= REQ_ATOMIC; >> >> return opflags; >> } >> @@ -293,7 +295,8 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter, >> const struct iomap *iomap = &iter->iomap; >> struct inode *inode = iter->inode; >> unsigned int fs_block_size = i_blocksize(inode), pad; >> - loff_t length = iomap_length(iter); >> + const loff_t length = iomap_length(iter); >> + bool atomic = iter->flags & IOMAP_ATOMIC; >> loff_t pos = iter->pos; >> blk_opf_t bio_opf; >> struct bio *bio; >> @@ -303,6 +306,9 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter, >> size_t copied = 0; >> size_t orig_count; >> >> + if (atomic && (length != fs_block_size)) > > Nit: no need for the inner braces here. ok > >> + if (atomic && n != length) { >> + /* >> + * This bio should have covered the complete length, >> + * which it doesn't, so error. We may need to zero out >> + * the tail (complete FS block), similar to when >> + * bio_iov_iter_get_pages() returns an error, above. >> + */ >> + ret = -EINVAL; > > Do we want a WARN_ON_ONCE here because this is a condition that should be > impossible to hit? I have no objection - as you said, it should not be hit. But it would be nice to see why we are getting the -EINVAL if it were hit (so I can add it) > > Otherwise looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de> cheers
diff --git a/Documentation/filesystems/iomap/operations.rst b/Documentation/filesystems/iomap/operations.rst index 8e6c721d2330..fb95e99ca1a0 100644 --- a/Documentation/filesystems/iomap/operations.rst +++ b/Documentation/filesystems/iomap/operations.rst @@ -513,6 +513,17 @@ IOMAP_WRITE`` with any combination of the following enhancements: if the mapping is unwritten and the filesystem cannot handle zeroing the unaligned regions without exposing stale contents. + * ``IOMAP_ATOMIC``: This write is being issued with torn-write + protection. Only a single bio can be created for the write, and the + write must not be split into multiple I/O requests, i.e. flag + REQ_ATOMIC must be set. + The file range to write must be aligned to satisfy the requirements + of both the filesystem and the underlying block device's atomic + commit capabilities. + If filesystem metadata updates are required (e.g. unwritten extent + conversion or copy on write), all updates for the entire file range + must be committed atomically as well. + Callers commonly hold ``i_rwsem`` in shared or exclusive mode before calling this function. diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c index f637aa0706a3..c968a0e2a60b 100644 --- a/fs/iomap/direct-io.c +++ b/fs/iomap/direct-io.c @@ -271,7 +271,7 @@ static int iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio, * 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) + const struct iomap *iomap, bool use_fua, bool atomic) { blk_opf_t opflags = REQ_SYNC | REQ_IDLE; @@ -283,6 +283,8 @@ static inline blk_opf_t iomap_dio_bio_opflags(struct iomap_dio *dio, opflags |= REQ_FUA; else dio->flags &= ~IOMAP_DIO_WRITE_THROUGH; + if (atomic) + opflags |= REQ_ATOMIC; return opflags; } @@ -293,7 +295,8 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter, const struct iomap *iomap = &iter->iomap; struct inode *inode = iter->inode; unsigned int fs_block_size = i_blocksize(inode), pad; - loff_t length = iomap_length(iter); + const loff_t length = iomap_length(iter); + bool atomic = iter->flags & IOMAP_ATOMIC; loff_t pos = iter->pos; blk_opf_t bio_opf; struct bio *bio; @@ -303,6 +306,9 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter, size_t copied = 0; size_t orig_count; + if (atomic && (length != fs_block_size)) + return -EINVAL; + if ((pos | length) & (bdev_logical_block_size(iomap->bdev) - 1) || !bdev_iter_is_aligned(iomap->bdev, dio->submit.iter)) return -EINVAL; @@ -382,7 +388,7 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter, * can set up the page vector appropriately for a ZONE_APPEND * operation. */ - bio_opf = iomap_dio_bio_opflags(dio, iomap, use_fua); + bio_opf = iomap_dio_bio_opflags(dio, iomap, use_fua, atomic); nr_pages = bio_iov_vecs_to_alloc(dio->submit.iter, BIO_MAX_VECS); do { @@ -415,6 +421,17 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter, } n = bio->bi_iter.bi_size; + if (atomic && n != length) { + /* + * This bio should have covered the complete length, + * which it doesn't, so error. We may need to zero out + * the tail (complete FS block), similar to when + * bio_iov_iter_get_pages() returns an error, above. + */ + ret = -EINVAL; + bio_put(bio); + goto zero_tail; + } if (dio->flags & IOMAP_DIO_WRITE) { task_io_account_write(n); } else { @@ -598,6 +615,9 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, if (iocb->ki_flags & IOCB_NOWAIT) iomi.flags |= IOMAP_NOWAIT; + if (iocb->ki_flags & IOCB_ATOMIC) + iomi.flags |= IOMAP_ATOMIC; + if (iov_iter_rw(iter) == READ) { /* reads can always complete inline */ dio->flags |= IOMAP_DIO_INLINE_COMP; @@ -659,7 +679,17 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, if (ret != -EAGAIN) { trace_iomap_dio_invalidate_fail(inode, iomi.pos, iomi.len); - ret = -ENOTBLK; + if (iocb->ki_flags & IOCB_ATOMIC) { + /* + * folio invalidation failed, maybe + * this is transient, unlock and see if + * the caller tries again. + */ + ret = -EAGAIN; + } else { + /* fall back to buffered write */ + ret = -ENOTBLK; + } } goto out_free_dio; } diff --git a/fs/iomap/trace.h b/fs/iomap/trace.h index 0a991c4ce87d..4118a42cdab0 100644 --- a/fs/iomap/trace.h +++ b/fs/iomap/trace.h @@ -98,7 +98,8 @@ DEFINE_RANGE_EVENT(iomap_dio_rw_queued); { IOMAP_REPORT, "REPORT" }, \ { IOMAP_FAULT, "FAULT" }, \ { IOMAP_DIRECT, "DIRECT" }, \ - { IOMAP_NOWAIT, "NOWAIT" } + { IOMAP_NOWAIT, "NOWAIT" }, \ + { IOMAP_ATOMIC, "ATOMIC" } #define IOMAP_F_FLAGS_STRINGS \ { IOMAP_F_NEW, "NEW" }, \ diff --git a/include/linux/iomap.h b/include/linux/iomap.h index 4ad12a3c8bae..c7644bdcfca3 100644 --- a/include/linux/iomap.h +++ b/include/linux/iomap.h @@ -178,6 +178,7 @@ struct iomap_folio_ops { #else #define IOMAP_DAX 0 #endif /* CONFIG_FS_DAX */ +#define IOMAP_ATOMIC (1 << 9) struct iomap_ops { /*
Support direct I/O atomic writes by producing a single bio with REQ_ATOMIC flag set. Initially FSes (XFS) should only support writing a single FS block atomically. As with any atomic write, we should produce a single bio which covers the complete write length. Signed-off-by: John Garry <john.g.garry@oracle.com> --- .../filesystems/iomap/operations.rst | 11 ++++++ fs/iomap/direct-io.c | 38 +++++++++++++++++-- fs/iomap/trace.h | 3 +- include/linux/iomap.h | 1 + 4 files changed, 48 insertions(+), 5 deletions(-)