Re: [PATCH RFC 3/5] dm-table: Atomic writes support

[John Garry <john.g.garry@oracle.com>]

On 06/01/2025 17:49, Mike Snitzer wrote:
>> +++ 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)?

ok, do I admit that my code did not feel quite right, so I will check 
the zoned code as a reference.

> 
> Just feels not quite right... but I could be wrong, please see if
> there is any "there" there

Will do.

Cheers,
John