[PATCH V2 0/7] block: use right accessor to read nr_setcs

[PATCH V2 0/7] block: use right accessor to read nr_setcs

[Chaitanya Kulkarni]

Hi,

In the blk-zoned, bcache, f2fs and blktrace implementation
block device->hd_part->number of sectors field is accessed directly
without any appropriate locking or accessor function. There is
an existing accessor function present in the in include/linux/genhd.h
which should be used to read the bdev->hd_part->nr_sects.

From ${KERN_DIR}/include/linux/genhd.h:-
<snip>
714 /*
715  * Any access of part->nr_sects which is not protected by partition
716  * bd_mutex or gendisk bdev bd_mutex, should be done using this
717  * accessor function.
718  *
719  * Code written along the lines of i_size_read() and i_size_write().
720  * CONFIG_PREEMPT case optimizes the case of UP kernel with preemption
721  * on.
722  */
723 static inline sector_t part_nr_sects_read(struct hd_struct *part)
724 {
<snip>

This patch series introduces a helper function on the top of the
part_nr_sects_read() and removes the all direct accesses to the
bdev->hd_part->nr_sects for blk-zoned.c.

This series is based on :-

1. Repo :-
   git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git.
2. Branch :- for-next.

Regards,
Chaitanya

Changes from V1:-

1. Drop the target_pscsi patch. (Bart)
2. Remove rcu locking which is not needed. (Bart)

Chaitanya Kulkarni (7):
  block: add a helper function to read nr_setcs
  blk-zoned: update blkdev_nr_zones() with helper
  blk-zoned: update blkdev_report_zone() with helper
  blk-zoned: update blkdev_reset_zones() with helper
  bcache: update cached_dev_init() with helper
  f2fs: use helper in init_blkz_info()
  blktrace: use helper in blk_trace_setup_lba()

 block/blk-zoned.c         | 12 ++++++------
 drivers/md/bcache/super.c |  2 +-
 fs/f2fs/super.c           |  2 +-
 include/linux/blkdev.h    | 10 ++++++++++
 kernel/trace/blktrace.c   |  2 +-
 5 files changed, 19 insertions(+), 9 deletions(-)

-- 
2.19.1

[PATCH V2 1/7] block: add a helper function to read nr_setcs

[Chaitanya Kulkarni]

This patch introduces helper function to read the number of sectors
from struct block_device->bd_part member. For more details Please refer
to the comment in the include/linux/genhd.h for part_nr_sects_read().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 include/linux/blkdev.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 592669bcc536..2ef1de20fd22 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1475,6 +1475,16 @@ static inline void put_dev_sector(Sector p)
 	put_page(p.v);
 }
 
+/* Helper function to read the bdev->bd_part->nr_sects */
+static inline sector_t bdev_nr_sects(struct block_device *bdev)
+{
+	sector_t nr_sects;
+
+	nr_sects = part_nr_sects_read(bdev->bd_part);
+
+	return nr_sects;
+}
+
 int kblockd_schedule_work(struct work_struct *work);
 int kblockd_schedule_work_on(int cpu, struct work_struct *work);
 int kblockd_mod_delayed_work_on(int cpu, struct delayed_work *dwork, unsigned long delay);
-- 
2.19.1

Re: [PATCH V2 1/7] block: add a helper function to read nr_setcs

[Bart Van Assche]

On 6/16/19 6:28 PM, Chaitanya Kulkarni wrote:
> This patch introduces helper function to read the number of sectors
> from struct block_device->bd_part member. For more details Please refer
> to the comment in the include/linux/genhd.h for part_nr_sects_read().
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>   include/linux/blkdev.h | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 592669bcc536..2ef1de20fd22 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1475,6 +1475,16 @@ static inline void put_dev_sector(Sector p)
>   	put_page(p.v);
>   }
>   
> +/* Helper function to read the bdev->bd_part->nr_sects */
> +static inline sector_t bdev_nr_sects(struct block_device *bdev)
> +{
> +	sector_t nr_sects;
> +
> +	nr_sects = part_nr_sects_read(bdev->bd_part);
> +
> +	return nr_sects;
> +}

Although this looks fine to me, is there any reason why the body of that 
function has not been written as follows?

static inline sector_t bdev_nr_sects(struct block_device *bdev)
{
	return part_nr_sects_read(bdev->bd_part);
}

Thanks,

Bart.

Re: [PATCH V2 1/7] block: add a helper function to read nr_setcs

[Chaitanya Kulkarni]

On 06/17/2019 08:47 AM, Bart Van Assche wrote:
> On 6/16/19 6:28 PM, Chaitanya Kulkarni wrote:
>> This patch introduces helper function to read the number of sectors
>> from struct block_device->bd_part member. For more details Please refer
>> to the comment in the include/linux/genhd.h for part_nr_sects_read().
>>
>> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>> ---
>>    include/linux/blkdev.h | 10 ++++++++++
>>    1 file changed, 10 insertions(+)
>>
>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>> index 592669bcc536..2ef1de20fd22 100644
>> --- a/include/linux/blkdev.h
>> +++ b/include/linux/blkdev.h
>> @@ -1475,6 +1475,16 @@ static inline void put_dev_sector(Sector p)
>>    	put_page(p.v);
>>    }
>>
>> +/* Helper function to read the bdev->bd_part->nr_sects */
>> +static inline sector_t bdev_nr_sects(struct block_device *bdev)
>> +{
>> +	sector_t nr_sects;
>> +
>> +	nr_sects = part_nr_sects_read(bdev->bd_part);
>> +
>> +	return nr_sects;
>> +}
>
> Although this looks fine to me, is there any reason why the body of that
> function has not been written as follows?
>
> static inline sector_t bdev_nr_sects(struct block_device *bdev)
> {
> 	return part_nr_sects_read(bdev->bd_part);
> }
This perfectly make sense, thanks for the suggestion, will add it in a 
next version.
>
> Thanks,
>
> Bart.
>

[PATCH V2 2/7] blk-zoned: update blkdev_nr_zones() with helper

[Chaitanya Kulkarni]

This patch updates the blkdev_nr_zones() with newly introduced helper
function to read the nr_sects from block device's hd_parts with the
help if part_nr_sects_read().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 block/blk-zoned.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index ae7e91bd0618..5051db35c3fd 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -90,7 +90,7 @@ unsigned int blkdev_nr_zones(struct block_device *bdev)
 	if (!blk_queue_is_zoned(q))
 		return 0;
 
-	return __blkdev_nr_zones(q, bdev->bd_part->nr_sects);
+	return __blkdev_nr_zones(q, bdev_nr_sects(bdev));
 }
 EXPORT_SYMBOL_GPL(blkdev_nr_zones);
 
-- 
2.19.1

[PATCH V2 3/7] blk-zoned: update blkdev_report_zone() with helper

[Chaitanya Kulkarni]

This patch updates the blkdev_report_zone(s)() with newly introduced
helper function to read the nr_sects from block device's hd_parts with
the help of part_nr_sects_read().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 block/blk-zoned.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 5051db35c3fd..9faf4488339d 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -106,7 +106,7 @@ static bool blkdev_report_zone(struct block_device *bdev, struct blk_zone *rep)
 		return false;
 
 	rep->start -= offset;
-	if (rep->start + rep->len > bdev->bd_part->nr_sects)
+	if (rep->start + rep->len > bdev_nr_sects(bdev))
 		return false;
 
 	if (rep->type = BLK_ZONE_TYPE_CONVENTIONAL)
@@ -176,13 +176,13 @@ int blkdev_report_zones(struct block_device *bdev, sector_t sector,
 	if (WARN_ON_ONCE(!bdev->bd_disk->fops->report_zones))
 		return -EOPNOTSUPP;
 
-	if (!*nr_zones || sector >= bdev->bd_part->nr_sects) {
+	if (!*nr_zones || sector >= bdev_nr_sects(bdev)) {
 		*nr_zones = 0;
 		return 0;
 	}
 
 	nrz = min(*nr_zones,
-		  __blkdev_nr_zones(q, bdev->bd_part->nr_sects - sector));
+		  __blkdev_nr_zones(q, bdev_nr_sects(bdev) - sector));
 	ret = blk_report_zones(bdev->bd_disk, get_start_sect(bdev) + sector,
 			       zones, &nrz, gfp_mask);
 	if (ret)
-- 
2.19.1

[PATCH V2 4/7] blk-zoned: update blkdev_reset_zones() with helper

[Chaitanya Kulkarni]

This patch updates the blkdev_reset_zones() with newly introduced
helper function to read the nr_sects from block device's hd_parts with
the help of part_nr_sects_read().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 block/blk-zoned.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 9faf4488339d..e7f2874b5d37 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -229,7 +229,7 @@ int blkdev_reset_zones(struct block_device *bdev,
 	if (bdev_read_only(bdev))
 		return -EPERM;
 
-	if (!nr_sectors || end_sector > bdev->bd_part->nr_sects)
+	if (!nr_sectors || end_sector > bdev_nr_sects(bdev))
 		/* Out of range */
 		return -EINVAL;
 
@@ -239,7 +239,7 @@ int blkdev_reset_zones(struct block_device *bdev,
 		return -EINVAL;
 
 	if ((nr_sectors & (zone_sectors - 1)) &&
-	    end_sector != bdev->bd_part->nr_sects)
+	    end_sector != bdev_nr_sects(bdev))
 		return -EINVAL;
 
 	blk_start_plug(&plug);
-- 
2.19.1

[PATCH V2 5/7] bcache: update cached_dev_init() with helper

[Chaitanya Kulkarni]

In the bcache when initializing the device we don't actually use any
sort of locking when reading the number of sectors from the part. This
patch updates the cached_dev_init() with newly introduced helper
function to read the nr_sects from block device's hd_parts with the help
of part_nr_sects_read().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/md/bcache/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 1b63ac876169..6a29ba89dae1 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1263,7 +1263,7 @@ static int cached_dev_init(struct cached_dev *dc, unsigned int block_size)
 			q->limits.raid_partial_stripes_expensive;
 
 	ret = bcache_device_init(&dc->disk, block_size,
-			 dc->bdev->bd_part->nr_sects - dc->sb.data_offset);
+			 bdev_nr_sects(dc->bdev) - dc->sb.data_offset);
 	if (ret)
 		return ret;
 
-- 
2.19.1

Re: [PATCH V2 5/7] bcache: update cached_dev_init() with helper

[Coly Li]

On 2019/6/17 9:28 上午, Chaitanya Kulkarni wrote:
> In the bcache when initializing the device we don't actually use any
> sort of locking when reading the number of sectors from the part. This
> patch updates the cached_dev_init() with newly introduced helper
> function to read the nr_sects from block device's hd_parts with the help
> of part_nr_sects_read().
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Acked-by: Coly Li <colyli@suse.de>

Thanks.

Coly Li

> ---
>  drivers/md/bcache/super.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 1b63ac876169..6a29ba89dae1 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -1263,7 +1263,7 @@ static int cached_dev_init(struct cached_dev *dc, unsigned int block_size)
>  			q->limits.raid_partial_stripes_expensive;
>  
>  	ret = bcache_device_init(&dc->disk, block_size,
> -			 dc->bdev->bd_part->nr_sects - dc->sb.data_offset);
> +			 bdev_nr_sects(dc->bdev) - dc->sb.data_offset);
>  	if (ret)
>  		return ret;
>  
> 

Re: [PATCH V2 5/7] bcache: update cached_dev_init() with helper

[Chaitanya Kulkarni]

On 06/16/2019 11:54 PM, Coly Li wrote:
> On 2019/6/17 9:28 上午, Chaitanya Kulkarni wrote:
>> In the bcache when initializing the device we don't actually use any
>> sort of locking when reading the number of sectors from the part. This
>> patch updates the cached_dev_init() with newly introduced helper
>> function to read the nr_sects from block device's hd_parts with the help
>> of part_nr_sects_read().
>>
>> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>
> Acked-by: Coly Li <colyli@suse.de>

Thanks Coly.
>
> Thanks.
>
> Coly Li
>
>> ---
>>   drivers/md/bcache/super.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
>> index 1b63ac876169..6a29ba89dae1 100644
>> --- a/drivers/md/bcache/super.c
>> +++ b/drivers/md/bcache/super.c
>> @@ -1263,7 +1263,7 @@ static int cached_dev_init(struct cached_dev *dc, unsigned int block_size)
>>   			q->limits.raid_partial_stripes_expensive;
>>
>>   	ret = bcache_device_init(&dc->disk, block_size,
>> -			 dc->bdev->bd_part->nr_sects - dc->sb.data_offset);
>> +			 bdev_nr_sects(dc->bdev) - dc->sb.data_offset);
>>   	if (ret)
>>   		return ret;
>>
>>
>

[PATCH V2 6/7] f2fs: use helper in init_blkz_info()

[Chaitanya Kulkarni]

This patch updates the init_blkz_info() with newly introduced helper
function to read the nr_sects from block device's hd_parts with the
help if part_nr_sects_read().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 fs/f2fs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 6b959bbb336a..24e2848afcf5 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2798,7 +2798,7 @@ static int init_percpu_info(struct f2fs_sb_info *sbi)
 static int init_blkz_info(struct f2fs_sb_info *sbi, int devi)
 {
 	struct block_device *bdev = FDEV(devi).bdev;
-	sector_t nr_sectors = bdev->bd_part->nr_sects;
+	sector_t nr_sectors = bdev_nr_sects(bdev);
 	sector_t sector = 0;
 	struct blk_zone *zones;
 	unsigned int i, nr_zones;
-- 
2.19.1

[PATCH V2 7/7] blktrace: use helper in blk_trace_setup_lba()

[Chaitanya Kulkarni]

This patch updates the blk_trace_setup_lba() with newly introduced helper
function to read the nr_sects from block device's hd_parts with the
help if part_nr_sects_read().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 kernel/trace/blktrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index e1c6d79fb4cc..35ff49503b85 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -461,7 +461,7 @@ static void blk_trace_setup_lba(struct blk_trace *bt,
 
 	if (part) {
 		bt->start_lba = part->start_sect;
-		bt->end_lba = part->start_sect + part->nr_sects;
+		bt->end_lba = part->start_sect + bdev_nr_sects(bdev);
 	} else {
 		bt->start_lba = 0;
 		bt->end_lba = -1ULL;
-- 
2.19.1

Re: [PATCH V2 0/7] block: use right accessor to read nr_setcs

[Bart Van Assche]

On 6/16/19 6:28 PM, Chaitanya Kulkarni wrote:
> In the blk-zoned, bcache, f2fs and blktrace implementation
> block device->hd_part->number of sectors field is accessed directly
> without any appropriate locking or accessor function. There is
> an existing accessor function present in the in include/linux/genhd.h
> which should be used to read the bdev->hd_part->nr_sects.

In the subject, "nr_setcs" should probably be changed into "nr_sects"?

Bart.

Re: [PATCH V2 0/7] block: use right accessor to read nr_setcs

[Chaitanya Kulkarni]

On 06/17/2019 08:45 AM, Bart Van Assche wrote:
> On 6/16/19 6:28 PM, Chaitanya Kulkarni wrote:
>> In the blk-zoned, bcache, f2fs and blktrace implementation
>> block device->hd_part->number of sectors field is accessed directly
>> without any appropriate locking or accessor function. There is
>> an existing accessor function present in the in include/linux/genhd.h
>> which should be used to read the bdev->hd_part->nr_sects.
>
> In the subject, "nr_setcs" should probably be changed into "nr_sects"?
>
Yes, will fix it in next version.
> Bart.
>

Re: [PATCH V2 0/7] block: use right accessor to read nr_setcs

[Bart Van Assche]

On 6/16/19 6:28 PM, Chaitanya Kulkarni wrote:
> Changes from V1:-
> 
> 1. Drop the target_pscsi patch. (Bart)
> 2. Remove rcu locking which is not needed. (Bart)
> 
> Chaitanya Kulkarni (7):
>    block: add a helper function to read nr_setcs
>    blk-zoned: update blkdev_nr_zones() with helper
>    blk-zoned: update blkdev_report_zone() with helper
>    blk-zoned: update blkdev_reset_zones() with helper
>    bcache: update cached_dev_init() with helper
>    f2fs: use helper in init_blkz_info()
>    blktrace: use helper in blk_trace_setup_lba()
> 
>   block/blk-zoned.c         | 12 ++++++------
>   drivers/md/bcache/super.c |  2 +-
>   fs/f2fs/super.c           |  2 +-
>   include/linux/blkdev.h    | 10 ++++++++++
>   kernel/trace/blktrace.c   |  2 +-
>   5 files changed, 19 insertions(+), 9 deletions(-)

My feedback about the pscsi_get_blocks() was misleading: what I meant is 
that it is not necessary to introduce RCU locking in that function. I 
think that using bdev_nr_sects() or part_nr_sects_read() to read 
nr_sects in that function is useful.

Is there any reason that the following Xen macro has not been converted?

#define vbd_sz(_v)	((_v)->bdev->bd_part ? \
			 (_v)->bdev->bd_part->nr_sects : \
			  get_capacity((_v)->bdev->bd_disk))

Thanks,

Bart.

Re: [PATCH V2 0/7] block: use right accessor to read nr_setcs

[Chaitanya Kulkarni]

On 06/17/2019 08:53 AM, Bart Van Assche wrote:
> On 6/16/19 6:28 PM, Chaitanya Kulkarni wrote:
>> Changes from V1:-
>>
>> 1. Drop the target_pscsi patch. (Bart)
>> 2. Remove rcu locking which is not needed. (Bart)
>>
>> Chaitanya Kulkarni (7):
>>     block: add a helper function to read nr_setcs
>>     blk-zoned: update blkdev_nr_zones() with helper
>>     blk-zoned: update blkdev_report_zone() with helper
>>     blk-zoned: update blkdev_reset_zones() with helper
>>     bcache: update cached_dev_init() with helper
>>     f2fs: use helper in init_blkz_info()
>>     blktrace: use helper in blk_trace_setup_lba()
>>
>>    block/blk-zoned.c         | 12 ++++++------
>>    drivers/md/bcache/super.c |  2 +-
>>    fs/f2fs/super.c           |  2 +-
>>    include/linux/blkdev.h    | 10 ++++++++++
>>    kernel/trace/blktrace.c   |  2 +-
>>    5 files changed, 19 insertions(+), 9 deletions(-)
>
> My feedback about the pscsi_get_blocks() was misleading: what I meant is
> that it is not necessary to introduce RCU locking in that function. I
> think that using bdev_nr_sects() or part_nr_sects_read() to read
> nr_sects in that function is useful.
>
My bad. I'll add pscsi patch.
> Is there any reason that the following Xen macro has not been converted?
>
> #define vbd_sz(_v)	((_v)->bdev->bd_part ? \
> 			 (_v)->bdev->bd_part->nr_sects : \
> 			  get_capacity((_v)->bdev->bd_disk))
>
I'll convert this too and appropriate mailing list.

Thanks Bart again for detailed feedback.
> Thanks,
>
> Bart.
>