Hi John & Darrick,
"Darrick J. Wong" <djwong@xxxxxxxxxx> writes:
> On Wed, Oct 30, 2024 at 09:27:38PM +0530, Ritesh Harjani (IBM) wrote:
>> 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 <ojaswin@xxxxxxxxxxxxx>
>> Signed-off-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx>
>> ---
>> fs/ext4/ext4.h | 9 +++++++++
>> fs/ext4/inode.c | 14 ++++++++++++++
>> fs/ext4/super.c | 31 +++++++++++++++++++++++++++++++
>> 3 files changed, 54 insertions(+)
>>
>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>> index 44b0d418143c..6ee49aaacd2b 100644
>> --- a/fs/ext4/ext4.h
>> +++ b/fs/ext4/ext4.h
>> @@ -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;
>
> 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);
> }
>
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)...
>> +}
>> +
>> extern int ext4_block_write_begin(handle_t *handle, struct folio *folio,
>> loff_t pos, unsigned len,
>> get_block_t *get_block);
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 54bdd4884fe6..fcdee27b9aa2 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -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?)
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
>
> Other than that nit, this looks good to me.
>
> --D
>
>> + 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;
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 16a4ce704460..ebe1660bd840 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -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;
>> --
>> 2.46.0
>>
>>
