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.