[PATCH v2 0/2] Introduce bytes_to_sectors helper in blkdev.h

[PATCH v2 0/2] Introduce bytes_to_sectors helper in blkdev.h

[Marcos Paulo de Souza]

Changes from v2:
Rename size_to_sectors o bytes_to_sectors. (suggested by Martin K. Petersen)

Changes from v1:
Reworked the documentation of size_to_sectors by removing a sentence that was
explaining the size -> sectors math, which wasn't necessary given the
description prior to the example. (suggested by Chaitanya Kulkarni)

Let me know if you have more suggestions to this code.

Here is the cover letter of the RFC sent prior to this patchset:

While reading code of drivers/block, I was curious about the set_capacity
argument, always shifting the value by 9, and so I took me a while to realize
this is done on purpose: the capacity is the number of sectors of 512 bytes
related to the storage space.

Rather the shifting by 9, there are other places where the value if shifted by
SECTOR_SHIFT, which is more readable.
This patch aims to reduce these differences by adding a new function called
bytes_to_sectors, adding a proper comment explaining why this is needed.

null_blk was changed to use this new function.

Thanks,
Marco

Marcos Paulo de Souza (2):
  blkdev.h: Introduce bytes_to_sectors helper function
  null_blk: Make use of bytes_to_sectors helper

 drivers/block/null_blk_main.c | 18 +++++++++---------
 include/linux/blkdev.h        | 17 +++++++++++++++++
 2 files changed, 26 insertions(+), 9 deletions(-)

-- 
2.16.4

[PATCH v2 1/2] blkdev.h: Introduce bytes_to_sectors helper function

[Marcos Paulo de Souza]

This function takes an argument to specify the size of a block device,
in bytes, and return the number of sectors of 512 bytes.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
---
 Changes from v1:
 Rename size_to_sectors to bytes_to_sectors. (Martin K. Petersen)

 Changes from RFC:
 Reworked the documentation of size_to_sectors by removing a sentence that was
 explaining the size -> sectors math, which wasn't necessary given the
 description prior to the example. (suggested by Chaitanya)

 include/linux/blkdev.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 317ab30d2904..7ade2e24dbae 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -871,6 +871,23 @@ static inline struct request_queue *bdev_get_queue(struct block_device *bdev)
 #define SECTOR_SIZE (1 << SECTOR_SHIFT)
 #endif
 
+/**
+ * bytes_to_sectors - Convert size in bytes to number of sectors of 512 bytes
+ * @bytes: number of bytes to be converted to sectors
+ *
+ * Description:
+ * Kernel I/O operations are always made in "sectors". In order to set the
+ * correct number of sectors for a given number of bytes, we need to group the
+ * number of bytes in "sectors of 512 bytes" by shifting the size value by 9,
+ * which is the same than dividing the size by 512.
+ *
+ * Returns the number of sectors by the given number of bytes.
+ */
+static inline sector_t bytes_to_sectors(long long bytes)
+{
+	return bytes >> SECTOR_SHIFT;
+}
+
 /*
  * blk_rq_pos()			: the current sector
  * blk_rq_bytes()		: bytes left in the entire request
-- 
2.16.4

[PATCH v2 2/2] null_blk: Make use of bytes_to_sectors helper

[Marcos Paulo de Souza]

This helper tries to make the code easier to read, and unifies the code
of returning the number of sectors for a given number of bytes.

Reviewed-by : Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>

---
 Changes from v1:
 Rename size_to_sectors to bytes_to_sectors. (Martin K. Petersen)

 drivers/block/null_blk_main.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index d7ac09c092f2..4c48f2a30941 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -853,7 +853,7 @@ static int null_flush_cache_page(struct nullb *nullb, struct nullb_page *c_page)
 	dst = kmap_atomic(t_page->page);
 
 	for (i = 0; i < PAGE_SECTORS;
-			i += (nullb->dev->blocksize >> SECTOR_SHIFT)) {
+			i += (bytes_to_sectors(nullb->dev->blocksize))) {
 		if (test_bit(i, c_page->bitmap)) {
 			offset = (i << SECTOR_SHIFT);
 			memcpy(dst + offset, src + offset,
@@ -957,7 +957,7 @@ static int copy_to_nullb(struct nullb *nullb, struct page *source,
 			null_free_sector(nullb, sector, true);
 
 		count += temp;
-		sector += temp >> SECTOR_SHIFT;
+		sector += bytes_to_sectors(temp);
 	}
 	return 0;
 }
@@ -989,7 +989,7 @@ static int copy_from_nullb(struct nullb *nullb, struct page *dest,
 		kunmap_atomic(dst);
 
 		count += temp;
-		sector += temp >> SECTOR_SHIFT;
+		sector += bytes_to_sectors(temp);
 	}
 	return 0;
 }
@@ -1004,7 +1004,7 @@ static void null_handle_discard(struct nullb *nullb, sector_t sector, size_t n)
 		null_free_sector(nullb, sector, false);
 		if (null_cache_active(nullb))
 			null_free_sector(nullb, sector, true);
-		sector += temp >> SECTOR_SHIFT;
+		sector += bytes_to_sectors(temp);
 		n -= temp;
 	}
 	spin_unlock_irq(&nullb->lock);
@@ -1074,7 +1074,7 @@ static int null_handle_rq(struct nullb_cmd *cmd)
 			spin_unlock_irq(&nullb->lock);
 			return err;
 		}
-		sector += len >> SECTOR_SHIFT;
+		sector += bytes_to_sectors(len);
 	}
 	spin_unlock_irq(&nullb->lock);
 
@@ -1109,7 +1109,7 @@ static int null_handle_bio(struct nullb_cmd *cmd)
 			spin_unlock_irq(&nullb->lock);
 			return err;
 		}
-		sector += len >> SECTOR_SHIFT;
+		sector += bytes_to_sectors(len);
 	}
 	spin_unlock_irq(&nullb->lock);
 	return 0;
@@ -1201,7 +1201,7 @@ static blk_status_t null_handle_cmd(struct nullb_cmd *cmd)
 		if (dev->queue_mode == NULL_Q_BIO) {
 			op = bio_op(cmd->bio);
 			sector = cmd->bio->bi_iter.bi_sector;
-			nr_sectors = cmd->bio->bi_iter.bi_size >> 9;
+			nr_sectors = bytes_to_sectors(cmd->bio->bi_iter.bi_size);
 		} else {
 			op = req_op(cmd->rq);
 			sector = blk_rq_pos(cmd->rq);
@@ -1406,7 +1406,7 @@ static void null_config_discard(struct nullb *nullb)
 		return;
 	nullb->q->limits.discard_granularity = nullb->dev->blocksize;
 	nullb->q->limits.discard_alignment = nullb->dev->blocksize;
-	blk_queue_max_discard_sectors(nullb->q, UINT_MAX >> 9);
+	blk_queue_max_discard_sectors(nullb->q, bytes_to_sectors(UINT_MAX));
 	blk_queue_flag_set(QUEUE_FLAG_DISCARD, nullb->q);
 }
 
@@ -1520,7 +1520,7 @@ static int null_gendisk_register(struct nullb *nullb)
 	if (!disk)
 		return -ENOMEM;
 	size = (sector_t)nullb->dev->size * 1024 * 1024ULL;
-	set_capacity(disk, size >> 9);
+	set_capacity(disk, bytes_to_sectors(size));
 
 	disk->flags |= GENHD_FL_EXT_DEVT | GENHD_FL_SUPPRESS_PARTITION_INFO;
 	disk->major		= null_major;
-- 
2.16.4

Re: [PATCH v2 0/2] Introduce bytes_to_sectors helper in blkdev.h

[Jens Axboe]

On 5/1/19 7:57 PM, Marcos Paulo de Souza wrote:
> Changes from v2:
> Rename size_to_sectors o bytes_to_sectors. (suggested by Martin K. Petersen)
> 
> Changes from v1:
> Reworked the documentation of size_to_sectors by removing a sentence that was
> explaining the size -> sectors math, which wasn't necessary given the
> description prior to the example. (suggested by Chaitanya Kulkarni)
> 
> Let me know if you have more suggestions to this code.
> 
> Here is the cover letter of the RFC sent prior to this patchset:
> 
> While reading code of drivers/block, I was curious about the set_capacity
> argument, always shifting the value by 9, and so I took me a while to realize
> this is done on purpose: the capacity is the number of sectors of 512 bytes
> related to the storage space.
> 
> Rather the shifting by 9, there are other places where the value if shifted by
> SECTOR_SHIFT, which is more readable.
> This patch aims to reduce these differences by adding a new function called
> bytes_to_sectors, adding a proper comment explaining why this is needed.
> 
> null_blk was changed to use this new function.

Maybe I'm alone in this, but I much prefer seeing this:

	sector = bytes >> SECTOR_SHIFT;

to

	sector = bytes_to_sectors(bytes);

The former tells me exactly what it does, the latter I'd need to look
up. I do agree that any hard coding of 9 should be SECTOR_SHIFT,
though.

However, if we are going to do this, the functions would need a blk_
prefix.

-- 
Jens Axboe