| Message ID | 20250320120250.4087011-4-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | further iomap large atomic writes changes | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
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. Looks good to me. Thanks for updating the iomap design document as well. Feel free to add: Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > > Signed-off-by: John Garry <john.g.garry@oracle.com> > --- > .../filesystems/iomap/operations.rst | 35 ++++++++++--------- > fs/ext4/inode.c | 6 +++- > fs/iomap/direct-io.c | 8 ++--- > fs/iomap/trace.h | 2 +- > fs/xfs/xfs_iomap.c | 4 +++ > include/linux/iomap.h | 12 +++---- > 6 files changed, 37 insertions(+), 30 deletions(-) > > diff --git a/Documentation/filesystems/iomap/operations.rst b/Documentation/filesystems/iomap/operations.rst > index b08a79d11d9f..3b628e370d88 100644 > --- a/Documentation/filesystems/iomap/operations.rst > +++ b/Documentation/filesystems/iomap/operations.rst > @@ -514,29 +514,32 @@ 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_HW``: This write is being issued with torn-write > - protection based on HW-offload support. > - 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. > + * ``IOMAP_ATOMIC``: This write is being issued with torn-write > + protection. > + Torn-write protection may be provided based on HW-offload or by a > + software mechanism provided by the filesystem. > + > + For HW-offload based support, 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 > + conversion or copy-on-write), all updates for the entire file range > must be committed atomically as well. > - Only one space mapping is allowed per untorn write. > - Untorn writes may be longer than a single file block. In all cases, > + Untorn-writes may be longer than a single file block. In all cases, > the mapping start disk block must have at least the same alignment as > the write offset. > - > - * ``IOMAP_ATOMIC_SW``: This write is being issued with torn-write > - protection via a software mechanism provided by the filesystem. > - All the disk block alignment and single bio restrictions which apply > - to IOMAP_ATOMIC_HW do not apply here. > - SW-based untorn writes would typically be used as a fallback when > - HW-based untorn writes may not be issued, e.g. the range of the write > - covers multiple extents, meaning that it is not possible to issue > + The filesystems must set IOMAP_F_ATOMIC_BIO to inform iomap core of an > + untorn-write based on HW-offload. > + > + For untorn-writes based on a software mechanism provided by the > + filesystem, all the disk block alignment and single bio restrictions > + which apply for HW-offload based untorn-writes do not apply. > + The mechanism would typically be used as a fallback for when > + HW-offload based untorn-writes may not be issued, e.g. the range of the > + write covers multiple extents, meaning that it is not possible to issue > a single bio. > All filesystem metadata updates for the entire file range must be > committed atomically as well. > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index ba2f1e3db7c7..d04d8a7f12e7 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -3290,6 +3290,10 @@ static void ext4_set_iomap(struct inode *inode, struct iomap *iomap, > if (map->m_flags & EXT4_MAP_NEW) > iomap->flags |= IOMAP_F_NEW; > > + /* HW-offload atomics are always used */ > + 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 +3471,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 b9f59ca43c15..6ac7a1534f7c 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 it won't be submitted as a single > @@ -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 5dd0922fe2d1..ee40dc509413 100644 > --- a/fs/xfs/xfs_iomap.c > +++ b/fs/xfs/xfs_iomap.c > @@ -828,6 +828,10 @@ xfs_direct_write_iomap_begin( > if (offset + length > i_size_read(inode)) > iomap_flags |= IOMAP_F_DIRTY; > > + /* HW-offload atomics are always used in this path */ > + 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..02fe001feebb 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 will 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); > -- > 2.31.1
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> > --- > .../filesystems/iomap/operations.rst | 35 ++++++++++--------- > fs/ext4/inode.c | 6 +++- > fs/iomap/direct-io.c | 8 ++--- > fs/iomap/trace.h | 2 +- > fs/xfs/xfs_iomap.c | 4 +++ > include/linux/iomap.h | 12 +++---- > 6 files changed, 37 insertions(+), 30 deletions(-) > > diff --git a/Documentation/filesystems/iomap/operations.rst b/Documentation/filesystems/iomap/operations.rst > index b08a79d11d9f..3b628e370d88 100644 > --- a/Documentation/filesystems/iomap/operations.rst > +++ b/Documentation/filesystems/iomap/operations.rst > @@ -514,29 +514,32 @@ 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_HW``: This write is being issued with torn-write > - protection based on HW-offload support. > - 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. > + * ``IOMAP_ATOMIC``: This write is being issued with torn-write > + protection. > + Torn-write protection may be provided based on HW-offload or by a > + software mechanism provided by the filesystem. > + > + For HW-offload based support, 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 > + conversion or copy-on-write), all updates for the entire file range > must be committed atomically as well. > - Only one space mapping is allowed per untorn write. > - Untorn writes may be longer than a single file block. In all cases, > + Untorn-writes may be longer than a single file block. In all cases, > the mapping start disk block must have at least the same alignment as > the write offset. > - > - * ``IOMAP_ATOMIC_SW``: This write is being issued with torn-write > - protection via a software mechanism provided by the filesystem. > - All the disk block alignment and single bio restrictions which apply > - to IOMAP_ATOMIC_HW do not apply here. > - SW-based untorn writes would typically be used as a fallback when > - HW-based untorn writes may not be issued, e.g. the range of the write > - covers multiple extents, meaning that it is not possible to issue > + The filesystems must set IOMAP_F_ATOMIC_BIO to inform iomap core of an > + untorn-write based on HW-offload. > + > + For untorn-writes based on a software mechanism provided by the > + filesystem, all the disk block alignment and single bio restrictions > + which apply for HW-offload based untorn-writes do not apply. > + The mechanism would typically be used as a fallback for when > + HW-offload based untorn-writes may not be issued, e.g. the range of the > + write covers multiple extents, meaning that it is not possible to issue > a single bio. > All filesystem metadata updates for the entire file range must be > committed atomically as well. > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index ba2f1e3db7c7..d04d8a7f12e7 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -3290,6 +3290,10 @@ static void ext4_set_iomap(struct inode *inode, struct iomap *iomap, > if (map->m_flags & EXT4_MAP_NEW) > iomap->flags |= IOMAP_F_NEW; > > + /* HW-offload atomics are always used */ > + 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 +3471,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 b9f59ca43c15..6ac7a1534f7c 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 it won't be submitted as a single > @@ -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 5dd0922fe2d1..ee40dc509413 100644 > --- a/fs/xfs/xfs_iomap.c > +++ b/fs/xfs/xfs_iomap.c > @@ -828,6 +828,10 @@ xfs_direct_write_iomap_begin( > if (offset + length > i_size_read(inode)) > iomap_flags |= IOMAP_F_DIRTY; > > + /* HW-offload atomics are always used in this path */ > + 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..02fe001feebb 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 will 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) Oops, sorry I am not sure how did I miss this during review. (1U << 8) is already taken by IOMAP_F_SIZE_CHANGED flag. Then I guess it will be wrong to use the same value for IOMAP_F_ATOMIC_BIO too, since both are used for setting iomap->flags. Although IOMAP_F_SIZE_CHANGED is only set in buffered-io operation i.e. iomap_write_iter() , so it wouldn't break anything as of now, until the atomic write support gets added to buffered-io, at which this will be a problem. Either ways I guess, this needs to be fixed. <snip from include/linux/iomap.h> #define IOMAP_F_ATOMIC_BIO (1U << 8) /* * Flags set by the core iomap code during operations: * * IOMAP_F_SIZE_CHANGED indicates to the iomap_end method that the file size * has changed as the result of this write operation. * * IOMAP_F_STALE indicates that the iomap is not valid any longer and the file * range it covers needs to be remapped by the high level before the operation * can proceed. */ #define IOMAP_F_SIZE_CHANGED (1U << 8) So, I guess we can shift IOMAP_F_SIZE_CHANGED and IOMAP_F_STALE by 1 bit. So it will all look like.. #define IOMAP_F_ATOMIC_BIO (1U << 8) /* * Flags set by the core iomap code during operations: * * IOMAP_F_SIZE_CHANGED indicates to the iomap_end method that the file size * has changed as the result of this write operation. * * IOMAP_F_STALE indicates that the iomap is not valid any longer and the file * range it covers needs to be remapped by the high level before the operation * can proceed. */ #define IOMAP_F_SIZE_CHANGED (1U << 9) #define IOMAP_F_STALE (1U << 10) ... /* * Flags from 0x1000 up are for file system specific usage: */ #define IOMAP_F_PRIVATE (1U << 12) Thoughts? -ritesh > > /* > * 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); > -- > 2.31.1
On Sun, Mar 23, 2025 at 01:17:08AM +0530, Ritesh Harjani wrote: [full quote deleted, can you please properly trim your replies?] > So, I guess we can shift IOMAP_F_SIZE_CHANGED and IOMAP_F_STALE by > 1 bit. So it will all look like.. Let's create some more space to avoid this for the next round, e.g. count the core set flags from 31 down, and limit IOMAP_F_PRIVATE to a single flag, which is how it is used.
On 23/03/2025 06:38, Christoph Hellwig wrote: > On Sun, Mar 23, 2025 at 01:17:08AM +0530, Ritesh Harjani wrote: @ Ritesh, thanks for the notice - are you ok to send a fix for this? At a glance, it seems that those two conflicting flags won't cross paths in practice (but obvs still need to fix this). > > [full quote deleted, can you please properly trim your replies?] > >> So, I guess we can shift IOMAP_F_SIZE_CHANGED and IOMAP_F_STALE by >> 1 bit. So it will all look like.. > > Let's create some more space to avoid this for the next round, e.g. > count the core set flags from 31 down, and limit IOMAP_F_PRIVATE to a > single flag, which is how it is used. >
Christoph Hellwig <hch@lst.de> writes: > On Sun, Mar 23, 2025 at 01:17:08AM +0530, Ritesh Harjani wrote: > > [full quote deleted, can you please properly trim your replies?] > Sure. >> So, I guess we can shift IOMAP_F_SIZE_CHANGED and IOMAP_F_STALE by >> 1 bit. So it will all look like.. > > Let's create some more space to avoid this for the next round, e.g. Sure, that make sense. > count the core set flags from 31 down, and limit IOMAP_F_PRIVATE to a > single flag, which is how it is used. flags in struct iomap is of type u16. So will make core iomap flags starting from bit 15, moving downwards. Here is a diff of what I think you meant - let me know if this diff looks good to you? diff --git a/include/linux/iomap.h b/include/linux/iomap.h index 02fe001feebb..68416b135151 100644 --- a/include/linux/iomap.h +++ b/include/linux/iomap.h @@ -78,6 +78,11 @@ struct vm_fault; #define IOMAP_F_ANON_WRITE (1U << 7) #define IOMAP_F_ATOMIC_BIO (1U << 8) +/* + * Flag reserved for file system specific usage + */ +#define IOMAP_F_PRIVATE (1U << 12) + /* * Flags set by the core iomap code during operations: * @@ -88,14 +93,8 @@ struct vm_fault; * range it covers needs to be remapped by the high level before the operation * can proceed. */ -#define IOMAP_F_SIZE_CHANGED (1U << 8) -#define IOMAP_F_STALE (1U << 9) - -/* - * Flags from 0x1000 up are for file system specific usage: - */ -#define IOMAP_F_PRIVATE (1U << 12) - +#define IOMAP_F_SIZE_CHANGED (1U << 14) +#define IOMAP_F_STALE (1U << 15) /* * Magic value for addr: (PS: I might be on transit / travel for some other work for a week. My reponses may be delayed.) -ritesh
>>> So, I guess we can shift IOMAP_F_SIZE_CHANGED and IOMAP_F_STALE by >>> 1 bit. So it will all look like.. >> >> Let's create some more space to avoid this for the next round, e.g. > > Sure, that make sense. > >> count the core set flags from 31 down, and limit IOMAP_F_PRIVATE to a >> single flag, which is how it is used. > > flags in struct iomap is of type u16. So will make core iomap flags > starting from bit 15, moving downwards. > > Here is a diff of what I think you meant - let me know if this diff > looks good to you? This is still outstanding, and it would be nice to fix this ASAP. How about we go to 32b and change IOMAP_F_PRIVATE for v6.16, while just fix as suggested originally (by renumbering) for v6.15?
On Sun, Mar 23, 2025 at 07:12:02PM +0530, Ritesh Harjani wrote: > flags in struct iomap is of type u16. So will make core iomap flags > starting from bit 15, moving downwards. > > Here is a diff of what I think you meant - let me know if this diff > looks good to you? Yes, this looks good to me.
diff --git a/Documentation/filesystems/iomap/operations.rst b/Documentation/filesystems/iomap/operations.rst index b08a79d11d9f..3b628e370d88 100644 --- a/Documentation/filesystems/iomap/operations.rst +++ b/Documentation/filesystems/iomap/operations.rst @@ -514,29 +514,32 @@ 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_HW``: This write is being issued with torn-write - protection based on HW-offload support. - 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. + * ``IOMAP_ATOMIC``: This write is being issued with torn-write + protection. + Torn-write protection may be provided based on HW-offload or by a + software mechanism provided by the filesystem. + + For HW-offload based support, 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 + conversion or copy-on-write), all updates for the entire file range must be committed atomically as well. - Only one space mapping is allowed per untorn write. - Untorn writes may be longer than a single file block. In all cases, + Untorn-writes may be longer than a single file block. In all cases, the mapping start disk block must have at least the same alignment as the write offset. - - * ``IOMAP_ATOMIC_SW``: This write is being issued with torn-write - protection via a software mechanism provided by the filesystem. - All the disk block alignment and single bio restrictions which apply - to IOMAP_ATOMIC_HW do not apply here. - SW-based untorn writes would typically be used as a fallback when - HW-based untorn writes may not be issued, e.g. the range of the write - covers multiple extents, meaning that it is not possible to issue + The filesystems must set IOMAP_F_ATOMIC_BIO to inform iomap core of an + untorn-write based on HW-offload. + + For untorn-writes based on a software mechanism provided by the + filesystem, all the disk block alignment and single bio restrictions + which apply for HW-offload based untorn-writes do not apply. + The mechanism would typically be used as a fallback for when + HW-offload based untorn-writes may not be issued, e.g. the range of the + write covers multiple extents, meaning that it is not possible to issue a single bio. All filesystem metadata updates for the entire file range must be committed atomically as well. diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index ba2f1e3db7c7..d04d8a7f12e7 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -3290,6 +3290,10 @@ static void ext4_set_iomap(struct inode *inode, struct iomap *iomap, if (map->m_flags & EXT4_MAP_NEW) iomap->flags |= IOMAP_F_NEW; + /* HW-offload atomics are always used */ + 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 +3471,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 b9f59ca43c15..6ac7a1534f7c 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 it won't be submitted as a single @@ -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 5dd0922fe2d1..ee40dc509413 100644 --- a/fs/xfs/xfs_iomap.c +++ b/fs/xfs/xfs_iomap.c @@ -828,6 +828,10 @@ xfs_direct_write_iomap_begin( if (offset + length > i_size_read(inode)) iomap_flags |= IOMAP_F_DIRTY; + /* HW-offload atomics are always used in this path */ + 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..02fe001feebb 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 will 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> --- .../filesystems/iomap/operations.rst | 35 ++++++++++--------- fs/ext4/inode.c | 6 +++- fs/iomap/direct-io.c | 8 ++--- fs/iomap/trace.h | 2 +- fs/xfs/xfs_iomap.c | 4 +++ include/linux/iomap.h | 12 +++---- 6 files changed, 37 insertions(+), 30 deletions(-)