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

[Bart Van Assche <bvanassche@acm.org>]

On 1/20/23 18:43, Jens Axboe wrote:
> On 1/20/23 5:11?PM, Bart Van Assche wrote:
>> 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...

Hi Jens,

How about the following approach?
* Remove CONFIG_BLK_SUB_PAGE_SEGMENTS.
* Use the static key mechanism instead of #ifdef
   CONFIG_BLK_SUB_PAGE_SEGMENTS to prevent that this patch series makes
   the hot path in the block layer slower.
* Count the number of request queues that need support for segments
   smaller than a page or max_hw_sector values smaller than
   PAGE_SIZE >> SECTOR_SHIFT. If that number changes from zero to one,
   enable the code introduced by this patch series. If that number drops
   to zero, toggle the static key such that the overhead of the code that
   supports small segments is eliminated.

> 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.

One of the functions modified by this patch series is 
blk_rq_append_bio(). That function is called by code that builds a bio 
(e.g. filesystems). Code that builds a bio does not call any block 
driver code. This is why I think it would be hard to move the 
functionality introduced by this patch series into block drivers.

Thanks,

Bart.