From patchwork Tue May 10 14:48:06 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jinpu Wang X-Patchwork-Id: 9058611 Return-Path: X-Original-To: patchwork-linux-scsi@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 8A99F9F1D3 for ; Tue, 10 May 2016 14:48:31 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 4D54020154 for ; Tue, 10 May 2016 14:48:30 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A9DAF20148 for ; Tue, 10 May 2016 14:48:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751981AbcEJOs2 (ORCPT ); Tue, 10 May 2016 10:48:28 -0400 Received: from mail-oi0-f51.google.com ([209.85.218.51]:33465 "EHLO mail-oi0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751069AbcEJOs0 (ORCPT ); Tue, 10 May 2016 10:48:26 -0400 Received: by mail-oi0-f51.google.com with SMTP id v145so19120644oie.0 for ; Tue, 10 May 2016 07:48:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=profitbricks-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=SC6knqdzHju8tFWKukC3EKLtTA/BDYfNeU0sQhi3liU=; b=Mtla5WhNeCPUbKiRL/oRIU6Qr4DqcoY7vEN+lpQny0NKDA8bDgFHfm1L4OKjYePife mt18ZDrFEdlrUKYWjcG+5biz60/ZkoHsC9bjyXwFOU2e7Q4caaSHBqFMBQMS+VA5sgOb FkHxYSSIp2a56qXNA7HuUUoyvjFE6SkXMNLV6u68U0B/2SuHBV1wz5woAVa0a4XbFGNg VHz5zQkHuOUqp1gr+jwEYYv7ROKy1m4uGJmYa9oY5Z4jtBF8Qan2/ArxCgILUmE+wkXc fge4PN4N0z8hCyK0nD8WTm3nlWyrgTDtZJXqDsxDJoa1HgZ4kWK9pKLbXkCRnjVDqGA2 m3Ow== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=SC6knqdzHju8tFWKukC3EKLtTA/BDYfNeU0sQhi3liU=; b=GHfwUX8/XhdZ4IaMzB7Tcbt6DPUQMqjeoXTbeeNaIUOnzaZY94oSDevEAuT+qmamYd HeJCSSYeLUAa0JxF6FHhyLhAllCYC//JVhcBKddIrDyz+pBwdxBJbhyPoTWLnjlJiqF9 1CbDgcArreEXwXb2xuZzvdwG9vMqDQN1/YXewOhJ6OnKsKFgE4GmDtpWO/RGXacO+YI5 5V27a41jTZIeC9MTXhL0lbgIbB45rMbtXeGGE/tyrsbJWcAgvU0AtCbdjdgat08DyQIQ dJybr7dFmu1hujJfHuBm0mW2qzHHPfdDaNRhh2HjIumUkYPdKgEgtcc1KOQQDv0fkWGk G9tA== X-Gm-Message-State: AOPr4FXdSxt9Iv0x+Egiecd3rfygvMhfHqYiuLg5eSnahCn+gTiMZg99nyZvgNK7MlY7V31aQSbJuM/zxDlLPwts X-Received: by 10.202.66.5 with SMTP id p5mr17945699oia.65.1462891705611; Tue, 10 May 2016 07:48:25 -0700 (PDT) MIME-Version: 1.0 Received: by 10.182.227.5 with HTTP; Tue, 10 May 2016 07:48:06 -0700 (PDT) In-Reply-To: References: <5727266E.7040905@suse.de> <1462196672.2665.5.camel@linux.vnet.ibm.com> From: Jinpu Wang Date: Tue, 10 May 2016 16:48:06 +0200 Message-ID: Subject: Re: [PATCHv2]sd: Don't treat succeeded SYNC as error To: James Bottomley Cc: Hannes Reinecke , "Martin K. Petersen" , Christoph Hellwig , linux-scsi@vger.kernel.org, Sebastian Parschauer , Bart Van Assche 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.9 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,T_DKIM_INVALID,T_TVD_MIME_EPI, 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, May 9, 2016 at 6:41 PM, Jinpu Wang wrote: > On Wed, May 4, 2016 at 7:02 PM, Jinpu Wang wrote: >> On Mon, May 2, 2016 at 3:44 PM, James Bottomley wrote: >>> On Mon, 2016-05-02 at 12:05 +0200, Hannes Reinecke wrote: >>>> On 04/29/2016 02:49 PM, Jinpu Wang wrote: >>>> > Hi, all >>>> > >>>> > We hit IO error on fsync, it turns out was because sd treat >>>> > succeeded >>>> > SYNC as error. From what I checked in SBC spec there is no >>>> > indication >>>> > we should fail IO in this case, so we create this patch. >>>> > >>>> > >>>> > Best Regards, >>>> > >>>> > Jack Wang >>>> > >>>> > v2: >>>> > No change on patch itself, only resend in body as suggested by >>>> > Bart, >>>> > still keep the attachment in case mail client break the format. >>>> > >>>> > From 5d1f72d9643ce61cd9f3d312377378c43f171d0c Mon Sep 17 00:00:00 >>>> > 2001 >>>> > From: Jack Wang >>>> > Date: Mon, 25 Apr 2016 12:05:22 +0200 >>>> > Subject: [PATCH] sd: Don't treat succeeded SYNC as error >>>> > >>>> > We hit IO error in our production on multipath devices during >>>> > resize >>>> > device on target side, the problem turns out sd driver passes up as >>>> > IO >>>> > error when sense data is UNIT_ATTENTION and ASC && ASCQ indicate >>>> > Capacity data has changed, even storage side sync the data >>>> > properly. >>>> > >>>> > In order to fix this check in sd_done, report success if condition >>>> > matches. >>>> > >>>> > Sebastian Parschauer report/analyze the bug here: >>>> > https://sourceforge.net/p/scst/mailman/message/34953416/ >>>> > >>>> > Signed-off-by: Sebastian Parschauer >>>> > Signed-off-by: Jack Wang >>>> > --- >>>> > drivers/scsi/sd.c | 13 +++++++++++++ >>>> > 1 file changed, 13 insertions(+) >>>> > >>>> Well. >>>> Is there anything which guarantees us that 'capacity data has >>>> changed' will be the only sense code which we'll be seeing as a >>>> response to SYNCHRONIZE CACHE? >>>> I sincerely doubt so. >>>> So why don't you fall back to the default action (ie retry the >>>> command) whenever you hit an UNIT ATTENTION? >>>> This way we would cove any resulting sense code, _and_ would get rid >>>> of the rather ugly special case here. >>> >>> Actually, why are we getting here at all? should we be eating this >>> unit attention once we've reported it in scsi_check_sense()? >>> >>> I also don't quite understand why the normal retry mechanism in >>> scsi_io_completion() (called after drv->done()) isn't handling this. >>> We set retries on a flush command and we give sd_sync_cache three >>> goes. Any one of those should also cause the CC/UA to be ignored. >>> >>> James >>> >>> >> >> Sorry for delay, I agree safer to retry this command. >> I checked the code path, in scsi_io_completion, we call >> __scsi_error_from_host_byte for FLUSH request, >> and we set error to EIO by default, somehow the code report error >> directly to user space without retry. >> [ 647.206270] sd 1:0:0:0: [sdb] tag#0 Send: scmd 0xffff8800b6558480 >> [ 647.206422] sd 1:0:0:0: [sdb] tag#0 CDB: Synchronize Cache(10) 35 >> 00 00 00 00 00 00 00 00 00 >> [ 647.209748] sd 1:0:0:0: Capacity data has changed >> [ 647.209896] sd 1:0:0:0: [sdb] tag#0 Done: SUCCESS Result: >> hostbyte=DID_OK driverbyte=DRIVER_OK >> [ 647.210157] sd 1:0:0:0: [sdb] tag#0 CDB: Synchronize Cache(10) 35 >> 00 00 00 00 00 00 00 00 00 >> [ 647.210419] sd 1:0:0:0: [sdb] tag#0 Sense Key : Unit Attention [current] >> [ 647.210567] sd 1:0:0:0: [sdb] tag#0 Add. Sense: Capacity data has changed >> [ 647.210741] sd 1:0:0:0: [sdb] tag#0 scsi host busy 1 failed 0 >> [ 647.210888] sd 1:0:0:0: Notifying upper driver of completion (result 8000002) >> [ 647.211035] sd 1:0:0:0: [sdb] tag#0 sd_done: completed 0 of 0 bytes >> [ 647.211182] sd 1:0:0:0: [sdb] tag#0 0 sectors total, 0 bytes done, error -5 >> [ 647.211330] blk_update_request: I/O error, dev sdb, sector 0 >> >> Will figure out why retry doesn't work. >> >> Thanks James and Hannes for all your input. >> >> Regards, >> Jack > > Hi James, Hannes and all, > > I find out it's code below which report error directly back to user > space without any retry. > 913 /* > 914 * If we finished all bytes in the request we are done now. > 915 */ > 916 if (!scsi_end_request(req, error, good_bytes, 0)) > 917 return; > > But not sure, what's the best way to fix the behavior to let it retry, > maybe add condition with sense key && asc && ascq direct go to > requeue before line 913? > > Thanks > Jack Hi James , Hannes and all, I created a patch below, I did basic test on my test machines. Please share your comments! Thanks, Jack From 72ab860811e14e37db81fb409abf0fa7e7fe32cb Mon Sep 17 00:00:00 2001 From: Jack Wang Date: Tue, 10 May 2016 10:10:59 +0200 Subject: [PATCH] scsi: requeue command on capacity data has changed We hit IO error in our production on multipath devices during resize device on target side, the problem turns out scsi driver passes up as IO error when sense data is UNIT_ATTENTION and ASC && ASCQ indicate Capacity data has changed, even storage side sync the data properly. To fix this, when condition met, we simply requeue the command. Reported-by: Sebastian Parschauer Signed-off-by: Jack Wang --- drivers/scsi/scsi_lib.c | 5 +++++ 1 file changed, 5 insertions(+) */ From 72ab860811e14e37db81fb409abf0fa7e7fe32cb Mon Sep 17 00:00:00 2001 From: Jack Wang Date: Tue, 10 May 2016 10:10:59 +0200 Subject: [PATCH] scsi: requeue command on capacity data has changed We hit IO error in our production on multipath devices during resize device on target side, the problem turns out scsi driver passes up as IO error when sense data is UNIT_ATTENTION and ASC && ASCQ indicate Capacity data has changed, even storage side sync the data properly. To fix this, when condition met, we simply requeue the command. Reported-by: Sebastian Parschauer Signed-off-by: Jack Wang --- drivers/scsi/scsi_lib.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 8106515..b00310f 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -910,6 +910,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes) error = 0; } + if (sense_valid && (sshdr.sense_key == UNIT_ATTENTION)) { + if ((sshdr.asc == 0x2a && sshdr.ascq == 0x09)) + goto requeue; + } + /* * If we finished all bytes in the request we are done now. */ -- 1.9.1