Re: [PATCH 2/4] blk-zoned: implement REQ_OP_ZONE_RESET_ALL

[Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>]

From: Damien Le Moal <Damien.LeMoal@wdc.com>

Sent: Wednesday, July 31, 2019 5:55 PM

To: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>; linux-block@vger.kernel.org <linux-block@vger.kernel.org>; linux-scsi@vger.kernel.org <linux-scsi@vger.kernel.org>

Cc: axboe@kernel.dk <axboe@kernel.dk>; jejb@linux.ibm.com <jejb@linux.ibm.com>; dennis@kernel.org <dennis@kernel.org>; hare@suse.com <hare@suse.com>; sagi@grimberg.me <sagi@grimberg.me>; dennisszhou@gmail.com <dennisszhou@gmail.com>; jthumshirn@suse.de
 <jthumshirn@suse.de>; osandov@fb.com <osandov@fb.com>; ming.lei@redhat.com <ming.lei@redhat.com>; tj@kernel.org <tj@kernel.org>; bvanassche@acm.org <bvanassche@acm.org>; martin.petersen@oracle.com <martin.petersen@oracle.com>

Subject: Re: [PATCH 2/4] blk-zoned: implement REQ_OP_ZONE_RESET_ALL

 


On 2019/08/01 6:01, Chaitanya Kulkarni wrote:

> This implements REQ_OP_ZONE_RESET_ALL as a special case of the block

> device zone reset operations where we just simply issue bio with the

> newly introduced req op.

> 

> We issue this req op when the number of sectors is equal to the device's

> partition's number of sectors and device has no partitions.

> 

> We also add support so that blk_op_str() can print the new reset-all

> zone operation.

> 

> This patch also adds a generic make request check for newly

> introduced REQ_OP_ZONE_RESET_ALL req_opf. We simply return error

> when queue is zoned and reset-all flag is not set for

> REQ_OP_ZONE_RESET_ALL.

> 

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

> ---

>  block/blk-core.c  |  5 +++++

>  block/blk-zoned.c | 40 ++++++++++++++++++++++++++++++++++++++++

>  2 files changed, 45 insertions(+)

> 

> diff --git a/block/blk-core.c b/block/blk-core.c

> index d0cc6e14d2f0..1b53ab56228b 100644

> --- a/block/blk-core.c

> +++ b/block/blk-core.c

> @@ -129,6 +129,7 @@ static const char *const blk_op_name[] = {

>        REQ_OP_NAME(DISCARD),

>        REQ_OP_NAME(SECURE_ERASE),

>        REQ_OP_NAME(ZONE_RESET),

> +     REQ_OP_NAME(ZONE_RESET_ALL),

>        REQ_OP_NAME(WRITE_SAME),

>        REQ_OP_NAME(WRITE_ZEROES),

>        REQ_OP_NAME(SCSI_IN),

> @@ -931,6 +932,10 @@ generic_make_request_checks(struct bio *bio)

>                if (!blk_queue_is_zoned(q))

>                        goto not_supported;

>                break;

> +     case REQ_OP_ZONE_RESET_ALL:

> +             if (!blk_queue_is_zoned(q) || !blk_queue_zone_resetall(q))

> +                     goto not_supported;

> +             break;

>        case REQ_OP_WRITE_ZEROES:

>                if (!q->limits.max_write_zeroes_sectors)

>                        goto not_supported;

> diff --git a/block/blk-zoned.c b/block/blk-zoned.c

> index 6c503824ba3f..d1ed728b7464 100644

> --- a/block/blk-zoned.c

> +++ b/block/blk-zoned.c

> @@ -202,6 +202,43 @@ int blkdev_report_zones(struct block_device *bdev, sector_t sector,

>  }

>  EXPORT_SYMBOL_GPL(blkdev_report_zones);

>  

> +/*

> + * Special case of zone reset operation to reset all zones in one command,

> + * useful for applications like mkfs.

> + */

> +static int __blkdev_reset_all_zones(struct block_device *bdev, gfp_t gfp_mask)

> +{

> +     struct bio *bio = NULL;



There is no need to initialize the bio to NULL here.

[CK] Would you prefer something like following that declares and allocate
in one line which is similar in blk-lib.c:-blk_next_bio() function ? or we should keep
declaration and allocation on the different line :-
/*
 * Special case of zone reset operation to reset all zones in one command,
 * useful for applications like mkfs.
 */
static int __blkdev_reset_all_zones(struct block_device *bdev, gfp_t gfp_mask)
{
        struct bio *bio = bio_alloc(gfp_mask, 0); 
        int ret;                                                                                                                                           

        /* across the zones operations, don't need any sectors */
        bio_set_dev(bio, bdev);
        bio_set_op_attrs(bio, REQ_OP_ZONE_RESET_ALL, 0); 

        ret = submit_bio_wait(bio);
        bio_put(bio);

        return ret;
}



> +     int ret;

> +

> +     /* across the zones operations, don't need any sectors */

> +     bio = bio_alloc(gfp_mask, 0);

> +     bio_set_dev(bio, bdev);

> +     bio_set_op_attrs(bio, REQ_OP_ZONE_RESET_ALL, 0);

> +

> +     ret = submit_bio_wait(bio);

> +     bio_put(bio);

> +

> +     return ret;

> +}

> +

> +static inline bool blkdev_allow_reset_all_zones(struct block_device *bdev,

> +                                             sector_t nr_sectors)

> +{

> +     if (!blk_queue_zone_resetall(bdev_get_queue(bdev)))

> +             return false;

> +

> +     if (nr_sectors != part_nr_sects_read(bdev->bd_part))

> +             return false;

> +     /*

> +      * REQ_OP_ZONE_RESET_ALL can be executed only if the block device is

> +      * the entire disk, that is, if the blocks device start offset is 0 and

> +      * its capacity is the same as the entire disk.

> +      */

> +     return get_start_sect(bdev) == 0 &&

> +            part_nr_sects_read(bdev->bd_part) == get_capacity(bdev->bd_disk);

> +}

> +

>  /**

>   * blkdev_reset_zones - Reset zones write pointer

>   * @bdev:    Target block device

> @@ -235,6 +272,9 @@ int blkdev_reset_zones(struct block_device *bdev,

>                /* Out of range */

>                return -EINVAL;

>  

> +     if (blkdev_allow_reset_all_zones(bdev, nr_sectors))

> +             return  __blkdev_reset_all_zones(bdev, gfp_mask);

> +

>        /* Check alignment (handle eventual smaller last zone) */

>        zone_sectors = blk_queue_zone_sectors(q);

>        if (sector & (zone_sectors - 1))

> 





-- 

Damien Le Moal

Western Digital Research