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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241015121539.GB32583@lst.de>
Date: Tue, 15 Oct 2024 14:15:39 +0200
From: Christoph Hellwig <hch@....de>
To: John Garry <john.g.garry@...cle.com>
Cc: axboe@...nel.dk, brauner@...nel.org, djwong@...nel.org,
	viro@...iv.linux.org.uk, jack@...e.cz, dchinner@...hat.com,
	hch@....de, cem@...nel.org, linux-block@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-xfs@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, hare@...e.de,
	martin.petersen@...cle.com, catherine.hoang@...cle.com,
	mcgrof@...nel.org, ritesh.list@...il.com, ojaswin@...ux.ibm.com
Subject: Re: [PATCH v8 5/7] xfs: Support atomic write for statx

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@...nel.org>
> Signed-off-by: John Garry <john.g.garry@...cle.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@....de>