convert dasd to the atomic queue limits update API

convert dasd to the atomic queue limits update API

[Christoph Hellwig]

Hi dasd maintainers,

this series against the block/for-6.9 tree converts dasd to the new atomic
queue limits update API.  It is compile tested only as I don't have any
s390 hardware.

Diffstat:
 dasd.c       |   74 ++++++++++++++++++++++++++++++++++++-----------------------
 dasd_diag.c  |   22 ++---------------
 dasd_eckd.c  |   29 ++++-------------------
 dasd_fba.c   |   33 +++-----------------------
 dasd_genhd.c |   13 +++++++++-
 dasd_int.h   |    6 +---
 6 files changed, 73 insertions(+), 104 deletions(-)

[PATCH 1/3] dasd: cleamup dasd_state_basic_to_ready

[Christoph Hellwig]

Reflow dasd_state_basic_to_ready a bit to make it easier to modify.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/s390/block/dasd.c | 54 +++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 28 deletions(-)

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 2f3adf5d8fee44..e754e4f81b2dff 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -307,39 +307,37 @@ static int dasd_state_basic_to_known(struct dasd_device *device)
  */
 static int dasd_state_basic_to_ready(struct dasd_device *device)
 {
-	int rc;
-	struct dasd_block *block;
-	struct gendisk *disk;
+	struct dasd_block *block = device->block;
+	int rc = 0;
 
-	rc = 0;
-	block = device->block;
 	/* make disk known with correct capacity */
-	if (block) {
-		if (block->base->discipline->do_analysis != NULL)
-			rc = block->base->discipline->do_analysis(block);
-		if (rc) {
-			if (rc != -EAGAIN) {
-				device->state = DASD_STATE_UNFMT;
-				disk = device->block->gdp;
-				kobject_uevent(&disk_to_dev(disk)->kobj,
-					       KOBJ_CHANGE);
-				goto out;
-			}
-			return rc;
-		}
-		if (device->discipline->setup_blk_queue)
-			device->discipline->setup_blk_queue(block);
-		set_capacity(block->gdp,
-			     block->blocks << block->s2b_shift);
+	if (!block) {
 		device->state = DASD_STATE_READY;
-		rc = dasd_scan_partitions(block);
-		if (rc) {
-			device->state = DASD_STATE_BASIC;
+		goto out;
+	}
+
+	if (block->base->discipline->do_analysis != NULL)
+		rc = block->base->discipline->do_analysis(block);
+	if (rc) {
+		if (rc == -EAGAIN)
 			return rc;
-		}
-	} else {
-		device->state = DASD_STATE_READY;
+		device->state = DASD_STATE_UNFMT;
+		kobject_uevent(&disk_to_dev(device->block->gdp)->kobj,
+			       KOBJ_CHANGE);
+		goto out;
 	}
+
+	if (device->discipline->setup_blk_queue)
+		device->discipline->setup_blk_queue(block);
+	set_capacity(block->gdp, block->blocks << block->s2b_shift);
+	device->state = DASD_STATE_READY;
+
+	rc = dasd_scan_partitions(block);
+	if (rc) {
+		device->state = DASD_STATE_BASIC;
+		return rc;
+	}
+
 out:
 	if (device->discipline->basic_to_ready)
 		rc = device->discipline->basic_to_ready(device);
-- 
2.39.2

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

[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)
 {
-	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;
 }
 
 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;
 }
 
 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;
 }
 
 static int dasd_fba_pe_handler(struct dasd_device *device,
@@ -789,10 +763,11 @@ static struct dasd_discipline dasd_fba_discipline = {
 	.owner = THIS_MODULE,
 	.name = "FBA ",
 	.ebcname = "FBA ",
+	.has_discard = true,
 	.check_device = dasd_fba_check_characteristics,
 	.do_analysis = dasd_fba_do_analysis,
 	.pe_handler = dasd_fba_pe_handler,
-	.setup_blk_queue = dasd_fba_setup_blk_queue,
+	.max_transfer = dasd_fba_max_transfer,
 	.fill_geometry = dasd_fba_fill_geometry,
 	.start_IO = dasd_start_IO,
 	.term_IO = dasd_term_IO,
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index a6c5f1fa2d8798..fac69985df5aa7 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -293,6 +293,7 @@ struct dasd_discipline {
 	struct module *owner;
 	char ebcname[8];	/* a name used for tagging and printks */
 	char name[8];		/* a name used for tagging and printks */
+	bool has_discard;
 
 	struct list_head list;	/* used for list of disciplines */
 
@@ -331,10 +332,7 @@ struct dasd_discipline {
 	int (*online_to_ready) (struct dasd_device *);
 	int (*basic_to_known)(struct dasd_device *);
 
-	/*
-	 * Initialize block layer request queue.
-	 */
-	void (*setup_blk_queue)(struct dasd_block *);
+	unsigned int (*max_transfer)(struct dasd_block *);
 	/* (struct dasd_device *);
 	 * Device operation functions. build_cp creates a ccw chain for
 	 * a block device request, start_io starts the request and
-- 
2.39.2

[PATCH 3/3] dasd: use the atomic queue limits API

[Christoph Hellwig]

Pass the constant limits directly to blk_mq_alloc_disk, set the nonrot
flag there as well, and then use the commit API to change the transfer
size and logical block size dependent values.

This relies on the assumption that no I/O can be pending before the
devices moves into the ready state and doesn't need extra freezing
for changes to the queue limits.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/s390/block/dasd.c       | 29 ++++++++++++-----------------
 drivers/s390/block/dasd_genhd.c | 13 ++++++++++++-
 2 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 665f69dbb9eab1..51fbff81bc1214 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -308,7 +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;
+	struct queue_limits lim;
 	int rc = 0;
 
 	/* make disk known with correct capacity */
@@ -328,31 +328,26 @@ static int dasd_state_basic_to_ready(struct dasd_device *device)
 		goto out;
 	}
 
-	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);
+	lim = queue_limits_start_update(block->gdp->queue);
+	lim.max_dev_sectors = device->discipline->max_transfer(block);
+	lim.max_hw_sectors = lim.max_dev_sectors;
+	lim.logical_block_size = block->bp_block;
 
 	if (device->discipline->has_discard) {
-		unsigned int max_bytes, max_discard_sectors;
+		unsigned int max_bytes;
 
-		q->limits.discard_granularity = block->bp_block;
+		lim.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);
+		lim.max_hw_discard_sectors = max_bytes / block->bp_block;
+		lim.max_write_zeroes_sectors = lim.max_hw_discard_sectors;
 	}
+	rc = queue_limits_commit_update(block->gdp->queue, &lim);
+	if (rc)
+		return rc;
 
 	set_capacity(block->gdp, block->blocks << block->s2b_shift);
 	device->state = DASD_STATE_READY;
diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c
index 0465b706745f64..528e2d38d9bfcc 100644
--- a/drivers/s390/block/dasd_genhd.c
+++ b/drivers/s390/block/dasd_genhd.c
@@ -34,6 +34,16 @@ MODULE_PARM_DESC(nr_hw_queues, "Default number of hardware queues for new DASD d
  */
 int dasd_gendisk_alloc(struct dasd_block *block)
 {
+	struct queue_limits lim = {
+		/*
+		 * With page sized segments, each segment can be translated into
+		 * one idaw/tidaw.
+		 */
+		.max_segment_size = PAGE_SIZE,
+		.seg_boundary_mask = PAGE_SIZE - 1,
+		.dma_alignment = PAGE_SIZE - 1,
+		.max_segments = USHRT_MAX,
+	};
 	struct gendisk *gdp;
 	struct dasd_device *base;
 	int len, rc;
@@ -53,11 +63,12 @@ int dasd_gendisk_alloc(struct dasd_block *block)
 	if (rc)
 		return rc;
 
-	gdp = blk_mq_alloc_disk(&block->tag_set, NULL, block);
+	gdp = blk_mq_alloc_disk(&block->tag_set, &lim, block);
 	if (IS_ERR(gdp)) {
 		blk_mq_free_tag_set(&block->tag_set);
 		return PTR_ERR(gdp);
 	}
+	blk_queue_flag_set(QUEUE_FLAG_NONROT, gdp->queue);
 
 	/* Initialize gendisk structure. */
 	gdp->major = DASD_MAJOR;
-- 
2.39.2

Re: convert dasd to the atomic queue limits update API

[Stefan Haberland]

Am 21.02.24 um 13:54 schrieb Christoph Hellwig:
> Hi dasd maintainers,
>
> this series against the block/for-6.9 tree converts dasd to the new atomic
> queue limits update API.  It is compile tested only as I don't have any
> s390 hardware.
>
> Diffstat:
>   dasd.c       |   74 ++++++++++++++++++++++++++++++++++++-----------------------
>   dasd_diag.c  |   22 ++---------------
>   dasd_eckd.c  |   29 ++++-------------------
>   dasd_fba.c   |   33 +++-----------------------
>   dasd_genhd.c |   13 +++++++++-
>   dasd_int.h   |    6 +---
>   6 files changed, 73 insertions(+), 104 deletions(-)

Thanks for addressing this.
I corrected one thing in patch 2 and let it run through some basic 
testing over the weekend.
Worked as expected. Please see comments for patch 2.

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

[Stefan Haberland]

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

[...]

Re: [PATCH 1/3] dasd: cleamup dasd_state_basic_to_ready

[Stefan Haberland]

Am 21.02.24 um 13:54 schrieb Christoph Hellwig:
> Reflow dasd_state_basic_to_ready a bit to make it easier to modify.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Stefan Haberland <sth@linux.ibm.com>

Re: [PATCH 3/3] dasd: use the atomic queue limits API

[Stefan Haberland]

Am 21.02.24 um 13:54 schrieb Christoph Hellwig:
> Pass the constant limits directly to blk_mq_alloc_disk, set the nonrot
> flag there as well, and then use the commit API to change the transfer
> size and logical block size dependent values.
>
> This relies on the assumption that no I/O can be pending before the
> devices moves into the ready state and doesn't need extra freezing
> for changes to the queue limits.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Stefan Haberland <sth@linux.ibm.com>

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

[Christoph Hellwig]

On Mon, Feb 26, 2024 at 05:49:30PM +0100, Stefan Haberland wrote:
> Could we call this dasd_*_max_sectors() or something like this?

Sure.

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

I actually wanted to move the shift to the caller, but forgot to add
it there.  But with the max_sectors naming it's probably better to
keep it in the disciplines.

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

[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..bdeab447adfc9d 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_sectors(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..ea4b1d01bb767e 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_sectors(struct dasd_block *block)
 {
-	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 << 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_sectors = dasd_diag_max_sectors,
 	.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..373c1a86c33ed5 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_sectors(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 << 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 DASD_ECKD_MAX_BLOCKS << block->s2b_shift;
 }
 
 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_sectors = dasd_eckd_max_sectors,
 	.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..bcbb2f8e91feb6 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_sectors(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 << block->s2b_shift;
 }
 
 static int dasd_fba_pe_handler(struct dasd_device *device,
@@ -789,10 +763,11 @@ static struct dasd_discipline dasd_fba_discipline = {
 	.owner = THIS_MODULE,
 	.name = "FBA ",
 	.ebcname = "FBA ",
+	.has_discard = true,
 	.check_device = dasd_fba_check_characteristics,
 	.do_analysis = dasd_fba_do_analysis,
 	.pe_handler = dasd_fba_pe_handler,
-	.setup_blk_queue = dasd_fba_setup_blk_queue,
+	.max_sectors = dasd_fba_max_sectors,
 	.fill_geometry = dasd_fba_fill_geometry,
 	.start_IO = dasd_start_IO,
 	.term_IO = dasd_term_IO,
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index a6c5f1fa2d8798..b56d683a991dfd 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -293,6 +293,7 @@ struct dasd_discipline {
 	struct module *owner;
 	char ebcname[8];	/* a name used for tagging and printks */
 	char name[8];		/* a name used for tagging and printks */
+	bool has_discard;
 
 	struct list_head list;	/* used for list of disciplines */
 
@@ -331,10 +332,7 @@ struct dasd_discipline {
 	int (*online_to_ready) (struct dasd_device *);
 	int (*basic_to_known)(struct dasd_device *);
 
-	/*
-	 * Initialize block layer request queue.
-	 */
-	void (*setup_blk_queue)(struct dasd_block *);
+	unsigned int (*max_sectors)(struct dasd_block *);
 	/* (struct dasd_device *);
 	 * Device operation functions. build_cp creates a ccw chain for
 	 * a block device request, start_io starts the request and
-- 
2.39.2

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

[Christoph Hellwig]

Can you review the v3 with this addressed?

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

[Stefan Haberland]

Am 28.02.24 um 14:37 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>
> ---

looks good to me, also did some basic testing.

Reviewed-by: Stefan Haberland <sth@linux.ibm.com>