John Garry <john.g.garry@xxxxxxxxxx> writes:
> Flag IOMAP_ATOMIC_SW is not really required. The idea of having this flag
> is that the FS ->iomap_begin callback could check if this flag is set to
> decide whether to do a SW (FS-based) atomic write. But the FS can set
> which ->iomap_begin callback it wants when deciding to do a FS-based
> atomic write.
>
> Furthermore, it was thought that IOMAP_ATOMIC_HW is not a proper name, as
> the block driver can use SW-methods to emulate an atomic write. So change
> back to IOMAP_ATOMIC.
>
> The ->iomap_begin callback needs though to indicate to iomap core that
> REQ_ATOMIC needs to be set, so add IOMAP_F_ATOMIC_BIO for that.
>
> These changes were suggested by Christoph Hellwig and Dave Chinner.
>
> Signed-off-by: John Garry <john.g.garry@xxxxxxxxxx>
> ---
> fs/ext4/inode.c | 5 ++++-
> fs/iomap/direct-io.c | 8 +++-----
> fs/iomap/trace.h | 2 +-
> fs/xfs/xfs_iomap.c | 3 +++
> include/linux/iomap.h | 12 +++++-------
> 5 files changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index ba2f1e3db7c7..949d74d34926 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3290,6 +3290,9 @@ static void ext4_set_iomap(struct inode *inode, struct iomap *iomap,
> if (map->m_flags & EXT4_MAP_NEW)
> iomap->flags |= IOMAP_F_NEW;
>
> + if (flags & IOMAP_ATOMIC)
> + iomap->flags |= IOMAP_F_ATOMIC_BIO;
> +
> if (flags & IOMAP_DAX)
> iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev;
> else
> @@ -3467,7 +3470,7 @@ static inline bool ext4_want_directio_fallback(unsigned flags, ssize_t written)
> return false;
>
> /* atomic writes are all-or-nothing */
> - if (flags & IOMAP_ATOMIC_HW)
> + if (flags & IOMAP_ATOMIC)
> return false;
>
The changes in ext4 is mostly straight forward. Essentially for
an IOMAP_ATOMIC write requests we are always setting IOMAP_F_ATOMIC_BIO in
the ->iomap_begin() routine. This is done to inform the iomap that this
write request needs to issue an atomic bio, so iomap then goes and sets
REQ_ATOMIC flag in the bio.
> /* can only try again if we wrote nothing */
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index 9d72b99cb447..c28685fd3362 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -349,7 +349,7 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
> if (dio->flags & IOMAP_DIO_WRITE) {
> bio_opf |= REQ_OP_WRITE;
>
> - if (iter->flags & IOMAP_ATOMIC_HW) {
> + if (iomap->flags & IOMAP_F_ATOMIC_BIO) {
> /*
> * Ensure that the mapping covers the full write length,
> * otherwise we will submit multiple BIOs, which is
> @@ -677,10 +677,8 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
> iomi.flags |= IOMAP_OVERWRITE_ONLY;
> }
>
> - if (dio_flags & IOMAP_DIO_ATOMIC_SW)
> - iomi.flags |= IOMAP_ATOMIC_SW;
> - else if (iocb->ki_flags & IOCB_ATOMIC)
> - iomi.flags |= IOMAP_ATOMIC_HW;
> + if (iocb->ki_flags & IOCB_ATOMIC)
> + iomi.flags |= IOMAP_ATOMIC;
>
> /* for data sync or sync, we need sync completion processing */
> if (iocb_is_dsync(iocb)) {
> diff --git a/fs/iomap/trace.h b/fs/iomap/trace.h
> index 69af89044ebd..9eab2c8ac3c5 100644
> --- a/fs/iomap/trace.h
> +++ b/fs/iomap/trace.h
> @@ -99,7 +99,7 @@ DEFINE_RANGE_EVENT(iomap_dio_rw_queued);
> { IOMAP_FAULT, "FAULT" }, \
> { IOMAP_DIRECT, "DIRECT" }, \
> { IOMAP_NOWAIT, "NOWAIT" }, \
> - { IOMAP_ATOMIC_HW, "ATOMIC_HW" }
> + { IOMAP_ATOMIC, "ATOMIC" }
>
> #define IOMAP_F_FLAGS_STRINGS \
> { IOMAP_F_NEW, "NEW" }, \
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 30e257f683bb..9a22ecd794eb 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -831,6 +831,9 @@ xfs_direct_write_iomap_begin(
> if (offset + length > i_size_read(inode))
> iomap_flags |= IOMAP_F_DIRTY;
>
> + if (flags & IOMAP_ATOMIC)
> + iomap_flags |= IOMAP_F_ATOMIC_BIO;
> +
> /*
> * COW writes may allocate delalloc space or convert unwritten COW
> * extents, so we need to make sure to take the lock exclusively here.
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index 9cd93530013c..51f4c13bd17a 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -60,6 +60,9 @@ struct vm_fault;
> * IOMAP_F_ANON_WRITE indicates that (write) I/O does not have a target block
> * assigned to it yet and the file system will do that in the bio submission
> * handler, splitting the I/O as needed.
> + *
> + * IOMAP_F_ATOMIC_BIO indicates that (write) I/O needs to be issued as an
> + * atomic bio, i.e. set REQ_ATOMIC.
> */
Maybe we can be more explicit here?
IOMAP_F_ATOMIC_BIO flag indicates that write I/O must be issued as an
atomic bio by setting the REQ_ATOMIC flag. Filesystems need to set this
flag to inform iomap that the write I/O operation should be submitted as
an atomic bio.
This definition (or whatever you feel is the better version), should also
go in Documentation/filesystems/iomap/design.rst
> #define IOMAP_F_NEW (1U << 0)
> #define IOMAP_F_DIRTY (1U << 1)
> @@ -73,6 +76,7 @@ struct vm_fault;
> #define IOMAP_F_XATTR (1U << 5)
> #define IOMAP_F_BOUNDARY (1U << 6)
> #define IOMAP_F_ANON_WRITE (1U << 7)
> +#define IOMAP_F_ATOMIC_BIO (1U << 8)
>
> /*
> * Flags set by the core iomap code during operations:
> @@ -189,9 +193,8 @@ struct iomap_folio_ops {
> #else
> #define IOMAP_DAX 0
> #endif /* CONFIG_FS_DAX */
> -#define IOMAP_ATOMIC_HW (1 << 9) /* HW-based torn-write protection */
> +#define IOMAP_ATOMIC (1 << 9) /* torn-write protection */
> #define IOMAP_DONTCACHE (1 << 10)
> -#define IOMAP_ATOMIC_SW (1 << 11)/* SW-based torn-write protection */
Now that we are killing separate IOMAP_ATOMIC_** names, we may would
like to update the iomap design document as well. Otherwise it will
carry use of IOMAP_ATOMIC_HW & IOMAP_ATOMIC_SW definitions. Instead we
should only keep IOMAP_ATOMIC and update the design info there.
-ritesh
>
> struct iomap_ops {
> /*
> @@ -503,11 +506,6 @@ struct iomap_dio_ops {
> */
> #define IOMAP_DIO_PARTIAL (1 << 2)
>
> -/*
> - * Use software-based torn-write protection.
> - */
> -#define IOMAP_DIO_ATOMIC_SW (1 << 3)
> -
> ssize_t iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
> const struct iomap_ops *ops, const struct iomap_dio_ops *dops,
> unsigned int dio_flags, void *private, size_t done_before);
> --
> 2.31.1
