[dm-devel] dm: fix iterate_device sanity check

[JeffleXu]

On 2/9/21 1:29 PM, Mike Snitzer wrote:
> On Mon, Feb 08 2021 at 10:17am -0500,
> Mike Snitzer <snitzer at redhat.com> wrote:
> 
>> On Fri, Feb 05 2021 at  9:03pm -0500,
>> JeffleXu <jefflexu at linux.alibaba.com> wrote:
>>
>>>
>>>
>>> On 2/6/21 2:39 AM, Mike Snitzer wrote:
>>>> On Mon, Feb 01 2021 at 10:35pm -0500,
>>>> Jeffle Xu <jefflexu at linux.alibaba.com> wrote:
>>>>
>>>>> According to the definition of dm_iterate_devices_fn:
>>>>>  * This function must iterate through each section of device used by the
>>>>>  * target until it encounters a non-zero return code, which it then returns.
>>>>>  * Returns zero if no callout returned non-zero.
>>>>>
>>>>> For some target type (e.g., dm-stripe), one call of iterate_devices() may
>>>>> iterate multiple underlying devices internally, in which case a non-zero
>>>>> return code returned by iterate_devices_callout_fn will stop the iteration
>>>>> in advance.
>>>>>
>>>>> Thus if we want to ensure that _all_ underlying devices support some kind of
>>>>> attribute, the iteration structure like dm_table_supports_nowait() should be
>>>>> used, while the input iterate_devices_callout_fn should handle the 'not
>>>>> support' semantics. On the opposite, the iteration structure like
>>>>> dm_table_any_device_attribute() should be used if _any_ underlying device
>>>>> supporting this attibute is sufficient. In this case, the input
>>>>> iterate_devices_callout_fn should handle the 'support' semantics.
>>>>>
>>>>> Fixes: 545ed20e6df6 ("dm: add infrastructure for DAX support")
>>>>> Fixes: c3c4555edd10 ("dm table: clear add_random unless all devices have it set")
>>>>> Fixes: 4693c9668fdc ("dm table: propagate non rotational flag")
>>>>> Cc: stable at vger.kernel.org
>>>>> Signed-off-by: Jeffle Xu <jefflexu at linux.alibaba.com>
>>>>
>>>> Thanks for auditing and fixing this up.  It has been on my todo so
>>>> you've really helped me out -- your changes look correct to me.
>>>>
>>>> I've staged it for 5.12, the stable fix will likely need manual fixups
>>>> depending on the stable tree... we'll just need to assist with
>>>> backport(s) as needed.
>>>
>>> I'm glad to help offer the stable backport. But I don't know which
>>> kernel version the stable kernel is still being maintained. Also which
>>> mailing list I should send to when I finished backporting?
>>
>> All your v2 changes speak to needing more discipline in crafting
>> individual stable@ fixes that are applicable to various kernels.. when
>> all applied to mainline then it'd be the equivalent of your single
>> monolithic patch.
>>
>> But without splitting the changes into separate patches, for stable@'s
>> benefit, we'll have a much more difficult time of shepherding the
>> applicable changes into the disparate stable@ kernels.
>>
>> I'll have a look at splitting your v2 up accordingly.
> 
> Hi, please see these commits that I've staged in linux-next via:
> https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/log/?h=for-next
> 
> 1141b9133777 dm table: fix iterate_devices based device capability checks
> 0224c5e6fd07 dm table: fix DAX iterate_devices based device capability checks
> 76b0e14be03f dm table: fix zoned iterate_devices based device capability checks
> 55cdd7435e97 dm table: remove needless request_queue NULL pointer checks
> 

Thanks. This series looks good to me.

I suddenly find that the semantics of patch 1 (1141b9133777 dm table:
fix iterate_devices based device capability checks) is a little
different with the original context.

-	if (blk_queue_add_random(q) && dm_table_all_devices_attribute(t,
device_is_not_random))
+	if (dm_table_any_dev_attr(t, device_is_not_random))
 		blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, q);
+	else
+		blk_queue_flag_set(QUEUE_FLAG_ADD_RANDOM, q);

In the original context, QUEUE_FLAG_ADD_RANDOM will only be cleared, it
won't be set, while it could be set after patch 1. But I could see no
harm of setting QUEUE_FLAG_ADD_RANDOM flag though.

FYI. Currently only scsi devices are still using QUEUE_FLAG_ADD_RANDOM
flag, as all non-rotational devices should not set this flag since
commit b277da0a8a59 ("block: disable entropy contributions for nonrot
devices").


-- 
Thanks,
Jeffle