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

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

[Marcos Paulo de Souza]

My previous submission was an RFC, but I got a reviewed-by from Chaitanya, so
I removed the RFc tag.

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.

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
size_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 size_to_sectors hlper function
  null_blk: Make use of size_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 1/2] blkdev.h: Introduce size_to_sectors hlper 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:
 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..f6cfe6970756 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
 
+/**
+ * size_to_sectors - Convert size in bytes to number of sectors of 512 bytes
+ * @size: size in 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 size_to_sectors(long long size)
+{
+	return size >> SECTOR_SHIFT;
+}
+
 /*
  * blk_rq_pos()			: the current sector
  * blk_rq_bytes()		: bytes left in the entire request
-- 
2.16.4

[PATCH 2/2] null_blk: Make use of size_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>
---
 No changes from v1.

 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..05f0bef54296 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 += (size_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 += size_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 += size_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 += size_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 += size_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 += size_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 = size_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, size_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, size_to_sectors(size));
 
 	disk->flags |= GENHD_FL_EXT_DEVT | GENHD_FL_SUPPRESS_PARTITION_INFO;
 	disk->major		= null_major;
-- 
2.16.4

Re: [PATCH 1/2] blkdev.h: Introduce size_to_sectors hlper function

[Martin K. Petersen]

Hi Marco,

> +static inline sector_t size_to_sectors(long long size)
> +{
> +	return size >> SECTOR_SHIFT;
> +}
> +

FWIW, in SCSI we have:

	logical_to_sectors()
        logical_to_bytes()
        bytes_to_logical()
        sectors_to_logical()

I'm not attached to "bytes" in any way but it would be nice to be
consistent.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 1/2] blkdev.h: Introduce size_to_sectors hlper function

[Marcos Paulo de Souza]

On Mon, Apr 29, 2019 at 10:50:32PM -0400, Martin K. Petersen wrote:
> 
> Hi Marco,
> 
> > +static inline sector_t size_to_sectors(long long size)
> > +{
> > +	return size >> SECTOR_SHIFT;
> > +}
> > +
> 
> FWIW, in SCSI we have:
> 
> 	logical_to_sectors()
>         logical_to_bytes()
>         bytes_to_logical()
>         sectors_to_logical()
> 
> I'm not attached to "bytes" in any way but it would be nice to be
> consistent.
> 

Thanks for the suggestion. I will send a new version using "bytes_to_sectors"
instead.

> -- 
> Martin K. Petersen	Oracle Linux Engineering

-- 
Thanks,
Marcos

RE: [PATCH 1/2] blkdev.h: Introduce size_to_sectors hlper function

[Elliott, Robert (Servers)]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of
> Marcos Paulo de Souza
> Sent: Monday, April 29, 2019 8:32 PM
> Subject: [PATCH 1/2] blkdev.h: Introduce size_to_sectors hlper function
> 
> This function takes an argument to specify the size of a block device,
> in bytes, and return the number of sectors of 512 bytes.
> 
...
> +static inline sector_t size_to_sectors(long long size)
> +{
> +	return size >> SECTOR_SHIFT;
> +}

At least one of the users in PATCH 2/2 passes an unsigned value that won't
fit in a signed argument:

-	blk_queue_max_discard_sectors(nullb->q, UINT_MAX >> 9);
+	blk_queue_max_discard_sectors(nullb->q, size_to_sectors(UINT_MAX));