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

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

[Marcos Paulo de Souza]

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.

This is an RFC, please let me know if you have other suggestions.

Thanks,
Marcos

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        | 18 ++++++++++++++++++
 2 files changed, 27 insertions(+), 9 deletions(-)

-- 
2.16.4

[RFC 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.

Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
---
 include/linux/blkdev.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 317ab30d2904..7bf7b99161b7 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -871,6 +871,24 @@ 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 operation 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. For example, for a capacity
+ * of 32M (33554432 bytes), the number of sectors will be 65536.
+ *
+ * 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

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

[Chaitanya Kulkarni]

Looks good, with one small comment.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 04/20/2019 08:53 PM, Marcos Paulo de Souza wrote:
> This function takes an argument to specify the size of a block device,
> in bytes, and return the number of sectors of 512 bytes.
>
> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
> ---
>   include/linux/blkdev.h | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 317ab30d2904..7bf7b99161b7 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -871,6 +871,24 @@ 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 operation 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. For example, for a capacity
> + * of 32M (33554432 bytes), the number of sectors will be 65536.
I am not sure we need an example because description prior to the 
example and the code is self explanatory.
> + *
> + * 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
>

[RFC 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.

Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
---
 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: [RFC PATCH 2/2] null_blk: Make use of size_to_sectors helper

[Chaitanya Kulkarni]

Looks good.

Reviewed-by : Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 04/20/2019 08:54 PM, Marcos Paulo de Souza wrote:
> 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.
>
> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
> ---
>   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;
>