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