linux-kernel - Re: [PATCH v2 2/5] block: Support atomic writes limits for stacked devices

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: <20241030135006.GC27762@lst.de>
Date: Wed, 30 Oct 2024 14:50:06 +0100
From: Christoph Hellwig <hch@....de>
To: John Garry <john.g.garry@...cle.com>
Cc: axboe@...nel.dk, song@...nel.org, yukuai3@...wei.com, hch@....de,
	linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-raid@...r.kernel.org, martin.petersen@...cle.com
Subject: Re: [PATCH v2 2/5] block: Support atomic writes limits for stacked
 devices

On Wed, Oct 30, 2024 at 09:49:09AM +0000, John Garry wrote:
> Allow stacked devices to support atomic writes by aggregating the minimum
> capability of all bottom devices.
> 
> Flag BLK_FEAT_ATOMIC_WRITES_STACKED is set for stacked devices which
> have been enabled to support atomic writes.
> 
> Some things to note on the implementation:
> - For simplicity, all bottom devices must have same atomic write boundary
>   value (if any)
> - The atomic write boundary must be a power-of-2 already, but this
>   restriction could be relaxed. Furthermore, it is now required that the
>   chunk sectors for a top device must be aligned with this boundary.
> - If a bottom device atomic write unit min/max are not aligned with the
>   top device chunk sectors, the top device atomic write unit min/max are
>   reduced to a value which works for the chunk sectors.
> 
> Signed-off-by: John Garry <john.g.garry@...cle.com>
> ---
>  block/blk-settings.c   | 89 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/blkdev.h |  4 ++
>  2 files changed, 93 insertions(+)
> 
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index 1642e65a6521..6a900ef86e5a 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -496,6 +496,93 @@ static unsigned int blk_round_down_sectors(unsigned int sectors, unsigned int lb
>  	return sectors;
>  }
>  
> +static void blk_stack_atomic_writes_limits(struct queue_limits *t, struct queue_limits *b)

Avoid the overly long line here.

> +	if (t->atomic_write_hw_max) {

Maybe split this branch and the code for when it is not set into
separate helpers to keep the function to a size where it can be
easily understood?

> +	/* Check first bottom device limits */
> +	if (!b->atomic_write_hw_boundary)
> +		goto check_unit;
> +	/*
> +	 * Ensure atomic write boundary is aligned with chunk sectors. Stacked
> +	 * devices store chunk sectors in t->io_min.
> +	 */
> +	if (b->atomic_write_hw_boundary > t->io_min &&
> +	    b->atomic_write_hw_boundary % t->io_min)
> +		goto unsupported;
> +	else if (t->io_min > b->atomic_write_hw_boundary &&

No need for the else here.

> +		 t->io_min % b->atomic_write_hw_boundary)
> +		goto unsupported;
> +
> +	t->atomic_write_hw_boundary = b->atomic_write_hw_boundary;
> +
> +check_unit:

Maybe instead of the check_unit goto just move the checks between the
goto above and this into a branch?

Otherwise this looks conceptually fine to me.