Re: [PATCH 2/3] dasd: move queue setup to common code

[Stefan Haberland <sth@linux.ibm.com>]

Please see comments below.

Am 21.02.24 um 13:54 schrieb Christoph Hellwig:
> Most of the code in setup_blk_queue is shared between all disciplines.
> Move it to common code and leave a method to query the maximum number
> of transferable blocks, and a flag to indicate discard support.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/s390/block/dasd.c      | 29 +++++++++++++++++++++++++++--
>   drivers/s390/block/dasd_diag.c | 22 +++-------------------
>   drivers/s390/block/dasd_eckd.c | 29 ++++++-----------------------
>   drivers/s390/block/dasd_fba.c  | 33 ++++-----------------------------
>   drivers/s390/block/dasd_int.h  |  6 ++----
>   5 files changed, 42 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index e754e4f81b2dff..665f69dbb9eab1 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
> @@ -308,6 +308,7 @@ static int dasd_state_basic_to_known(struct dasd_device *device)
>   static int dasd_state_basic_to_ready(struct dasd_device *device)
>   {
>   	struct dasd_block *block = device->block;
> +	struct request_queue *q;
>   	int rc = 0;
>   
>   	/* make disk known with correct capacity */
> @@ -327,8 +328,32 @@ static int dasd_state_basic_to_ready(struct dasd_device *device)
>   		goto out;
>   	}
>   
> -	if (device->discipline->setup_blk_queue)
> -		device->discipline->setup_blk_queue(block);
> +	q = block->gdp->queue;
> +	blk_queue_flag_set(QUEUE_FLAG_NONROT, q);
> +	q->limits.max_dev_sectors = device->discipline->max_transfer(block);
> +	blk_queue_max_hw_sectors(q, q->limits.max_dev_sectors);
> +	blk_queue_logical_block_size(q, block->bp_block);
> +	blk_queue_max_segments(q, USHRT_MAX);
> +
> +	/* With page sized segments each segment can be translated into one idaw/tidaw */
> +	blk_queue_max_segment_size(q, PAGE_SIZE);
> +	blk_queue_segment_boundary(q, PAGE_SIZE - 1);
> +	blk_queue_dma_alignment(q, PAGE_SIZE - 1);
> +
> +	if (device->discipline->has_discard) {
> +		unsigned int max_bytes, max_discard_sectors;
> +
> +		q->limits.discard_granularity = block->bp_block;
> +
> +		/* Calculate max_discard_sectors and make it PAGE aligned */
> +		max_bytes = USHRT_MAX * block->bp_block;
> +		max_bytes = ALIGN_DOWN(max_bytes, PAGE_SIZE);
> +		max_discard_sectors = max_bytes / block->bp_block;
> +
> +		blk_queue_max_discard_sectors(q, max_discard_sectors);
> +		blk_queue_max_write_zeroes_sectors(q, max_discard_sectors);
> +	}
> +
>   	set_capacity(block->gdp, block->blocks << block->s2b_shift);
>   	device->state = DASD_STATE_READY;
>   
> diff --git a/drivers/s390/block/dasd_diag.c b/drivers/s390/block/dasd_diag.c
> index 041088c7e90915..688097036c6a37 100644
> --- a/drivers/s390/block/dasd_diag.c
> +++ b/drivers/s390/block/dasd_diag.c
> @@ -617,25 +617,9 @@ dasd_diag_dump_sense(struct dasd_device *device, struct dasd_ccw_req * req,
>   		    "dump sense not available for DIAG data");
>   }
>   
> -/*
> - * Initialize block layer request queue.
> - */
> -static void dasd_diag_setup_blk_queue(struct dasd_block *block)
> +static unsigned int dasd_diag_max_transfer(struct dasd_block *block)

Could we call this dasd_*_max_sectors() or something like this?
We have a storage server value 'transfer length factor' (referred as 
'unsigned int tlf' in the code).
This might be a little bit misleading for someone reading it with this 
background.

>   {
> -	unsigned int logical_block_size = block->bp_block;
> -	struct request_queue *q = block->gdp->queue;
> -	int max;
> -
> -	max = DIAG_MAX_BLOCKS << block->s2b_shift;
> -	blk_queue_flag_set(QUEUE_FLAG_NONROT, q);
> -	q->limits.max_dev_sectors = max;
> -	blk_queue_logical_block_size(q, logical_block_size);
> -	blk_queue_max_hw_sectors(q, max);
> -	blk_queue_max_segments(q, USHRT_MAX);
> -	/* With page sized segments each segment can be translated into one idaw/tidaw */
> -	blk_queue_max_segment_size(q, PAGE_SIZE);
> -	blk_queue_segment_boundary(q, PAGE_SIZE - 1);
> -	blk_queue_dma_alignment(q, PAGE_SIZE - 1);
> +	return DIAG_MAX_BLOCKS;

You are dropping the shift here (and in the other discipline cases). 
This might lead to smaller request sizes and decreased performance.
Should be:

return DIAG_MAX_BLOCKS << block->s2b_shift;


>   }
>   
>   static int dasd_diag_pe_handler(struct dasd_device *device,
> @@ -648,10 +632,10 @@ static struct dasd_discipline dasd_diag_discipline = {
>   	.owner = THIS_MODULE,
>   	.name = "DIAG",
>   	.ebcname = "DIAG",
> +	.max_transfer = dasd_diag_max_transfer,
>   	.check_device = dasd_diag_check_device,
>   	.pe_handler = dasd_diag_pe_handler,
>   	.fill_geometry = dasd_diag_fill_geometry,
> -	.setup_blk_queue = dasd_diag_setup_blk_queue,
>   	.start_IO = dasd_start_diag,
>   	.term_IO = dasd_diag_term_IO,
>   	.handle_terminated_request = dasd_diag_handle_terminated_request,
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 8aade17d885cc9..8574516bf66e01 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -6826,17 +6826,9 @@ static void dasd_eckd_handle_hpf_error(struct dasd_device *device,
>   	dasd_schedule_requeue(device);
>   }
>   
> -/*
> - * Initialize block layer request queue.
> - */
> -static void dasd_eckd_setup_blk_queue(struct dasd_block *block)
> +static unsigned int dasd_eckd_max_transfer(struct dasd_block *block)
>   {
> -	unsigned int logical_block_size = block->bp_block;
> -	struct request_queue *q = block->gdp->queue;
> -	struct dasd_device *device = block->base;
> -	int max;
> -
> -	if (device->features & DASD_FEATURE_USERAW) {
> +	if (block->base->features & DASD_FEATURE_USERAW) {
>   		/*
>   		 * the max_blocks value for raw_track access is 256
>   		 * it is higher than the native ECKD value because we
> @@ -6844,19 +6836,10 @@ static void dasd_eckd_setup_blk_queue(struct dasd_block *block)
>   		 * so the max_hw_sectors are
>   		 * 2048 x 512B = 1024kB = 16 tracks
>   		 */
> -		max = DASD_ECKD_MAX_BLOCKS_RAW << block->s2b_shift;
> -	} else {
> -		max = DASD_ECKD_MAX_BLOCKS << block->s2b_shift;
> +		return DASD_ECKD_MAX_BLOCKS_RAW;
>   	}
> -	blk_queue_flag_set(QUEUE_FLAG_NONROT, q);
> -	q->limits.max_dev_sectors = max;
> -	blk_queue_logical_block_size(q, logical_block_size);
> -	blk_queue_max_hw_sectors(q, max);
> -	blk_queue_max_segments(q, USHRT_MAX);
> -	/* With page sized segments each segment can be translated into one idaw/tidaw */
> -	blk_queue_max_segment_size(q, PAGE_SIZE);
> -	blk_queue_segment_boundary(q, PAGE_SIZE - 1);
> -	blk_queue_dma_alignment(q, PAGE_SIZE - 1);
> +
> +	return DASD_ECKD_MAX_BLOCKS;

same here

>   }
>   
>   static struct ccw_driver dasd_eckd_driver = {
> @@ -6888,7 +6871,7 @@ static struct dasd_discipline dasd_eckd_discipline = {
>   	.basic_to_ready = dasd_eckd_basic_to_ready,
>   	.online_to_ready = dasd_eckd_online_to_ready,
>   	.basic_to_known = dasd_eckd_basic_to_known,
> -	.setup_blk_queue = dasd_eckd_setup_blk_queue,
> +	.max_transfer = dasd_eckd_max_transfer,
>   	.fill_geometry = dasd_eckd_fill_geometry,
>   	.start_IO = dasd_start_IO,
>   	.term_IO = dasd_term_IO,
> diff --git a/drivers/s390/block/dasd_fba.c b/drivers/s390/block/dasd_fba.c
> index 045e548630dfb1..d075e70d3796bd 100644
> --- a/drivers/s390/block/dasd_fba.c
> +++ b/drivers/s390/block/dasd_fba.c
> @@ -748,35 +748,9 @@ dasd_fba_dump_sense(struct dasd_device *device, struct dasd_ccw_req * req,
>   	free_page((unsigned long) page);
>   }
>   
> -/*
> - * Initialize block layer request queue.
> - */
> -static void dasd_fba_setup_blk_queue(struct dasd_block *block)
> +static unsigned int dasd_fba_max_transfer(struct dasd_block *block)
>   {
> -	unsigned int logical_block_size = block->bp_block;
> -	struct request_queue *q = block->gdp->queue;
> -	unsigned int max_bytes, max_discard_sectors;
> -	int max;
> -
> -	max = DASD_FBA_MAX_BLOCKS << block->s2b_shift;
> -	blk_queue_flag_set(QUEUE_FLAG_NONROT, q);
> -	q->limits.max_dev_sectors = max;
> -	blk_queue_logical_block_size(q, logical_block_size);
> -	blk_queue_max_hw_sectors(q, max);
> -	blk_queue_max_segments(q, USHRT_MAX);
> -	/* With page sized segments each segment can be translated into one idaw/tidaw */
> -	blk_queue_max_segment_size(q, PAGE_SIZE);
> -	blk_queue_segment_boundary(q, PAGE_SIZE - 1);
> -
> -	q->limits.discard_granularity = logical_block_size;
> -
> -	/* Calculate max_discard_sectors and make it PAGE aligned */
> -	max_bytes = USHRT_MAX * logical_block_size;
> -	max_bytes = ALIGN_DOWN(max_bytes, PAGE_SIZE);
> -	max_discard_sectors = max_bytes / logical_block_size;
> -
> -	blk_queue_max_discard_sectors(q, max_discard_sectors);
> -	blk_queue_max_write_zeroes_sectors(q, max_discard_sectors);
> +	return DASD_FBA_MAX_BLOCKS;

and here

[...]