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

From: Dave Chinner <david@fromorbit.com>
To: John Garry <john.g.garry@oracle.com>
Cc: axboe@kernel.dk, kbusch@kernel.org, hch@lst.de, sagi@grimberg.me,
	martin.petersen@oracle.com, djwong@kernel.org,
	viro@zeniv.linux.org.uk, brauner@kernel.org, dchinner@redhat.com,
	jejb@linux.ibm.com, linux-block@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org,
	linux-scsi@vger.kernel.org, linux-xfs@vger.kernel.org,
	linux-fsdevel@vger.kernel.org,
	linux-security-module@vger.kernel.org, paul@paul-moore.com,
	jmorris@namei.org, serge@hallyn.com,
	Prasad Singamsetty <prasad.singamsetty@oracle.com>
Subject: Re: [PATCH RFC 02/16] fs/bdev: Add atomic write support info to statx
Date: Thu, 4 May 2023 07:58:46 +1000	[thread overview]
Message-ID: <20230503215846.GE3223426@dread.disaster.area> (raw)
In-Reply-To: <20230503183821.1473305-3-john.g.garry@oracle.com>

On Wed, May 03, 2023 at 06:38:07PM +0000, John Garry wrote:
> From: Prasad Singamsetty <prasad.singamsetty@oracle.com>
> 
> Extend statx system call to return additional info for atomic write support
> support if the specified file is a block device.
> 
> Add initial support for a block device.
> 
> Signed-off-by: Prasad Singamsetty <prasad.singamsetty@oracle.com>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
>  block/bdev.c              | 21 +++++++++++++++++++++
>  fs/stat.c                 | 10 ++++++++++
>  include/linux/blkdev.h    |  4 ++++
>  include/linux/stat.h      |  2 ++
>  include/uapi/linux/stat.h |  7 ++++++-
>  5 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/block/bdev.c b/block/bdev.c
> index 1795c7d4b99e..6a5fd5abaadc 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -1014,3 +1014,24 @@ void bdev_statx_dioalign(struct inode *inode, struct kstat *stat)
>  
>  	blkdev_put_no_open(bdev);
>  }
> +
> +/*
> + * Handle statx for block devices to get properties of WRITE ATOMIC
> + * feature support.
> + */
> +void bdev_statx_atomic(struct inode *inode, struct kstat *stat)
> +{
> +	struct block_device *bdev;
> +
> +	bdev = blkdev_get_no_open(inode->i_rdev);
> +	if (!bdev)
> +		return;
> +
> +	stat->atomic_write_unit_min = queue_atomic_write_unit_min(bdev->bd_queue);
> +	stat->atomic_write_unit_max = queue_atomic_write_unit_max(bdev->bd_queue);
> +	stat->attributes |= STATX_ATTR_WRITE_ATOMIC;
> +	stat->attributes_mask |= STATX_ATTR_WRITE_ATOMIC;
> +	stat->result_mask |= STATX_WRITE_ATOMIC;
> +
> +	blkdev_put_no_open(bdev);
> +}
> diff --git a/fs/stat.c b/fs/stat.c
> index 7c238da22ef0..d20334a0e9ae 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -256,6 +256,14 @@ static int vfs_statx(int dfd, struct filename *filename, int flags,
>  			bdev_statx_dioalign(inode, stat);
>  	}
>  
> +	/* 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()....

> 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....

> 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. Is there a statx() man
page update for this addition?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com