Re: [PATCH v3 1/9] block: Introduce QUEUE_FLAG_SUB_PAGE_SEGMENTS and CONFIG_BLK_SUB_PAGE_SEGMENTS

[Jens Axboe <axboe@kernel.dk>]

On 1/20/23 5:11?PM, Bart Van Assche wrote:
> On 1/18/23 15:02, Jens Axboe wrote:
>> On 1/18/23 3:54?PM, Bart Van Assche wrote:
>>> Prepare for introducing support for segments smaller than the page size
>>> by introducing the request queue flag QUEUE_FLAG_SUB_PAGE_SEGMENTS.
>>> Introduce CONFIG_BLK_SUB_PAGE_SEGMENTS to prevent that performance of
>>> block drivers that support segments >= PAGE_SIZE would be affected.
>>>
>>> Cc: Christoph Hellwig <hch@lst.de>
>>> Cc: Ming Lei <ming.lei@redhat.com>
>>> Cc: Keith Busch <kbusch@kernel.org>
>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>> ---
>>>   block/Kconfig          | 9 +++++++++
>>>   include/linux/blkdev.h | 7 +++++++
>>>   2 files changed, 16 insertions(+)
>>>
>>> diff --git a/block/Kconfig b/block/Kconfig
>>> index 5d9d9c84d516..e85061d2175b 100644
>>> --- a/block/Kconfig
>>> +++ b/block/Kconfig
>>> @@ -35,6 +35,15 @@ config BLOCK_LEGACY_AUTOLOAD
>>>         created on demand, but scripts that manually create device nodes and
>>>         then call losetup might rely on this behavior.
>>>   +config BLK_SUB_PAGE_SEGMENTS
>>> +       bool "Support segments smaller than the page size"
>>> +       default n
>>> +       help
>>> +      Most storage controllers support DMA segments larger than the typical
>>> +      size of a virtual memory page. Some embedded controllers only support
>>> +      DMA segments smaller than the page size. Enable this option to support
>>> +      such controllers.
>>
>> This should not be a visible option at all, affected drivers should just
>> select it.
> 
> Hi Jens,
> 
> If CONFIG_BLK_SUB_PAGE_SEGMENTS is made invisible, how should this
> option be enabled for the scsi_debug and null_blk drivers? Adding
> "select BLK_SUB_PAGE_SEGMENTS" to the Kconfig section of these drivers
> would have the unfortunate side effect that enabling either driver
> would make all block drivers slower. How about making sub-page segment
> support configurable for the scsi_debug and null_blk drivers only?
> That would allow kernel developers who want to test the sub-page
> segment support to enable this functionality without making e.g.
> distro kernels slower.

You'd need to have a separate sub-option for each of them, Kconfig
style. But this also highlights the usual issue with pretending that
Kconfig options means that this doesn't matter, because inevitably they
end up getting enabled by default in distros anyway. And then you may as
well not even have them...

Why can't we just handle this in the driver? The segment path is hard
enough to grok in the first place, and this just makes it worse.
Generally not a huge fan of punting this to the driver, but just maybe
it'd make sense in this case since it's just the one. At least that
seems a lot more palatable than the alternative.

-- 
Jens Axboe