Re: [PATCH v8 5/7] xfs: Support atomic write for statx

[Christoph Hellwig <hch@lst.de>]

On Tue, Oct 15, 2024 at 09:01:40AM +0000, John Garry wrote:
> Support providing info on atomic write unit min and max for an inode.
> 
> For simplicity, currently we limit the min at the FS block size. As for
> max, we limit also at FS block size, as there is no current method to
> guarantee extent alignment or granularity for regular files.
> 
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
>  fs/xfs/xfs_buf.c   |  7 +++++++
>  fs/xfs/xfs_buf.h   |  3 +++
>  fs/xfs/xfs_inode.h | 15 +++++++++++++++
>  fs/xfs/xfs_iops.c  | 25 +++++++++++++++++++++++++
>  4 files changed, 50 insertions(+)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index aa4dbda7b536..e279e5e139ff 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -2115,6 +2115,13 @@ xfs_alloc_buftarg(
>  	btp->bt_daxdev = fs_dax_get_by_bdev(btp->bt_bdev, &btp->bt_dax_part_off,
>  					    mp, ops);
>  
> +	if (bdev_can_atomic_write(btp->bt_bdev)) {
> +		struct request_queue *q = bdev_get_queue(btp->bt_bdev);
> +
> +		btp->bt_bdev_awu_min = queue_atomic_write_unit_min_bytes(q);
> +		btp->bt_bdev_awu_max = queue_atomic_write_unit_max_bytes(q);

Consumers of the block layer should never see request_queue.  While there
is a few leftovers still I've cleaned most of this up.  Please add
bdev_atomic_write_unit_min_bytes and bdev_atomic_write_unit_max_bytes
helpers for use by file systems and stacking drivers, similar to the
other queue limits.

> +	/* Atomic write unit values */
> +	unsigned int		bt_bdev_awu_min, bt_bdev_awu_max;

Nit: While having two struct members declare on the same line using the
same type specification is perfectly valid C, it looks odd and we avoid
it in XFS (and most of the kernel).  Please split this into two lines.

> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_sb		*sbp = &mp->m_sb;
> +
> +	if (!xfs_inode_can_atomicwrite(ip)) {
> +		*unit_min = *unit_max = 0;
> +		return;
> +	}
> +
> +	*unit_min = *unit_max = sbp->sb_blocksize;

Nit: I'd do with the single use sbp local variable here.

> +}
> +
>  STATIC int
>  xfs_vn_getattr(
>  	struct mnt_idmap	*idmap,
> @@ -643,6 +660,14 @@ xfs_vn_getattr(
>  			stat->dio_mem_align = bdev_dma_alignment(bdev) + 1;
>  			stat->dio_offset_align = bdev_logical_block_size(bdev);
>  		}
> +		if (request_mask & STATX_WRITE_ATOMIC) {
> +			unsigned int unit_min, unit_max;

Nit: XFS (unlike the rest of the kernel) uses tab alignments for
variables.

Otherwise this looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>