[PATCH] null_blk: remove duplicate check for report zone

[PATCH] null_blk: remove duplicate check for report zone

[Chaitanya Kulkarni]

This patch removes the check in the null_blk_zoned for report zone
command, where it checks for the dev-,>zoned before executing the report
zone.

The null_zone_report() function is a block_device operation callback
which is initialized in the null_blk_main.c and gets called as a part
of blkdev for report zone IOCTL (BLKREPORTZONE).

blkdev_ioctl()
blkdev_report_zones_ioctl()
        blkdev_report_zones()
                blk_report_zones()
                        disk->fops->report_zones()
                                nullb_zone_report();

The null_zone_report() will never get executed on the non-zoned block
device, in the non zoned block device blk_queue_is_zoned() will always
be false which is first check the blkdev_report_zones_ioctl()
before actual low level driver report zone callback is executed.

Here is the detailed scenario:-

1. modprobe null_blk
null_init
null_alloc_dev
        dev->zoned = 0 
null_add_dev
        dev->zoned == 0
                so we don't set the q->limits.zoned = BLK_ZONED_HR

2. blkzone report /dev/nullb0

blkdev_ioctl()
blkdev_report_zones_ioctl()
        blk_queue_is_zoned()
                blk_queue_is_zoned
                        q->limits.zoned == 0
                        return false
        if (!blk_queue_is_zoned(q)) <--- true
                return -ENOTTY;              
Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/null_blk_zoned.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/block/null_blk_zoned.c b/drivers/block/null_blk_zoned.c
index 5d1c261a2cfd..fca0c97ff1aa 100644
--- a/drivers/block/null_blk_zoned.c
+++ b/drivers/block/null_blk_zoned.c
@@ -74,10 +74,6 @@ int null_zone_report(struct gendisk *disk, sector_t sector,
 	struct nullb_device *dev = nullb->dev;
 	unsigned int zno, nrz = 0;
 
-	if (!dev->zoned)
-		/* Not a zoned null device */
-		return -EOPNOTSUPP;
-
 	zno = null_zone_no(dev, sector);
 	if (zno < dev->nr_zones) {
 		nrz = min_t(unsigned int, *nr_zones, dev->nr_zones - zno);
-- 
2.19.1

Re: [PATCH] null_blk: remove duplicate check for report zone

[Damien Le Moal]

On 2019/06/12 7:10, Chaitanya Kulkarni wrote:
> This patch removes the check in the null_blk_zoned for report zone
> command, where it checks for the dev-,>zoned before executing the report
> zone.
> 
> The null_zone_report() function is a block_device operation callback
> which is initialized in the null_blk_main.c and gets called as a part
> of blkdev for report zone IOCTL (BLKREPORTZONE).
> 
> blkdev_ioctl()
> blkdev_report_zones_ioctl()
>         blkdev_report_zones()
>                 blk_report_zones()
>                         disk->fops->report_zones()
>                                 nullb_zone_report();
> 
> The null_zone_report() will never get executed on the non-zoned block
> device, in the non zoned block device blk_queue_is_zoned() will always
> be false which is first check the blkdev_report_zones_ioctl()
> before actual low level driver report zone callback is executed.
> 
> Here is the detailed scenario:-
> 
> 1. modprobe null_blk
> null_init
> null_alloc_dev
>         dev->zoned = 0 
> null_add_dev
>         dev->zoned == 0
>                 so we don't set the q->limits.zoned = BLK_ZONED_HR
> 
> 2. blkzone report /dev/nullb0
> 
> blkdev_ioctl()
> blkdev_report_zones_ioctl()
>         blk_queue_is_zoned()
>                 blk_queue_is_zoned
>                         q->limits.zoned == 0
>                         return false
>         if (!blk_queue_is_zoned(q)) <--- true
>                 return -ENOTTY;              
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Looks good. The SCSI layer has a similar check too. But since there are scsi
layer internal execution of the report zones driver function (e.g. device scan),
we need to keep that check there. But I think it is safe to remove in nullblk
since revalidate is the only pass other than the user ioctl that would call
report zones.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>


> ---
>  drivers/block/null_blk_zoned.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/block/null_blk_zoned.c b/drivers/block/null_blk_zoned.c
> index 5d1c261a2cfd..fca0c97ff1aa 100644
> --- a/drivers/block/null_blk_zoned.c
> +++ b/drivers/block/null_blk_zoned.c
> @@ -74,10 +74,6 @@ int null_zone_report(struct gendisk *disk, sector_t sector,
>  	struct nullb_device *dev = nullb->dev;
>  	unsigned int zno, nrz = 0;
>  
> -	if (!dev->zoned)
> -		/* Not a zoned null device */
> -		return -EOPNOTSUPP;
> -
>  	zno = null_zone_no(dev, sector);
>  	if (zno < dev->nr_zones) {
>  		nrz = min_t(unsigned int, *nr_zones, dev->nr_zones - zno);
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] null_blk: remove duplicate check for report zone

[Bob Liu]

On 6/12/19 6:10 AM, Chaitanya Kulkarni wrote:
> This patch removes the check in the null_blk_zoned for report zone
> command, where it checks for the dev-,>zoned before executing the report
> zone.
> 
> The null_zone_report() function is a block_device operation callback
> which is initialized in the null_blk_main.c and gets called as a part
> of blkdev for report zone IOCTL (BLKREPORTZONE).
> 
> blkdev_ioctl()
> blkdev_report_zones_ioctl()
>         blkdev_report_zones()
>                 blk_report_zones()
>                         disk->fops->report_zones()
>                                 nullb_zone_report();
> 
> The null_zone_report() will never get executed on the non-zoned block
> device, in the non zoned block device blk_queue_is_zoned() will always
> be false which is first check the blkdev_report_zones_ioctl()
> before actual low level driver report zone callback is executed.
> 
> Here is the detailed scenario:-
> 
> 1. modprobe null_blk
> null_init
> null_alloc_dev
>         dev->zoned = 0 
> null_add_dev
>         dev->zoned == 0
>                 so we don't set the q->limits.zoned = BLK_ZONED_HR
> 
> 2. blkzone report /dev/nullb0
> 
> blkdev_ioctl()
> blkdev_report_zones_ioctl()
>         blk_queue_is_zoned()
>                 blk_queue_is_zoned
>                         q->limits.zoned == 0
>                         return false
>         if (!blk_queue_is_zoned(q)) <--- true
>                 return -ENOTTY;              
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Looks good to me.
Reviewed-by: Bob Liu <bob.liu@oracle.com>

> ---
>  drivers/block/null_blk_zoned.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/block/null_blk_zoned.c b/drivers/block/null_blk_zoned.c
> index 5d1c261a2cfd..fca0c97ff1aa 100644
> --- a/drivers/block/null_blk_zoned.c
> +++ b/drivers/block/null_blk_zoned.c
> @@ -74,10 +74,6 @@ int null_zone_report(struct gendisk *disk, sector_t sector,
>  	struct nullb_device *dev = nullb->dev;
>  	unsigned int zno, nrz = 0;
>  
> -	if (!dev->zoned)
> -		/* Not a zoned null device */
> -		return -EOPNOTSUPP;
> -
>  	zno = null_zone_no(dev, sector);
>  	if (zno < dev->nr_zones) {
>  		nrz = min_t(unsigned int, *nr_zones, dev->nr_zones - zno);
> 

Re: [PATCH] null_blk: remove duplicate check for report zone

[Jens Axboe]

On 6/11/19 4:10 PM, Chaitanya Kulkarni wrote:
> This patch removes the check in the null_blk_zoned for report zone
> command, where it checks for the dev-,>zoned before executing the report
> zone.
> 
> The null_zone_report() function is a block_device operation callback
> which is initialized in the null_blk_main.c and gets called as a part
> of blkdev for report zone IOCTL (BLKREPORTZONE).
> 
> blkdev_ioctl()
> blkdev_report_zones_ioctl()
>          blkdev_report_zones()
>                  blk_report_zones()
>                          disk->fops->report_zones()
>                                  nullb_zone_report();
> 
> The null_zone_report() will never get executed on the non-zoned block
> device, in the non zoned block device blk_queue_is_zoned() will always
> be false which is first check the blkdev_report_zones_ioctl()
> before actual low level driver report zone callback is executed.
> 
> Here is the detailed scenario:-
> 
> 1. modprobe null_blk
> null_init
> null_alloc_dev
>          dev->zoned = 0
> null_add_dev
>          dev->zoned == 0
>                  so we don't set the q->limits.zoned = BLK_ZONED_HR
> 
> 2. blkzone report /dev/nullb0
> 
> blkdev_ioctl()
> blkdev_report_zones_ioctl()
>          blk_queue_is_zoned()
>                  blk_queue_is_zoned
>                          q->limits.zoned == 0
>                          return false
>          if (!blk_queue_is_zoned(q)) <--- true
>                  return -ENOTTY;

Applied, thanks.

-- 
Jens Axboe