ext4: Add atomic writes support for DIO
Subsystems (2)
Ritesh Harjani (IBM)
reviewerv3 cover ext4: Add atomic writes support for DIO
v2 -> v3:
==========
1. Patch-1 adds an "experimental" string in dmesg log during mount when EXT4 detects that it is capable of doing DIO atomic writes on a given device with min and max unit details.
2. Patch-4 has been updated to avoid returning -ENOTBLK (in ext4_iomap_end) if the request belongs to atomic write. This patch also adds a WARN_ON_ONCE() if atomic write ever fallback to buffered-io (to catch any unwanted bugs in the future). More details in the commit log of patch-4.
3. Collected RBs tag from John for Patch 2 & 3.
[v2]: https://lore.kernel.org/linux-ext4/[email protected]/
Previous cover letter log:
In v2, we had split the series and this one only takes care of atomic writes for single fsblock.
That means for now this gets only enabled on bs < ps systems on ext4. Enablement of atomic writes for bigalloc (multi-fsblock support) is still under discussion and may require general consensus within the filesystem community [1].
This series adds the base feature support to enable atomic writes in direct-io path for ext4. We advertise the minimum and the maximum atomic write unit sizes via statx on a regular file.
This series allows users to utilize atomic write support using -
1. on bs < ps systems via - mkfs.ext4 -F -b 16384 /dev/sda
This can then be utilized using -
xfs_io -fdc "pwrite -V 1 -A -b16k 0 16k" /mnt/f1
This is built on top of John's DIO atomic write series for XFS [2]. The VFS and block layer enablement for atomic writes were merged already.
[1]: https://lore.kernel.org/linux-ext4/[email protected]
[2]: https://lore.kernel.org/linux-xfs/[email protected]/
Changelogs:
===========
PATCH -> PATCH v2:
- addressed review comments from John and Darrick.
- renamed ext4_sb_info variables names: fs_awu* -> s_awu*
- [PATCH]: https://lore.kernel.org/linux-ext4/[email protected]/
RFC -> PATCH:
- Dropped RFC tag
- Last RFC was posted a while ago but back then a lot of VFS and block layer interfaces were still not merged. Those are now merged, thanks to John and everyone else.
- [RFC] - https://lore.kernel.org/linux-ext4/[email protected]/
Ritesh Harjani (IBM) (4):
ext4: Add statx support for atomic writes
ext4: Check for atomic writes support in write iter
ext4: Support setting FMODE_CAN_ATOMIC_WRITE
ext4: Do not fallback to buffered-io for DIO atomic write
fs/ext4/ext4.h | 9 +++++++++ fs/ext4/file.c | 24 ++++++++++++++++++++++++ fs/ext4/inode.c | 28 +++++++++++++++++++++++----- fs/ext4/super.c | 31 +++++++++++++++++++++++++++++++ 4 files changed, 87 insertions(+), 5 deletions(-)
--
2.46.0
Ritesh Harjani (IBM)
reviewerPATCH v3 1/4 ext4: Add statx support for atomic writes
This patch adds base support for atomic writes via statx getattr. On bs < ps systems, we can create FS with say bs of 16k. That means both atomic write min and max unit can be set to 16k for supporting atomic writes.
--- fs/ext4/ext4.h | 9 +++++++++ fs/ext4/inode.c | 14 ++++++++++++++ fs/ext4/super.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+)
fs/ext4/ext4.h +9 -0
@@ -1729,6 +1729,10 @@ struct ext4_sb_info {*/ struct work_struct s_sb_upd_work; +/* Atomic write unit values in bytes */ +unsigned int s_awu_min; +unsigned int s_awu_max; +/* Ext4 fast commit sub transaction ID */ atomic_t s_fc_subtid;
@@ -3855,6 +3859,11 @@ static inline int ext4_buffer_uptodate(struct buffer_head *bh)return buffer_uptodate (bh); } +static inline bool ext4_can_atomic_write (struct super_block *sb) +{ +return EXT4_SB (sb)->s_awu_min >0 ; +} +extern int ext4_block_write_begin (handle_t *handle,struct folio *folio,loff_t pos,unsigned len,get_block_t *get_block);
fs/ext4/inode.c +14 -0
@@ -5578,6 +5578,20 @@ int ext4_getattr(struct mnt_idmap *idmap, const struct path *path, } } +if (S_ISREG (inode->i_mode) && (request_mask &STATX_WRITE_ATOMIC )) { +struct ext4_sb_info *sbi =EXT4_SB (inode->i_sb); +unsigned int awu_min, awu_max; + +if (ext4_can_atomic_write (inode->i_sb)) { + awu_min = sbi->s_awu_min; + awu_max = sbi->s_awu_max; + }else { + awu_min = awu_max =0 ; + } + +generic_fill_statx_atomic_writes (stat, awu_min, awu_max); + } + flags = ei->i_flags &EXT4_FL_USER_VISIBLE ;if (flags &EXT4_APPEND_FL ) stat->attributes |=STATX_ATTR_APPEND ;
fs/ext4/super.c +31 -0
@@ -4425,6 +4425,36 @@ static int ext4_handle_clustersize(struct super_block *sb)return 0 ; } +/* +* ext4_atomic_write_init: Initializes filesystem min & max atomic write units. +* @sb: super block +* TODO: Later add support for bigalloc +*/ +static void ext4_atomic_write_init (struct super_block *sb) +{ +struct ext4_sb_info *sbi =EXT4_SB (sb); +struct block_device *bdev = sb->s_bdev; + +if (!bdev_can_atomic_write (bdev)) +return ; + +if (!ext4_has_feature_extents (sb)) +return ; + + sbi->s_awu_min =max (sb->s_blocksize, +bdev_atomic_write_unit_min_bytes (bdev)); + sbi->s_awu_max =min (sb->s_blocksize, +bdev_atomic_write_unit_max_bytes (bdev)); +if (sbi->s_awu_min && sbi->s_awu_max && + sbi->s_awu_min <= sbi->s_awu_max) { +ext4_msg (sb,KERN_NOTICE ,"Supports (experimental) DIO atomic writes awu_min: %u, awu_max: %u" , + sbi->s_awu_min, sbi->s_awu_max); + }else { + sbi->s_awu_min =0 ; + sbi->s_awu_max =0 ; + } +} +static void ext4_fast_commit_init (struct super_block *sb) {struct ext4_sb_info *sbi =EXT4_SB (sb);
@@ -5336,6 +5366,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)spin_lock_init (&sbi->s_bdev_wb_lock); +ext4_atomic_write_init (sb);ext4_fast_commit_init (sb); sb->s_root =NULL ;
Signature · 1 line
2.46.0
Ritesh Harjani (IBM)
reviewerPATCH v3 2/4 ext4: Check for atomic writes support in write iter
In reply to Ritesh Harjani (IBM)Let's validate the given constraints for atomic write request. Otherwise it will fail with -EINVAL. Currently atomic write is only supported on DIO, so for buffered-io it will return -EOPNOTSUPP.
--- fs/ext4/file.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
fs/ext4/file.c +14 -0
@@ -692,6 +692,20 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)if (IS_DAX (inode))return ext4_dax_write_iter (iocb, from);#endif + +if (iocb->ki_flags &IOCB_ATOMIC ) { +size_t len =iov_iter_count (from); +int ret; + +if (len <EXT4_SB (inode->i_sb)->s_awu_min || + len >EXT4_SB (inode->i_sb)->s_awu_max) +return -EINVAL ; + + ret =generic_atomic_write_valid (iocb, from); +if (ret) +return ret; + } +if (iocb->ki_flags &IOCB_DIRECT )return ext4_dio_write_iter (iocb, from);else
Signature · 1 line
2.46.0
Ritesh Harjani (IBM)
reviewerPATCH v3 3/4 ext4: Support setting FMODE_CAN_ATOMIC_WRITE
In reply to Ritesh Harjani (IBM)FS needs to add the fmode capability in order to support atomic writes during file open (refer kiocb_set_rw_flags()). Set this capability on a regular file if ext4 can do atomic write.
--- fs/ext4/file.c | 3 +++ 1 file changed, 3 insertions(+)
fs/ext4/file.c +3 -0
@@ -898,6 +898,9 @@ static int ext4_file_open(struct inode *inode, struct file *filp)return ret; } +if (S_ISREG (inode->i_mode) &&ext4_can_atomic_write (inode->i_sb)) + filp->f_mode |=FMODE_CAN_ATOMIC_WRITE ; + filp->f_mode |=FMODE_NOWAIT |FMODE_CAN_ODIRECT ;return dquot_file_open (inode, filp); }
Signature · 1 line
2.46.0
Ritesh Harjani (IBM)
reviewerPATCH v3 4/4 ext4: Do not fallback to buffered-io for DIO atomic write
In reply to Ritesh Harjani (IBM)atomic writes is currently only supported for single fsblock and only for direct-io. We should not return -ENOTBLK for atomic writes since we want the atomic write request to either complete fully or fail otherwise. We should not fallback to buffered-io in case of DIO atomic write requests.
Let's also catch if this ever happens by adding some WARN_ON_ONCE before buffered-io handling for direct-io atomic writes.
More details of the discussion [1].
[1]: https://lore.kernel.org/linux-xfs/[email protected]/T/#m9dbecc11bed713ed0d7a486432c56b105b555f04
--- fs/ext4/file.c | 7 +++++++ fs/ext4/inode.c | 14 +++++++++----- 2 files changed, 16 insertions(+), 5 deletions(-)
fs/ext4/file.c +7 -0
@@ -599,6 +599,13 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)ssize_t err;loff_t endbyte; +/* +* There is no support for atomic writes on buffered-io yet, +* we should never fallback to buffered-io for DIO atomic +* writes. +*/ +WARN_ON_ONCE (iocb->ki_flags &IOCB_ATOMIC ); + offset = iocb->ki_pos; err =ext4_buffered_write_iter (iocb, from);if (err <0 )
fs/ext4/inode.c +9 -5
@@ -3449,12 +3449,16 @@ static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length, {/* * Check to see whether an error occurred while writing out the data to -* the allocated blocks. If so, return the magic error code so that we -* fallback to buffered I/O and attempt to complete the remainder of -* the I/O. Any blocks that may have been allocated in preparation for -* the direct I/O will be reused during buffered I/O. +* the allocated blocks. If so, return the magic error code for +* non-atomic write so that we fallback to buffered I/O and attempt to +* complete the remainder of the I/O. +* For atomic writes we will simply fail the I/O request if we coudn't +* write anything. For non-atomic writes, any blocks that may have been +* allocated in preparation for the direct I/O will be reused during +* buffered I/O. */ -if (flags & (IOMAP_WRITE |IOMAP_DIRECT ) && written ==0 ) +if (!(flags &IOMAP_ATOMIC ) && (flags & (IOMAP_WRITE |IOMAP_DIRECT )) + && written ==0 )return -ENOTBLK ;return 0 ;
Signature · 1 line
2.46.0
Re: [PATCH v3 1/4] ext4: Add statx support for atomic writes
In reply to Ritesh Harjani (IBM)Quoted 30 lines · Ritesh Harjani (IBM)
This patch adds base support for atomic writes via statx getattr. On bs < ps systems, we can create FS with say bs of 16k. That means both atomic write min and max unit can be set to 16k for supporting atomic writes.
Co-developed-by Ojaswin Mujoo <[email protected]>Signed-off-by Ojaswin Mujoo <[email protected]>Signed-off-by Ritesh Harjani (IBM) <[email protected]>--- fs/ext4/ext4.h | 9 +++++++++ fs/ext4/inode.c | 14 ++++++++++++++ fs/ext4/super.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+)fs/ext4/ext4.h +4
-0@@ -1729,6 +1729,10 @@ struct ext4_sb_info {*/ struct work_struct s_sb_upd_work; +/* Atomic write unit values in bytes */ +unsigned int s_awu_min; +unsigned int s_awu_max; +/* Ext4 fast commit sub transaction ID */ atomic_t s_fc_subtid;
fs/ext4/ext4.h static inline int ext4_buffer_uptodate(struct buffer_head *bh) · line 3864
Quoted 6 lines
fs/ext4/ext4.h +3
-0@@ -3855,6 +3859,11 @@ static inline int ext4_buffer_uptodate(struct buffer_head *bh)return buffer_uptodate (bh); } +static inline bool ext4_can_atomic_write (struct super_block *sb) +{ +return EXT4_SB (sb)->s_awu_min >0 ;
Huh, I was expecting you to stick to passing in the struct inode, and then you end up with:
static inline bool ext4_can_atomic_write(struct inode *inode)
{
return S_ISREG(inode->i_mode) &&
EXT4_SB(inode->i_sb)->s_awu_min > 0);
}Quoted 10 lines
fs/ext4/ext4.h +2
-0+} +extern int ext4_block_write_begin (handle_t *handle,struct folio *folio,loff_t pos,unsigned len,get_block_t *get_block);fs/ext4/inode.c +0
-0
fs/ext4/inode.c int ext4_getattr(struct mnt_idmap *idmap, const struct path *path, · line 5581
Quoted 4 lines
fs/ext4/inode.c +1
-0@@ -5578,6 +5578,20 @@ int ext4_getattr(struct mnt_idmap *idmap, const struct path *path, } } +if (S_ISREG (inode->i_mode) && (request_mask &STATX_WRITE_ATOMIC )) {
...and then the callsites become:
if (request_mask & STATX_WRITE_ATOMIC) {
unsigned int awu_min = 0, awu_max = 0;
if (ext4_can_atomic_write(inode)) {
awu_min = sbi->s_awu_min;
awu_max = sbi->s_awu_max;
}
generic_fill_statx_atomic_writes(stat, awu_min, awu_max);
}(I forget, is it bad if statx to a directory returns STATX_WRITE_ATOMIC even with awu_{min,max} set to zero?)
Other than that nit, this looks good to me.
--D
Quoted 69 lines
fs/ext4/inode.c +13
-0+struct ext4_sb_info *sbi =EXT4_SB (inode->i_sb); +unsigned int awu_min, awu_max; + +if (ext4_can_atomic_write (inode->i_sb)) { + awu_min = sbi->s_awu_min; + awu_max = sbi->s_awu_max; + }else { + awu_min = awu_max =0 ; + } + +generic_fill_statx_atomic_writes (stat, awu_min, awu_max); + } + flags = ei->i_flags &EXT4_FL_USER_VISIBLE ;if (flags &EXT4_APPEND_FL ) stat->attributes |=STATX_ATTR_APPEND ;fs/ext4/super.c +31
-0@@ -4425,6 +4425,36 @@ static int ext4_handle_clustersize(struct super_block *sb)return 0 ; } +/* +* ext4_atomic_write_init: Initializes filesystem min & max atomic write units. +* @sb: super block +* TODO: Later add support for bigalloc +*/ +static void ext4_atomic_write_init (struct super_block *sb) +{ +struct ext4_sb_info *sbi =EXT4_SB (sb); +struct block_device *bdev = sb->s_bdev; + +if (!bdev_can_atomic_write (bdev)) +return ; + +if (!ext4_has_feature_extents (sb)) +return ; + + sbi->s_awu_min =max (sb->s_blocksize, +bdev_atomic_write_unit_min_bytes (bdev)); + sbi->s_awu_max =min (sb->s_blocksize, +bdev_atomic_write_unit_max_bytes (bdev)); +if (sbi->s_awu_min && sbi->s_awu_max && + sbi->s_awu_min <= sbi->s_awu_max) { +ext4_msg (sb,KERN_NOTICE ,"Supports (experimental) DIO atomic writes awu_min: %u, awu_max: %u" , + sbi->s_awu_min, sbi->s_awu_max); + }else { + sbi->s_awu_min =0 ; + sbi->s_awu_max =0 ; + } +} +static void ext4_fast_commit_init (struct super_block *sb) {struct ext4_sb_info *sbi =EXT4_SB (sb);@@ -5336,6 +5366,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)spin_lock_init (&sbi->s_bdev_wb_lock); +ext4_atomic_write_init (sb);ext4_fast_commit_init (sb); sb->s_root =NULL ;Signature · 1 line
2.46.0
Re: [PATCH v3 2/4] ext4: Check for atomic writes support in write iter
In reply to Ritesh Harjani (IBM)Quoted 6 lines · Ritesh Harjani (IBM)
Let's validate the given constraints for atomic write request. Otherwise it will fail with
-EINVAL. Currently atomic write is only supported on DIO, so for buffered-io it will return-EOPNOTSUPP.Reviewed-by John Garry <[email protected]>Signed-off-by Ritesh Harjani (IBM) <[email protected]>
Looks decent,
--D
Quoted 33 lines
--- fs/ext4/file.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)fs/ext4/file.c +14
-0@@ -692,6 +692,20 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)if (IS_DAX (inode))return ext4_dax_write_iter (iocb, from);#endif + +if (iocb->ki_flags &IOCB_ATOMIC ) { +size_t len =iov_iter_count (from); +int ret; + +if (len <EXT4_SB (inode->i_sb)->s_awu_min || + len >EXT4_SB (inode->i_sb)->s_awu_max) +return -EINVAL ; + + ret =generic_atomic_write_valid (iocb, from); +if (ret) +return ret; + } +if (iocb->ki_flags &IOCB_DIRECT )return ext4_dio_write_iter (iocb, from);else Signature · 1 line
2.46.0
Re: [PATCH v3 3/4] ext4: Support setting FMODE_CAN_ATOMIC_WRITE
In reply to Ritesh Harjani (IBM)Quoted 15 lines · Ritesh Harjani (IBM)
FS needs to add the fmode capability in order to support atomic writes during file open (refer
kiocb_set_rw_flags()). Set this capability on a regular file if ext4 can do atomic write.Reviewed-by John Garry <[email protected]>Signed-off-by Ritesh Harjani (IBM) <[email protected]>--- fs/ext4/file.c | 3 +++ 1 file changed, 3 insertions(+)fs/ext4/file.c +0
-0
fs/ext4/file.c static int ext4_file_open(struct inode *inode, struct file *filp) · line 901
Quoted 4 lines
fs/ext4/file.c +1
-0@@ -898,6 +898,9 @@ static int ext4_file_open(struct inode *inode, struct file *filp)return ret; } +if (S_ISREG (inode->i_mode) &&ext4_can_atomic_write (inode->i_sb))
Modulo my comment earlier about ext4_can_atomic_write, this looks ok to me. With either variant, I say:
--D
Quoted 9 lines
fs/ext4/file.c +2
-0+ filp->f_mode |=FMODE_CAN_ATOMIC_WRITE ; + filp->f_mode |=FMODE_NOWAIT |FMODE_CAN_ODIRECT ;return dquot_file_open (inode, filp); }Signature · 1 line
2.46.0
Re: [PATCH v3 4/4] ext4: Do not fallback to buffered-io for DIO atomic write
In reply to Ritesh Harjani (IBM)Quoted 48 lines · Ritesh Harjani (IBM)
atomic writes is currently only supported for single fsblock and only for direct-io. We should not return
-ENOTBLKfor atomic writes since we want the atomic write request to either complete fully or fail otherwise. We should not fallback to buffered-io in case of DIO atomic write requests.
Let's also catch if this ever happens by adding someWARN_ON_ONCEbefore buffered-io handling for direct-io atomic writes.More details of the discussion [1].
[1]: https://lore.kernel.org/linux-xfs/[email protected]/T/#m9dbecc11bed713ed0d7a486432c56b105b555f04
Signed-off-by Ritesh Harjani (IBM) <[email protected]>--- fs/ext4/file.c | 7 +++++++ fs/ext4/inode.c | 14 +++++++++----- 2 files changed, 16 insertions(+), 5 deletions(-)fs/ext4/file.c +7
-0@@ -599,6 +599,13 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)ssize_t err;loff_t endbyte; +/* +* There is no support for atomic writes on buffered-io yet, +* we should never fallback to buffered-io for DIO atomic +* writes. +*/ +WARN_ON_ONCE (iocb->ki_flags &IOCB_ATOMIC ); + offset = iocb->ki_pos; err =ext4_buffered_write_iter (iocb, from);if (err <0 )fs/ext4/inode.c +0
-4@@ -3449,12 +3449,16 @@ static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length, {/* * Check to see whether an error occurred while writing out the data to -* the allocated blocks. If so, return the magic error code so that we -* fallback to buffered I/O and attempt to complete the remainder of -* the I/O. Any blocks that may have been allocated in preparation for -* the direct I/O will be reused during buffered I/O.
fs/ext4/inode.c static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length, · line 3460
Quoted 10 lines
fs/ext4/inode.c +8
-1+* the allocated blocks. If so, return the magic error code for +* non-atomic write so that we fallback to buffered I/O and attempt to +* complete the remainder of the I/O. +* For atomic writes we will simply fail the I/O request if we coudn't +* write anything. For non-atomic writes, any blocks that may have been +* allocated in preparation for the direct I/O will be reused during +* buffered I/O. */ -if (flags & (IOMAP_WRITE |IOMAP_DIRECT ) && written ==0 ) + if (!(flags & IOMAP_ATOMIC) && (flags & (IOMAP_WRITE | IOMAP_DIRECT))
Huh. The WRITE|DIRECT check doesn't look right to me, because the expression returns true for any write or any directio. I think that's currently "ok" because ext4_iomap_end is only called for directio writes, but this bugs me anyway. For a directio write fallback, that comparison really should be:
(flags & (WRITE|DIRECT)) == (WRITE|DIRECT)
static inline bool ext4_want_directio_fallback(unsigned flags, ssize_t written)
{
/* must be a directio to fall back to buffered */
if (flags & (IOMAP_WRITE | IOMAP_DIRECT)) !=
(IOMAP_WRITE | IOMAP_DIRECT)
return false;
/* atomic writes are all-or-nothing */
if (flags & IOMAP_ATOMIC)
return false;
/* can only try again if we wrote nothing */
return written == 0;
}if (ext4_want_directio_fallback(flags, written)) return -ENOTBLK;
fs/ext4/inode.c static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length, · line 3461
Quoted 1 line
fs/ext4/inode.c +1
-0+ && written == 0)
Nit: put the '&&' operator on the previous line when there's a multiline expression.
--D
Quoted 7 lines
fs/ext4/inode.c +0
-0return -ENOTBLK ;return 0 ;Signature · 1 line
2.46.0
On Wed, Oct 30, 2024 at 09:27:37PM +0530, Ritesh Harjani (IBM) wrote:
Assuming Ted acks this series, I have a fun question: Can we merge this for 6.13 alongside the single-fsblock xfs implementation?
And how do we want to merge this? It looks like Jens took only the first three patches from John's series, leaving this:
https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/log/?h=for-6.13/block-atomic
[PATCH v10 4/8] fs: Export generic_atomic_write_valid() John Garry [PATCH v10 5/8] fs: iomap: Atomic write support John Garry [PATCH v10 6/8] xfs: Support atomic write for statx John Garry [PATCH v10 7/8] xfs: Validate atomic writes John Garry [PATCH v10 8/8] xfs: Support setting FMODE_CAN_ATOMIC_WRITE John Garry
Note the fs and iomap stuff is not in that branch.
So should xfs create a 6.13 merge branch from block-atomic containing all of its new stuff including the xfs atomic writes changes? And then I guess merge the ext4 changes too?? ext4 code coming in via xfs, yuck.
Or should cem just create a 6.13 merge branch with everything *except* the atomic writes stuff? Call that branch "xfs-6.13-merge". Then one of us with commit privileges creates a separate branch off of block-atomic, add both the xfs series and then the ext4 series? Call that branch "fs-atomic-writes".
Then I guess cem could create a third branch from xfs-6.13-merge, merge the fs-atomic-writes branch into that third branch, and push that third branch to for-next on git.kernel.org so it can get picked up by rothwell's for-next and fs-next?
(Note that Ted could do likewise with ext4; cem doesn't have to be part of this.)
Does that work for people? The "sending multiple branches to linus" way is the best method I can think of, though it's more release manager work.
--D
Quoted 72 lines
v2 -> v3:
==========
1. Patch-1 adds an "experimental" string in dmesg log during mount when EXT4 detects that it is capable of doing DIO atomic writes on a given device with min and max unit details.
2. Patch-4 has been updated to avoid returning-ENOTBLK(inext4_iomap_end) if the request belongs to atomic write. This patch also adds aWARN_ON_ONCE()if atomic write ever fallback to buffered-io (to catch any unwanted bugs in the future). More details in the commit log of patch-4.
3. Collected RBs tag from John for Patch 2 & 3.[v2]: https://lore.kernel.org/linux-ext4/[email protected]/
Previous cover letter log:
In v2, we had split the series and this one only takes care of atomic writes for single fsblock.
That means for now this gets only enabled on bs < ps systems on ext4. Enablement of atomic writes for bigalloc (multi-fsblock support) is still under discussion and may require general consensus within the filesystem community [1].This series adds the base feature support to enable atomic writes in direct-io path for ext4. We advertise the minimum and the maximum atomic write unit sizes via statx on a regular file.
This series allows users to utilize atomic write support using -
1. on bs < ps systems via - mkfs.ext4 -F -b 16384/dev/sdaThis can then be utilized using -
xfs_io -fdc "pwrite -V 1 -A -b16k 0 16k" /mnt/f1This is built on top of John's DIO atomic write series for XFS [2]. The VFS and block layer enablement for atomic writes were merged already.
[1]: https://lore.kernel.org/linux-ext4/[email protected]
[2]: https://lore.kernel.org/linux-xfs/[email protected]/Changelogs:
===========
PATCH -> PATCH v2:
- addressed review comments from John and Darrick.
- renamedext4_sb_infovariables names: fs_awu* -> s_awu*
- [PATCH]: https://lore.kernel.org/linux-ext4/[email protected]/RFC -> PATCH:
- Dropped RFC tag
- Last RFC was posted a while ago but back then a lot of VFS and block layer interfaces were still not merged. Those are now merged, thanks to John and everyone else.
- [RFC] - https://lore.kernel.org/linux-ext4/[email protected]/Ritesh Harjani (IBM) (4):
ext4: Add statx support for atomic writes
ext4: Check for atomic writes support in write iter
ext4: Support settingFMODE_CAN_ATOMIC_WRITE
ext4: Do not fallback to buffered-io for DIO atomic writefs/ext4/ext4.h | 9 +++++++++ fs/ext4/file.c | 24 ++++++++++++++++++++++++ fs/ext4/inode.c | 28 +++++++++++++++++++++++----- fs/ext4/super.c | 31 +++++++++++++++++++++++++++++++ 4 files changed, 87 insertions(+), 5 deletions(-)--
2.46.0
Ritesh Harjani (IBM)
reviewerRe: [PATCH v3 1/4] ext4: Add statx support for atomic writes
In reply to Darrick J. WongHi John & Darrick,
Quoted 41 lines · Darrick J. Wong
Quoted 30 lines · Ritesh Harjani (IBM)
This patch adds base support for atomic writes via statx getattr. On bs < ps systems, we can create FS with say bs of 16k. That means both atomic write min and max unit can be set to 16k for supporting atomic writes.
Co-developed-by Ojaswin Mujoo <[email protected]>Signed-off-by Ojaswin Mujoo <[email protected]>Signed-off-by Ritesh Harjani (IBM) <[email protected]>--- fs/ext4/ext4.h | 9 +++++++++ fs/ext4/inode.c | 14 ++++++++++++++ fs/ext4/super.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+)fs/ext4/ext4.h +4
-0@@ -1729,6 +1729,10 @@ struct ext4_sb_info {*/ struct work_struct s_sb_upd_work; +/* Atomic write unit values in bytes */ +unsigned int s_awu_min; +unsigned int s_awu_max; +/* Ext4 fast commit sub transaction ID */ atomic_t s_fc_subtid;fs/ext4/ext4.h static inline int ext4_buffer_uptodate(struct buffer_head *bh) · line 3864
Quoted 6 lines
fs/ext4/ext4.h +3
-0@@ -3855,6 +3859,11 @@ static inline int ext4_buffer_uptodate(struct buffer_head *bh)return buffer_uptodate (bh); } +static inline bool ext4_can_atomic_write (struct super_block *sb) +{ +return EXT4_SB (sb)->s_awu_min >0 ;Huh, I was expecting you to stick to passing in the struct inode, and then you end up with:
Quoted 6 lines
static inline bool
ext4_can_atomic_write(struct inode *inode){ return S_ISREG(inode->i_mode) && EXT4_SB(inode->i_sb)->s_awu_min > 0); }
Ok. John also had commented on the same thing before.
We may only need this, when ext4 get extsize hint support. But for now we mainly only need to check that EXT4 SB supports atomic write or not. i.e. s_awu_min should be greater than 0.
But sure I can make above suggested change to keep it consistent with XFS, along with below discussed change (Please have a look)...
Quoted 28 lines
Quoted 10 lines
fs/ext4/ext4.h +2
-0+} +extern int ext4_block_write_begin (handle_t *handle,struct folio *folio,loff_t pos,unsigned len,get_block_t *get_block);fs/ext4/inode.c +0
-0fs/ext4/inode.c int ext4_getattr(struct mnt_idmap *idmap, const struct path *path, · line 5581
Quoted 4 lines
fs/ext4/inode.c +1
-0@@ -5578,6 +5578,20 @@ int ext4_getattr(struct mnt_idmap *idmap, const struct path *path, } } +if (S_ISREG (inode->i_mode) && (request_mask &STATX_WRITE_ATOMIC )) {...and then the callsites become:
if (request_mask & STATX_WRITE_ATOMIC) { unsigned int awu_min = 0, awu_max = 0; if (ext4_can_atomic_write(inode)) { awu_min = sbi->s_awu_min; awu_max = sbi->s_awu_max; } generic_fill_statx_atomic_writes(stat, awu_min, awu_max); }
Quoted 2 lines
(I forget, is it bad if statx to a directory returns
STATX_WRITE_ATOMICeven with awu_{min,max} set to zero?)
I mainly kept it consistent with XFS. But it's not a bad idea to do that.
That will help applications check for atomic write support on the root
directory mount point rather than creating a regular file just for
verification. Because of below result_mask, which we only set within generic_fill_statx_atomic_writes()
stat->result_mask |= STATX_WRITE_ATOMIC;
If we make this change to ext4, XFS will have to fix it too, to keep the behavior consistent for both.
Shall I go ahead and make the change in v4 for EXT4?
-ritesh
Quoted 74 lines
Other than that nit, this looks good to me.
--D
Quoted 69 lines
fs/ext4/inode.c +13
-0+struct ext4_sb_info *sbi =EXT4_SB (inode->i_sb); +unsigned int awu_min, awu_max; + +if (ext4_can_atomic_write (inode->i_sb)) { + awu_min = sbi->s_awu_min; + awu_max = sbi->s_awu_max; + }else { + awu_min = awu_max =0 ; + } + +generic_fill_statx_atomic_writes (stat, awu_min, awu_max); + } + flags = ei->i_flags &EXT4_FL_USER_VISIBLE ;if (flags &EXT4_APPEND_FL ) stat->attributes |=STATX_ATTR_APPEND ;fs/ext4/super.c +31
-0@@ -4425,6 +4425,36 @@ static int ext4_handle_clustersize(struct super_block *sb)return 0 ; } +/* +* ext4_atomic_write_init: Initializes filesystem min & max atomic write units. +* @sb: super block +* TODO: Later add support for bigalloc +*/ +static void ext4_atomic_write_init (struct super_block *sb) +{ +struct ext4_sb_info *sbi =EXT4_SB (sb); +struct block_device *bdev = sb->s_bdev; + +if (!bdev_can_atomic_write (bdev)) +return ; + +if (!ext4_has_feature_extents (sb)) +return ; + + sbi->s_awu_min =max (sb->s_blocksize, +bdev_atomic_write_unit_min_bytes (bdev)); + sbi->s_awu_max =min (sb->s_blocksize, +bdev_atomic_write_unit_max_bytes (bdev)); +if (sbi->s_awu_min && sbi->s_awu_max && + sbi->s_awu_min <= sbi->s_awu_max) { +ext4_msg (sb,KERN_NOTICE ,"Supports (experimental) DIO atomic writes awu_min: %u, awu_max: %u" , + sbi->s_awu_min, sbi->s_awu_max); + }else { + sbi->s_awu_min =0 ; + sbi->s_awu_max =0 ; + } +} +static void ext4_fast_commit_init (struct super_block *sb) {struct ext4_sb_info *sbi =EXT4_SB (sb);@@ -5336,6 +5366,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)spin_lock_init (&sbi->s_bdev_wb_lock); +ext4_atomic_write_init (sb);ext4_fast_commit_init (sb); sb->s_root =NULL ;Signature · 1 line
2.46.0
Ritesh Harjani (IBM)
reviewerRe: [PATCH v3 4/4] ext4: Do not fallback to buffered-io for DIO atomic write
In reply to Darrick J. WongQuoted 66 lines · Darrick J. Wong
Quoted 48 lines · Ritesh Harjani (IBM)
atomic writes is currently only supported for single fsblock and only for direct-io. We should not return
-ENOTBLKfor atomic writes since we want the atomic write request to either complete fully or fail otherwise. We should not fallback to buffered-io in case of DIO atomic write requests.
Let's also catch if this ever happens by adding someWARN_ON_ONCEbefore buffered-io handling for direct-io atomic writes.More details of the discussion [1].
[1]: https://lore.kernel.org/linux-xfs/[email protected]/T/#m9dbecc11bed713ed0d7a486432c56b105b555f04
Signed-off-by Ritesh Harjani (IBM) <[email protected]>--- fs/ext4/file.c | 7 +++++++ fs/ext4/inode.c | 14 +++++++++----- 2 files changed, 16 insertions(+), 5 deletions(-)fs/ext4/file.c +7
-0@@ -599,6 +599,13 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)ssize_t err;loff_t endbyte; +/* +* There is no support for atomic writes on buffered-io yet, +* we should never fallback to buffered-io for DIO atomic +* writes. +*/ +WARN_ON_ONCE (iocb->ki_flags &IOCB_ATOMIC ); + offset = iocb->ki_pos; err =ext4_buffered_write_iter (iocb, from);if (err <0 )fs/ext4/inode.c +0
-4@@ -3449,12 +3449,16 @@ static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length, {/* * Check to see whether an error occurred while writing out the data to -* the allocated blocks. If so, return the magic error code so that we -* fallback to buffered I/O and attempt to complete the remainder of -* the I/O. Any blocks that may have been allocated in preparation for -* the direct I/O will be reused during buffered I/O. fs/ext4/inode.c static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length, · line 3460
Quoted 10 lines
fs/ext4/inode.c +8
-1+* the allocated blocks. If so, return the magic error code for +* non-atomic write so that we fallback to buffered I/O and attempt to +* complete the remainder of the I/O. +* For atomic writes we will simply fail the I/O request if we coudn't +* write anything. For non-atomic writes, any blocks that may have been +* allocated in preparation for the direct I/O will be reused during +* buffered I/O. */ -if (flags & (IOMAP_WRITE |IOMAP_DIRECT ) && written ==0 ) + if (!(flags & IOMAP_ATOMIC) && (flags & (IOMAP_WRITE | IOMAP_DIRECT))Huh. The WRITE|DIRECT check doesn't look right to me, because the expression returns true for any write or any directio. I think that's currently "ok" because
ext4_iomap_endis only called for directio writes, but this bugs me anyway. For a directio write fallback, that comparison really should be:
Quoted 2 lines
(flags & (WRITE|DIRECT)) == (WRITE|DIRECT)
yes. You are right. It is working since ext4 only supports iomap for DIRECTIO. But I agree it's better be fixed to avoid problem in future.
Quoted 16 lines
static inline bool
ext4_want_directio_fallback(unsigned flags, ssize_t written){ /* must be a directio to fall back to buffered */ if (flags & (IOMAP_WRITE | IOMAP_DIRECT)) != (IOMAP_WRITE | IOMAP_DIRECT) return false; /* atomic writes are all-or-nothing */ if (flags & IOMAP_ATOMIC) return false; /* can only try again if we wrote nothing */ return written == 0; }
Quoted 3 lines
if (ext4_want_directio_fallback(flags, written)) return -ENOTBLK;
I like the above helper. Thanks for doing that.
I will incorporate this in v4.
Quoted 5 lines
fs/ext4/inode.c static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length, · line 3461
Quoted 1 line
fs/ext4/inode.c +1
-0+ && written == 0)Nit: put the '&&' operator on the previous line when there's a multiline expression.
I guess we don't need this if we do it with your above inline helper. But sure, next time will keep in mind for any such changes.
Quoted 2 lines
--D
Thanks for the review!
-ritesh
Quoted 7 lines
Quoted 7 lines
fs/ext4/inode.c +0
-0return -ENOTBLK ;return 0 ;Signature · 1 line
2.46.0
Re: [PATCH v3 1/4] ext4: Add statx support for atomic writes
In reply to Ritesh Harjani (IBM)Quoted 97 lines · Ritesh Harjani
Hi John & Darrick,
Quoted 41 lines · Darrick J. Wong
Quoted 30 lines · Ritesh Harjani (IBM)
This patch adds base support for atomic writes via statx getattr. On bs < ps systems, we can create FS with say bs of 16k. That means both atomic write min and max unit can be set to 16k for supporting atomic writes.
Co-developed-by Ojaswin Mujoo <[email protected]>Signed-off-by Ojaswin Mujoo <[email protected]>Signed-off-by Ritesh Harjani (IBM) <[email protected]>--- fs/ext4/ext4.h | 9 +++++++++ fs/ext4/inode.c | 14 ++++++++++++++ fs/ext4/super.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+)fs/ext4/ext4.h +4
-0@@ -1729,6 +1729,10 @@ struct ext4_sb_info {*/ struct work_struct s_sb_upd_work; +/* Atomic write unit values in bytes */ +unsigned int s_awu_min; +unsigned int s_awu_max; +/* Ext4 fast commit sub transaction ID */ atomic_t s_fc_subtid;fs/ext4/ext4.h static inline int ext4_buffer_uptodate(struct buffer_head *bh) · line 3864
Quoted 6 lines
fs/ext4/ext4.h +3
-0@@ -3855,6 +3859,11 @@ static inline int ext4_buffer_uptodate(struct buffer_head *bh)return buffer_uptodate (bh); } +static inline bool ext4_can_atomic_write (struct super_block *sb) +{ +return EXT4_SB (sb)->s_awu_min >0 ;Huh, I was expecting you to stick to passing in the struct inode, and then you end up with:
Quoted 6 lines
static inline bool
ext4_can_atomic_write(struct inode *inode){ return S_ISREG(inode->i_mode) && EXT4_SB(inode->i_sb)->s_awu_min > 0); }Ok. John also had commented on the same thing before.
We may only need this, when ext4 get extsize hint support. But for now we mainly only need to check that EXT4 SB supports atomic write or not. i.e.s_awu_minshould be greater than 0.But sure I can make above suggested change to keep it consistent with XFS, along with below discussed change (Please have a look)...
Quoted 28 lines
Quoted 10 lines
fs/ext4/ext4.h +2
-0+} +extern int ext4_block_write_begin (handle_t *handle,struct folio *folio,loff_t pos,unsigned len,get_block_t *get_block);fs/ext4/inode.c +0
-0fs/ext4/inode.c int ext4_getattr(struct mnt_idmap *idmap, const struct path *path, · line 5581
Quoted 4 lines
fs/ext4/inode.c +1
-0@@ -5578,6 +5578,20 @@ int ext4_getattr(struct mnt_idmap *idmap, const struct path *path, } } +if (S_ISREG (inode->i_mode) && (request_mask &STATX_WRITE_ATOMIC )) {...and then the callsites become:
if (request_mask & STATX_WRITE_ATOMIC) { unsigned int awu_min = 0, awu_max = 0; if (ext4_can_atomic_write(inode)) { awu_min = sbi->s_awu_min; awu_max = sbi->s_awu_max; } generic_fill_statx_atomic_writes(stat, awu_min, awu_max); }Quoted 2 lines
(I forget, is it bad if statx to a directory returns
STATX_WRITE_ATOMICeven with awu_{min,max} set to zero?)I mainly kept it consistent with XFS. But it's not a bad idea to do that.
That will help applications check for atomic write support on the root
directory mount point rather than creating a regular file just for
verification. Because of belowresult_mask, which we only set withingeneric_fill_statx_atomic_writes()
Quoted 5 lines
stat->result_mask |= STATX_WRITE_ATOMIC;If we make this change to ext4, XFS will have to fix it too, to keep the behavior consistent for both.
Shall I go ahead and make the change in v4 for EXT4?
Hmmm, that's a good question -- if a program asks for STATX_WRITE_ATOMIC on a directory, should we set the ATOMIC flag in statx.stx_mask but leave the values as zeroes if the underlying block device/fs supports atomic writes at all? For XFS I guess the "underlying bdev" is determined by the directory's RTINHERIT bit && xfs_has_realtime().
Thoughts?
But maybe that doesn't make sense since (a) fundamentally you can't do a directio write to a directory and (b) it's not that hard to create a file, set the REALTIME bit on it as desired (on xfs) and then query the untorn write geometry. So maybe that check should be:
if (request_mask & STATX_WRITE_ATOMIC && S_ISREG(inode->i_mode))
--D
Quoted 77 lines
-ritesh
Quoted 74 lines
Other than that nit, this looks good to me.
--D
Quoted 69 lines
fs/ext4/inode.c +13
-0+struct ext4_sb_info *sbi =EXT4_SB (inode->i_sb); +unsigned int awu_min, awu_max; + +if (ext4_can_atomic_write (inode->i_sb)) { + awu_min = sbi->s_awu_min; + awu_max = sbi->s_awu_max; + }else { + awu_min = awu_max =0 ; + } + +generic_fill_statx_atomic_writes (stat, awu_min, awu_max); + } + flags = ei->i_flags &EXT4_FL_USER_VISIBLE ;if (flags &EXT4_APPEND_FL ) stat->attributes |=STATX_ATTR_APPEND ;fs/ext4/super.c +31
-0@@ -4425,6 +4425,36 @@ static int ext4_handle_clustersize(struct super_block *sb)return 0 ; } +/* +* ext4_atomic_write_init: Initializes filesystem min & max atomic write units. +* @sb: super block +* TODO: Later add support for bigalloc +*/ +static void ext4_atomic_write_init (struct super_block *sb) +{ +struct ext4_sb_info *sbi =EXT4_SB (sb); +struct block_device *bdev = sb->s_bdev; + +if (!bdev_can_atomic_write (bdev)) +return ; + +if (!ext4_has_feature_extents (sb)) +return ; + + sbi->s_awu_min =max (sb->s_blocksize, +bdev_atomic_write_unit_min_bytes (bdev)); + sbi->s_awu_max =min (sb->s_blocksize, +bdev_atomic_write_unit_max_bytes (bdev)); +if (sbi->s_awu_min && sbi->s_awu_max && + sbi->s_awu_min <= sbi->s_awu_max) { +ext4_msg (sb,KERN_NOTICE ,"Supports (experimental) DIO atomic writes awu_min: %u, awu_max: %u" , + sbi->s_awu_min, sbi->s_awu_max); + }else { + sbi->s_awu_min =0 ; + sbi->s_awu_max =0 ; + } +} +static void ext4_fast_commit_init (struct super_block *sb) {struct ext4_sb_info *sbi =EXT4_SB (sb);@@ -5336,6 +5366,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)spin_lock_init (&sbi->s_bdev_wb_lock); +ext4_atomic_write_init (sb);ext4_fast_commit_init (sb); sb->s_root =NULL ;Signature · 1 line
2.46.0