ext4: Add atomic writes support for DIO
Subsystems (2)
Ritesh Harjani (IBM)
reviewerv4 cover ext4: Add atomic writes support for DIO
v3 -> v4:
=========
1. Patch-1 changes the helper function from ext4_can_atomic_write() to ext4_inode_can_atomic_write() based on suggestions from Darrick and John. Patch-3 then goes and uses it.
2. Patch-4 adds an inline helper ext4_want_directio_fallback() which simplifies the logic checks and inherently fixes condition on when to return -ENOTBLK which otherwise was always returning true for any write or directio in ext4_iomap_end(). It was ok since ext4 only supports direct-io via iomap.
[v3]: https://lore.kernel.org/linux-xfs/[email protected]/
(Note as mentioned below as well this is built on top of John XFS atomic series [2]) [2]: https://lore.kernel.org/linux-xfs/[email protected]/
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 | 10 ++++++++++ fs/ext4/file.c | 24 ++++++++++++++++++++++++ fs/ext4/inode.c | 39 ++++++++++++++++++++++++++++++++++----- fs/ext4/super.c | 31 +++++++++++++++++++++++++++++++ 4 files changed, 99 insertions(+), 5 deletions(-)
--
2.46.0
Ritesh Harjani (IBM)
reviewerPATCH v4 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 | 10 ++++++++++ fs/ext4/inode.c | 12 ++++++++++++ fs/ext4/super.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+)
fs/ext4/ext4.h +10 -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,12 @@ static inline int ext4_buffer_uptodate(struct buffer_head *bh)return buffer_uptodate (bh); } +static inline bool ext4_inode_can_atomic_write (struct inode *inode) +{ + +return S_ISREG (inode->i_mode) &&EXT4_SB (inode->i_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 +12 -0
@@ -5578,6 +5578,18 @@ int ext4_getattr(struct mnt_idmap *idmap, const struct path *path, } } +if ((request_mask &STATX_WRITE_ATOMIC ) &&S_ISREG (inode->i_mode)) { +struct ext4_sb_info *sbi =EXT4_SB (inode->i_sb); +unsigned int awu_min =0 , awu_max =0 ; + +if (ext4_inode_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); + } + 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 v4 1/4] ext4: Add statx support for atomic writes
Quoted 8 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]>
I guess this is a good start. Feel free to add:
Honza
Quoted 109 lines
--- fs/ext4/ext4.h | 10 ++++++++++ fs/ext4/inode.c | 12 ++++++++++++ fs/ext4/super.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+)fs/ext4/ext4.h +10
-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,12 @@ static inline int ext4_buffer_uptodate(struct buffer_head *bh)return buffer_uptodate (bh); } +static inline bool ext4_inode_can_atomic_write (struct inode *inode) +{ + +return S_ISREG (inode->i_mode) &&EXT4_SB (inode->i_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 +12
-0@@ -5578,6 +5578,18 @@ int ext4_getattr(struct mnt_idmap *idmap, const struct path *path, } } +if ((request_mask &STATX_WRITE_ATOMIC ) &&S_ISREG (inode->i_mode)) { +struct ext4_sb_info *sbi =EXT4_SB (inode->i_sb); +unsigned int awu_min =0 , awu_max =0 ; + +if (ext4_inode_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); + } + 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
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH v4 1/4] ext4: Add statx support for atomic writes
In reply to Ritesh Harjani (IBM)Quoted 8 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]>
Looks good to me,
--D
Quoted 110 lines
--- fs/ext4/ext4.h | 10 ++++++++++ fs/ext4/inode.c | 12 ++++++++++++ fs/ext4/super.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+)fs/ext4/ext4.h +10
-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,12 @@ static inline int ext4_buffer_uptodate(struct buffer_head *bh)return buffer_uptodate (bh); } +static inline bool ext4_inode_can_atomic_write (struct inode *inode) +{ + +return S_ISREG (inode->i_mode) &&EXT4_SB (inode->i_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 +12
-0@@ -5578,6 +5578,18 @@ int ext4_getattr(struct mnt_idmap *idmap, const struct path *path, } } +if ((request_mask &STATX_WRITE_ATOMIC ) &&S_ISREG (inode->i_mode)) { +struct ext4_sb_info *sbi =EXT4_SB (inode->i_sb); +unsigned int awu_min =0 , awu_max =0 ; + +if (ext4_inode_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); + } + 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 v4 1/4] ext4: Add statx support for atomic writes
In reply to Ritesh Harjani (IBM)Quoted 8 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]>
Regardless of nitpicks:
Quoted 22 lines
--- fs/ext4/ext4.h | 10 ++++++++++ fs/ext4/inode.c | 12 ++++++++++++ fs/ext4/super.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 53 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,12 @@ static inline int ext4_buffer_uptodate(struct buffer_head *bh)return buffer_uptodate (bh); } +static inline bool ext4_inode_can_atomic_write (struct inode *inode) +{ +
nit: superfluous blank line
fs/ext4/ext4.h static inline int ext4_buffer_uptodate(struct buffer_head *bh) · line 3865
Quoted 1 line
fs/ext4/ext4.h +1
-0+return S_ISREG (inode->i_mode) &&EXT4_SB (inode->i_sb)->s_awu_min >0 ;
I am not sure if the S_ISREG() check is required. Other callers also do the check (like ext4_getattr() for when calling ext4_inode_can_atomic_write()) or don't need it (ext4_file_open()). I say ext4_file_open() doesn't need it as ext4_file_open() is only ever called for regular files, right?
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,18 @@ int ext4_getattr(struct mnt_idmap *idmap, const struct path *path, } } +if ((request_mask &STATX_WRITE_ATOMIC ) &&S_ISREG (inode->i_mode)) {
nit: maybe you could have factored out the S_ISREG() check with STATX_DIOALIGN
Quoted 63 lines
fs/ext4/inode.c +11
-0+struct ext4_sb_info *sbi =EXT4_SB (inode->i_sb); +unsigned int awu_min =0 , awu_max =0 ; + +if (ext4_inode_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); + } + 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 ;
Ritesh Harjani (IBM)
reviewerRe: [PATCH v4 1/4] ext4: Add statx support for atomic writes
Quoted 6 lines · John Garry
Quoted 5 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.
Quoted 8 lines
Quoted 3 lines
Co-developed-by Ojaswin Mujoo <[email protected]>Signed-off-by Ojaswin Mujoo <[email protected]>Signed-off-by Ritesh Harjani (IBM) <[email protected]>Regardless of nitpicks:
Reviewed-by John Garry <[email protected]>
Thanks John for the review!
Since as you too mentioned the remaining points are minor and not critical review comments, I will address them next time in the multi-fsblock variant. With all other aspects now finalized in this v4 version, this looks ready to be picked up for the merge window.
-ritesh
Quoted 25 lines
Quoted 25 lines
--- fs/ext4/ext4.h | 10 ++++++++++ fs/ext4/inode.c | 12 ++++++++++++ fs/ext4/super.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 53 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;@@ -3855,6 +3859,12 @@ static inline int ext4_buffer_uptodate(struct buffer_head *bh)return buffer_uptodate (bh); }
Quoted 6 lines
fs/ext4/ext4.h static inline int ext4_buffer_uptodate(struct buffer_head *bh) · line 3864
Quoted 3 lines
fs/ext4/ext4.h +3
-0+static inline bool ext4_inode_can_atomic_write (struct inode *inode) +{ +nit: superfluous blank line
Sure.
Quoted 8 lines
fs/ext4/ext4.h static inline int ext4_buffer_uptodate(struct buffer_head *bh) · line 3865
Quoted 1 line
fs/ext4/ext4.h +1
-0+return S_ISREG (inode->i_mode) &&EXT4_SB (inode->i_sb)->s_awu_min >0 ;I am not sure if the
S_ISREG()check is required. Other callers also do the check (likeext4_getattr()for when callingext4_inode_can_atomic_write()) or don't need it (ext4_file_open()). I sayext4_file_open()doesn't need it asext4_file_open()is only ever called for regular files, right?
Yes. However I believe we might end up using this from other places when we add support of extsize. So we might need S_ISREG check. But sure let me re-think on that during the multi-fsblock variant time.
Quoted 13 lines
Quoted 13 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@@ -5578,6 +5578,18 @@ int ext4_getattr(struct mnt_idmap *idmap, const struct path *path, } }
Quoted 5 lines
fs/ext4/inode.c int ext4_getattr(struct mnt_idmap *idmap, const struct path *path, · line 5581
Quoted 1 line
fs/ext4/inode.c +1
-0+if ((request_mask &STATX_WRITE_ATOMIC ) &&S_ISREG (inode->i_mode)) {nit: maybe you could have factored out the
S_ISREG()check withSTATX_DIOALIGN
Sure.
Quoted 63 lines
Quoted 63 lines
fs/ext4/inode.c +11
-0+struct ext4_sb_info *sbi =EXT4_SB (inode->i_sb); +unsigned int awu_min =0 , awu_max =0 ; + +if (ext4_inode_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); + } + 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 ;
Ritesh Harjani (IBM)
reviewerPATCH v4 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
--
2.46.0
Re: [PATCH v4 2/4] ext4: Check for atomic writes support in write iter
Quoted 7 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]>Reviewed-by Darrick J. Wong <[email protected]>Signed-off-by Ritesh Harjani (IBM) <[email protected]>
Looks good. Feel free to add:
Honza
Quoted 32 lines
--- fs/ext4/file.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)fs/ext4/file.c +14
-1@@ -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 --2.46.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Ritesh Harjani (IBM)
reviewerPATCH v4 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 (ext4_inode_can_atomic_write (inode)) + filp->f_mode |=FMODE_CAN_ATOMIC_WRITE ; + filp->f_mode |=FMODE_NOWAIT |FMODE_CAN_ODIRECT ;return dquot_file_open (inode, filp); }
--
2.46.0
Re: [PATCH v4 3/4] ext4: Support setting FMODE_CAN_ATOMIC_WRITE
Quoted 7 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]>Reviewed-by Darrick J. Wong <[email protected]>Signed-off-by Ritesh Harjani (IBM) <[email protected]>
Looks good. Feel free to add:
Honza
Quoted 21 lines
--- fs/ext4/file.c | 3 +++ 1 file changed, 3 insertions(+)fs/ext4/file.c +3
-1@@ -898,6 +898,9 @@ static int ext4_file_open(struct inode *inode, struct file *filp)return ret; } +if (ext4_inode_can_atomic_write (inode)) + filp->f_mode |=FMODE_CAN_ATOMIC_WRITE ; + filp->f_mode |=FMODE_NOWAIT |FMODE_CAN_ODIRECT ;return dquot_file_open (inode, filp); } --2.46.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Ritesh Harjani (IBM)
reviewerPATCH v4 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. Hence, we should never 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].
While at it let's add an inline helper ext4_want_directio_fallback() which simplifies the logic checks and inherently fixes condition on when to return -ENOTBLK which otherwise was always returning true for any write or directio in ext4_iomap_end(). It was ok since ext4 only supports direct-io via iomap.
[1]: https://lore.kernel.org/linux-xfs/[email protected]/T/#m9dbecc11bed713ed0d7a486432c56b105b555f04
--- fs/ext4/file.c | 7 +++++++ fs/ext4/inode.c | 27 ++++++++++++++++++++++----- 2 files changed, 29 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 +22 -5
@@ -3444,17 +3444,34 @@ static int ext4_iomap_overwrite_begin(struct inode *inode, loff_t offset,return ret; } +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 ; +} +static int ext4_iomap_end (struct inode *inode,loff_t offset,loff_t length,ssize_t written,unsigned flags,struct iomap *iomap) {/* * 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 non-atomic writes, any blocks that may have been +* allocated in preparation for the direct I/O will be reused during +* buffered I/O. For atomic write, we never fallback to buffered-io. */ -if (flags & (IOMAP_WRITE |IOMAP_DIRECT ) && written ==0 ) +if (ext4_want_directio_fallback (flags, written))return -ENOTBLK ;return 0 ;
--
2.46.0
Re: [PATCH v4 4/4] ext4: Do not fallback to buffered-io for DIO atomic write
Quoted 9 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. Hence, we should never 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].
Quoted 8 lines
While at it let's add an inline helper
ext4_want_directio_fallback()which simplifies the logic checks and inherently fixes condition on when to return-ENOTBLKwhich otherwise was always returning true for any write or directio inext4_iomap_end(). It was ok since ext4 only supports direct-io via iomap.[1]: https://lore.kernel.org/linux-xfs/[email protected]/T/#m9dbecc11bed713ed0d7a486432c56b105b555f04
Suggested-by Darrick J. Wong <[email protected]> # inline helperSigned-off-by Ritesh Harjani (IBM) <[email protected]>
Looks good. Feel free to add:
Honza
Quoted 70 lines
--- fs/ext4/file.c | 7 +++++++ fs/ext4/inode.c | 27 ++++++++++++++++++++++----- 2 files changed, 29 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 +22
-6@@ -3444,17 +3444,34 @@ static int ext4_iomap_overwrite_begin(struct inode *inode, loff_t offset,return ret; } +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 ; +} +static int ext4_iomap_end (struct inode *inode,loff_t offset,loff_t length,ssize_t written,unsigned flags,struct iomap *iomap) {/* * 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 non-atomic writes, any blocks that may have been +* allocated in preparation for the direct I/O will be reused during +* buffered I/O. For atomic write, we never fallback to buffered-io. */ -if (flags & (IOMAP_WRITE |IOMAP_DIRECT ) && written ==0 ) +if (ext4_want_directio_fallback (flags, written))return -ENOTBLK ;return 0 ; --2.46.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH v4 4/4] ext4: Do not fallback to buffered-io for DIO atomic write
In reply to Ritesh Harjani (IBM)Quoted 9 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. Hence, we should never 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].
Quoted 7 lines
While at it let's add an inline helper
ext4_want_directio_fallback()which simplifies the logic checks and inherently fixes condition on when to return-ENOTBLKwhich otherwise was always returning true for any write or directio inext4_iomap_end(). It was ok since ext4 only supports direct-io via iomap.[1]: https://lore.kernel.org/linux-xfs/[email protected]/T/#m9dbecc11bed713ed0d7a486432c56b105b555f04
Suggested-by Darrick J. Wong <[email protected]> # inline helper
Looks good to me now,
--D
Quoted 72 lines
Signed-off-by Ritesh Harjani (IBM) <[email protected]>--- fs/ext4/file.c | 7 +++++++ fs/ext4/inode.c | 27 ++++++++++++++++++++++----- 2 files changed, 29 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 +22
-6@@ -3444,17 +3444,34 @@ static int ext4_iomap_overwrite_begin(struct inode *inode, loff_t offset,return ret; } +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 ; +} +static int ext4_iomap_end (struct inode *inode,loff_t offset,loff_t length,ssize_t written,unsigned flags,struct iomap *iomap) {/* * 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 non-atomic writes, any blocks that may have been +* allocated in preparation for the direct I/O will be reused during +* buffered I/O. For atomic write, we never fallback to buffered-io. */ -if (flags & (IOMAP_WRITE |IOMAP_DIRECT ) && written ==0 ) +if (ext4_want_directio_fallback (flags, written))return -ENOTBLK ;return 0 ; --2.46.0
kernel test robot
build botRe: [PATCH v4 4/4] ext4: Do not fallback to buffered-io for DIO atomic write
In reply to Ritesh Harjani (IBM)kernel test robot noticed the following build errors:
Hi Ritesh,
kernel test robot noticed the following build errors:
[auto build test ERROR on tytso-ext4/dev]
[also build test ERROR on linus/master v6.12-rc5 next-20241101] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Ritesh-Harjani-IBM/ext4-Add-statx-support-for-atomic-writes/20241101-145608 base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev patch link: https://lore.kernel.org/r/78fb5c40dde4847dc32af09e668a6f81fa251137.1730437365.git.ritesh.list%40gmail.com patch subject: [PATCH v4 4/4] ext4: Do not fallback to buffered-io for DIO atomic write config: x86_64-kexec (https://download.01.org/0day-ci/archive/20241102/[email protected]/config) compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241102/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
In file included from fs/ext4/inode.c:26:
In file included from include/linux/pagemap.h:8:
In file included from include/linux/mm.h:2213:
include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
504 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
505 | item];
| ~~~~
include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
511 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
512 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
524 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
525 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
>> fs/ext4/inode.c:3455:14: error: use of undeclared identifier 'IOMAP_ATOMIC' 3455 | if (flags & IOMAP_ATOMIC)
| ^
4 warnings and 1 error generated.
vim +/IOMAP_ATOMIC +3455 fs/ext4/inode.c
3446
3447 static inline bool ext4_want_directio_fallback(unsigned flags, ssize_t written)
3448 {
3449 /* must be a directio to fall back to buffered */
3450 if ((flags & (IOMAP_WRITE | IOMAP_DIRECT)) !=
3451 (IOMAP_WRITE | IOMAP_DIRECT))
3452 return false;
3453
3454 /* atomic writes are all-or-nothing */
> 3455 if (flags & IOMAP_ATOMIC)
3456 return false;
3457
3458 /* can only try again if we wrote nothing */
3459 return written == 0;
3460 }
3461
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki