| Message ID | 1468934710-93876-3-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
Hi Hannes, On 7/19/16 22:25, Hannes Reinecke wrote: > We can map the RESET WRITE POINTER command onto a 'discard' > request. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/scsi/sd.c | 65 ++++++++++++++++++++++++++++++++++++++++++++----------- > drivers/scsi/sd.h | 1 + > 2 files changed, 53 insertions(+), 13 deletions(-) > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 249ea81..52dda83 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -369,6 +369,7 @@ static const char *lbp_mode[] = { > [SD_LBP_WS16] = "writesame_16", > [SD_LBP_WS10] = "writesame_10", > [SD_LBP_ZERO] = "writesame_zero", > + [SD_ZBC_RESET_WP] = "reset_wp", > [SD_LBP_DISABLE] = "disabled", > }; > > @@ -391,6 +392,13 @@ provisioning_mode_store(struct device *dev, struct device_attribute *attr, > if (!capable(CAP_SYS_ADMIN)) > return -EACCES; > > + if (sdkp->zoned == 1) { > + if (!strncmp(buf, lbp_mode[SD_ZBC_RESET_WP], 20)) { > + sd_config_discard(sdkp, SD_ZBC_RESET_WP); > + return count; > + } > + return -EINVAL; > + } > if (sdp->type != TYPE_DISK) > return -EINVAL; > > @@ -683,6 +691,11 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode) > q->limits.discard_zeroes_data = sdkp->lbprz; > break; > > + case SD_ZBC_RESET_WP: > + max_blocks = sdkp->unmap_granularity; > + q->limits.discard_zeroes_data = 1; > + break; > + > case SD_LBP_ZERO: > max_blocks = min_not_zero(sdkp->max_ws_blocks, > (u32)SD_MAX_WS10_BLOCKS); I am still wondering if setting discard_zeroes_data to 1 is the right choice here since nothing will happen for conventional zones (no zeroing, no reset, nothing). discard_zeroes_data=0 may be a safer choice, even though I have not hit any issue with it set to 1. Reviewed-by: Damien Le Moal <damien.lemoal@hgst.com> Tested-by: Damien Le Moal <damien.lemoal@hgst.com>
On 07/20/2016 02:49 AM, Damien Le Moal wrote: > Hi Hannes, > > On 7/19/16 22:25, Hannes Reinecke wrote: >> We can map the RESET WRITE POINTER command onto a 'discard' >> request. >> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> drivers/scsi/sd.c | 65 >> ++++++++++++++++++++++++++++++++++++++++++++----------- >> drivers/scsi/sd.h | 1 + >> 2 files changed, 53 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c >> index 249ea81..52dda83 100644 >> --- a/drivers/scsi/sd.c >> +++ b/drivers/scsi/sd.c >> @@ -369,6 +369,7 @@ static const char *lbp_mode[] = { >> [SD_LBP_WS16] = "writesame_16", >> [SD_LBP_WS10] = "writesame_10", >> [SD_LBP_ZERO] = "writesame_zero", >> + [SD_ZBC_RESET_WP] = "reset_wp", >> [SD_LBP_DISABLE] = "disabled", >> }; >> >> @@ -391,6 +392,13 @@ provisioning_mode_store(struct device *dev, >> struct device_attribute *attr, >> if (!capable(CAP_SYS_ADMIN)) >> return -EACCES; >> >> + if (sdkp->zoned == 1) { >> + if (!strncmp(buf, lbp_mode[SD_ZBC_RESET_WP], 20)) { >> + sd_config_discard(sdkp, SD_ZBC_RESET_WP); >> + return count; >> + } >> + return -EINVAL; >> + } >> if (sdp->type != TYPE_DISK) >> return -EINVAL; >> >> @@ -683,6 +691,11 @@ static void sd_config_discard(struct scsi_disk >> *sdkp, unsigned int mode) >> q->limits.discard_zeroes_data = sdkp->lbprz; >> break; >> >> + case SD_ZBC_RESET_WP: >> + max_blocks = sdkp->unmap_granularity; >> + q->limits.discard_zeroes_data = 1; >> + break; >> + >> case SD_LBP_ZERO: >> max_blocks = min_not_zero(sdkp->max_ws_blocks, >> (u32)SD_MAX_WS10_BLOCKS); > > I am still wondering if setting discard_zeroes_data to 1 is the right > choice here since nothing will happen for conventional zones (no > zeroing, no reset, nothing). discard_zeroes_data=0 may be a safer > choice, even though I have not hit any issue with it set to 1. > This setting needs to be reviewed once hchs zero-out patches are in. Thing is, we need to properly differentiate between 'discard' and 'zero-out'. Unfortunately ATM the libata stack only implements a translation for 'write_same', which is then mapped onto DSM TRIM. So the 'write_discard_zeroes' is even incorrect for current libata usage. Cheers, Hannes
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 249ea81..52dda83 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -369,6 +369,7 @@ static const char *lbp_mode[] = { [SD_LBP_WS16] = "writesame_16", [SD_LBP_WS10] = "writesame_10", [SD_LBP_ZERO] = "writesame_zero", + [SD_ZBC_RESET_WP] = "reset_wp", [SD_LBP_DISABLE] = "disabled", }; @@ -391,6 +392,13 @@ provisioning_mode_store(struct device *dev, struct device_attribute *attr, if (!capable(CAP_SYS_ADMIN)) return -EACCES; + if (sdkp->zoned == 1) { + if (!strncmp(buf, lbp_mode[SD_ZBC_RESET_WP], 20)) { + sd_config_discard(sdkp, SD_ZBC_RESET_WP); + return count; + } + return -EINVAL; + } if (sdp->type != TYPE_DISK) return -EINVAL; @@ -683,6 +691,11 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode) q->limits.discard_zeroes_data = sdkp->lbprz; break; + case SD_ZBC_RESET_WP: + max_blocks = sdkp->unmap_granularity; + q->limits.discard_zeroes_data = 1; + break; + case SD_LBP_ZERO: max_blocks = min_not_zero(sdkp->max_ws_blocks, (u32)SD_MAX_WS10_BLOCKS); @@ -711,16 +724,20 @@ static int sd_setup_discard_cmnd(struct scsi_cmnd *cmd) unsigned int nr_sectors = blk_rq_sectors(rq); unsigned int nr_bytes = blk_rq_bytes(rq); unsigned int len; - int ret; + int ret = 0; char *buf; - struct page *page; + struct page *page = NULL; sector >>= ilog2(sdp->sector_size) - 9; nr_sectors >>= ilog2(sdp->sector_size) - 9; - page = alloc_page(GFP_ATOMIC | __GFP_ZERO); - if (!page) - return BLKPREP_DEFER; + if (sdkp->provisioning_mode != SD_ZBC_RESET_WP) { + page = alloc_page(GFP_ATOMIC | __GFP_ZERO); + if (!page) + return BLKPREP_DEFER; + } + + rq->completion_data = page; switch (sdkp->provisioning_mode) { case SD_LBP_UNMAP: @@ -760,12 +777,21 @@ static int sd_setup_discard_cmnd(struct scsi_cmnd *cmd) len = sdkp->device->sector_size; break; + case SD_ZBC_RESET_WP: + cmd->cmd_len = 16; + cmd->cmnd[0] = ZBC_OUT; + cmd->cmnd[1] = ZO_RESET_WRITE_POINTER; + put_unaligned_be64(sector, &cmd->cmnd[2]); + /* Reset Write Pointer doesn't have a payload */ + len = 0; + cmd->sc_data_direction = DMA_NONE; + break; + default: ret = BLKPREP_INVALID; goto out; } - rq->completion_data = page; rq->timeout = SD_TIMEOUT; cmd->transfersize = len; @@ -779,13 +805,17 @@ static int sd_setup_discard_cmnd(struct scsi_cmnd *cmd) * discarded on disk. This allows us to report completion on the full * amount of blocks described by the request. */ - blk_add_request_payload(rq, page, 0, len); - ret = scsi_init_io(cmd); + if (len) { + blk_add_request_payload(rq, page, 0, len); + ret = scsi_init_io(cmd); + } rq->__data_len = nr_bytes; out: - if (ret != BLKPREP_OK) + if (page && ret != BLKPREP_OK) { + rq->completion_data = NULL; __free_page(page); + } return ret; } @@ -1151,7 +1181,8 @@ static void sd_uninit_command(struct scsi_cmnd *SCpnt) { struct request *rq = SCpnt->request; - if (rq->cmd_flags & REQ_DISCARD) + if (rq->cmd_flags & REQ_DISCARD && + rq->completion_data) __free_page(rq->completion_data); if (SCpnt->cmnd != rq->cmd) { @@ -1768,6 +1799,7 @@ static int sd_done(struct scsi_cmnd *SCpnt) int sense_deferred = 0; unsigned char op = SCpnt->cmnd[0]; unsigned char unmap = SCpnt->cmnd[1] & 8; + unsigned char sa = SCpnt->cmnd[1] & 0xf; if (req->cmd_flags & REQ_DISCARD || req->cmd_flags & REQ_WRITE_SAME) { if (!result) { @@ -1819,6 +1851,10 @@ static int sd_done(struct scsi_cmnd *SCpnt) case UNMAP: sd_config_discard(sdkp, SD_LBP_DISABLE); break; + case ZBC_OUT: + if (sa == ZO_RESET_WRITE_POINTER) + sd_config_discard(sdkp, SD_LBP_DISABLE); + break; case WRITE_SAME_16: case WRITE_SAME: if (unmap) @@ -2113,9 +2149,12 @@ static void sd_read_zones(struct scsi_disk *sdkp, unsigned char *buffer) } /* Read the zone length from the first zone descriptor */ desc = &buffer[64]; - zone_len = logical_to_sectors(sdkp->device, - get_unaligned_be64(&desc[8])); - blk_queue_chunk_sectors(sdkp->disk->queue, zone_len); + zone_len = get_unaligned_be64(&desc[8]); + sdkp->unmap_alignment = zone_len; + sdkp->unmap_granularity = zone_len; + blk_queue_chunk_sectors(sdkp->disk->queue, + logical_to_sectors(sdkp->device, zone_len)); + sd_config_discard(sdkp, SD_ZBC_RESET_WP); } static void read_capacity_error(struct scsi_disk *sdkp, struct scsi_device *sdp, diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h index 74ec357..4439693 100644 --- a/drivers/scsi/sd.h +++ b/drivers/scsi/sd.h @@ -56,6 +56,7 @@ enum { SD_LBP_WS16, /* Use WRITE SAME(16) with UNMAP bit */ SD_LBP_WS10, /* Use WRITE SAME(10) with UNMAP bit */ SD_LBP_ZERO, /* Use WRITE SAME(10) with zero payload */ + SD_ZBC_RESET_WP, /* Use RESET WRITE POINTER */ SD_LBP_DISABLE, /* Discard disabled due to failed cmd */ };
We can map the RESET WRITE POINTER command onto a 'discard' request. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/scsi/sd.c | 65 ++++++++++++++++++++++++++++++++++++++++++++----------- drivers/scsi/sd.h | 1 + 2 files changed, 53 insertions(+), 13 deletions(-)