Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] block: change REQ_OP_ZONE_RESET and REQ_OP_ZONE_RESET_ALL to be odd numbers
@ 2020-05-22 13:23 Coly Li
  2020-05-25  1:27 ` Damien Le Moal
  2020-05-25  4:01 ` Chaitanya Kulkarni
  0 siblings, 2 replies; 3+ messages in thread
From: Coly Li @ 2020-05-22 13:23 UTC (permalink / raw)
  To: linux-block, damien.lemoal, hare, hch, axboe
  Cc: linux-bcache, Coly Li, Chaitanya Kulkarni, Damien Le Moal,
	Hannes Reinecke, Jens Axboe, Johannes Thumshirn, Keith Busch,
	Shaun Tancheff

Currently REQ_OP_ZONE_RESET and REQ_OP_ZONE_RESET_ALL are defined as
even numbers 6 and 8, such zone reset bios are treated as READ bios by
bio_data_dir(), which is obviously misleading.

The macro bio_data_dir() is defined in include/linux/bio.h as,
 55 #define bio_data_dir(bio) \
 56         (op_is_write(bio_op(bio)) ? WRITE : READ)

And op_is_write() is defined in include/linux/blk_types.h as,
397 static inline bool op_is_write(unsigned int op)
398 {
399         return (op & 1);
400 }

The convention of op_is_write() is when there is data transfer then the
op code should be odd number, and treat as a write op. bio_data_dir()
treats all bio direction as READ if op_is_write() reports false, and
WRITE if op_is_write() reports true.

Because REQ_OP_ZONE_RESET and REQ_OP_ZONE_RESET_ALL are even numbers,
although they don't transfer data but reporting them as READ bio by
bio_data_dir() is misleading and might be wrong. Because these two
commands will reset the writer pointers of the resetting zones, and all
content after the reset write pointer will be invalid and unaccessible,
obviously they are not READ bios in any means.

This patch changes REQ_OP_ZONE_RESET from 6 to 15, and changes
REQ_OP_ZONE_RESET_ALL from 8 to 17. Now bios with these two op code
can be treated as WRITE by bio_data_dir(). Although they don't transfer
data, now we keep them consistent with REQ_OP_DISCARD and
REQ_OP_WRITE_ZEROES with the ituition that they change on-media content
and should be WRITE request.

Signed-off-by: Coly Li <colyli@suse.de>
Cc: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Damien Le Moal <Damien.LeMoal@wdc.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Jens Axboe <axboe@fb.com>
Cc: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Cc: Keith Busch <kbusch@kernel.org>
Cc: Shaun Tancheff <shaun.tancheff@seagate.com>
---
 include/linux/blk_types.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index ccb895f911b1..447b46a0accf 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -300,12 +300,8 @@ enum req_opf {
 	REQ_OP_DISCARD		= 3,
 	/* securely erase sectors */
 	REQ_OP_SECURE_ERASE	= 5,
-	/* reset a zone write pointer */
-	REQ_OP_ZONE_RESET	= 6,
 	/* write the same sector many times */
 	REQ_OP_WRITE_SAME	= 7,
-	/* reset all the zone present on the device */
-	REQ_OP_ZONE_RESET_ALL	= 8,
 	/* write the zero filled sector many times */
 	REQ_OP_WRITE_ZEROES	= 9,
 	/* Open a zone */
@@ -316,6 +312,10 @@ enum req_opf {
 	REQ_OP_ZONE_FINISH	= 12,
 	/* write data at the current zone write pointer */
 	REQ_OP_ZONE_APPEND	= 13,
+	/* reset a zone write pointer */
+	REQ_OP_ZONE_RESET	= 15,
+	/* reset all the zone present on the device */
+	REQ_OP_ZONE_RESET_ALL	= 17,
 
 	/* SCSI passthrough using struct scsi_request */
 	REQ_OP_SCSI_IN		= 32,
-- 
2.25.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] block: change REQ_OP_ZONE_RESET and REQ_OP_ZONE_RESET_ALL to be odd numbers
  2020-05-22 13:23 [PATCH] block: change REQ_OP_ZONE_RESET and REQ_OP_ZONE_RESET_ALL to be odd numbers Coly Li
@ 2020-05-25  1:27 ` Damien Le Moal
  2020-05-25  4:01 ` Chaitanya Kulkarni
  1 sibling, 0 replies; 3+ messages in thread
From: Damien Le Moal @ 2020-05-25  1:27 UTC (permalink / raw)
  To: Coly Li, linux-block, hare, hch, axboe
  Cc: linux-bcache, Chaitanya Kulkarni, Hannes Reinecke, Jens Axboe,
	Johannes Thumshirn, Keith Busch, Shaun Tancheff

On 2020/05/22 22:25, Coly Li wrote:
> Currently REQ_OP_ZONE_RESET and REQ_OP_ZONE_RESET_ALL are defined as
> even numbers 6 and 8, such zone reset bios are treated as READ bios by
> bio_data_dir(), which is obviously misleading.
> 
> The macro bio_data_dir() is defined in include/linux/bio.h as,
>  55 #define bio_data_dir(bio) \
>  56         (op_is_write(bio_op(bio)) ? WRITE : READ)
> 
> And op_is_write() is defined in include/linux/blk_types.h as,
> 397 static inline bool op_is_write(unsigned int op)
> 398 {
> 399         return (op & 1);
> 400 }
> 
> The convention of op_is_write() is when there is data transfer then the
> op code should be odd number, and treat as a write op. bio_data_dir()
> treats all bio direction as READ if op_is_write() reports false, and
> WRITE if op_is_write() reports true.
> 
> Because REQ_OP_ZONE_RESET and REQ_OP_ZONE_RESET_ALL are even numbers,
> although they don't transfer data but reporting them as READ bio by
> bio_data_dir() is misleading and might be wrong. Because these two
> commands will reset the writer pointers of the resetting zones, and all
> content after the reset write pointer will be invalid and unaccessible,
> obviously they are not READ bios in any means.
> 
> This patch changes REQ_OP_ZONE_RESET from 6 to 15, and changes
> REQ_OP_ZONE_RESET_ALL from 8 to 17. Now bios with these two op code
> can be treated as WRITE by bio_data_dir(). Although they don't transfer
> data, now we keep them consistent with REQ_OP_DISCARD and
> REQ_OP_WRITE_ZEROES with the ituition that they change on-media content
> and should be WRITE request.
> 
> Signed-off-by: Coly Li <colyli@suse.de>
> Cc: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Damien Le Moal <Damien.LeMoal@wdc.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Jens Axboe <axboe@fb.com>
> Cc: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Cc: Keith Busch <kbusch@kernel.org>
> Cc: Shaun Tancheff <shaun.tancheff@seagate.com>
> ---
>  include/linux/blk_types.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index ccb895f911b1..447b46a0accf 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -300,12 +300,8 @@ enum req_opf {
>  	REQ_OP_DISCARD		= 3,
>  	/* securely erase sectors */
>  	REQ_OP_SECURE_ERASE	= 5,
> -	/* reset a zone write pointer */
> -	REQ_OP_ZONE_RESET	= 6,
>  	/* write the same sector many times */
>  	REQ_OP_WRITE_SAME	= 7,
> -	/* reset all the zone present on the device */
> -	REQ_OP_ZONE_RESET_ALL	= 8,
>  	/* write the zero filled sector many times */
>  	REQ_OP_WRITE_ZEROES	= 9,
>  	/* Open a zone */
> @@ -316,6 +312,10 @@ enum req_opf {
>  	REQ_OP_ZONE_FINISH	= 12,
>  	/* write data at the current zone write pointer */
>  	REQ_OP_ZONE_APPEND	= 13,
> +	/* reset a zone write pointer */
> +	REQ_OP_ZONE_RESET	= 15,
> +	/* reset all the zone present on the device */
> +	REQ_OP_ZONE_RESET_ALL	= 17,
>  
>  	/* SCSI passthrough using struct scsi_request */
>  	REQ_OP_SCSI_IN		= 32,
> 

I think this is OK. Not sure if it needs to be a separate patch or if it should
be part of the bcache series... Jens ?

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


-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] block: change REQ_OP_ZONE_RESET and REQ_OP_ZONE_RESET_ALL to be odd numbers
  2020-05-22 13:23 [PATCH] block: change REQ_OP_ZONE_RESET and REQ_OP_ZONE_RESET_ALL to be odd numbers Coly Li
  2020-05-25  1:27 ` Damien Le Moal
@ 2020-05-25  4:01 ` Chaitanya Kulkarni
  1 sibling, 0 replies; 3+ messages in thread
From: Chaitanya Kulkarni @ 2020-05-25  4:01 UTC (permalink / raw)
  To: Coly Li, linux-block, Damien Le Moal, hare, hch, axboe
  Cc: linux-bcache, Hannes Reinecke, Jens Axboe, Johannes Thumshirn,
	Keith Busch, Shaun Tancheff

On 5/22/20 6:25 AM, Coly Li wrote:
>   	/* write data at the current zone write pointer */
>   	REQ_OP_ZONE_APPEND	= 13,
> +	/* reset a zone write pointer */
> +	REQ_OP_ZONE_RESET	= 15,
> +	/* reset all the zone present on the device */
> +	REQ_OP_ZONE_RESET_ALL	= 17,
>   
>   	/* SCSI passthrough using struct scsi_request */
>   	REQ_OP_SCSI_IN		= 32,
> 

Looks good to me.

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-22 13:23 [PATCH] block: change REQ_OP_ZONE_RESET and REQ_OP_ZONE_RESET_ALL to be odd numbers Coly Li
2020-05-25  1:27 ` Damien Le Moal
2020-05-25  4:01 ` Chaitanya Kulkarni

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org
	public-inbox-index linux-block

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git