From: Chaitanya Kulkarni <chaitanyak@nvidia.com> To: Damien Le Moal <damien.lemoal@opensource.wdc.com> Cc: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>, "linux-raid@vger.kernel.org" <linux-raid@vger.kernel.org>, "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>, "target-devel@vger.kernel.org" <target-devel@vger.kernel.org>, "linux-xfs@vger.kernel.org" <linux-xfs@vger.kernel.org>, "dm-devel@redhat.com" <dm-devel@redhat.com>, "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>, "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org> Subject: Re: [RFC PATCH 2/8] scsi: add REQ_OP_VERIFY support Date: Thu, 11 Nov 2021 08:07:39 +0000 [thread overview] Message-ID: <cb9cac6d-1206-0bd3-0883-dc7f1a9fcc78@nvidia.com> (raw) In-Reply-To: <bd36ee58-8273-cd0a-295e-0c66b0142bcd@opensource.wdc.com> On 11/4/2021 5:33 AM, Damien Le Moal wrote: > External email: Use caution opening links or attachments > > > On 2021/11/04 15:46, Chaitanya Kulkarni wrote: >> From: Chaitanya Kulkarni <kch@nvidia.com> >> >> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> >> --- >> drivers/scsi/sd.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++ >> drivers/scsi/sd.h | 1 + >> 2 files changed, 53 insertions(+) >> >> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c >> index a3d2d4bc4a3d..7f2c4eb98cf8 100644 >> --- a/drivers/scsi/sd.c >> +++ b/drivers/scsi/sd.c >> @@ -106,6 +106,7 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_ZBC); >> >> static void sd_config_discard(struct scsi_disk *, unsigned int); >> static void sd_config_write_same(struct scsi_disk *); >> +static void sd_config_verify(struct scsi_disk *sdkp); >> static int sd_revalidate_disk(struct gendisk *); >> static void sd_unlock_native_capacity(struct gendisk *disk); >> static int sd_probe(struct device *); >> @@ -995,6 +996,41 @@ static blk_status_t sd_setup_write_zeroes_cmnd(struct scsi_cmnd *cmd) >> return sd_setup_write_same10_cmnd(cmd, false); >> } >> >> +static void sd_config_verify(struct scsi_disk *sdkp) >> +{ >> + struct request_queue *q = sdkp->disk->queue; >> + >> + /* XXX: use same pattern as sd_config_write_same(). */ >> + blk_queue_max_verify_sectors(q, UINT_MAX >> 9); > > VERIFY 10, 12, 16 and 32 commands are optional and may not be implemented by a > device. So setting this unconditionally is wrong. > At the very least you must have an "if (sdkp->verify_16)" here, and call > "blk_queue_max_verify_sectors(q, 0);" if the device does not support verify. > Yes, I put it together for the RFC, this needs to consider the device unsupported case just like what we do for write zeroes and emulate the same. >> +} >> + >> +static blk_status_t sd_setup_verify_cmnd(struct scsi_cmnd *cmd) >> +{ >> + struct request *rq = cmd->request; >> + struct scsi_device *sdp = cmd->device; >> + struct scsi_disk *sdkp = scsi_disk(rq->rq_disk); >> + u64 lba = sectors_to_logical(sdp, blk_rq_pos(rq)); >> + u32 nr_blocks = sectors_to_logical(sdp, blk_rq_sectors(rq)); >> + >> + if (!sdkp->verify_16) >> + return BLK_STS_NOTSUPP; > > I think this should be "return BLK_STS_TARGET;" > >> + >> + cmd->cmd_len = 16; >> + cmd->cmnd[0] = VERIFY_16; > > And what if the device supports VERIFY 10 or 12 but not VERIFY 16 ? For first implementation we can only VERIFY 16, later we can add cases for VERIFY 10-12 versions. > >> + /* skip veprotect / dpo / bytchk */ >> + cmd->cmnd[1] = 0; >> + put_unaligned_be64(lba, &cmd->cmnd[2]); >> + put_unaligned_be32(nr_blocks, &cmd->cmnd[10]); >> + cmd->cmnd[14] = 0; >> + cmd->cmnd[15] = 0; >> + >> + cmd->allowed = SD_MAX_RETRIES; >> + cmd->sc_data_direction = DMA_NONE; >> + cmd->transfersize = 0; >> + >> + return BLK_STS_OK; >> +} >> + >> static void sd_config_write_same(struct scsi_disk *sdkp) >> { >> struct request_queue *q = sdkp->disk->queue; >> @@ -1345,6 +1381,8 @@ static blk_status_t sd_init_command(struct scsi_cmnd *cmd) >> } >> case REQ_OP_WRITE_ZEROES: >> return sd_setup_write_zeroes_cmnd(cmd); >> + case REQ_OP_VERIFY: >> + return sd_setup_verify_cmnd(cmd); >> case REQ_OP_WRITE_SAME: >> return sd_setup_write_same_cmnd(cmd); >> case REQ_OP_FLUSH: >> @@ -2029,6 +2067,7 @@ static int sd_done(struct scsi_cmnd *SCpnt) >> switch (req_op(req)) { >> case REQ_OP_DISCARD: >> case REQ_OP_WRITE_ZEROES: >> + case REQ_OP_VERIFY: >> case REQ_OP_WRITE_SAME: >> case REQ_OP_ZONE_RESET: >> case REQ_OP_ZONE_RESET_ALL: >> @@ -3096,6 +3135,17 @@ static void sd_read_write_same(struct scsi_disk *sdkp, unsigned char *buffer) >> sdkp->ws10 = 1; >> } >> >> +static void sd_read_verify(struct scsi_disk *sdkp, unsigned char *buffer) >> +{ >> + struct scsi_device *sdev = sdkp->device; >> + >> + sd_printk(KERN_INFO, sdkp, "VERIFY16 check.\n"); > > Remove this message please. > >> + if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, VERIFY_16) == 1) { >> + sd_printk(KERN_INFO, sdkp, " VERIFY16 in ON .\n"); > > And this one too. > >> + sdkp->verify_16 = 1; > > Why not checking for VERIFY 10 and 12 if VERIFY 16 is not supported ? > Also, why don't you call "blk_queue_max_verify_sectors(q, UINT_MAX >> 9);" here > instead of adding the not so useful sd_config_verify() helper ? > Okay, let me see if I can add that in V1. >> + } >> +} >> + >> static void sd_read_security(struct scsi_disk *sdkp, unsigned char *buffer) >> { >> struct scsi_device *sdev = sdkp->device; >> @@ -3224,6 +3274,7 @@ static int sd_revalidate_disk(struct gendisk *disk) >> sd_read_cache_type(sdkp, buffer); >> sd_read_app_tag_own(sdkp, buffer); >> sd_read_write_same(sdkp, buffer); >> + sd_read_verify(sdkp, buffer); >> sd_read_security(sdkp, buffer); >> } >> >> @@ -3265,6 +3316,7 @@ static int sd_revalidate_disk(struct gendisk *disk) >> >> set_capacity_and_notify(disk, logical_to_sectors(sdp, sdkp->capacity)); >> sd_config_write_same(sdkp); >> + sd_config_verify(sdkp); >> kfree(buffer); >> >> /* >> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h >> index b59136c4125b..94a86bf6dac4 100644 >> --- a/drivers/scsi/sd.h >> +++ b/drivers/scsi/sd.h >> @@ -120,6 +120,7 @@ struct scsi_disk { >> unsigned lbpvpd : 1; >> unsigned ws10 : 1; >> unsigned ws16 : 1; >> + unsigned verify_16 : 1; > > See right above this line how write same supports the 10 and 16 variants. I > think you need the same here. And very likely, you also need the 32 version in > case the device has DIF/DIX (type 2 protection). > Agree with write same 10/16 versions, let me see if I can add that for V1. >> unsigned rc_basis: 2; >> unsigned zoned: 2; >> unsigned urswrz : 1; >> > > > -- > Damien Le Moal > Western Digital Research > Thanks for the comments Damien.
WARNING: multiple messages have this Message-ID (diff)
From: Chaitanya Kulkarni <chaitanyak@nvidia.com> To: Damien Le Moal <damien.lemoal@opensource.wdc.com> Cc: "linux-raid@vger.kernel.org" <linux-raid@vger.kernel.org>, "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>, "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>, "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>, "dm-devel@redhat.com" <dm-devel@redhat.com>, "target-devel@vger.kernel.org" <target-devel@vger.kernel.org>, "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>, "linux-xfs@vger.kernel.org" <linux-xfs@vger.kernel.org> Subject: Re: [dm-devel] [RFC PATCH 2/8] scsi: add REQ_OP_VERIFY support Date: Thu, 11 Nov 2021 08:07:39 +0000 [thread overview] Message-ID: <cb9cac6d-1206-0bd3-0883-dc7f1a9fcc78@nvidia.com> (raw) In-Reply-To: <bd36ee58-8273-cd0a-295e-0c66b0142bcd@opensource.wdc.com> On 11/4/2021 5:33 AM, Damien Le Moal wrote: > External email: Use caution opening links or attachments > > > On 2021/11/04 15:46, Chaitanya Kulkarni wrote: >> From: Chaitanya Kulkarni <kch@nvidia.com> >> >> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> >> --- >> drivers/scsi/sd.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++ >> drivers/scsi/sd.h | 1 + >> 2 files changed, 53 insertions(+) >> >> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c >> index a3d2d4bc4a3d..7f2c4eb98cf8 100644 >> --- a/drivers/scsi/sd.c >> +++ b/drivers/scsi/sd.c >> @@ -106,6 +106,7 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_ZBC); >> >> static void sd_config_discard(struct scsi_disk *, unsigned int); >> static void sd_config_write_same(struct scsi_disk *); >> +static void sd_config_verify(struct scsi_disk *sdkp); >> static int sd_revalidate_disk(struct gendisk *); >> static void sd_unlock_native_capacity(struct gendisk *disk); >> static int sd_probe(struct device *); >> @@ -995,6 +996,41 @@ static blk_status_t sd_setup_write_zeroes_cmnd(struct scsi_cmnd *cmd) >> return sd_setup_write_same10_cmnd(cmd, false); >> } >> >> +static void sd_config_verify(struct scsi_disk *sdkp) >> +{ >> + struct request_queue *q = sdkp->disk->queue; >> + >> + /* XXX: use same pattern as sd_config_write_same(). */ >> + blk_queue_max_verify_sectors(q, UINT_MAX >> 9); > > VERIFY 10, 12, 16 and 32 commands are optional and may not be implemented by a > device. So setting this unconditionally is wrong. > At the very least you must have an "if (sdkp->verify_16)" here, and call > "blk_queue_max_verify_sectors(q, 0);" if the device does not support verify. > Yes, I put it together for the RFC, this needs to consider the device unsupported case just like what we do for write zeroes and emulate the same. >> +} >> + >> +static blk_status_t sd_setup_verify_cmnd(struct scsi_cmnd *cmd) >> +{ >> + struct request *rq = cmd->request; >> + struct scsi_device *sdp = cmd->device; >> + struct scsi_disk *sdkp = scsi_disk(rq->rq_disk); >> + u64 lba = sectors_to_logical(sdp, blk_rq_pos(rq)); >> + u32 nr_blocks = sectors_to_logical(sdp, blk_rq_sectors(rq)); >> + >> + if (!sdkp->verify_16) >> + return BLK_STS_NOTSUPP; > > I think this should be "return BLK_STS_TARGET;" > >> + >> + cmd->cmd_len = 16; >> + cmd->cmnd[0] = VERIFY_16; > > And what if the device supports VERIFY 10 or 12 but not VERIFY 16 ? For first implementation we can only VERIFY 16, later we can add cases for VERIFY 10-12 versions. > >> + /* skip veprotect / dpo / bytchk */ >> + cmd->cmnd[1] = 0; >> + put_unaligned_be64(lba, &cmd->cmnd[2]); >> + put_unaligned_be32(nr_blocks, &cmd->cmnd[10]); >> + cmd->cmnd[14] = 0; >> + cmd->cmnd[15] = 0; >> + >> + cmd->allowed = SD_MAX_RETRIES; >> + cmd->sc_data_direction = DMA_NONE; >> + cmd->transfersize = 0; >> + >> + return BLK_STS_OK; >> +} >> + >> static void sd_config_write_same(struct scsi_disk *sdkp) >> { >> struct request_queue *q = sdkp->disk->queue; >> @@ -1345,6 +1381,8 @@ static blk_status_t sd_init_command(struct scsi_cmnd *cmd) >> } >> case REQ_OP_WRITE_ZEROES: >> return sd_setup_write_zeroes_cmnd(cmd); >> + case REQ_OP_VERIFY: >> + return sd_setup_verify_cmnd(cmd); >> case REQ_OP_WRITE_SAME: >> return sd_setup_write_same_cmnd(cmd); >> case REQ_OP_FLUSH: >> @@ -2029,6 +2067,7 @@ static int sd_done(struct scsi_cmnd *SCpnt) >> switch (req_op(req)) { >> case REQ_OP_DISCARD: >> case REQ_OP_WRITE_ZEROES: >> + case REQ_OP_VERIFY: >> case REQ_OP_WRITE_SAME: >> case REQ_OP_ZONE_RESET: >> case REQ_OP_ZONE_RESET_ALL: >> @@ -3096,6 +3135,17 @@ static void sd_read_write_same(struct scsi_disk *sdkp, unsigned char *buffer) >> sdkp->ws10 = 1; >> } >> >> +static void sd_read_verify(struct scsi_disk *sdkp, unsigned char *buffer) >> +{ >> + struct scsi_device *sdev = sdkp->device; >> + >> + sd_printk(KERN_INFO, sdkp, "VERIFY16 check.\n"); > > Remove this message please. > >> + if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, VERIFY_16) == 1) { >> + sd_printk(KERN_INFO, sdkp, " VERIFY16 in ON .\n"); > > And this one too. > >> + sdkp->verify_16 = 1; > > Why not checking for VERIFY 10 and 12 if VERIFY 16 is not supported ? > Also, why don't you call "blk_queue_max_verify_sectors(q, UINT_MAX >> 9);" here > instead of adding the not so useful sd_config_verify() helper ? > Okay, let me see if I can add that in V1. >> + } >> +} >> + >> static void sd_read_security(struct scsi_disk *sdkp, unsigned char *buffer) >> { >> struct scsi_device *sdev = sdkp->device; >> @@ -3224,6 +3274,7 @@ static int sd_revalidate_disk(struct gendisk *disk) >> sd_read_cache_type(sdkp, buffer); >> sd_read_app_tag_own(sdkp, buffer); >> sd_read_write_same(sdkp, buffer); >> + sd_read_verify(sdkp, buffer); >> sd_read_security(sdkp, buffer); >> } >> >> @@ -3265,6 +3316,7 @@ static int sd_revalidate_disk(struct gendisk *disk) >> >> set_capacity_and_notify(disk, logical_to_sectors(sdp, sdkp->capacity)); >> sd_config_write_same(sdkp); >> + sd_config_verify(sdkp); >> kfree(buffer); >> >> /* >> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h >> index b59136c4125b..94a86bf6dac4 100644 >> --- a/drivers/scsi/sd.h >> +++ b/drivers/scsi/sd.h >> @@ -120,6 +120,7 @@ struct scsi_disk { >> unsigned lbpvpd : 1; >> unsigned ws10 : 1; >> unsigned ws16 : 1; >> + unsigned verify_16 : 1; > > See right above this line how write same supports the 10 and 16 variants. I > think you need the same here. And very likely, you also need the 32 version in > case the device has DIF/DIX (type 2 protection). > Agree with write same 10/16 versions, let me see if I can add that for V1. >> unsigned rc_basis: 2; >> unsigned zoned: 2; >> unsigned urswrz : 1; >> > > > -- > Damien Le Moal > Western Digital Research > Thanks for the comments Damien. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
next prev parent reply other threads:[~2021-11-11 8:07 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-11-04 6:46 [RFC PATCH 0/8] block: add support for REQ_OP_VERIFY Chaitanya Kulkarni 2021-11-04 6:46 ` [dm-devel] " Chaitanya Kulkarni 2021-11-04 6:46 ` [RFC PATCH 1/8] " Chaitanya Kulkarni 2021-11-04 6:46 ` [dm-devel] " Chaitanya Kulkarni 2021-11-04 17:25 ` Darrick J. Wong 2021-11-04 17:25 ` [dm-devel] " Darrick J. Wong 2021-11-11 8:01 ` Chaitanya Kulkarni 2021-11-11 8:01 ` [dm-devel] " Chaitanya Kulkarni 2021-11-04 6:46 ` [RFC PATCH 2/8] scsi: add REQ_OP_VERIFY support Chaitanya Kulkarni 2021-11-04 6:46 ` [dm-devel] " Chaitanya Kulkarni 2021-11-04 12:33 ` Damien Le Moal 2021-11-04 12:33 ` [dm-devel] " Damien Le Moal 2021-11-11 8:07 ` Chaitanya Kulkarni [this message] 2021-11-11 8:07 ` Chaitanya Kulkarni 2021-11-04 6:46 ` [RFC PATCH 3/8] nvme: add support for the Verify command Chaitanya Kulkarni 2021-11-04 6:46 ` [dm-devel] " Chaitanya Kulkarni 2021-11-04 22:44 ` Keith Busch 2021-11-04 22:44 ` [dm-devel] " Keith Busch 2021-11-11 8:09 ` Chaitanya Kulkarni 2021-11-11 8:09 ` [dm-devel] " Chaitanya Kulkarni 2021-11-04 6:46 ` [PATCH 4/8] nvmet: add Verify command support for bdev-ns Chaitanya Kulkarni 2021-11-04 6:46 ` [dm-devel] " Chaitanya Kulkarni 2021-11-04 6:46 ` [RFC PATCH 5/8] nvmet: add Verify emulation " Chaitanya Kulkarni 2021-11-04 6:46 ` [dm-devel] " Chaitanya Kulkarni 2021-11-04 6:46 ` [RFC PATCH 6/8] nvmet: add verify emulation support for file-ns Chaitanya Kulkarni 2021-11-04 6:46 ` [dm-devel] " Chaitanya Kulkarni 2021-11-04 6:46 ` [RFC PATCH 7/8] null_blk: add REQ_OP_VERIFY support Chaitanya Kulkarni 2021-11-04 6:46 ` [dm-devel] " Chaitanya Kulkarni 2021-11-04 6:46 ` [RFC PATCH 8/8] md: add support for REQ_OP_VERIFY Chaitanya Kulkarni 2021-11-04 6:46 ` [dm-devel] " Chaitanya Kulkarni 2021-11-11 8:13 ` Chaitanya Kulkarni 2021-11-11 8:13 ` [dm-devel] " Chaitanya Kulkarni 2021-11-12 15:19 ` Mike Snitzer 2021-11-12 15:19 ` [dm-devel] " Mike Snitzer 2021-11-04 7:14 ` [RFC PATCH 0/8] block: " Christoph Hellwig 2021-11-04 7:14 ` [dm-devel] " Christoph Hellwig 2021-11-04 9:27 ` Chaitanya Kulkarni 2021-11-04 9:27 ` [dm-devel] " Chaitanya Kulkarni 2021-11-04 17:32 ` Darrick J. Wong 2021-11-04 17:32 ` [dm-devel] " Darrick J. Wong 2021-11-04 17:34 ` Christoph Hellwig 2021-11-04 17:34 ` [dm-devel] " Christoph Hellwig 2021-11-04 22:37 ` Keith Busch 2021-11-04 22:37 ` [dm-devel] " Keith Busch 2021-11-05 8:25 ` javier 2021-11-05 8:25 ` [dm-devel] " javier 2021-11-11 8:18 ` Chaitanya Kulkarni 2021-11-11 8:18 ` [dm-devel] " Chaitanya Kulkarni 2021-11-04 15:16 ` Douglas Gilbert 2021-11-04 15:16 ` [dm-devel] " Douglas Gilbert 2021-11-11 8:27 ` Chaitanya Kulkarni 2021-11-11 8:27 ` [dm-devel] " Chaitanya Kulkarni
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=cb9cac6d-1206-0bd3-0883-dc7f1a9fcc78@nvidia.com \ --to=chaitanyak@nvidia.com \ --cc=damien.lemoal@opensource.wdc.com \ --cc=dm-devel@redhat.com \ --cc=linux-block@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-nvme@lists.infradead.org \ --cc=linux-raid@vger.kernel.org \ --cc=linux-scsi@vger.kernel.org \ --cc=linux-xfs@vger.kernel.org \ --cc=target-devel@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.