Re: [PATCH v3 5/6] null_blk: Support configuring the maximum DMA segment size

[Nilay Shroff <nilay@linux.ibm.com>]

On 3/30/26 7:53 AM, Ming Lei wrote:
> On Sun, Mar 29, 2026 at 06:00:31PM +0530, Nilay Shroff wrote:
>> On 3/28/26 2:43 AM, Bart Van Assche wrote:
>>> Add support for configuring the maximum DMA segment size. The maximum DMA
>>> segment size may be set to a value smaller than the virtual memory page
>>> size. Reject invalid max_segment_size values.
>>>
>>> Since rq_for_each_segment() may yield bvecs larger than the maximum DMA
>>> segment size, add code in the rq_for_each_segment() loop that restricts
>>> the bvec length to the maximum DMA segment size.
>>>
>>> Cc: Christoph Hellwig<hch@lst.de>
>>> Cc: Ming Lei<ming.lei@redhat.com>
>>> Cc: Damien Le Moal<damien.lemoal@opensource.wdc.com>
>>> Cc: Chaitanya Kulkarni<kch@nvidia.com>
>>> Signed-off-by: Bart Van Assche<bvanassche@acm.org>
>>> ---
>>>    drivers/block/null_blk/main.c     | 43 +++++++++++++++++++++++++++++++
>>>    drivers/block/null_blk/null_blk.h |  1 +
>>>    2 files changed, 44 insertions(+)
>>>
>>> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
>>> index f8c0fd57e041..d5fbbc5d63ed 100644
>>> --- a/drivers/block/null_blk/main.c
>>> +++ b/drivers/block/null_blk/main.c
>>> @@ -169,6 +169,32 @@ static int g_max_sectors;
>>>    module_param_named(max_sectors, g_max_sectors, int, 0444);
>>>    MODULE_PARM_DESC(max_sectors, "Maximum size of a command (in 512B sectors)");
>>> +static unsigned int g_max_segment_size = BLK_MAX_SEGMENT_SIZE;
>>> +
>>> +static int nullb_set_max_segment_size(const char *val,
>>> +				      const struct kernel_param *kp)
>>> +{
>>> +	int res;
>>> +
>>> +	res = kstrtouint(val, 0, &g_max_segment_size);
>>> +	if (res < 0)
>>> +		return res;
>>> +
>>> +	if (g_max_segment_size < BLK_MIN_SEGMENT_SIZE)
>>> +		return -EINVAL;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct kernel_param_ops max_segment_size_ops = {
>>> +	.set = nullb_set_max_segment_size,
>>> +	.get = param_get_uint,
>>> +};
>>> +
>>> +module_param_cb(max_segment_size, &max_segment_size_ops, &g_max_segment_size,
>>> +		0444);
>>> +MODULE_PARM_DESC(max_segment_size, "Maximum size of a DMA segment in bytes");
>>> +
>>>    static unsigned int nr_devices = 1;
>>>    module_param(nr_devices, uint, 0444);
>>>    MODULE_PARM_DESC(nr_devices, "Number of devices to register");
>>> @@ -442,6 +468,14 @@ static int nullb_apply_poll_queues(struct nullb_device *dev,
>>>    	return ret;
>>>    }
>>> +static int nullb_apply_max_segment_size(struct nullb_device *dev,
>>> +					unsigned int max_segment_size)
>>> +{
>>> +	if (max_segment_size < BLK_MIN_SEGMENT_SIZE)
>>> +		return -EINVAL;
>>> +	return 0;
>>> +}
>>> +
>>>    NULLB_DEVICE_ATTR(size, ulong, NULL);
>>>    NULLB_DEVICE_ATTR(completion_nsec, ulong, NULL);
>>>    NULLB_DEVICE_ATTR(submit_queues, uint, nullb_apply_submit_queues);
>>> @@ -450,6 +484,7 @@ NULLB_DEVICE_ATTR(home_node, uint, NULL);
>>>    NULLB_DEVICE_ATTR(queue_mode, uint, NULL);
>>>    NULLB_DEVICE_ATTR(blocksize, uint, NULL);
>>>    NULLB_DEVICE_ATTR(max_sectors, uint, NULL);
>>> +NULLB_DEVICE_ATTR(max_segment_size, uint, nullb_apply_max_segment_size);
>>>    NULLB_DEVICE_ATTR(irqmode, uint, NULL);
>>>    NULLB_DEVICE_ATTR(hw_queue_depth, uint, NULL);
>>>    NULLB_DEVICE_ATTR(index, uint, NULL);
>>> @@ -608,6 +643,7 @@ static struct configfs_attribute *nullb_device_attrs[] = {
>>>    	&nullb_device_attr_index,
>>>    	&nullb_device_attr_irqmode,
>>>    	&nullb_device_attr_max_sectors,
>>> +	&nullb_device_attr_max_segment_size,
>>>    	&nullb_device_attr_mbps,
>>>    	&nullb_device_attr_memory_backed,
>>>    	&nullb_device_attr_no_sched,
>>> @@ -805,6 +841,7 @@ static struct nullb_device *null_alloc_dev(void)
>>>    	dev->queue_mode = g_queue_mode;
>>>    	dev->blocksize = g_bs;
>>>    	dev->max_sectors = g_max_sectors;
>>> +	dev->max_segment_size = g_max_segment_size;
>>>    	dev->irqmode = g_irqmode;
>>>    	dev->hw_queue_depth = g_hw_queue_depth;
>>>    	dev->blocking = g_blocking;
>>> @@ -1248,6 +1285,9 @@ static blk_status_t null_transfer(struct nullb *nullb, struct page *page,
>>>    	unsigned int valid_len = len;
>>>    	void *p;
>>> +	WARN_ONCE(len > dev->max_segment_size, "%u > %u\n", len,
>>> +		  dev->max_segment_size);
>>> +
>>>    	p = kmap_local_page(page) + off;
>>>    	if (!is_write) {
>>>    		if (dev->zoned) {
>>> @@ -1295,6 +1335,8 @@ static blk_status_t null_handle_data_transfer(struct nullb_cmd *cmd,
>>>    	spin_lock_irq(&nullb->lock);
>>>    	rq_for_each_segment(bvec, rq, iter) {
>>>    		len = bvec.bv_len;
>>> +		len = min(bvec.bv_len, nullb->dev->max_segment_size);
>>> +		bvec.bv_len = len;
>>>    		if (transferred_bytes + len > max_bytes)
>>>    			len = max_bytes - transferred_bytes;
>>>    		err = null_transfer(nullb, bvec.bv_page, len, bvec.bv_offset,
>>
>>
>> IMO, since max_segment_size is now configurable, should we consider using
>> blk_rq_map_sg() instead of rq_for_each_segment()?
> 
> blk_rq_map_sg() requires to allocate sgl, actually it can be done in
> request sg iterator way:
> 
> 	blk_rq_map_iter_init();
> 	while (blk_map_iter_next(rq, &iter, &vec)) {
> 		consume each segment;
> 	}
> 
> Just the two helpers needs to be exported.
> 
Yeah this should also work (and a good start as it avoids
allocating sgl), if we could export those two helpers.

Thanks,
--Nilay