| Message ID | 20250313171310.1886394-4-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | large atomic writes for xfs with CoW | expand |
> iomap->flags |= IOMAP_F_NEW; > > + if (flags & IOMAP_ATOMIC) > + iomap->flags |= IOMAP_F_ATOMIC_BIO; > + Add a comment here that ext4 is always using hardware atomics? > + if (flags & IOMAP_ATOMIC) > + iomap_flags |= IOMAP_F_ATOMIC_BIO; Same here (at least for now until it is changed later). > + * IOMAP_F_ATOMIC_BIO indicates that (write) I/O needs to be issued as an > + * atomic bio, i.e. set REQ_ATOMIC. s/needs to/will be/ ?
On 17/03/2025 06:11, Christoph Hellwig wrote: >> iomap->flags |= IOMAP_F_NEW; >> >> + if (flags & IOMAP_ATOMIC) >> + iomap->flags |= IOMAP_F_ATOMIC_BIO; >> + > > Add a comment here that ext4 is always using hardware atomics? > >> + if (flags & IOMAP_ATOMIC) >> + iomap_flags |= IOMAP_F_ATOMIC_BIO; > > Same here (at least for now until it is changed later). Please note that Christian plans on sending the earlier iomap changes related to this work for 6.15. Those changes are also in the xfs queue. We are kinda reverting those changes here, so I think that it would still make sense for the iomap changes in this series to make 6.15 The xfs changes in this series are unlikely to make 6.15 As such, if we say that ext4 always uses hardware atomics, then we should mention that xfs does also (until it doesn't). So, in the end, I'd rather not add those comments at all - ok? > >> + * IOMAP_F_ATOMIC_BIO indicates that (write) I/O needs to be issued as an >> + * atomic bio, i.e. set REQ_ATOMIC. > > s/needs to/will be/ ? > ok
John Garry <john.g.garry@oracle.com> 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@oracle.com> > --- > 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
On 17/03/2025 13:44, Ritesh Harjani (IBM) wrote: >> 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. Right > > >> /* 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. The comment for all these flags is that they should be set by the FS: "Flags reported by the file system from iomap_begin" So the second sentence seems to just repeat what is already said. > > This definition (or whatever you feel is the better version), should also > go in Documentation/filesystems/iomap/design.rst Yes, I need to update that again > >> #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. Yes, I will update it. Thanks for the reminder. John
On Mon, Mar 17, 2025 at 09:05:39AM +0000, John Garry wrote: >> Same here (at least for now until it is changed later). > > Please note that Christian plans on sending the earlier iomap changes > related to this work for 6.15. Those changes are also in the xfs queue. We > are kinda reverting those changes here, so I think that it would still make > sense for the iomap changes in this series to make 6.15 > > The xfs changes in this series are unlikely to make 6.15 > > As such, if we say that ext4 always uses hardware atomics, then we should > mention that xfs does also (until it doesn't). That's what I meant. > So, in the end, I'd rather not add those comments at all - ok? If I read through this code it would be kinda nice to figure out why we're instructing the iomap code to do it. If you look at xfs_direct_write_iomap_begin it also generally comments on why we set specific flags.
On 18/03/2025 05:32, Christoph Hellwig wrote: > On Mon, Mar 17, 2025 at 09:05:39AM +0000, John Garry wrote: >>> Same here (at least for now until it is changed later). >> Please note that Christian plans on sending the earlier iomap changes >> related to this work for 6.15. Those changes are also in the xfs queue. We >> are kinda reverting those changes here, so I think that it would still make >> sense for the iomap changes in this series to make 6.15 >> >> The xfs changes in this series are unlikely to make 6.15 >> >> As such, if we say that ext4 always uses hardware atomics, then we should >> mention that xfs does also (until it doesn't). > That's what I meant. ok > >> So, in the end, I'd rather not add those comments at all - ok? > If I read through this code it would be kinda nice to figure out why > we're instructing the iomap code to do it. If you look at > xfs_direct_write_iomap_begin it also generally comments on why we > set specific flags. understood
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; /* 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. */ #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 */ 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);
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@oracle.com> --- 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(-)