Re: [PATCH 2/5] sd: Implement new RESET_WP provisioning mode

From: Hannes Reinecke <hare@suse.de>
To: Damien Le Moal <damien.lemoal@hgst.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>
Cc: James Bottomley <james.bottomley@hansenpartnership.com>,
	linux-scsi@vger.kernel.org, Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH 2/5] sd: Implement new RESET_WP provisioning mode
Date: Wed, 20 Jul 2016 16:52:31 +0200	[thread overview]
Message-ID: <62ea4016-dc5d-3df6-8764-e62291f7adc3@suse.de> (raw)
In-Reply-To: <6c0e2de2-fbed-1fc1-3d2c-4063d034cb58@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
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html