Re: [PATCH 12/23] sd: handle REQ_UNMAP
On Thu, 2017-03-23 at 10:33 -0400, Christoph Hellwig wrote:
> Try to use a write same with unmap bit variant if the device supports it
> and the caller asks for it.
>
> Signed-off-by: Christoph Hellwig
> ---
> drivers/scsi/sd.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index b6f70a09a301..ca96bb33471b 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -871,6 +871,16 @@ static int sd_setup_write_zeroes_cmnd(struct scsi_cmnd
> *cmd)
> return BLKPREP_INVALID;
> return sd_setup_ata_trim_cmnd(cmd);
> }
> +
> + if (rq->cmd_flags & REQ_UNMAP) {
> + switch (sdkp->provisioning_mode) {
> + case SD_LBP_WS16:
> + return sd_setup_write_same16_cmnd(cmd, true);
> + case SD_LBP_WS10:
> + return sd_setup_write_same10_cmnd(cmd, true);
> + }
> + }
> +
> if (sdp->no_write_same)
> return BLKPREP_INVALID;
> if (sdkp->ws16 || sector > 0x || nr_sectors > 0x)
Users can change the provisioning mode from user space from SD_LBP_WS16 into
SD_LBP_WS10 so I'm not sure it's safe to skip the (sdkp->ws16 || sector >
0x || nr_sectors > 0x) check if REQ_UNMAP is set.
Bart.
Re: [PATCH 12/23] sd: handle REQ_UNMAP
On 28/03/2017 18:48, Bart Van Assche wrote:
>> +if (rq->cmd_flags & REQ_UNMAP) {
>> +switch (sdkp->provisioning_mode) {
>> +case SD_LBP_WS16:
>> +return sd_setup_write_same16_cmnd(cmd, true);
>> +case SD_LBP_WS10:
>> +return sd_setup_write_same10_cmnd(cmd, true);
>> +}
>> +}
>> +
>> if (sdp->no_write_same)
>> return BLKPREP_INVALID;
>> if (sdkp->ws16 || sector > 0x || nr_sectors > 0x)
> Users can change the provisioning mode from user space from SD_LBP_WS16 into
> SD_LBP_WS10 so I'm not sure it's safe to skip the (sdkp->ws16 || sector >
> 0x || nr_sectors > 0x) check if REQ_UNMAP is set.
Yeah, if REQ_UNMAP is set you should probably check sdkp->provisioning_mode
instead of sdkp->ws16, but apart from this it should still go through the
checks below.
Plus, if the provisioning mode is not ws10 or ws16, should
sd_setup_write_zeroes_cmnd:
1) do a WRITE SAME without UNMAP (what Christoph's code does)
2) return BLKPREP_INVALID
3) ignore provisioning mode and do a WRITE SAME with UNMAP
4) do a WRITE SAME without UNMAP for SD_LBP_{ZERO,FULL,DISABLE},
do a WRITE SAME with UNMAP for SD_LBP_{WS10,WS16,UNMAP}.
I'm in favor of (4). The distinction between SD_LBP_UNMAP, SD_LBP_WS10
and SD_LBP_WS16 is as problematic as discard_zeroes_data in my opinion.
Thanks,
Paolo
Re: [PATCH 12/23] sd: handle REQ_UNMAP
On Tue, Mar 28, 2017 at 04:48:55PM +, Bart Van Assche wrote: > > if (sdp->no_write_same) > > return BLKPREP_INVALID; > > if (sdkp->ws16 || sector > 0x || nr_sectors > 0x) > > Users can change the provisioning mode from user space from SD_LBP_WS16 into > SD_LBP_WS10 so I'm not sure it's safe to skip the (sdkp->ws16 || sector > > 0x || nr_sectors > 0x) check if REQ_UNMAP is set. They can, and if the device has too many sectors that will already cause discard to fail, and in this case it will cause write zeroes to fail as well. The intent behind this patch is to keep the behavior the same as the old path that uses discards for zeroing. The logic looks a bit clumsy, but I'd rather keep it as-is.
Re: [PATCH 12/23] sd: handle REQ_UNMAP
"h...@lst.de" writes: Christoph, > On Tue, Mar 28, 2017 at 04:48:55PM +, Bart Van Assche wrote: >> >if (sdp->no_write_same) >> >return BLKPREP_INVALID; >> >if (sdkp->ws16 || sector > 0x || nr_sectors > 0x) >> >> Users can change the provisioning mode from user space from SD_LBP_WS16 into >> SD_LBP_WS10 so I'm not sure it's safe to skip the (sdkp->ws16 || sector > >> 0x || nr_sectors > 0x) check if REQ_UNMAP is set. > > They can, and if the device has too many sectors that will already cause > discard to fail, I'm not sure I understand what you mean by that? -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 12/23] sd: handle REQ_UNMAP
On Thu, Mar 30, 2017 at 11:28:32AM -0400, Martin K. Petersen wrote: > "h...@lst.de" writes: > > Christoph, > > > On Tue, Mar 28, 2017 at 04:48:55PM +, Bart Van Assche wrote: > >> > if (sdp->no_write_same) > >> > return BLKPREP_INVALID; > >> > if (sdkp->ws16 || sector > 0x || nr_sectors > 0x) > >> > >> Users can change the provisioning mode from user space from SD_LBP_WS16 > >> into > >> SD_LBP_WS10 so I'm not sure it's safe to skip the (sdkp->ws16 || sector > > >> 0x || nr_sectors > 0x) check if REQ_UNMAP is set. > > > > They can, and if the device has too many sectors that will already cause > > discard to fail, > > I'm not sure I understand what you mean by that? If you manually change the provisioning mode to WS10 on a device that must use WRITE SAME (16) to be able to address all blocks you're already screwed right now, and with this patch you can screw yourself through the WRITE_ZEROES path in addition to the DISCARD path.
Re: [PATCH 12/23] sd: handle REQ_UNMAP
"h...@lst.de" writes: > If you manually change the provisioning mode to WS10 on a device that > must use WRITE SAME (16) to be able to address all blocks you're already > screwed right now, and with this patch you can screw yourself through > the WRITE_ZEROES path in addition to the DISCARD path. Oh, I see. We only had the LBA sanity check in place for write same, not for discard. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 12/23] sd: handle REQ_UNMAP
On Thu, Mar 30, 2017 at 10:19:55PM -0400, Martin K. Petersen wrote: > > If you manually change the provisioning mode to WS10 on a device that > > must use WRITE SAME (16) to be able to address all blocks you're already > > screwed right now, and with this patch you can screw yourself through > > the WRITE_ZEROES path in addition to the DISCARD path. > > Oh, I see. We only had the LBA sanity check in place for write same, not > for discard. And btw, I'd be happy to add such a check, I'd just rather keep it out of this patch. It might be a good idea if you give it a turn after this series given that you have all the devices that have weird provisioning modes while I don't..
