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

Commit Message

Kirill Tkhai Jan. 21, 2020, 10:42 a.m. UTC

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 <[e-mail]>
---
 drivers/md/dm-table.c |    2 ++
 drivers/md/md.h       |    1 +
 2 files changed, 3 insertions(+)




--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Comments

Kirill Tkhai Jan. 21, 2020, 1:33 p.m. UTC | #1

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 <[e-mail]> 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 <[e-mail]>
>>> ---
>>>  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.


--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Kirill Tkhai Jan. 21, 2020, 2:20 p.m. UTC | #2

On 21.01.2020 16:48, Mike Snitzer wrote:
> On Tue, Jan 21 2020 at  8:33am -0500,
> Kirill Tkhai <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
>>>>> ---
>>>>>  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


--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Kirill Tkhai Jan. 21, 2020, 3:13 p.m. UTC | #3

On 21.01.2020 17:43, Mike Snitzer wrote:
> On Tue, Jan 21 2020 at  9:20am -0500,
> Kirill Tkhai <[e-mail]> wrote:
> 
>> On 21.01.2020 16:48, Mike Snitzer wrote:
>>> On Tue, Jan 21 2020 at  8:33am -0500,
>>> Kirill Tkhai <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
>>>>>>> ---
>>>>>>>  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?
> 
> I don't have time to work through it at the moment (e.g. implementing
> dm-thinp support to know what the block core code should be) so I'll
> just defer to you on a disabling it for now.
> 
>>> 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.
> 
> Yeah, I'm aware nothing is ever set in stone.
> 
> Setting to 0 in blk_set_stacking_limits() is OK for now.

I get your point. Thanks for the suggestion and comments, Mike.

Kirill


--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Patch

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 0a2cc197f62b..e245c0d882aa 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -489,6 +489,7 @@  static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,
 		       (unsigned long long) start << SECTOR_SHIFT);
 
 	limits->zoned = blk_queue_zoned_model(q);
+	blk_queue_max_allocate_sectors(q, 0);
 
 	return 0;
 }
@@ -1548,6 +1549,7 @@  int dm_calculate_queue_limits(struct dm_table *table,
 			       dm_device_name(table->md),
 			       (unsigned long long) ti->begin,
 			       (unsigned long long) ti->len);
+		limits->max_allocate_sectors = 0;
 
 		/*
 		 * FIXME: this should likely be moved to blk_stack_limits(), would
diff --git a/drivers/md/md.h b/drivers/md/md.h
index acd681939112..d9088122674d 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -798,5 +798,6 @@  static inline void mddev_check_write_zeroes(struct mddev *mddev, struct bio *bio
 	if (bio_op(bio) == REQ_OP_WRITE_ZEROES &&
 	    !bio->bi_disk->queue->limits.max_write_zeroes_sectors)
 		mddev->queue->limits.max_write_zeroes_sectors = 0;
+	blk_queue_max_allocate_sectors(mddev->queue, 0);
 }
 #endif /* _MD_MD_H */