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

[John Garry]

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