On 11/21/23 05:44, Bart Van Assche wrote:
> On 11/19/23 15:29, Damien Le Moal wrote:
>> On 11/15/23 06:16, Bart Van Assche wrote:
>>> @@ -82,6 +84,8 @@ void blk_set_stacking_limits(struct queue_limits *lim)
>>> lim->max_dev_sectors = UINT_MAX;
>>> lim->max_write_zeroes_sectors = UINT_MAX;
>>> lim->max_zone_append_sectors = UINT_MAX;
>>> + /* Request-based stacking drivers do not reorder requests. */
>>
>> Rereading this patch, I do not think this statement is correct. I seriously
>> doubt that multipath will preserve write command order in all cases...
>>
>>> + lim->driver_preserves_write_order = true;
>>
>> ... so it is likely much safer to set the default to "false" as that is the
>> default for all requests in general.
>
> How about applying this (untested) patch on top of this patch series?
>
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index 4c776c08f190..aba1972e9767 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -84,8 +84,6 @@ void blk_set_stacking_limits(struct queue_limits *lim)
> lim->max_dev_sectors = UINT_MAX;
> lim->max_write_zeroes_sectors = UINT_MAX;
> lim->max_zone_append_sectors = UINT_MAX;
> - /* Request-based stacking drivers do not reorder requests. */
> - lim->driver_preserves_write_order = true;
> }
> EXPORT_SYMBOL(blk_set_stacking_limits);
>
> diff --git a/drivers/md/dm-linear.c b/drivers/md/dm-linear.c
> index 2d3e186ca87e..cb9abe4bd065 100644
> --- a/drivers/md/dm-linear.c
> +++ b/drivers/md/dm-linear.c
> @@ -147,6 +147,11 @@ static int linear_report_zones(struct dm_target *ti,
> #define linear_report_zones NULL
> #endif
>
> +static void linear_io_hints(struct dm_target *ti, struct queue_limits *limits)
> +{
> + limits->driver_preserves_write_order = true;
> +}
Hmm, but does dm-linear preserve write order ? I am not convinced. And what
about dm-flakey, dm-error and dm-crypt ? All of these also support zoned
devices. I do not think that we can say that any of these preserve write order.
> +
> static int linear_iterate_devices(struct dm_target *ti,
> iterate_devices_callout_fn fn, void *data)
> {
> @@ -208,6 +213,7 @@ static struct target_type linear_target = {
> .map = linear_map,
> .status = linear_status,
> .prepare_ioctl = linear_prepare_ioctl,
> + .io_hints = linear_io_hints,
> .iterate_devices = linear_iterate_devices,
> .direct_access = linear_dax_direct_access,
> .dax_zero_page_range = linear_dax_zero_page_range,
>
>>> @@ -685,6 +689,10 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
>>> b->max_secure_erase_sectors);
>>> t->zone_write_granularity = max(t->zone_write_granularity,
>>> b->zone_write_granularity);
>>> + t->driver_preserves_write_order = t->driver_preserves_write_order &&
>>> + b->driver_preserves_write_order;
>>> + t->use_zone_write_lock = t->use_zone_write_lock ||
>>> + b->use_zone_write_lock;
>>
>> Very minor nit: splitting the line after the equal would make this more readable.
>
> Hmm ... I have often seen other reviewers asking to maximize the use of each
> source code line as much as reasonably possible.
As I said, very minor nit :) Feel free to ignore.
>
> Thanks,
>
> Bart.
>
--
Damien Le Moal
Western Digital Research
