lkml.org 
[lkml]   [2014]   [Oct]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] Do not silently discard WRITE_SAME requests
On 10/14/2014 5:57 PM, Martin K. Petersen wrote:
>>>>>> "Petr" == Petr Vandrovec <petr@vmware.com> writes:
>
> Petr,
>
> Petr> Logic (from 2011, commit 8af1954d172a46a63e5e79dae523a6d74715e458)
> Petr> says that EOPNOTSUPP is returned when DISCARD request failed, as
> Petr> discarding is optional, and failures can be safely ignored. That
> Petr> is definitely not true for WRITE_SAME failures, and so unsupported
> Petr> WRITE_SAME should return different error code than unsupported
> Petr> DISCARD.
>
> I agree. With Lukas' change the bio batch error handling is too
> permissive for the write same case. It also made the regular zeroout
> error handling case a bit fishy but it is unlikely that we'd get
> EOPNOTSUPP on a regular write.
>
> So for the block portion:
>
> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: stable@vger.kernel.org # 3.7+
>
> Petr> revert of blacklisting VMware's LSI (if anything, blacklist should
> Petr> be for current firmware version of 'VMware Virtual SCSI Disk', as
> Petr> f.e. passed-through (RDM) SCSI disks do support WRITE_SAME under
> Petr> VMware) -- see attached updated diff.
>
> If you have a better heuristic then let's by all means use it. Please
> adjust the pattern matching strings to match the actual values returned
> by the target.
>
>
> SCSI: Only blacklist WRITE SAME for VMware virtual disks
>
> Commit 4089b71cc820 blacklisted WRITE SAME for all VMware disks.
> However, the WRITE SAME commands are supported for passthrough
> disks. Change the heuristic so we only blacklist virtual disks.

Hi,
is there any reason to do blacklisting? Why not let first request
fail, and switch to non-write-same code path once that happens?

If you want to go ahead with blacklisting, then entries should be:

{ "VMware", "Virtual disk", "1.0", BLIST_NO_WRITE_SAME }, //ESX
{ "VMware,", "VMware Virtual S", "1.0", BLIST_NO_WRITE_SAME }, //WS,Fusion

(yes, on Workstation and Fusion there is really comma in the vendor name
and VMware is repeated in the device ID - first is 'VMware, Inc.'
truncated to 8 characters, and second is 'VMware Virtual SCSI Hard
Drive' truncated to 16; we got revision right...)
Thanks,
Petr


> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> Reported-by: Petr Vandrovec <petr@vmware.com>
>
> diff --git a/drivers/message/fusion/mptspi.c b/drivers/message/fusion/mptspi.c
> index 613231c16194..787933d43d32 100644
> --- a/drivers/message/fusion/mptspi.c
> +++ b/drivers/message/fusion/mptspi.c
> @@ -1419,11 +1419,6 @@ mptspi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> goto out_mptspi_probe;
> }
>
> - /* VMWare emulation doesn't properly implement WRITE_SAME
> - */
> - if (pdev->subsystem_vendor == 0x15AD)
> - sh->no_write_same = 1;
> -
> spin_lock_irqsave(&ioc->FreeQlock, flags);
>
> /* Attach the SCSI Host to the IOC structure
> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> index 49014a143c6a..402aebfb97dd 100644
> --- a/drivers/scsi/scsi_devinfo.c
> +++ b/drivers/scsi/scsi_devinfo.c
> @@ -249,6 +249,7 @@ static struct {
> {"TOSHIBA", "CD-ROM", NULL, BLIST_ISROM},
> {"Traxdata", "CDR4120", NULL, BLIST_NOLUN}, /* locks up */
> {"USB2.0", "SMARTMEDIA/XD", NULL, BLIST_FORCELUN | BLIST_INQUIRY_36},
> + {"VMware", "Virtual SCSI Disk", NULL, BLIST_NO_WRITE_SAME},
> {"WangDAT", "Model 2600", "01.7", BLIST_SELECT_NO_ATN},
> {"WangDAT", "Model 3200", "02.2", BLIST_SELECT_NO_ATN},
> {"WangDAT", "Model 1300", "02.4", BLIST_SELECT_NO_ATN},
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index ba3f1e8d0d57..c57daffe57af 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -955,6 +955,9 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
> if (*bflags & BLIST_NO_DIF)
> sdev->no_dif = 1;
>
> + if (*bflags & BLIST_NO_WRITE_SAME)
> + sdev->no_write_same = 1;
> +
> sdev->eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
>
> if (*bflags & BLIST_TRY_VPD_PAGES)
> diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
> index 183eaab7c380..1a24efb4b1d6 100644
> --- a/include/scsi/scsi_devinfo.h
> +++ b/include/scsi/scsi_devinfo.h
> @@ -36,5 +36,6 @@
> for sequential scan */
> #define BLIST_TRY_VPD_PAGES 0x10000000 /* Attempt to read VPD pages */
> #define BLIST_NO_RSOC 0x20000000 /* don't try to issue RSOC */
> +#define BLIST_NO_WRITE_SAME 0x40000000 /* don't try to issue WRITE SAME */
>
> #endif
>
>



\
 
 \ /
  Last update: 2014-10-15 09:21    [W:0.072 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site