On Mon, Jan 06, 2025 at 12:41:17PM +0000, John Garry wrote:
> Support stacking atomic write limits for DM devices.
>
> All the pre-existing code in blk_stack_atomic_writes_limits() already takes
> care of finding the aggregate limits from the bottom devices.
>
> Feature flag DM_TARGET_ATOMIC_WRITES is introduced so that atomic writes
> can be enabled on personalities selectively. This is to ensure that atomic
> writes are only enabled when verified to be working properly (for a
> specific personality). In addition, it just may not make sense to enable
> atomic writes on some personalities (so this flag also helps there).
>
> When testing for bottom device atomic writes support, only the bdev
> queue limits are tested. There is no need to test the bottom bdev
> start sector (like which bdev_can_atomic_write() does), as this would
> already be checked in the dm_calculate_queue_limits() -> ..
> iterate_devices() -> dm_set_device_limits() -> blk_stack_limits()
> callchain.
>
> Signed-off-by: John Garry <john.g.garry@xxxxxxxxxx>
> ---
> drivers/md/dm-table.c | 12 ++++++++++++
> include/linux/device-mapper.h | 3 +++
> 2 files changed, 15 insertions(+)
>
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index bd8b796ae683..1e0b7e364546 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -1593,6 +1593,7 @@ int dm_calculate_queue_limits(struct dm_table *t,
> struct queue_limits ti_limits;
> unsigned int zone_sectors = 0;
> bool zoned = false;
> + bool atomic_writes = true;
>
> dm_set_stacking_limits(limits);
>
> @@ -1602,8 +1603,12 @@ int dm_calculate_queue_limits(struct dm_table *t,
>
> if (!dm_target_passes_integrity(ti->type))
> t->integrity_supported = false;
> + if (!dm_target_supports_atomic_writes(ti->type))
> + atomic_writes = false;
> }
>
> + if (atomic_writes)
> + limits->features |= BLK_FEAT_ATOMIC_WRITES_STACKED;
> for (unsigned int i = 0; i < t->num_targets; i++) {
> struct dm_target *ti = dm_table_get_target(t, i);
>
> @@ -1616,6 +1621,13 @@ int dm_calculate_queue_limits(struct dm_table *t,
> goto combine_limits;
> }
>
> + /*
> + * dm_set_device_limits() -> blk_stack_limits() considers
> + * ti_limits as 'top', so set BLK_FEAT_ATOMIC_WRITES_STACKED
> + * here also.
> + */
> + if (atomic_writes)
> + ti_limits.features |= BLK_FEAT_ATOMIC_WRITES_STACKED;
> /*
> * Combine queue limits of all the devices this target uses.
> */
You're referring to this code that immediately follows this ^ comment
which stacks up the limits of a target's potential to have N component
data devices:
ti->type->iterate_devices(ti, dm_set_device_limits,
&ti_limits);
Your comment and redundant feature flag setting is feels wrong. I'd
expect code comparable to what is done for zoned, e.g.:
if (!zoned && (ti_limits.features & BLK_FEAT_ZONED)) {
/*
* After stacking all limits, validate all devices
* in table support this zoned model and zone sectors.
*/
zoned = (ti_limits.features & BLK_FEAT_ZONED);
zone_sectors = ti_limits.chunk_sectors;
}
Meaning, for zoned devices, a side-effect of the
ti->type->iterate_devices() call (and N blk_stack_limits calls) is
ti_limits.features having BLK_FEAT_ZONED enabled. Why wouldn't the same
side-effect happen for BLK_FEAT_ATOMIC_WRITES_STACKED (speaks to
blk_stack_limits being different/wrong for atomic writes support)?
Just feels not quite right... but I could be wrong, please see if
there is any "there" there ;)
Thanks,
Mike
> diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
> index 8321f65897f3..bcc6d7b69470 100644
> --- a/include/linux/device-mapper.h
> +++ b/include/linux/device-mapper.h
> @@ -299,6 +299,9 @@ struct target_type {
> #define dm_target_supports_mixed_zoned_model(type) (false)
> #endif
>
> +#define DM_TARGET_ATOMIC_WRITES 0x00000400
> +#define dm_target_supports_atomic_writes(type) ((type)->features & DM_TARGET_ATOMIC_WRITES)
> +
> struct dm_target {
> struct dm_table *table;
> struct target_type *type;
> --
> 2.31.1
>
