From patchwork Mon Aug 15 13:08:29 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mike Snitzer X-Patchwork-Id: 9280951 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 2D5526086A for ; Mon, 15 Aug 2016 13:10:02 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1EC0728C41 for ; Mon, 15 Aug 2016 13:10:02 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 1333A28C7F; Mon, 15 Aug 2016 13:10:02 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 95FE828C7C for ; Mon, 15 Aug 2016 13:10:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752915AbcHONJ5 (ORCPT ); Mon, 15 Aug 2016 09:09:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34098 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752925AbcHONIb (ORCPT ); Mon, 15 Aug 2016 09:08:31 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1BC2B85540; Mon, 15 Aug 2016 13:08:31 +0000 (UTC) Received: from localhost (dhcp-25-149.bos.redhat.com [10.18.25.149]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u7FD8TeG007273 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA256 bits=256 verify=NO); Mon, 15 Aug 2016 09:08:30 -0400 Date: Mon, 15 Aug 2016 09:08:29 -0400 From: Mike Snitzer To: Christoph Hellwig Cc: dm-devel@redhat.com, linux-scsi@vger.kernel.org, hare@suse.de Subject: Re: dm-mpath: always return reservation conflict Message-ID: <20160815130829.GA14829@redhat.com> References: <1470141392-25479-1-git-send-email-hch@lst.de> <1470141392-25479-2-git-send-email-hch@lst.de> <20160811183832.GA27144@lst.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20160811183832.GA27144@lst.de> User-Agent: Mutt/1.5.21 (2010-09-15) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Mon, 15 Aug 2016 13:08:31 +0000 (UTC) Sender: linux-scsi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-scsi@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Not sure how Hannes' original patch was overlooked but... One issue I see with the patch is it will return -EBADE regardless of whether 'queue_if_no_path' is set. That's fine (since path isn't being failed for this case any more). But why not just return error immediately? But taking a step back, shouldn't all paths be tried once before returning an error? Obviously that'd impose the use of a new 'conflict_seen' (or whatever) flag at the end of 'struct pgpath'. And then only return error if the flag is set. I threw together the following RFC patch to illustrate what I'm thinking, but thinking about this further it is tough to know all paths have seen the reservation conflict (my patch assumes if 'conflict_seen' is set then the conflict iterated through all paths.. but if paths aren't being failed there isn't a guarantee that the path selector didn't just hand us back the same path that just experienced the conflict). So this is throw-away for now (and I'll get Hannes' patch applied for 4.8-rc3, with the tweak of returning -EBADE immediately): --- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c index ac734e5..c3d92db 100644 --- a/drivers/md/dm-mpath.c +++ b/drivers/md/dm-mpath.c @@ -41,6 +41,7 @@ struct pgpath { struct delayed_work activate_path; bool is_active:1; /* Path status */ + bool conflict_seen:1; }; #define path_to_pgpath(__pgp) container_of((__pgp), struct pgpath, path) @@ -1569,6 +1570,33 @@ static int do_end_io(struct multipath *m, struct request *clone, if (noretry_error(error)) return error; + if (error == -EBADE) { + /* + * EBADE signals a reservation conflict. + * We shouldn't fail the path here as we can communicate with + * the target. We should failover to the next path, but in + * doing so we might be causing a ping-pong between paths. + * Avoid this by only returning the reservation conflict error + * if a conflict has been seen on all paths. + */ + if (!mpio->pgpath || mpio->pgpath->conflict_seen) { + struct priority_group *pg; + struct pgpath *p; + + /* clear 'conflict_seen' for all pgpaths */ + list_for_each_entry(pg, &m->priority_groups, list) { + list_for_each_entry(p, &pg->pgpaths, list) { + p->conflict_seen = false; + } + } + return error; + } + else if (mpio->pgpath) { + mpio->pgpath->conflict_seen = true; + return r; + } + } + if (mpio->pgpath) fail_path(mpio->pgpath); @@ -1576,9 +1604,6 @@ static int do_end_io(struct multipath *m, struct request *clone, if (!test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) { if (!must_push_back_rq(m)) r = -EIO; - } else { - if (error == -EBADE) - r = error; } }