clean up the chunk_sizehandling helpers a little

clean up the chunk_sizehandling helpers a little

[Christoph Hellwig]

Hi Jens,

this series cleans up a bunch of block layer helpers related to the chunk
size.

Diffstat:
 block/blk-merge.c      |   28 ++++++++++++++++------------
 block/blk.h            |   13 +++++++++++++
 drivers/md/dm.c        |   17 ++++++-----------
 include/linux/blkdev.h |   39 +++++++--------------------------------
 4 files changed, 42 insertions(+), 55 deletions(-)

[dm-devel] clean up the chunk_sizehandling helpers a little

[Christoph Hellwig]

Hi Jens,

this series cleans up a bunch of block layer helpers related to the chunk
size.

Diffstat:
 block/blk-merge.c      |   28 ++++++++++++++++------------
 block/blk.h            |   13 +++++++++++++
 drivers/md/dm.c        |   17 ++++++-----------
 include/linux/blkdev.h |   39 +++++++--------------------------------
 4 files changed, 42 insertions(+), 55 deletions(-)

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 1/6] block: factor out a chunk_size_left helper

[Christoph Hellwig]

Factor out a helper from blk_max_size_offset so that it can be reused
independently.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/blkdev.h | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 914c613d81da7..e66ad451274ec 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -933,6 +933,17 @@ static inline unsigned int blk_queue_get_max_sectors(struct request_queue *q,
 	return q->limits.max_sectors;
 }
 
+/*
+ * Return how much of the chunk is left to be used for I/O at a given offset.
+ */
+static inline unsigned int blk_chunk_sectors_left(sector_t offset,
+		unsigned int chunk_sectors)
+{
+	if (unlikely(!is_power_of_2(chunk_sectors)))
+		return chunk_sectors - sector_div(offset, chunk_sectors);
+	return chunk_sectors - (offset & (chunk_sectors - 1));
+}
+
 /*
  * Return maximum size of a request at given offset. Only valid for
  * file system requests.
@@ -948,12 +959,8 @@ static inline unsigned int blk_max_size_offset(struct request_queue *q,
 			return q->limits.max_sectors;
 	}
 
-	if (likely(is_power_of_2(chunk_sectors)))
-		chunk_sectors -= offset & (chunk_sectors - 1);
-	else
-		chunk_sectors -= sector_div(offset, chunk_sectors);
-
-	return min(q->limits.max_sectors, chunk_sectors);
+	return min(q->limits.max_sectors,
+			blk_chunk_sectors_left(offset, chunk_sectors));
 }
 
 /*
-- 
2.30.2

[dm-devel] [PATCH 1/6] block: factor out a chunk_size_left helper

[Christoph Hellwig]

Factor out a helper from blk_max_size_offset so that it can be reused
independently.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/blkdev.h | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 914c613d81da7..e66ad451274ec 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -933,6 +933,17 @@ static inline unsigned int blk_queue_get_max_sectors(struct request_queue *q,
 	return q->limits.max_sectors;
 }
 
+/*
+ * Return how much of the chunk is left to be used for I/O at a given offset.
+ */
+static inline unsigned int blk_chunk_sectors_left(sector_t offset,
+		unsigned int chunk_sectors)
+{
+	if (unlikely(!is_power_of_2(chunk_sectors)))
+		return chunk_sectors - sector_div(offset, chunk_sectors);
+	return chunk_sectors - (offset & (chunk_sectors - 1));
+}
+
 /*
  * Return maximum size of a request at given offset. Only valid for
  * file system requests.
@@ -948,12 +959,8 @@ static inline unsigned int blk_max_size_offset(struct request_queue *q,
 			return q->limits.max_sectors;
 	}
 
-	if (likely(is_power_of_2(chunk_sectors)))
-		chunk_sectors -= offset & (chunk_sectors - 1);
-	else
-		chunk_sectors -= sector_div(offset, chunk_sectors);
-
-	return min(q->limits.max_sectors, chunk_sectors);
+	return min(q->limits.max_sectors,
+			blk_chunk_sectors_left(offset, chunk_sectors));
 }
 
 /*
-- 
2.30.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 2/6] dm: open code blk_max_size_offset in max_io_len

[Christoph Hellwig]

max_io_len always passes an explicitly non-zero chunk_sectors into
blk_max_size_offset.  That means much of blk_max_size_offset is not
needed and can be open coded to simplify the code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/dm.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index d8f16183bf27c..0514358a1f8e5 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1079,23 +1079,18 @@ static sector_t max_io_len(struct dm_target *ti, sector_t sector)
 {
 	sector_t target_offset = dm_target_offset(ti, sector);
 	sector_t len = max_io_len_target_boundary(ti, target_offset);
-	sector_t max_len;
 
 	/*
 	 * Does the target need to split IO even further?
 	 * - varied (per target) IO splitting is a tenet of DM; this
 	 *   explains why stacked chunk_sectors based splitting via
-	 *   blk_max_size_offset() isn't possible here. So pass in
-	 *   ti->max_io_len to override stacked chunk_sectors.
+	 *   blk_queue_split() isn't possible here.
 	 */
-	if (ti->max_io_len) {
-		max_len = blk_max_size_offset(ti->table->md->queue,
-					      target_offset, ti->max_io_len);
-		if (len > max_len)
-			len = max_len;
-	}
-
-	return len;
+	if (!ti->max_io_len)
+		return len;
+	return min_t(sector_t, len,
+		min(queue_max_sectors(ti->table->md->queue),
+		    blk_chunk_sectors_left(target_offset, ti->max_io_len)));
 }
 
 int dm_set_target_max_io_len(struct dm_target *ti, sector_t len)
-- 
2.30.2

[dm-devel] [PATCH 2/6] dm: open code blk_max_size_offset in max_io_len

[Christoph Hellwig]

max_io_len always passes an explicitly non-zero chunk_sectors into
blk_max_size_offset.  That means much of blk_max_size_offset is not
needed and can be open coded to simplify the code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/dm.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index d8f16183bf27c..0514358a1f8e5 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1079,23 +1079,18 @@ static sector_t max_io_len(struct dm_target *ti, sector_t sector)
 {
 	sector_t target_offset = dm_target_offset(ti, sector);
 	sector_t len = max_io_len_target_boundary(ti, target_offset);
-	sector_t max_len;
 
 	/*
 	 * Does the target need to split IO even further?
 	 * - varied (per target) IO splitting is a tenet of DM; this
 	 *   explains why stacked chunk_sectors based splitting via
-	 *   blk_max_size_offset() isn't possible here. So pass in
-	 *   ti->max_io_len to override stacked chunk_sectors.
+	 *   blk_queue_split() isn't possible here.
 	 */
-	if (ti->max_io_len) {
-		max_len = blk_max_size_offset(ti->table->md->queue,
-					      target_offset, ti->max_io_len);
-		if (len > max_len)
-			len = max_len;
-	}
-
-	return len;
+	if (!ti->max_io_len)
+		return len;
+	return min_t(sector_t, len,
+		min(queue_max_sectors(ti->table->md->queue),
+		    blk_chunk_sectors_left(target_offset, ti->max_io_len)));
 }
 
 int dm_set_target_max_io_len(struct dm_target *ti, sector_t len)
-- 
2.30.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 3/6] block: open code blk_max_size_offset in blk_rq_get_max_sectors

[Christoph Hellwig]

blk_rq_get_max_sectors always uses q->limits.chunk_sectors as the
chunk_sectors argument, and already checks for max_sectors through the
call to blk_queue_get_max_sectors.  That means much of
blk_max_size_offset is not needed and open coding it simplifies the code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-merge.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index db2e03c8af7f4..df003ecfbd474 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -566,17 +566,18 @@ static inline unsigned int blk_rq_get_max_sectors(struct request *rq,
 						  sector_t offset)
 {
 	struct request_queue *q = rq->q;
+	unsigned int max_sectors;
 
 	if (blk_rq_is_passthrough(rq))
 		return q->limits.max_hw_sectors;
 
+	max_sectors = blk_queue_get_max_sectors(q, req_op(rq));
 	if (!q->limits.chunk_sectors ||
 	    req_op(rq) == REQ_OP_DISCARD ||
 	    req_op(rq) == REQ_OP_SECURE_ERASE)
-		return blk_queue_get_max_sectors(q, req_op(rq));
-
-	return min(blk_max_size_offset(q, offset, 0),
-			blk_queue_get_max_sectors(q, req_op(rq)));
+		return max_sectors;
+	return min(max_sectors,
+		   blk_chunk_sectors_left(offset, q->limits.chunk_sectors));
 }
 
 static inline int ll_new_hw_segment(struct request *req, struct bio *bio,
-- 
2.30.2

[dm-devel] [PATCH 3/6] block: open code blk_max_size_offset in blk_rq_get_max_sectors

[Christoph Hellwig]

blk_rq_get_max_sectors always uses q->limits.chunk_sectors as the
chunk_sectors argument, and already checks for max_sectors through the
call to blk_queue_get_max_sectors.  That means much of
blk_max_size_offset is not needed and open coding it simplifies the code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-merge.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index db2e03c8af7f4..df003ecfbd474 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -566,17 +566,18 @@ static inline unsigned int blk_rq_get_max_sectors(struct request *rq,
 						  sector_t offset)
 {
 	struct request_queue *q = rq->q;
+	unsigned int max_sectors;
 
 	if (blk_rq_is_passthrough(rq))
 		return q->limits.max_hw_sectors;
 
+	max_sectors = blk_queue_get_max_sectors(q, req_op(rq));
 	if (!q->limits.chunk_sectors ||
 	    req_op(rq) == REQ_OP_DISCARD ||
 	    req_op(rq) == REQ_OP_SECURE_ERASE)
-		return blk_queue_get_max_sectors(q, req_op(rq));
-
-	return min(blk_max_size_offset(q, offset, 0),
-			blk_queue_get_max_sectors(q, req_op(rq)));
+		return max_sectors;
+	return min(max_sectors,
+		   blk_chunk_sectors_left(offset, q->limits.chunk_sectors));
 }
 
 static inline int ll_new_hw_segment(struct request *req, struct bio *bio,
-- 
2.30.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 4/6] block: cleanup variable naming in get_max_io_size

[Christoph Hellwig]

get_max_io_size has a very odd choice of variables names and
initialization patterns.  Switch to more descriptive names and more
clear initialization of them.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-merge.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index df003ecfbd474..4da981efddeed 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -164,18 +164,16 @@ static struct bio *blk_bio_write_zeroes_split(struct request_queue *q,
 static inline unsigned get_max_io_size(struct request_queue *q,
 				       struct bio *bio)
 {
-	unsigned sectors = blk_max_size_offset(q, bio->bi_iter.bi_sector, 0);
-	unsigned max_sectors = sectors;
 	unsigned pbs = queue_physical_block_size(q) >> SECTOR_SHIFT;
 	unsigned lbs = queue_logical_block_size(q) >> SECTOR_SHIFT;
-	unsigned start_offset = bio->bi_iter.bi_sector & (pbs - 1);
-
-	max_sectors += start_offset;
-	max_sectors &= ~(pbs - 1);
-	if (max_sectors > start_offset)
-		return max_sectors - start_offset;
-
-	return sectors & ~(lbs - 1);
+	unsigned max_sectors, start, end;
+
+	max_sectors = blk_max_size_offset(q, bio->bi_iter.bi_sector, 0);
+	start = bio->bi_iter.bi_sector & (pbs - 1);
+	end = (start + max_sectors) & ~(pbs - 1);
+	if (end > start)
+		return end - start;
+	return max_sectors & ~(lbs - 1);
 }
 
 static inline unsigned get_max_segment_size(const struct request_queue *q,
-- 
2.30.2

[dm-devel] [PATCH 4/6] block: cleanup variable naming in get_max_io_size

[Christoph Hellwig]

get_max_io_size has a very odd choice of variables names and
initialization patterns.  Switch to more descriptive names and more
clear initialization of them.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-merge.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index df003ecfbd474..4da981efddeed 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -164,18 +164,16 @@ static struct bio *blk_bio_write_zeroes_split(struct request_queue *q,
 static inline unsigned get_max_io_size(struct request_queue *q,
 				       struct bio *bio)
 {
-	unsigned sectors = blk_max_size_offset(q, bio->bi_iter.bi_sector, 0);
-	unsigned max_sectors = sectors;
 	unsigned pbs = queue_physical_block_size(q) >> SECTOR_SHIFT;
 	unsigned lbs = queue_logical_block_size(q) >> SECTOR_SHIFT;
-	unsigned start_offset = bio->bi_iter.bi_sector & (pbs - 1);
-
-	max_sectors += start_offset;
-	max_sectors &= ~(pbs - 1);
-	if (max_sectors > start_offset)
-		return max_sectors - start_offset;
-
-	return sectors & ~(lbs - 1);
+	unsigned max_sectors, start, end;
+
+	max_sectors = blk_max_size_offset(q, bio->bi_iter.bi_sector, 0);
+	start = bio->bi_iter.bi_sector & (pbs - 1);
+	end = (start + max_sectors) & ~(pbs - 1);
+	if (end > start)
+		return end - start;
+	return max_sectors & ~(lbs - 1);
 }
 
 static inline unsigned get_max_segment_size(const struct request_queue *q,
-- 
2.30.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 5/6] block: fold blk_max_size_offset into get_max_io_size

[Christoph Hellwig]

Now that blk_max_size_offset has a single caller left, fold it into that
and clean up the naming convention for the local variables there.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-merge.c      |  9 +++++++--
 include/linux/blkdev.h | 19 -------------------
 2 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 4da981efddeed..0f5f42ebd0bb0 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -166,9 +166,14 @@ static inline unsigned get_max_io_size(struct request_queue *q,
 {
 	unsigned pbs = queue_physical_block_size(q) >> SECTOR_SHIFT;
 	unsigned lbs = queue_logical_block_size(q) >> SECTOR_SHIFT;
-	unsigned max_sectors, start, end;
+	unsigned max_sectors = queue_max_sectors(q), start, end;
+
+	if (q->limits.chunk_sectors) {
+		max_sectors = min(max_sectors,
+			blk_chunk_sectors_left(bio->bi_iter.bi_sector,
+					       q->limits.chunk_sectors));
+	}
 
-	max_sectors = blk_max_size_offset(q, bio->bi_iter.bi_sector, 0);
 	start = bio->bi_iter.bi_sector & (pbs - 1);
 	end = (start + max_sectors) & ~(pbs - 1);
 	if (end > start)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index e66ad451274ec..05e60ee269d91 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -944,25 +944,6 @@ static inline unsigned int blk_chunk_sectors_left(sector_t offset,
 	return chunk_sectors - (offset & (chunk_sectors - 1));
 }
 
-/*
- * Return maximum size of a request at given offset. Only valid for
- * file system requests.
- */
-static inline unsigned int blk_max_size_offset(struct request_queue *q,
-					       sector_t offset,
-					       unsigned int chunk_sectors)
-{
-	if (!chunk_sectors) {
-		if (q->limits.chunk_sectors)
-			chunk_sectors = q->limits.chunk_sectors;
-		else
-			return q->limits.max_sectors;
-	}
-
-	return min(q->limits.max_sectors,
-			blk_chunk_sectors_left(offset, chunk_sectors));
-}
-
 /*
  * Access functions for manipulating queue properties
  */
-- 
2.30.2

[dm-devel] [PATCH 5/6] block: fold blk_max_size_offset into get_max_io_size

[Christoph Hellwig]

Now that blk_max_size_offset has a single caller left, fold it into that
and clean up the naming convention for the local variables there.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-merge.c      |  9 +++++++--
 include/linux/blkdev.h | 19 -------------------
 2 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 4da981efddeed..0f5f42ebd0bb0 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -166,9 +166,14 @@ static inline unsigned get_max_io_size(struct request_queue *q,
 {
 	unsigned pbs = queue_physical_block_size(q) >> SECTOR_SHIFT;
 	unsigned lbs = queue_logical_block_size(q) >> SECTOR_SHIFT;
-	unsigned max_sectors, start, end;
+	unsigned max_sectors = queue_max_sectors(q), start, end;
+
+	if (q->limits.chunk_sectors) {
+		max_sectors = min(max_sectors,
+			blk_chunk_sectors_left(bio->bi_iter.bi_sector,
+					       q->limits.chunk_sectors));
+	}
 
-	max_sectors = blk_max_size_offset(q, bio->bi_iter.bi_sector, 0);
 	start = bio->bi_iter.bi_sector & (pbs - 1);
 	end = (start + max_sectors) & ~(pbs - 1);
 	if (end > start)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index e66ad451274ec..05e60ee269d91 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -944,25 +944,6 @@ static inline unsigned int blk_chunk_sectors_left(sector_t offset,
 	return chunk_sectors - (offset & (chunk_sectors - 1));
 }
 
-/*
- * Return maximum size of a request at given offset. Only valid for
- * file system requests.
- */
-static inline unsigned int blk_max_size_offset(struct request_queue *q,
-					       sector_t offset,
-					       unsigned int chunk_sectors)
-{
-	if (!chunk_sectors) {
-		if (q->limits.chunk_sectors)
-			chunk_sectors = q->limits.chunk_sectors;
-		else
-			return q->limits.max_sectors;
-	}
-
-	return min(q->limits.max_sectors,
-			blk_chunk_sectors_left(offset, chunk_sectors));
-}
-
 /*
  * Access functions for manipulating queue properties
  */
-- 
2.30.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 6/6] block: move blk_queue_get_max_sectors to blk.h

[Christoph Hellwig]

blk_queue_get_max_sectors is private to the block layer, so move it out
of blkdev.h.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk.h            | 13 +++++++++++++
 include/linux/blkdev.h | 13 -------------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/block/blk.h b/block/blk.h
index 434017701403f..8e79296ee97a2 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -159,6 +159,19 @@ static inline bool blk_discard_mergable(struct request *req)
 	return false;
 }
 
+static inline unsigned int blk_queue_get_max_sectors(struct request_queue *q,
+						     int op)
+{
+	if (unlikely(op == REQ_OP_DISCARD || op == REQ_OP_SECURE_ERASE))
+		return min(q->limits.max_discard_sectors,
+			   UINT_MAX >> SECTOR_SHIFT);
+
+	if (unlikely(op == REQ_OP_WRITE_ZEROES))
+		return q->limits.max_write_zeroes_sectors;
+
+	return q->limits.max_sectors;
+}
+
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 void blk_flush_integrity(void);
 bool __bio_integrity_endio(struct bio *);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 05e60ee269d91..5ef2f061feb08 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -920,19 +920,6 @@ static inline unsigned int bio_zone_is_seq(struct bio *bio)
 }
 #endif /* CONFIG_BLK_DEV_ZONED */
 
-static inline unsigned int blk_queue_get_max_sectors(struct request_queue *q,
-						     int op)
-{
-	if (unlikely(op == REQ_OP_DISCARD || op == REQ_OP_SECURE_ERASE))
-		return min(q->limits.max_discard_sectors,
-			   UINT_MAX >> SECTOR_SHIFT);
-
-	if (unlikely(op == REQ_OP_WRITE_ZEROES))
-		return q->limits.max_write_zeroes_sectors;
-
-	return q->limits.max_sectors;
-}
-
 /*
  * Return how much of the chunk is left to be used for I/O at a given offset.
  */
-- 
2.30.2

[dm-devel] [PATCH 6/6] block: move blk_queue_get_max_sectors to blk.h

[Christoph Hellwig]

blk_queue_get_max_sectors is private to the block layer, so move it out
of blkdev.h.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk.h            | 13 +++++++++++++
 include/linux/blkdev.h | 13 -------------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/block/blk.h b/block/blk.h
index 434017701403f..8e79296ee97a2 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -159,6 +159,19 @@ static inline bool blk_discard_mergable(struct request *req)
 	return false;
 }
 
+static inline unsigned int blk_queue_get_max_sectors(struct request_queue *q,
+						     int op)
+{
+	if (unlikely(op == REQ_OP_DISCARD || op == REQ_OP_SECURE_ERASE))
+		return min(q->limits.max_discard_sectors,
+			   UINT_MAX >> SECTOR_SHIFT);
+
+	if (unlikely(op == REQ_OP_WRITE_ZEROES))
+		return q->limits.max_write_zeroes_sectors;
+
+	return q->limits.max_sectors;
+}
+
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 void blk_flush_integrity(void);
 bool __bio_integrity_endio(struct bio *);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 05e60ee269d91..5ef2f061feb08 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -920,19 +920,6 @@ static inline unsigned int bio_zone_is_seq(struct bio *bio)
 }
 #endif /* CONFIG_BLK_DEV_ZONED */
 
-static inline unsigned int blk_queue_get_max_sectors(struct request_queue *q,
-						     int op)
-{
-	if (unlikely(op == REQ_OP_DISCARD || op == REQ_OP_SECURE_ERASE))
-		return min(q->limits.max_discard_sectors,
-			   UINT_MAX >> SECTOR_SHIFT);
-
-	if (unlikely(op == REQ_OP_WRITE_ZEROES))
-		return q->limits.max_write_zeroes_sectors;
-
-	return q->limits.max_sectors;
-}
-
 /*
  * Return how much of the chunk is left to be used for I/O at a given offset.
  */
-- 
2.30.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 1/6] block: factor out a chunk_size_left helper

[Bart Van Assche]

On 6/14/22 02:09, Christoph Hellwig wrote:
> Factor out a helper from blk_max_size_offset so that it can be reused
> independently. 
Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [dm-devel] [PATCH 1/6] block: factor out a chunk_size_left helper

[Bart Van Assche]

On 6/14/22 02:09, Christoph Hellwig wrote:
> Factor out a helper from blk_max_size_offset so that it can be reused
> independently. 
Reviewed-by: Bart Van Assche <bvanassche@acm.org>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 3/6] block: open code blk_max_size_offset in blk_rq_get_max_sectors

[Bart Van Assche]

On 6/14/22 02:09, Christoph Hellwig wrote:
> blk_rq_get_max_sectors always uses q->limits.chunk_sectors as the
> chunk_sectors argument, and already checks for max_sectors through the
> call to blk_queue_get_max_sectors.  That means much of
> blk_max_size_offset is not needed and open coding it simplifies the code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   block/blk-merge.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index db2e03c8af7f4..df003ecfbd474 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -566,17 +566,18 @@ static inline unsigned int blk_rq_get_max_sectors(struct request *rq,
>   						  sector_t offset)
>   {
>   	struct request_queue *q = rq->q;
> +	unsigned int max_sectors;
>   
>   	if (blk_rq_is_passthrough(rq))
>   		return q->limits.max_hw_sectors;
>   
> +	max_sectors = blk_queue_get_max_sectors(q, req_op(rq));
>   	if (!q->limits.chunk_sectors ||
>   	    req_op(rq) == REQ_OP_DISCARD ||
>   	    req_op(rq) == REQ_OP_SECURE_ERASE)
> -		return blk_queue_get_max_sectors(q, req_op(rq));
> -
> -	return min(blk_max_size_offset(q, offset, 0),
> -			blk_queue_get_max_sectors(q, req_op(rq)));
> +		return max_sectors;
> +	return min(max_sectors,
> +		   blk_chunk_sectors_left(offset, q->limits.chunk_sectors));
>   }

blk_set_default_limits() initializes chunk_sectors to zero and 
blk_chunk_sectors_left() triggers a division by zero if a zero is passed 
as the second argument. What am I missing?

Thanks,

Bart.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 3/6] block: open code blk_max_size_offset in blk_rq_get_max_sectors

[Bart Van Assche]

On 6/14/22 02:09, Christoph Hellwig wrote:
> blk_rq_get_max_sectors always uses q->limits.chunk_sectors as the
> chunk_sectors argument, and already checks for max_sectors through the
> call to blk_queue_get_max_sectors.  That means much of
> blk_max_size_offset is not needed and open coding it simplifies the code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   block/blk-merge.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index db2e03c8af7f4..df003ecfbd474 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -566,17 +566,18 @@ static inline unsigned int blk_rq_get_max_sectors(struct request *rq,
>   						  sector_t offset)
>   {
>   	struct request_queue *q = rq->q;
> +	unsigned int max_sectors;
>   
>   	if (blk_rq_is_passthrough(rq))
>   		return q->limits.max_hw_sectors;
>   
> +	max_sectors = blk_queue_get_max_sectors(q, req_op(rq));
>   	if (!q->limits.chunk_sectors ||
>   	    req_op(rq) == REQ_OP_DISCARD ||
>   	    req_op(rq) == REQ_OP_SECURE_ERASE)
> -		return blk_queue_get_max_sectors(q, req_op(rq));
> -
> -	return min(blk_max_size_offset(q, offset, 0),
> -			blk_queue_get_max_sectors(q, req_op(rq)));
> +		return max_sectors;
> +	return min(max_sectors,
> +		   blk_chunk_sectors_left(offset, q->limits.chunk_sectors));
>   }

blk_set_default_limits() initializes chunk_sectors to zero and 
blk_chunk_sectors_left() triggers a division by zero if a zero is passed 
as the second argument. What am I missing?

Thanks,

Bart.

Re: [PATCH 3/6] block: open code blk_max_size_offset in blk_rq_get_max_sectors

[Bart Van Assche]

On 6/14/22 09:43, Bart Van Assche wrote:
> On 6/14/22 02:09, Christoph Hellwig wrote:
>> blk_rq_get_max_sectors always uses q->limits.chunk_sectors as the
>> chunk_sectors argument, and already checks for max_sectors through the
>> call to blk_queue_get_max_sectors.  That means much of
>> blk_max_size_offset is not needed and open coding it simplifies the code.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> ---
>>   block/blk-merge.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/blk-merge.c b/block/blk-merge.c
>> index db2e03c8af7f4..df003ecfbd474 100644
>> --- a/block/blk-merge.c
>> +++ b/block/blk-merge.c
>> @@ -566,17 +566,18 @@ static inline unsigned int 
>> blk_rq_get_max_sectors(struct request *rq,
>>                             sector_t offset)
>>   {
>>       struct request_queue *q = rq->q;
>> +    unsigned int max_sectors;
>>       if (blk_rq_is_passthrough(rq))
>>           return q->limits.max_hw_sectors;
>> +    max_sectors = blk_queue_get_max_sectors(q, req_op(rq));
>>       if (!q->limits.chunk_sectors ||
>>           req_op(rq) == REQ_OP_DISCARD ||
>>           req_op(rq) == REQ_OP_SECURE_ERASE)
>> -        return blk_queue_get_max_sectors(q, req_op(rq));
>> -
>> -    return min(blk_max_size_offset(q, offset, 0),
>> -            blk_queue_get_max_sectors(q, req_op(rq)));
>> +        return max_sectors;
>> +    return min(max_sectors,
>> +           blk_chunk_sectors_left(offset, q->limits.chunk_sectors));
>>   }
> 
> blk_set_default_limits() initializes chunk_sectors to zero and 
> blk_chunk_sectors_left() triggers a division by zero if a zero is passed 
> as the second argument. What am I missing?

Answering my own question: I overlooked one of the return statements.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [dm-devel] [PATCH 3/6] block: open code blk_max_size_offset in blk_rq_get_max_sectors

[Bart Van Assche]

On 6/14/22 09:43, Bart Van Assche wrote:
> On 6/14/22 02:09, Christoph Hellwig wrote:
>> blk_rq_get_max_sectors always uses q->limits.chunk_sectors as the
>> chunk_sectors argument, and already checks for max_sectors through the
>> call to blk_queue_get_max_sectors.  That means much of
>> blk_max_size_offset is not needed and open coding it simplifies the code.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> ---
>>   block/blk-merge.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/blk-merge.c b/block/blk-merge.c
>> index db2e03c8af7f4..df003ecfbd474 100644
>> --- a/block/blk-merge.c
>> +++ b/block/blk-merge.c
>> @@ -566,17 +566,18 @@ static inline unsigned int 
>> blk_rq_get_max_sectors(struct request *rq,
>>                             sector_t offset)
>>   {
>>       struct request_queue *q = rq->q;
>> +    unsigned int max_sectors;
>>       if (blk_rq_is_passthrough(rq))
>>           return q->limits.max_hw_sectors;
>> +    max_sectors = blk_queue_get_max_sectors(q, req_op(rq));
>>       if (!q->limits.chunk_sectors ||
>>           req_op(rq) == REQ_OP_DISCARD ||
>>           req_op(rq) == REQ_OP_SECURE_ERASE)
>> -        return blk_queue_get_max_sectors(q, req_op(rq));
>> -
>> -    return min(blk_max_size_offset(q, offset, 0),
>> -            blk_queue_get_max_sectors(q, req_op(rq)));
>> +        return max_sectors;
>> +    return min(max_sectors,
>> +           blk_chunk_sectors_left(offset, q->limits.chunk_sectors));
>>   }
> 
> blk_set_default_limits() initializes chunk_sectors to zero and 
> blk_chunk_sectors_left() triggers a division by zero if a zero is passed 
> as the second argument. What am I missing?

Answering my own question: I overlooked one of the return statements.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 6/6] block: move blk_queue_get_max_sectors to blk.h

[Bart Van Assche]

On 6/14/22 02:09, Christoph Hellwig wrote:
> blk_queue_get_max_sectors is private to the block layer, so move it out
> of blkdev.h.
Reviewed-by: Bart Van Assche <bvanassche@acm.org>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 6/6] block: move blk_queue_get_max_sectors to blk.h

[Bart Van Assche]

On 6/14/22 02:09, Christoph Hellwig wrote:
> blk_queue_get_max_sectors is private to the block layer, so move it out
> of blkdev.h.
Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 4/6] block: cleanup variable naming in get_max_io_size

[Bart Van Assche]

On 6/14/22 02:09, Christoph Hellwig wrote:
> get_max_io_size has a very odd choice of variables names and
> initialization patterns.  Switch to more descriptive names and more
> clear initialization of them.

Hmm ... what is so odd about the variable names? I have done my best to 
chose clear and descriptive names when I introduced these names.

Bart.

Re: [dm-devel] [PATCH 4/6] block: cleanup variable naming in get_max_io_size

[Bart Van Assche]

On 6/14/22 02:09, Christoph Hellwig wrote:
> get_max_io_size has a very odd choice of variables names and
> initialization patterns.  Switch to more descriptive names and more
> clear initialization of them.

Hmm ... what is so odd about the variable names? I have done my best to 
chose clear and descriptive names when I introduced these names.

Bart.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 5/6] block: fold blk_max_size_offset into get_max_io_size

[Bart Van Assche]

On 6/14/22 02:09, Christoph Hellwig wrote:
> Now that blk_max_size_offset has a single caller left, fold it into that
> and clean up the naming convention for the local variables there.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 5/6] block: fold blk_max_size_offset into get_max_io_size

[Bart Van Assche]

On 6/14/22 02:09, Christoph Hellwig wrote:
> Now that blk_max_size_offset has a single caller left, fold it into that
> and clean up the naming convention for the local variables there.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 4/6] block: cleanup variable naming in get_max_io_size

[Christoph Hellwig]

On Tue, Jun 14, 2022 at 09:50:00AM -0700, Bart Van Assche wrote:
> On 6/14/22 02:09, Christoph Hellwig wrote:
>> get_max_io_size has a very odd choice of variables names and
>> initialization patterns.  Switch to more descriptive names and more
>> clear initialization of them.
>
> Hmm ... what is so odd about the variable names? I have done my best to 
> chose clear and descriptive names when I introduced these names.

Major confusion:

 - we have a local variable to hold the queue max sectors value,
   but it is callled sectors
 - while we have a variable the holds the rounded end of the I/O
   range that is called max_sectors

Minor confusion:

 - a variable that hold that rounded start of the range has a random
   _offset suffix despite not really being an offset

Re: [dm-devel] [PATCH 4/6] block: cleanup variable naming in get_max_io_size

[Christoph Hellwig]

On Tue, Jun 14, 2022 at 09:50:00AM -0700, Bart Van Assche wrote:
> On 6/14/22 02:09, Christoph Hellwig wrote:
>> get_max_io_size has a very odd choice of variables names and
>> initialization patterns.  Switch to more descriptive names and more
>> clear initialization of them.
>
> Hmm ... what is so odd about the variable names? I have done my best to 
> chose clear and descriptive names when I introduced these names.

Major confusion:

 - we have a local variable to hold the queue max sectors value,
   but it is callled sectors
 - while we have a variable the holds the rounded end of the I/O
   range that is called max_sectors

Minor confusion:

 - a variable that hold that rounded start of the range has a random
   _offset suffix despite not really being an offset

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 1/6] block: factor out a chunk_size_left helper

[Pankaj Raghav]

On Tue, Jun 14, 2022 at 11:09:29AM +0200, Christoph Hellwig wrote:
> Factor out a helper from blk_max_size_offset so that it can be reused
> independently.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/linux/blkdev.h | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 

Looks good,
Reviewed-by: Pankaj Raghav <p.raghav@samsung.com>

Re: [dm-devel] [PATCH 1/6] block: factor out a chunk_size_left helper

[Pankaj Raghav]

On Tue, Jun 14, 2022 at 11:09:29AM +0200, Christoph Hellwig wrote:
> Factor out a helper from blk_max_size_offset so that it can be reused
> independently.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/linux/blkdev.h | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 

Looks good,
Reviewed-by: Pankaj Raghav <p.raghav@samsung.com>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 5/6] block: fold blk_max_size_offset into get_max_io_size

[Pankaj Raghav]

On Tue, Jun 14, 2022 at 11:09:33AM +0200, Christoph Hellwig wrote:
> Now that blk_max_size_offset has a single caller left, fold it into that
> and clean up the naming convention for the local variables there.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-merge.c      |  9 +++++++--
>  include/linux/blkdev.h | 19 -------------------
>  2 files changed, 7 insertions(+), 21 deletions(-)

Looks good,
Reviewed-by: Pankaj Raghav <p.raghav@samsung.com>

Re: [dm-devel] [PATCH 5/6] block: fold blk_max_size_offset into get_max_io_size

[Pankaj Raghav]

On Tue, Jun 14, 2022 at 11:09:33AM +0200, Christoph Hellwig wrote:
> Now that blk_max_size_offset has a single caller left, fold it into that
> and clean up the naming convention for the local variables there.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-merge.c      |  9 +++++++--
>  include/linux/blkdev.h | 19 -------------------
>  2 files changed, 7 insertions(+), 21 deletions(-)

Looks good,
Reviewed-by: Pankaj Raghav <p.raghav@samsung.com>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 1/6] block: factor out a chunk_size_left helper

[Mike Snitzer]

On Tue, Jun 14 2022 at  5:09P -0400,
Christoph Hellwig <hch@lst.de> wrote:

> Factor out a helper from blk_max_size_offset so that it can be reused
> independently.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/linux/blkdev.h | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 914c613d81da7..e66ad451274ec 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -933,6 +933,17 @@ static inline unsigned int blk_queue_get_max_sectors(struct request_queue *q,
>  	return q->limits.max_sectors;
>  }
>  
> +/*
> + * Return how much of the chunk is left to be used for I/O at a given offset.
> + */
> +static inline unsigned int blk_chunk_sectors_left(sector_t offset,
> +		unsigned int chunk_sectors)
> +{
> +	if (unlikely(!is_power_of_2(chunk_sectors)))
> +		return chunk_sectors - sector_div(offset, chunk_sectors);
> +	return chunk_sectors - (offset & (chunk_sectors - 1));
> +}
> +
>  /*
>   * Return maximum size of a request at given offset. Only valid for
>   * file system requests.
> @@ -948,12 +959,8 @@ static inline unsigned int blk_max_size_offset(struct request_queue *q,
>  			return q->limits.max_sectors;
>  	}
>  
> -	if (likely(is_power_of_2(chunk_sectors)))
> -		chunk_sectors -= offset & (chunk_sectors - 1);
> -	else
> -		chunk_sectors -= sector_div(offset, chunk_sectors);
> -
> -	return min(q->limits.max_sectors, chunk_sectors);
> +	return min(q->limits.max_sectors,
> +			blk_chunk_sectors_left(offset, chunk_sectors));
>  }

While you're at it, any reason not to use queue_max_sectors() here?

Re: [dm-devel] [PATCH 1/6] block: factor out a chunk_size_left helper

[Mike Snitzer]

On Tue, Jun 14 2022 at  5:09P -0400,
Christoph Hellwig <hch@lst.de> wrote:

> Factor out a helper from blk_max_size_offset so that it can be reused
> independently.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/linux/blkdev.h | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 914c613d81da7..e66ad451274ec 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -933,6 +933,17 @@ static inline unsigned int blk_queue_get_max_sectors(struct request_queue *q,
>  	return q->limits.max_sectors;
>  }
>  
> +/*
> + * Return how much of the chunk is left to be used for I/O at a given offset.
> + */
> +static inline unsigned int blk_chunk_sectors_left(sector_t offset,
> +		unsigned int chunk_sectors)
> +{
> +	if (unlikely(!is_power_of_2(chunk_sectors)))
> +		return chunk_sectors - sector_div(offset, chunk_sectors);
> +	return chunk_sectors - (offset & (chunk_sectors - 1));
> +}
> +
>  /*
>   * Return maximum size of a request at given offset. Only valid for
>   * file system requests.
> @@ -948,12 +959,8 @@ static inline unsigned int blk_max_size_offset(struct request_queue *q,
>  			return q->limits.max_sectors;
>  	}
>  
> -	if (likely(is_power_of_2(chunk_sectors)))
> -		chunk_sectors -= offset & (chunk_sectors - 1);
> -	else
> -		chunk_sectors -= sector_div(offset, chunk_sectors);
> -
> -	return min(q->limits.max_sectors, chunk_sectors);
> +	return min(q->limits.max_sectors,
> +			blk_chunk_sectors_left(offset, chunk_sectors));
>  }

While you're at it, any reason not to use queue_max_sectors() here?

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 2/6] dm: open code blk_max_size_offset in max_io_len

[Mike Snitzer]

On Tue, Jun 14 2022 at  5:09P -0400,
Christoph Hellwig <hch@lst.de> wrote:

> max_io_len always passes an explicitly non-zero chunk_sectors into
> blk_max_size_offset.  That means much of blk_max_size_offset is not
> needed and can be open coded to simplify the code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/md/dm.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index d8f16183bf27c..0514358a1f8e5 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1079,23 +1079,18 @@ static sector_t max_io_len(struct dm_target *ti, sector_t sector)
>  {
>  	sector_t target_offset = dm_target_offset(ti, sector);
>  	sector_t len = max_io_len_target_boundary(ti, target_offset);
> -	sector_t max_len;
>  
>  	/*
>  	 * Does the target need to split IO even further?
>  	 * - varied (per target) IO splitting is a tenet of DM; this
>  	 *   explains why stacked chunk_sectors based splitting via
> -	 *   blk_max_size_offset() isn't possible here. So pass in
> -	 *   ti->max_io_len to override stacked chunk_sectors.
> +	 *   blk_queue_split() isn't possible here.
>  	 */
> -	if (ti->max_io_len) {
> -		max_len = blk_max_size_offset(ti->table->md->queue,
> -					      target_offset, ti->max_io_len);
> -		if (len > max_len)
> -			len = max_len;
> -	}
> -
> -	return len;
> +	if (!ti->max_io_len)
> +		return len;
> +	return min_t(sector_t, len,
> +		min(queue_max_sectors(ti->table->md->queue),
> +		    blk_chunk_sectors_left(target_offset, ti->max_io_len)));
>  }
>  
>  int dm_set_target_max_io_len(struct dm_target *ti, sector_t len)
> -- 
> 2.30.2
> 

Not in love with the nested min() but don't have a better suggestion.

Reviewed-by: Mike Snitzer <snitzer@kernel.org>

Re: [dm-devel] [PATCH 2/6] dm: open code blk_max_size_offset in max_io_len

[Mike Snitzer]

On Tue, Jun 14 2022 at  5:09P -0400,
Christoph Hellwig <hch@lst.de> wrote:

> max_io_len always passes an explicitly non-zero chunk_sectors into
> blk_max_size_offset.  That means much of blk_max_size_offset is not
> needed and can be open coded to simplify the code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/md/dm.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index d8f16183bf27c..0514358a1f8e5 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1079,23 +1079,18 @@ static sector_t max_io_len(struct dm_target *ti, sector_t sector)
>  {
>  	sector_t target_offset = dm_target_offset(ti, sector);
>  	sector_t len = max_io_len_target_boundary(ti, target_offset);
> -	sector_t max_len;
>  
>  	/*
>  	 * Does the target need to split IO even further?
>  	 * - varied (per target) IO splitting is a tenet of DM; this
>  	 *   explains why stacked chunk_sectors based splitting via
> -	 *   blk_max_size_offset() isn't possible here. So pass in
> -	 *   ti->max_io_len to override stacked chunk_sectors.
> +	 *   blk_queue_split() isn't possible here.
>  	 */
> -	if (ti->max_io_len) {
> -		max_len = blk_max_size_offset(ti->table->md->queue,
> -					      target_offset, ti->max_io_len);
> -		if (len > max_len)
> -			len = max_len;
> -	}
> -
> -	return len;
> +	if (!ti->max_io_len)
> +		return len;
> +	return min_t(sector_t, len,
> +		min(queue_max_sectors(ti->table->md->queue),
> +		    blk_chunk_sectors_left(target_offset, ti->max_io_len)));
>  }
>  
>  int dm_set_target_max_io_len(struct dm_target *ti, sector_t len)
> -- 
> 2.30.2
> 

Not in love with the nested min() but don't have a better suggestion.

Reviewed-by: Mike Snitzer <snitzer@kernel.org>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 1/6] block: factor out a chunk_size_left helper

[Christoph Hellwig]

On Thu, Jun 16, 2022 at 07:05:48PM -0400, Mike Snitzer wrote:
> > +	return min(q->limits.max_sectors,
> > +			blk_chunk_sectors_left(offset, chunk_sectors));
> >  }
> 
> While you're at it, any reason not to use queue_max_sectors() here?

I'm not sure if it is a good reason, but this code sniplet goes
away later in the series, and all replacemens do use queue_max_sectors.

Re: [dm-devel] [PATCH 1/6] block: factor out a chunk_size_left helper

[Christoph Hellwig]

On Thu, Jun 16, 2022 at 07:05:48PM -0400, Mike Snitzer wrote:
> > +	return min(q->limits.max_sectors,
> > +			blk_chunk_sectors_left(offset, chunk_sectors));
> >  }
> 
> While you're at it, any reason not to use queue_max_sectors() here?

I'm not sure if it is a good reason, but this code sniplet goes
away later in the series, and all replacemens do use queue_max_sectors.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: clean up the chunk_sizehandling helpers a little

[Jens Axboe]

On Tue, 14 Jun 2022 11:09:28 +0200, Christoph Hellwig wrote:
> this series cleans up a bunch of block layer helpers related to the chunk
> size.
> 
> Diffstat:
>  block/blk-merge.c      |   28 ++++++++++++++++------------
>  block/blk.h            |   13 +++++++++++++
>  drivers/md/dm.c        |   17 ++++++-----------
>  include/linux/blkdev.h |   39 +++++++--------------------------------
>  4 files changed, 42 insertions(+), 55 deletions(-)
> 
> [...]

Applied, thanks!

[1/6] block: factor out a chunk_size_left helper
      commit: d0e3310bb972f65c4b614c29f8022f57a52123c8
[2/6] dm: open code blk_max_size_offset in max_io_len
      commit: 6d5661a5d0e513dde5d49820315c5d6249a5c732
[3/6] block: open code blk_max_size_offset in blk_rq_get_max_sectors
      commit: 92ac28684e7eccf968b556893ca09c57d1fb3cdd
[4/6] block: cleanup variable naming in get_max_io_size
      commit: 08fdba80df1fd78a22b00e96ffd062a5bbaf8d8e
[5/6] block: fold blk_max_size_offset into get_max_io_size
      commit: d8f1d38c87b87ea3a0a0c58b6386333731e29470
[6/6] block: move blk_queue_get_max_sectors to blk.h
      commit: d8fca63495fb21e9b2dfcf722346aa844459139a

Best regards,
-- 
Jens Axboe

Re: [dm-devel] clean up the chunk_sizehandling helpers a little

[Jens Axboe]

On Tue, 14 Jun 2022 11:09:28 +0200, Christoph Hellwig wrote:
> this series cleans up a bunch of block layer helpers related to the chunk
> size.
> 
> Diffstat:
>  block/blk-merge.c      |   28 ++++++++++++++++------------
>  block/blk.h            |   13 +++++++++++++
>  drivers/md/dm.c        |   17 ++++++-----------
>  include/linux/blkdev.h |   39 +++++++--------------------------------
>  4 files changed, 42 insertions(+), 55 deletions(-)
> 
> [...]

Applied, thanks!

[1/6] block: factor out a chunk_size_left helper
      commit: d0e3310bb972f65c4b614c29f8022f57a52123c8
[2/6] dm: open code blk_max_size_offset in max_io_len
      commit: 6d5661a5d0e513dde5d49820315c5d6249a5c732
[3/6] block: open code blk_max_size_offset in blk_rq_get_max_sectors
      commit: 92ac28684e7eccf968b556893ca09c57d1fb3cdd
[4/6] block: cleanup variable naming in get_max_io_size
      commit: 08fdba80df1fd78a22b00e96ffd062a5bbaf8d8e
[5/6] block: fold blk_max_size_offset into get_max_io_size
      commit: d8f1d38c87b87ea3a0a0c58b6386333731e29470
[6/6] block: move blk_queue_get_max_sectors to blk.h
      commit: d8fca63495fb21e9b2dfcf722346aa844459139a

Best regards,
-- 
Jens Axboe


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 1/6] block: factor out a chunk_size_left helper

[Christoph Hellwig]

Factor out a helper from blk_max_size_offset so that it can be reused
independently.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/blkdev.h | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 17705c970d7e1..c6ecd996c2d46 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -622,6 +622,18 @@ static inline unsigned int blk_queue_get_max_sectors(struct request_queue *q,
 	return q->limits.max_sectors;
 }
 
+/*
+ * Return how much of the chunk sectors is left to be used for an I/O at the
+ * given offset.
+ */
+static inline unsigned int blk_chunk_sectors_left(sector_t offset,
+		unsigned int chunk_sectors)
+{
+	if (unlikely(!is_power_of_2(chunk_sectors)))
+		return chunk_sectors - sector_div(offset, chunk_sectors);
+	return chunk_sectors - (offset & (chunk_sectors - 1));
+}
+
 /*
  * Return maximum size of a request at given offset. Only valid for
  * file system requests.
@@ -637,12 +649,8 @@ static inline unsigned int blk_max_size_offset(struct request_queue *q,
 			return q->limits.max_sectors;
 	}
 
-	if (likely(is_power_of_2(chunk_sectors)))
-		chunk_sectors -= offset & (chunk_sectors - 1);
-	else
-		chunk_sectors -= sector_div(offset, chunk_sectors);
-
-	return min(q->limits.max_sectors, chunk_sectors);
+	return min(q->limits.max_sectors,
+			blk_chunk_sectors_left(offset, chunk_sectors));
 }
 
 /*
-- 
2.30.2