From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754256AbbJNPqA (ORCPT ); Wed, 14 Oct 2015 11:46:00 -0400 Received: from bedivere.hansenpartnership.com ([66.63.167.143]:48500 "EHLO bedivere.hansenpartnership.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753574AbbJNPp7 (ORCPT ); Wed, 14 Oct 2015 11:45:59 -0400 Message-ID: <1444837556.2220.43.camel@HansenPartnership.com> Subject: Re: [PATCH 0/3] SCSI: Fix hard lockup in scsi_remove_target() From: James Bottomley To: Johannes Thumshirn Cc: Christoph Hellwig , Hannes Reinecke , linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org Date: Wed, 14 Oct 2015 08:45:56 -0700 In-Reply-To: <1444833547.19542.21.camel@suse.de> References: <1444833036.2220.38.camel@HansenPartnership.com> <1444833547.19542.21.camel@suse.de> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.11 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2015-10-14 at 16:39 +0200, Johannes Thumshirn wrote: > On Wed, 2015-10-14 at 07:30 -0700, James Bottomley wrote: > > On Wed, 2015-10-14 at 15:50 +0200, Johannes Thumshirn wrote: > > > Removing a SCSI target via scsi_remove_target() suspected to be > > > racy. When a > > > sibling get's removed from the list it can occassionly happen that > > > one CPU is > > > stuck endlessly looping around this code block > > > > > > list_for_each_entry(starget, &shost->__targets, siblings) { > > > if (starget->state == STARGET_DEL) > > > continue; > > > > How long is the __targets list? It seems a bit unlikely that this is > > the exact cause, because for a short list all in STARGET_DEL that > > loop > > should exit very quickly. Where in the code does scsi_remove_target > > +0x68/0x240 actually point to? > > > > Is it not a bit more likely that we're following a removed list > > element? > > Since that points back to itself, the list_for_each_entry() would > > then > > circulate forever. If that's the case the simple fix would be to use > > the safe version of the list traversal macro. > > Yes it is traversing a removed element and yes the patches 2/3 and 3/3 > are introducing the safe version of list_for_each_entry(), but they > also decouple the search for elements to be removed from the actual > removal. This is what my initial proposal did as well. Christoph wanted > me to decouple the whole process from the host_lock though and this is > what this patches do as well now. OK, so I really need you to separate the problems. Fixing the bug you're reporting does not require a complete rework of the locking infrastructure; it just requires replacing the traversal macro with the safe version, can you verify that and it can go into fixes? Then we can discuss the merits of doing a locking rework in this area separately from the idea that it's some sort of bug fix. James