Re: [PATCH RFC 02/16] fs/bdev: Add atomic write support info to statx

[John Garry <john.g.garry@oracle.com>]

On 03/05/2023 22:58, Dave Chinner wrote:

Hi Dave,

>> +	/* Handle STATX_WRITE_ATOMIC for block devices */
>> +	if (request_mask & STATX_WRITE_ATOMIC) {
>> +		struct inode *inode = d_backing_inode(path.dentry);
>> +
>> +		if (S_ISBLK(inode->i_mode))
>> +			bdev_statx_atomic(inode, stat);
>> +	}
> This duplicates STATX_DIOALIGN bdev handling.
> 
> Really, the bdev attribute handling should be completely factored
> out of vfs_statx() - blockdevs are not the common fastpath for stat
> operations. Somthing like:
> 
> 	/*
> 	 * If this is a block device inode, override the filesystem
> 	 * attributes with the block device specific parameters
> 	 * that need to be obtained from the bdev backing inode.
> 	 */
> 	if (S_ISBLK(d_backing_inode(path.dentry)->i_mode))
> 		bdev_statx(path.dentry, stat);
> 
> And then all the overrides can go in the one function that doesn't
> need to repeatedly check S_ISBLK()....

ok, that looks like a reasonable idea. We'll look to make that change.

> 
> 
>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>> index 6b6f2992338c..19d33b2897b2 100644
>> --- a/include/linux/blkdev.h
>> +++ b/include/linux/blkdev.h
>> @@ -1527,6 +1527,7 @@ int sync_blockdev_range(struct block_device *bdev, loff_t lstart, loff_t lend);
>>   int sync_blockdev_nowait(struct block_device *bdev);
>>   void sync_bdevs(bool wait);
>>   void bdev_statx_dioalign(struct inode *inode, struct kstat *stat);
>> +void bdev_statx_atomic(struct inode *inode, struct kstat *stat);
>>   void printk_all_partitions(void);
>>   #else
>>   static inline void invalidate_bdev(struct block_device *bdev)
>> @@ -1546,6 +1547,9 @@ static inline void sync_bdevs(bool wait)
>>   static inline void bdev_statx_dioalign(struct inode *inode, struct kstat *stat)
>>   {
>>   }
>> +static inline void bdev_statx_atomic(struct inode *inode, struct kstat *stat)
>> +{
>> +}
>>   static inline void printk_all_partitions(void)
>>   {
>>   }
> That also gets rid of the need for all these fine grained exports
> out of the bdev code for statx....

Sure

> 
>> diff --git a/include/linux/stat.h b/include/linux/stat.h
>> index 52150570d37a..dfa69ecfaacf 100644
>> --- a/include/linux/stat.h
>> +++ b/include/linux/stat.h
>> @@ -53,6 +53,8 @@ struct kstat {
>>   	u32		dio_mem_align;
>>   	u32		dio_offset_align;
>>   	u64		change_cookie;
>> +	u32		atomic_write_unit_max;
>> +	u32		atomic_write_unit_min;
>>   };
>>   
>>   /* These definitions are internal to the kernel for now. Mainly used by nfsd. */
>> diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
>> index 7cab2c65d3d7..c99d7cac2aa6 100644
>> --- a/include/uapi/linux/stat.h
>> +++ b/include/uapi/linux/stat.h
>> @@ -127,7 +127,10 @@ struct statx {
>>   	__u32	stx_dio_mem_align;	/* Memory buffer alignment for direct I/O */
>>   	__u32	stx_dio_offset_align;	/* File offset alignment for direct I/O */
>>   	/* 0xa0 */
>> -	__u64	__spare3[12];	/* Spare space for future expansion */
>> +	__u32	stx_atomic_write_unit_max;
>> +	__u32	stx_atomic_write_unit_min;
>> +	/* 0xb0 */
>> +	__u64	__spare3[11];	/* Spare space for future expansion */
>>   	/* 0x100 */
>>   };
> No documentation on what units these are in.

It's in bytes, we're really just continuing the pattern of what we do 
for dio. I think that it would be reasonable to limit to u32.

> Is there a statx() man
> page update for this addition?

No, not yet. Is it normally expected to provide a proposed man page 
update in parallel? Or somewhat later, when the kernel API change has 
some appreciable level of agreement?

Thanks,
John