From patchwork Mon Jun 22 15:43:47 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: James Bottomley X-Patchwork-Id: 6656761 Return-Path: X-Original-To: patchwork-linux-scsi@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 17DC4C05AC for ; Mon, 22 Jun 2015 15:43:54 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 5BC0A204E0 for ; Mon, 22 Jun 2015 15:43:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 7752920395 for ; Mon, 22 Jun 2015 15:43:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754047AbbFVPnv (ORCPT ); Mon, 22 Jun 2015 11:43:51 -0400 Received: from bedivere.hansenpartnership.com ([66.63.167.143]:60592 "EHLO bedivere.hansenpartnership.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752855AbbFVPnt (ORCPT ); Mon, 22 Jun 2015 11:43:49 -0400 Received: from localhost (localhost [127.0.0.1]) by bedivere.hansenpartnership.com (Postfix) with ESMTP id B55418EE0E0; Mon, 22 Jun 2015 08:43:48 -0700 (PDT) Received: from bedivere.hansenpartnership.com ([127.0.0.1]) by localhost (bedivere.hansenpartnership.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id GEGqR9t2XQ6Z; Mon, 22 Jun 2015 08:43:48 -0700 (PDT) Received: from [153.66.254.242] (unknown [184.11.141.41]) by bedivere.hansenpartnership.com (Postfix) with ESMTPSA id 19A738EE086; Mon, 22 Jun 2015 08:43:48 -0700 (PDT) Message-ID: <1434987827.2237.71.camel@HansenPartnership.com> Subject: Re: [PATCH] USB: storage: add "no SYNCHRONIZE CACHE" quirk From: James Bottomley To: Alan Stern Cc: Greg KH , James Bottomley , Jun Itou , Markus Rathgeb , Matt , SCSI development list , USB Storage list , USB list , Jens Axboe Date: Mon, 22 Jun 2015 08:43:47 -0700 In-Reply-To: References: X-Mailer: Evolution 3.12.11 Mime-Version: 1.0 Sender: linux-scsi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-scsi@vger.kernel.org X-Spam-Status: No, score=-8.3 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 --- -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in 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: