Re: [PATCH v4 6/7] dm: Directly disable max_allocate_sectors for now

[Kirill Tkhai <ktkhai@virtuozzo.com>]

On 21.01.2020 16:48, Mike Snitzer wrote:
> On Tue, Jan 21 2020 at  8:33am -0500,
> Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
> 
>> On 21.01.2020 15:36, Kirill Tkhai wrote:
>>> On 21.01.2020 15:24, Mike Snitzer wrote:
>>>> On Tue, Jan 21 2020 at  5:42am -0500,
>>>> Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>>>>
>>>>> Since dm inherits limits from underlining block devices,
>>>>> this patch directly disables max_allocate_sectors for dm
>>>>> till full allocation support is implemented.
>>>>>
>>>>> This prevents high-level primitives (generic_make_request_checks(),
>>>>> __blkdev_issue_write_zeroes(), ...) from sending REQ_ALLOCATE
>>>>> requests.
>>>>>
>>>>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>>>> ---
>>>>>  drivers/md/dm-table.c |    2 ++
>>>>>  drivers/md/md.h       |    1 +
>>>>>  2 files changed, 3 insertions(+)
>>>>
>>>> You're mixing DM and MD changes in the same patch.
>>>>
>>>> But I'm wondering if it might be best to set this default for stacking
>>>> devices in blk_set_stacking_limits()?
>>>>
>>>> And then it is up to each stacking driver to override as needed.
>>>
>>> Hm. Sound like a good idea. This "lim->max_allocate_sectors = 0" in blk_set_stacking_limits()
>>> should work for dm's dm_calculate_queue_limits(), since it calls blk_stack_limits(), which is:
>>>
>>> 	t->max_allocate_sectors = min(t->max_allocate_sectors,
>>> 				      b->max_allocate_sectors);
>>>
>>> Could you please tell is this fix is also enough for md?
>>
>> It looks like it's enough since queue defaults are set in md_alloc()->blk_set_stacking_limits().
>> In case of we set "max_allocate_sectors = 0", in further it can be changed only manually,
>> but nobody does this.
> 
> Yes, it will work to disable this capability for MD and DM.
> 
> But if/when a stacked device _dooes_ want to support this then it'll be
> awkward to override this stacking default to allow blk_stack_limits()
> to properly stack up this limit.  blk_limits are extremely fiddley so
> this isn't necessarily new.  But by explicitly defaulting to 0 and then
> having blk_stack_limits use min() for this limit: it results in stacking
> drivers needing to clumsily unwind the default.  E.g. DM will need to
> tweak its blk_stack_limits() related code to allow override that
> actually _does_  stack up the underlying devices' capability (and not
> just impose its own limit that ignores the underlying devices).
> 
> So I'm not convinced this is the right way to go (be it the v4 approach
> you took or the cleaner use of blk_set_stacking_limits I suggested).

Is there a strong vision about the way we should go? Or you leave this choose
up to me?

> And to be clear, I'm interested in having DM thinp support this
> capability to preallocate blocks.

My opinion is it would be better to not mix several subsystem related
support in a single patch set. Both of the approaches (v4 or that you
suggested) do not prevents us to implement allocation support in next
patch series. After we have the base functionality enabled, we may add
support in other subsystems and drivers one by one with more focus
on the subsystem specificities and with the best possible attention.

Regards,
Kirill