[PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[Alan Stern]

Some USB mass-storage devices claim to have a write-back cache but
don't support the SYNCHRONIZE CACHE command, which means there is no
way to tell these devices to flush their caches out to permanent
storage.  Unfortunately, there is nothing we can do about this.

Until recently this deficiency did not cause any noticeable problems.
But following commit 89fb4cd1f717 ("scsi: handle flush errors
properly"), the errors are propagated to userspace where they get
reported as failures.

As a workaround, this patch adds a quirks flag for these devices to
the usb-storage driver, and a corresponding SCSI device flag, to
indicate the device can't handle SYNCHRONIZE CACHE.  If the flag is
set, the sd driver will override the cache settings reported by the
device, so that the device appears to be write-through.  This will
prevent the unsupported command from being sent.

This addresses Bugzilla #89511.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Reported-by: Jun Itou <itou_jun@infoseek.jp>
Reported-by: Markus Rathgeb <maggu2810@gmail.com>
Reported-by: Matt <vickm78@hotmail.com>
Tested-by: Markus Rathgeb <maggu2810@gmail.com>
Tested-by: Matt <vickm78@hotmail.com>
Fixes: 89fb4cd1f717a871ef79fa7debbe840e3225cd54
CC: James Bottomley <JBottomley@Parallels.com>
CC: <stable@vger.kernel.org>

---

As far as I'm concerned, this can be merged by either Greg or James 
(once the current merge window is over, of course).

Alan Stern


[as1779]


 drivers/scsi/sd.c                  |    7 +++++++
 drivers/usb/storage/scsiglue.c     |    4 ++++
 drivers/usb/storage/unusual_devs.h |   21 +++++++++++++++++++++
 include/linux/usb_usual.h          |    2 ++
 include/scsi/scsi_device.h         |    1 +
 5 files changed, 35 insertions(+)

Index: usb-4.0/include/scsi/scsi_device.h
===================================================================
--- usb-4.0.orig/include/scsi/scsi_device.h
+++ usb-4.0/include/scsi/scsi_device.h
@@ -174,6 +174,7 @@ struct scsi_device {
 	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
 	unsigned broken_fua:1;		/* Don't set FUA bit */
 	unsigned lun_in_cdb:1;		/* Store LUN bits in CDB[1] */
+	unsigned broken_synch_cache:1;	/* SYNCHRONIZE_CACHE is broken */
 
 	atomic_t disk_events_disable_depth; /* disable depth for disk events */
 
Index: usb-4.0/drivers/scsi/sd.c
===================================================================
--- usb-4.0.orig/drivers/scsi/sd.c
+++ usb-4.0/drivers/scsi/sd.c
@@ -2929,6 +2929,13 @@ static void sd_probe_async(void *data, a
 	sdkp->first_scan = 1;
 	sdkp->max_medium_access_timeouts = SD_MAX_MEDIUM_TIMEOUTS;
 
+	/*
+	 * Some buggy USB bridges don't implement SYNCHRONIZE_CACHE
+	 * but still claim to be write-back.  Override them.
+	 */
+	if (sdp->broken_synch_cache)
+		sdkp->cache_override = 1;
+
 	sd_revalidate_disk(gd);
 
 	gd->driverfs_dev = &sdp->sdev_gendev;
Index: usb-4.0/include/linux/usb_usual.h
===================================================================
--- usb-4.0.orig/include/linux/usb_usual.h
+++ usb-4.0/include/linux/usb_usual.h
@@ -79,6 +79,8 @@
 		/* Cannot handle MI_REPORT_SUPPORTED_OPERATION_CODES */	\
 	US_FLAG(MAX_SECTORS_240,	0x08000000)		\
 		/* Sets max_sectors to 240 */			\
+	US_FLAG(NO_SYNCHRONIZE_CACHE,	0x10000000)		\
+		/* Cannot handle SYNCHRONIZE CACHE */		\
 
 #define US_FLAG(name, value)	US_FL_##name = value ,
 enum { US_DO_ALL_FLAGS };
Index: usb-4.0/drivers/usb/storage/scsiglue.c
===================================================================
--- usb-4.0.orig/drivers/usb/storage/scsiglue.c
+++ usb-4.0/drivers/usb/storage/scsiglue.c
@@ -260,6 +260,10 @@ static int slave_configure(struct scsi_d
 		if (us->fflags & US_FL_BROKEN_FUA)
 			sdev->broken_fua = 1;
 
+		/* Likewise for SYNCHRONIZE CACHE */
+		if (us->fflags & US_FL_NO_SYNCHRONIZE_CACHE)
+			sdev->broken_synch_cache = 1;
+
 	} else {
 
 		/* Non-disk-type devices don't need to blacklist any pages
Index: usb-4.0/drivers/usb/storage/unusual_devs.h
===================================================================
--- usb-4.0.orig/drivers/usb/storage/unusual_devs.h
+++ usb-4.0/drivers/usb/storage/unusual_devs.h
@@ -392,6 +392,13 @@ UNUSUAL_DEV(  0x04b8, 0x0602, 0x0110, 0x
 		"785EPX Storage",
 		USB_SC_SCSI, USB_PR_BULK, NULL, US_FL_SINGLE_LUN),
 
+/* Reported by Jun Itou <itou_jun@infoseek.jp> */
+UNUSUAL_DEV(  0x04bb, 0x0109, 0x0100, 0x0100,
+		"I-O DATA DEVICE INC.",
+		"I-O DATA HDZ-UES",
+		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+		US_FL_NO_SYNCHRONIZE_CACHE),
+
 /* Not sure who reported this originally but
  * Pavel Machek <pavel@ucw.cz> reported that the extra US_FL_SINGLE_LUN
  * flag be added */
@@ -409,6 +416,13 @@ UNUSUAL_DEV(  0x04ce, 0x0002, 0x026c, 0x
 		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
 		US_FL_FIX_CAPACITY),
 
+/* Reported by Markus Rathgeb <maggu2810@gmail.com> */
+UNUSUAL_DEV(  0x04cf, 0x8818, 0xb007, 0xb007,
+		"Myson Century, Inc.",
+		"USB Mass Storage Device",
+		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+		US_FL_NO_SYNCHRONIZE_CACHE),
+
 /* Reported by Kriston Fincher <kriston@airmail.net>
  * Patch submitted by Sean Millichamp <sean@bruenor.org>
  * This is to support the Panasonic PalmCam PV-SD4090
@@ -1372,6 +1386,13 @@ UNUSUAL_DEV( 0x0bc2, 0x3010, 0x0000, 0x0
 		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
 		US_FL_SANE_SENSE ),
 
+/* Reported by matt <vickm78@hotmail.com> */
+UNUSUAL_DEV(  0x0bc2, 0x3332, 0x0012, 0x0012,
+		"Seagate",
+		"External",
+		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+		US_FL_NO_SYNCHRONIZE_CACHE),
+
 UNUSUAL_DEV(  0x0d49, 0x7310, 0x0000, 0x9999,
 		"Maxtor",
 		"USB to SATA",

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in

Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[James Bottomley]

On Mon, 2015-06-22 at 10:55 -0400, Alan Stern wrote:
> Some USB mass-storage devices claim to have a write-back cache but
> don't support the SYNCHRONIZE CACHE command, which means there is no
> way to tell these devices to flush their caches out to permanent
> storage.  Unfortunately, there is nothing we can do about this.
> 
> Until recently this deficiency did not cause any noticeable problems.
> But following commit 89fb4cd1f717 ("scsi: handle flush errors
> properly"), the errors are propagated to userspace where they get
> reported as failures.
> 
> As a workaround, this patch adds a quirks flag for these devices to
> the usb-storage driver, and a corresponding SCSI device flag, to
> indicate the device can't handle SYNCHRONIZE CACHE.  If the flag is
> set, the sd driver will override the cache settings reported by the
> device, so that the device appears to be write-through.  This will
> prevent the unsupported command from being sent.
> 
> This addresses Bugzilla #89511.

I'm not sure I entirely like this:  we are back again treating data
corruption problems silently.

However, I also believe treating a single flush failure as a critical
filesystem error is also wrong:  The data's all there correctly; all it
does is introduce a potential window were the FS could get corrupted in
the unlikely event the system crashed.

Obviously, for a disk with a writeback cache that can't do flush, that
window is much wider and the real solution should be to try to switch
the cache to write through.
How about something like this patch?  It transforms FS FLUSH into a log
warning from an error but preserves the error on any other path.  You'll
still get a fairly continuous dump of warnings for one of these devices,
though ... do they respond to mode selects turning off the writeback?

James

---

diff --git a/block/blk-flush.c b/block/blk-flush.c
index 20badd7..bb9f9f3 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -173,10 +173,21 @@ static bool blk_flush_complete_seq(struct request *rq,
 	BUG_ON(rq->flush.seq & seq);
 	rq->flush.seq |= seq;
 
-	if (likely(!error))
+	if (likely(!error)) {
 		seq = blk_flush_cur_seq(rq);
-	else
+	} else {
 		seq = REQ_FSEQ_DONE;
+		printk_ratelimited(KERN_ERR "%s: flush failed: data integrity problem\n",
+				   rq->rq_disk ? rq->rq_disk->disk_name : "?");
+		/*
+		 * returning an error to the FS is wrong: the data is all
+		 * there, it just might not be written out in the expected
+		 * order and thus have a window where the integrity is suspect
+		 * in a crash.  Given the small likelihood of actually
+		 * crashing, we should just log a warning here.
+		 */
+		error = 0;
+	}
 
 	switch (seq) {
 	case REQ_FSEQ_PREFLUSH:


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in

Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[Alan Stern]

On Mon, 22 Jun 2015, James Bottomley wrote:

> I'm not sure I entirely like this:  we are back again treating data
> corruption problems silently.
> 
> However, I also believe treating a single flush failure as a critical
> filesystem error is also wrong:  The data's all there correctly; all it
> does is introduce a potential window were the FS could get corrupted in
> the unlikely event the system crashed.
> 
> Obviously, for a disk with a writeback cache that can't do flush, that
> window is much wider and the real solution should be to try to switch
> the cache to write through.

I agree.  Doing the switch manually (by writing to the "cache_type" 
attribute file) works, but it's a nuisance to do this when you have a 
portable USB drive that gets moved among a bunch of machines.

> How about something like this patch?  It transforms FS FLUSH into a log
> warning from an error but preserves the error on any other path.  You'll
> still get a fairly continuous dump of warnings for one of these devices,
> though ... do they respond to mode selects turning off the writeback?

I would be very surprised if any of those drives support MODE SELECT at 
all.

Maybe your patch will be acceptable, though.  We'll have to hear from 
Markus and Matt.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in

Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[James Bottomley]

On Mon, 2015-06-22 at 13:30 -0400, Alan Stern wrote:
> On Mon, 22 Jun 2015, James Bottomley wrote:
> 
> > I'm not sure I entirely like this:  we are back again treating data
> > corruption problems silently.
> > 
> > However, I also believe treating a single flush failure as a critical
> > filesystem error is also wrong:  The data's all there correctly; all it
> > does is introduce a potential window were the FS could get corrupted in
> > the unlikely event the system crashed.
> > 
> > Obviously, for a disk with a writeback cache that can't do flush, that
> > window is much wider and the real solution should be to try to switch
> > the cache to write through.
> 
> I agree.  Doing the switch manually (by writing to the "cache_type" 
> attribute file) works, but it's a nuisance to do this when you have a 
> portable USB drive that gets moved among a bunch of machines.

Perhaps it might be wise to do this to every USB device ... for external
devices, the small performance gain doesn't really make up for the
potential data loss.

> > How about something like this patch?  It transforms FS FLUSH into a log
> > warning from an error but preserves the error on any other path.  You'll
> > still get a fairly continuous dump of warnings for one of these devices,
> > though ... do they respond to mode selects turning off the writeback?
> 
> I would be very surprised if any of those drives support MODE SELECT at 
> all.

I assume the cache type attribute file you refer to above is just
pretending their cache is write through rather than actually setting it
to be so?  The original IDE device had no way of turning their cache
types to write through either, but the manufacturers were eventually
convinced of the error of their ways.

> Maybe your patch will be acceptable, though.  We'll have to hear from 
> Markus and Matt.

We'll probably have to take this to fsdevel as well ... they might have
opinions about whether the FS wants to be informed about flush failure.

James

> Alan Stern
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> 



--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in

Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[Alan Stern]

On Mon, 22 Jun 2015, James Bottomley wrote:

> On Mon, 2015-06-22 at 13:30 -0400, Alan Stern wrote:
> > On Mon, 22 Jun 2015, James Bottomley wrote:
> > 
> > > I'm not sure I entirely like this:  we are back again treating data
> > > corruption problems silently.
> > > 
> > > However, I also believe treating a single flush failure as a critical
> > > filesystem error is also wrong:  The data's all there correctly; all it
> > > does is introduce a potential window were the FS could get corrupted in
> > > the unlikely event the system crashed.
> > > 
> > > Obviously, for a disk with a writeback cache that can't do flush, that
> > > window is much wider and the real solution should be to try to switch
> > > the cache to write through.
> > 
> > I agree.  Doing the switch manually (by writing to the "cache_type" 
> > attribute file) works, but it's a nuisance to do this when you have a 
> > portable USB drive that gets moved among a bunch of machines.
> 
> Perhaps it might be wise to do this to every USB device ... for external
> devices, the small performance gain doesn't really make up for the
> potential data loss.
> 
> > > How about something like this patch?  It transforms FS FLUSH into a log
> > > warning from an error but preserves the error on any other path.  You'll
> > > still get a fairly continuous dump of warnings for one of these devices,
> > > though ... do they respond to mode selects turning off the writeback?
> > 
> > I would be very surprised if any of those drives support MODE SELECT at 
> > all.
> 
> I assume the cache type attribute file you refer to above is just
> pretending their cache is write through rather than actually setting it
> to be so?

Yes; I'm referring to cache_type_store() in sd.c, and writing
"temporary write through", which does not issue a MODE SELECT command.  
It would be easy enough for people to try leaving out the "temporary", 
but I don't expect it to work.

>  The original IDE device had no way of turning their cache
> types to write through either, but the manufacturers were eventually
> convinced of the error of their ways.

In this case the stupidity resides in the USB-ATA bridge.  You can see 
the gory details at

	https://bugzilla.kernel.org/show_bug.cgi?id=89511#c19

> > Maybe your patch will be acceptable, though.  We'll have to hear from 
> > Markus and Matt.
> 
> We'll probably have to take this to fsdevel as well ... they might have
> opinions about whether the FS wants to be informed about flush failure.

Okay.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in

Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[Markus Rathgeb]

>> > Maybe your patch will be acceptable, though.  We'll have to hear from
>> > Markus and Matt.
>>
>> We'll probably have to take this to fsdevel as well ... they might have
>> opinions about whether the FS wants to be informed about flush failure.

So, it is okay to wait for the end of that discussion before I start
further testing or should I test the patch above?
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in

Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[Alan Stern]

On Mon, 22 Jun 2015, Markus Rathgeb wrote:

> >> > Maybe your patch will be acceptable, though.  We'll have to hear from
> >> > Markus and Matt.
> >>
> >> We'll probably have to take this to fsdevel as well ... they might have
> >> opinions about whether the FS wants to be informed about flush failure.
> 
> So, it is okay to wait for the end of that discussion before I start
> further testing or should I test the patch above?

Please test it now.  I'd like to know if it fixes your problem, 
regardless of how the discussion goes.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in

Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[James Bottomley]

On Mon, 2015-06-22 at 13:48 -0400, Alan Stern wrote:
> On Mon, 22 Jun 2015, James Bottomley wrote:
> 
> > On Mon, 2015-06-22 at 13:30 -0400, Alan Stern wrote:
> > > On Mon, 22 Jun 2015, James Bottomley wrote:
> > > 
> > > > I'm not sure I entirely like this:  we are back again treating data
> > > > corruption problems silently.
> > > > 
> > > > However, I also believe treating a single flush failure as a critical
> > > > filesystem error is also wrong:  The data's all there correctly; all it
> > > > does is introduce a potential window were the FS could get corrupted in
> > > > the unlikely event the system crashed.
> > > > 
> > > > Obviously, for a disk with a writeback cache that can't do flush, that
> > > > window is much wider and the real solution should be to try to switch
> > > > the cache to write through.
> > > 
> > > I agree.  Doing the switch manually (by writing to the "cache_type" 
> > > attribute file) works, but it's a nuisance to do this when you have a 
> > > portable USB drive that gets moved among a bunch of machines.
> > 
> > Perhaps it might be wise to do this to every USB device ... for external
> > devices, the small performance gain doesn't really make up for the
> > potential data loss.
> > 
> > > > How about something like this patch?  It transforms FS FLUSH into a log
> > > > warning from an error but preserves the error on any other path.  You'll
> > > > still get a fairly continuous dump of warnings for one of these devices,
> > > > though ... do they respond to mode selects turning off the writeback?
> > > 
> > > I would be very surprised if any of those drives support MODE SELECT at 
> > > all.
> > 
> > I assume the cache type attribute file you refer to above is just
> > pretending their cache is write through rather than actually setting it
> > to be so?
> 
> Yes; I'm referring to cache_type_store() in sd.c, and writing
> "temporary write through", which does not issue a MODE SELECT command.  
> It would be easy enough for people to try leaving out the "temporary", 
> but I don't expect it to work.
> 
> >  The original IDE device had no way of turning their cache
> > types to write through either, but the manufacturers were eventually
> > convinced of the error of their ways.
> 
> In this case the stupidity resides in the USB-ATA bridge.  You can see 
> the gory details at
> 
> 	https://bugzilla.kernel.org/show_bug.cgi?id=89511#c19

OK, so that says the SAT in the bridge doesn't know what to do with
MODE_SELECT (probably unsurprising given that it's a usb bridge).  the
SATA disk should respond to the ATA command SET FEATURES, though.
Presuming we can get it through the bridge.

You can try it with

hdparm -W 0 <dev>

optionally with --prefer_ata_12 to do the 12 instead of 16 byte
encapsulation and see if that makes a difference.

James



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in

Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[Markus Rathgeb]

> Please test it now.  I'd like to know if it fixes your problem,
> regardless of how the discussion goes.

Seems to be working, I also attached the kernel log.
The "flush failed" will grow with disc activity (but that was to be expected).

Jun 22 23:24:50 m3800 kernel: usb 1-3: new high-speed USB device
number 6 using xhci_hcd
Jun 22 23:24:51 m3800 kernel: usb 1-3: New USB device found,
idVendor=04cf, idProduct=8818
Jun 22 23:24:51 m3800 kernel: usb 1-3: New USB device strings: Mfr=1,
Product=2, SerialNumber=3
Jun 22 23:24:51 m3800 kernel: usb 1-3: Product: USB Mass Storage Device
Jun 22 23:24:51 m3800 kernel: usb 1-3: Manufacturer: Myson Century, Inc.
Jun 22 23:24:51 m3800 kernel: usb 1-3: SerialNumber: 100
Jun 22 23:24:51 m3800 kernel: usb-storage 1-3:1.0: USB Mass Storage
device detected
Jun 22 23:24:51 m3800 kernel: scsi host6: usb-storage 1-3:1.0
Jun 22 23:24:51 m3800 kernel: usbcore: registered new interface driver
usb-storage
Jun 22 23:24:52 m3800 kernel: scsi 6:0:0:0: Direct-Access     SAMSUNG
MP0804H          UE10 PQ: 0 ANSI: 0 CCS
Jun 22 23:24:52 m3800 kernel: sd 6:0:0:0: Attached scsi generic sg1 type 0
Jun 22 23:24:52 m3800 kernel: sd 6:0:0:0: [sdb] 156368016 512-byte
logical blocks: (80.0 GB/74.5 GiB)
Jun 22 23:24:52 m3800 kernel: sd 6:0:0:0: [sdb] Write Protect is off
Jun 22 23:24:52 m3800 kernel: sd 6:0:0:0: [sdb] Mode Sense: 00 14 00 00
Jun 22 23:24:52 m3800 kernel: sd 6:0:0:0: [sdb] Write cache: enabled,
read cache: enabled, doesn't support DPO or FUA
Jun 22 23:24:52 m3800 kernel:  sdb: sdb1 sdb2
Jun 22 23:24:52 m3800 kernel: sd 6:0:0:0: [sdb] Attached SCSI disk
Jun 22 23:24:52 m3800 kernel: BTRFS: device label USB_BILDER devid 1
transid 9 /dev/sdb2
Jun 22 23:24:52 m3800 kernel: BTRFS info (device sdb2): disk space
caching is enabled
Jun 22 23:24:52 m3800 kernel: BTRFS: has skinny extents
Jun 22 23:24:52 m3800 kernel: FAT-fs (sdb1): Volume was not properly
unmounted. Some data may be corrupt. Please run fsck.
Jun 22 23:25:28 m3800 kernel: sdb: flush failed: data integrity problem
Jun 22 23:25:28 m3800 kernel: sdb: flush failed: data integrity problem
Jun 22 23:26:55 m3800 kernel: sdb: flush failed: data integrity problem
Jun 22 23:26:55 m3800 kernel: sdb: flush failed: data integrity problem
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in

RE: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1356 bytes --]

From: Of James Bottomley
> Sent: 22 June 2015 18:36
> To: Alan Stern
...
> > > Obviously, for a disk with a writeback cache that can't do flush, that
> > > window is much wider and the real solution should be to try to switch
> > > the cache to write through.
> >
> > I agree.  Doing the switch manually (by writing to the "cache_type"
> > attribute file) works, but it's a nuisance to do this when you have a
> > portable USB drive that gets moved among a bunch of machines.
> 
> Perhaps it might be wise to do this to every USB device ... for external
> devices, the small performance gain doesn't really make up for the
> potential data loss.

What about systems running from USB memory/disk directly plugged into the
motherboard and shut inside the case?

Since they shouldn't be removed you don't want to bypass any write cache.
(Any more than you'd want to on a real disk.)

There is an additional problem caused by very temporary USB disconnects
(easily caused by electrical noise causing (probably) Vbus to bounce).
In these conditions you don't want to signal a USB remove at all - at
least not to the filesystem - until the device has remained disconnected
for a short time.

	David

N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±ºÆâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br	šê+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹\x1e®w¥¢¸?™¨è­Ú&¢)ߢ^[f

Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[Stefan Richter]

On Jun 22 James Bottomley wrote:
> On Mon, 2015-06-22 at 13:30 -0400, Alan Stern wrote:
> > On Mon, 22 Jun 2015, James Bottomley wrote:
> > > Obviously, for a disk with a writeback cache that can't do flush, that
> > > window is much wider and the real solution should be to try to switch
> > > the cache to write through.
> > 
> > I agree.  Doing the switch manually (by writing to the "cache_type" 
> > attribute file) works, but it's a nuisance to do this when you have a 
> > portable USB drive that gets moved among a bunch of machines.
> 
> Perhaps it might be wise to do this to every USB device ... for external
> devices, the small performance gain doesn't really make up for the
> potential data loss.

Just a small note on the assumption of externally (and in extension,
temporarily) attached devices:  Not all USB-attached devices are external,
and not all external devices are used as removable devices.

(For example, I am using 2 internal CD-ROMs via USB-2 + usb-storage and 2
internal HDDs via USB 3 + uas in a PC with too few SATA ports and no PCIe
slot to spare for a controller add-on card, but plenty of USB headers
available on the mainboard.  Similarly, some NASes have their operating
system located on a USB-attached device.  Small offices use USB-attached
disks for backup and won't detach such a disk until rotation for off-site
deposit.  Not to mention embedded computers with USB-attached but fixed
disks.)
-- 
Stefan Richter
-=====-===== -==- ==-=-
http://arcgraph.de/sr/
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[James Bottomley]

On Fri, 2015-06-26 at 11:43 +0200, Stefan Richter wrote:
> On Jun 22 James Bottomley wrote:
> > On Mon, 2015-06-22 at 13:30 -0400, Alan Stern wrote:
> > > On Mon, 22 Jun 2015, James Bottomley wrote:
> > > > Obviously, for a disk with a writeback cache that can't do flush, that
> > > > window is much wider and the real solution should be to try to switch
> > > > the cache to write through.
> > > 
> > > I agree.  Doing the switch manually (by writing to the "cache_type" 
> > > attribute file) works, but it's a nuisance to do this when you have a 
> > > portable USB drive that gets moved among a bunch of machines.
> > 
> > Perhaps it might be wise to do this to every USB device ... for external
> > devices, the small performance gain doesn't really make up for the
> > potential data loss.
> 
> Just a small note on the assumption of externally (and in extension,
> temporarily) attached devices:  Not all USB-attached devices are external,
> and not all external devices are used as removable devices.

The problems don't depend on the connection type: internal devices which
have a writeback cache and don't accept flush commands have data
integrity problems too.  I can't really think of many situations where
you'd be willing to sacrifice data integrity for performance.

James

> (For example, I am using 2 internal CD-ROMs via USB-2 + usb-storage and 2
> internal HDDs via USB 3 + uas in a PC with too few SATA ports and no PCIe
> slot to spare for a controller add-on card, but plenty of USB headers
> available on the mainboard.  Similarly, some NASes have their operating
> system located on a USB-attached device.  Small offices use USB-attached
> disks for backup and won't detach such a disk until rotation for off-site
> deposit.  Not to mention embedded computers with USB-attached but fixed
> disks.)



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[Stefan Richter]

On Jun 26 James Bottomley wrote:
> On Fri, 2015-06-26 at 11:43 +0200, Stefan Richter wrote:
> > On Jun 22 James Bottomley wrote:
[...]
> > > Perhaps it might be wise to do this to every USB device ... for external
> > > devices, the small performance gain doesn't really make up for the
> > > potential data loss.
> > 
> > Just a small note on the assumption of externally (and in extension,
> > temporarily) attached devices:  Not all USB-attached devices are external,
> > and not all external devices are used as removable devices.
> 
> The problems don't depend on the connection type: internal devices which
> have a writeback cache and don't accept flush commands have data
> integrity problems too.  I can't really think of many situations where
> you'd be willing to sacrifice data integrity for performance.

Sure; writeback caches are prone to more issues besides sudden connection
loss.  (E.g. sudden power loss is but one of several more potential issues
of course.)

I merely wanted to remind that the bus type of a device (USB or whatever)
does not say much about the risk of sudden connection loss to that device,
since such devices may have physical protection against sudden connection
loss too.  I was not particularly commenting on the topic at hand, i.e.
on presence of writeback cache without working flush command.
-- 
Stefan Richter
-=====-===== -==- ==-=-
http://arcgraph.de/sr/

Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[Alan Stern]

On Mon, 22 Jun 2015, James Bottomley wrote:

> On Mon, 2015-06-22 at 13:48 -0400, Alan Stern wrote:
> > On Mon, 22 Jun 2015, James Bottomley wrote:
> > 
> > > On Mon, 2015-06-22 at 13:30 -0400, Alan Stern wrote:
> > > > On Mon, 22 Jun 2015, James Bottomley wrote:
> > > > 
> > > > > I'm not sure I entirely like this:  we are back again treating data
> > > > > corruption problems silently.
> > > > > 
> > > > > However, I also believe treating a single flush failure as a critical
> > > > > filesystem error is also wrong:  The data's all there correctly; all it
> > > > > does is introduce a potential window were the FS could get corrupted in
> > > > > the unlikely event the system crashed.
> > > > > 
> > > > > Obviously, for a disk with a writeback cache that can't do flush, that
> > > > > window is much wider and the real solution should be to try to switch
> > > > > the cache to write through.
> > > > 
> > > > I agree.  Doing the switch manually (by writing to the "cache_type" 
> > > > attribute file) works, but it's a nuisance to do this when you have a 
> > > > portable USB drive that gets moved among a bunch of machines.
> > > 
> > > Perhaps it might be wise to do this to every USB device ... for external
> > > devices, the small performance gain doesn't really make up for the
> > > potential data loss.
> > > 
> > > > > How about something like this patch?  It transforms FS FLUSH into a log
> > > > > warning from an error but preserves the error on any other path.  You'll
> > > > > still get a fairly continuous dump of warnings for one of these devices,
> > > > > though ... do they respond to mode selects turning off the writeback?
> > > > 
> > > > I would be very surprised if any of those drives support MODE SELECT at 
> > > > all.
> > > 
> > > I assume the cache type attribute file you refer to above is just
> > > pretending their cache is write through rather than actually setting it
> > > to be so?
> > 
> > Yes; I'm referring to cache_type_store() in sd.c, and writing
> > "temporary write through", which does not issue a MODE SELECT command.  
> > It would be easy enough for people to try leaving out the "temporary", 
> > but I don't expect it to work.
> > 
> > >  The original IDE device had no way of turning their cache
> > > types to write through either, but the manufacturers were eventually
> > > convinced of the error of their ways.
> > 
> > In this case the stupidity resides in the USB-ATA bridge.  You can see 
> > the gory details at
> > 
> > 	https://bugzilla.kernel.org/show_bug.cgi?id=89511#c19
> 
> OK, so that says the SAT in the bridge doesn't know what to do with
> MODE_SELECT (probably unsurprising given that it's a usb bridge).  the
> SATA disk should respond to the ATA command SET FEATURES, though.
> Presuming we can get it through the bridge.
> 
> You can try it with
> 
> hdparm -W 0 <dev>
> 
> optionally with --prefer_ata_12 to do the 12 instead of 16 byte
> encapsulation and see if that makes a difference.

Dan Williams recently posted the content below to the Bugzilla report:

> I have a drive with this problem and I tested 4.0.8 with this hunk 
> added to the patch above.
> 
> UNUSUAL_DEV(  0x0bc2, 0x0888, 0x0000, 0x0000,
>               "Seagate",
>               "USB 2.0 Pocket Hard Drive",
>               USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>               US_FL_NO_SYNCHRONIZE_CACHE),
> 
> I don't have the linux-usb thread around so I can reply to 
> http://www.spinics.net/lists/linux-usb/msg126364.html, but here's the 
> result of:
> 
> [dcbw@localhost ~]$ sudo hdparm -W 0 --prefer-ata12 /dev/sdb
> 
> /dev/sdb:
>  setting drive write-caching to 0 (off)
> SG_IO: bad/missing sense data, sb[]:  70 00 05 00 00 00 00 0a 00 00 00 
> 00 20 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

In English: "Illegal Request: Invalid command operation code".  
Apparently there's no way to tell the drive to change its caching.

> SG_IO: bad/missing sense data, sb[]:  70 00 05 00 00 00 00 0a 00 00 00 
> 00 20 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SG_IO: bad/missing sense data, sb[]:  70 00 05 00 00 00 00 0a 00 00 00 
> 00 20 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SG_IO: bad/missing sense data, sb[]:  70 00 05 00 00 00 00 0a 00 00 00 
> 00 20 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SG_IO: bad/missing sense data, sb[]:  70 00 05 00 00 00 00 0a 00 00 00 
> 00 20 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  write-caching = not supported
> 
> What was the upstream resolution on this one?

So far there isn't any.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[Alan Stern]

This is an old problem, but it was never resolved and it still affects
people (Bugzilla #89511).  In short, there are USB-(S)ATA bridges that
claim to be write-back but don't support the SYNCHRONIZE CACHE command.  
This causes errors when filesystems try to flush data out to the disk.

On Mon, 22 Jun 2015, James Bottomley wrote:

> On Mon, 2015-06-22 at 13:48 -0400, Alan Stern wrote:
> > On Mon, 22 Jun 2015, James Bottomley wrote:
> > 
> > > On Mon, 2015-06-22 at 13:30 -0400, Alan Stern wrote:
> > > > On Mon, 22 Jun 2015, James Bottomley wrote:
> > > > 
> > > > > I'm not sure I entirely like this:  we are back again treating data
> > > > > corruption problems silently.
> > > > > 
> > > > > However, I also believe treating a single flush failure as a critical
> > > > > filesystem error is also wrong:  The data's all there correctly; all it
> > > > > does is introduce a potential window were the FS could get corrupted in
> > > > > the unlikely event the system crashed.
> > > > > 
> > > > > Obviously, for a disk with a writeback cache that can't do flush, that
> > > > > window is much wider and the real solution should be to try to switch
> > > > > the cache to write through.
> > > > 
> > > > I agree.  Doing the switch manually (by writing to the "cache_type" 
> > > > attribute file) works, but it's a nuisance to do this when you have a 
> > > > portable USB drive that gets moved among a bunch of machines.
> > > 
> > > Perhaps it might be wise to do this to every USB device ... for external
> > > devices, the small performance gain doesn't really make up for the
> > > potential data loss.
> > > 
> > > > > How about something like this patch?  It transforms FS FLUSH into a log
> > > > > warning from an error but preserves the error on any other path.  You'll
> > > > > still get a fairly continuous dump of warnings for one of these devices,
> > > > > though ... do they respond to mode selects turning off the writeback?
> > > > 
> > > > I would be very surprised if any of those drives support MODE SELECT at 
> > > > all.
> > > 
> > > I assume the cache type attribute file you refer to above is just
> > > pretending their cache is write through rather than actually setting it
> > > to be so?
> > 
> > Yes; I'm referring to cache_type_store() in sd.c, and writing
> > "temporary write through", which does not issue a MODE SELECT command.  
> > It would be easy enough for people to try leaving out the "temporary", 
> > but I don't expect it to work.
> > 
> > >  The original IDE device had no way of turning their cache
> > > types to write through either, but the manufacturers were eventually
> > > convinced of the error of their ways.
> > 
> > In this case the stupidity resides in the USB-ATA bridge.  You can see 
> > the gory details at
> > 
> > 	https://bugzilla.kernel.org/show_bug.cgi?id=89511#c19
> 
> OK, so that says the SAT in the bridge doesn't know what to do with
> MODE_SELECT (probably unsurprising given that it's a usb bridge).  the
> SATA disk should respond to the ATA command SET FEATURES, though.
> Presuming we can get it through the bridge.
> 
> You can try it with
> 
> hdparm -W 0 <dev>
> 
> optionally with --prefer_ata_12 to do the 12 instead of 16 byte
> encapsulation and see if that makes a difference.

As reported by Dan Williams, the SET FEATURES command results in an 
"Illegal Request: Invalid command operation code" error response.

So the question remains, what should we do about these things?  We can
have a blacklist flag that overrides the cache setting, as in my
original patch.  We can leave the setting alone and change the errors
to warnings, as in James's patch earlier in this thread (but that will
generate a lot of warnings).

James, what do you think?

Alan Stern

Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[Oliver Neukum]

On Thu, 2015-12-03 at 13:36 -0500, Alan Stern wrote:
> This is an old problem, but it was never resolved and it still affects
> people (Bugzilla #89511).  In short, there are USB-(S)ATA bridges that
> claim to be write-back but don't support the SYNCHRONIZE CACHE
> command.  
> This causes errors when filesystems try to flush data out to the disk.

OK, maybe this is a stupid idea, but could we test the command as we
enumerate the slave and store the result?

	Regards
		Oliver

Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk

[Alan Stern]

On Tue, 15 Dec 2015, Oliver Neukum wrote:

> On Thu, 2015-12-03 at 13:36 -0500, Alan Stern wrote:
> > This is an old problem, but it was never resolved and it still affects
> > people (Bugzilla #89511).  In short, there are USB-(S)ATA bridges that
> > claim to be write-back but don't support the SYNCHRONIZE CACHE
> > command.  
> > This causes errors when filesystems try to flush data out to the disk.
> 
> OK, maybe this is a stupid idea, but could we test the command as we
> enumerate the slave and store the result?

Maybe, although doing so within usb-storage would be kind of difficult
-- that driver is set up to forward requests from the SCSI layer, not
to generate requests of its own.  Besides, usb-storage doesn't know 
anything about write-back vs. write-through; only sd does.  I suppose 
sd could perform that test.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html