From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758874Ab0JZXFr (ORCPT ); Tue, 26 Oct 2010 19:05:47 -0400 Received: from smtp104.sbc.mail.gq1.yahoo.com ([67.195.15.63]:24225 "HELO smtp104.sbc.mail.gq1.yahoo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1756444Ab0JZXFq (ORCPT ); Tue, 26 Oct 2010 19:05:46 -0400 X-Yahoo-SMTP: fzDSGlOswBCWnIOrNw7KwwK1j9PqyNbe5PtLKiS4dDU.UNl_t6bdEZu9tTLW X-YMail-OSG: 1NWPnToVM1lpeR2tuiJHEVurBfJUcJ4F9QZsWnWiAV38KjO 2R9ym6.LVCFgKlZWBPM6_kBzHDioMRK36PN_ga.9g.BkCciMCUdCSWxO0qax 9Hs4LoX8nWPHu53UCCyy.TODiZZ5eWUQEmqz0R59SHs4Y5iFarb5rBMoEOMS eu2O4Vd29JNWCn5mkeeQnTWBEudhs1Ft07q0D1nfwcVYYv539E89fDQ1wCI. l32p_tA.f4VzWoedaigINJErhEGKGcZPKrdZrEumc4b_RIFg- X-Yahoo-Newman-Property: ymail-3 Subject: Re: [ANNOUNCE] Status of unlocked_qcmds=1 operation for .37 From: "Nicholas A. Bellinger" To: James Bottomley Cc: Mike Anderson , linux-kernel , linux-scsi , Vasu Dev , Tim Chen , Andi Kleen , Matthew Wilcox , Mike Christie , Jens Axboe , James Smart , Andrew Vasquez , FUJITA Tomonori , Hannes Reinecke , Joe Eykholt , Christoph Hellwig , Jon Hawley , Brian King , Christof Schmitt , Tejun Heo , Andrew Morton , "H. Peter Anvin" In-Reply-To: <1288133450.8283.723.camel@mulgrave.site> References: <1287607774.10283.78.camel@haakon2.linux-iscsi.org> <20101021150840.GA24309@linux.vnet.ibm.com> <1288130914.5169.97.camel@haakon2.linux-iscsi.org> <1288132071.8283.689.camel@mulgrave.site> <1288132464.5169.112.camel@haakon2.linux-iscsi.org> <1288133450.8283.723.camel@mulgrave.site> Content-Type: text/plain Date: Tue, 26 Oct 2010 16:00:48 -0700 Message-Id: <1288134048.5169.132.camel@haakon2.linux-iscsi.org> Mime-Version: 1.0 X-Mailer: Evolution 2.22.3.1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2010-10-26 at 22:50 +0000, James Bottomley wrote: > On Tue, 2010-10-26 at 15:34 -0700, Nicholas A. Bellinger wrote: > > On Tue, 2010-10-26 at 17:27 -0500, James Bottomley wrote: > > > On Tue, 2010-10-26 at 15:08 -0700, Nicholas A. Bellinger wrote: > > > > [PATCH] scsi: Add SCSI_EH_SOFTIRQ_DONE usage > > > > > > > > This patch introduces a SCSI_EH_SOFTIRQ_DONE flag that is set in scsi_softirq_done() > > > > from block soft_irq context that is used to signal when scsi_try_to_abort_cmd() should > > > > be calling __scsi_try_to_abort_cmd() for a timed out struct scsi_cmnd instead of > > > > returning SUCCESS via checking only blk_test_rq_complete(). This is done because > > > > blk_rq_timed_out_timer() calls blk_mark_rq_complete() before blk_rq_timed_out() -> > > > > struct request_queue->rq_timed_out_fn(). > > > > > > This is getting pretty far off into the weeds. I think the first step > > > should be queue lock push down into ->queuecommand. This would still > > > necessitate locking around the serial number. > > > > Hmmm, I am not sure I understand what you mean here. My understanding > > is that the whole point of the series was to remove any locking around > > the serial number in order to make scsi_dispatch_cmd() lockless, > > right..? > > The point is that several things have to happen for that to be a > reality. The easiest and most obvious thing is lock push down in > ->queuecommand. > Ok, can you please explain to me what you mean here..? As I really thought we came to consensus that: *) running in unlocked_qcmd=1 was to be made the default for LLDs using the legacy optimization of ->queuecommand() -> unlock() -> do_some_lld_work() -> lock() -> return to scsi_dispatch_cmd() *) For the mpt-fusion / mpt2sas drivers which did not use the legacy optimization, but Tim Chen has tested with the former and I am awaiting an ACK from LSI on the latter. *) All other drivers will function with host_lock held (eg: legacy mode) in scsi_dispatch_cmd(). *) All drivers using cmd->serial_number for anything beyond an informational purpose converted to use scsi_cmd_get_serial(). > The next is most likely serial number elimination. > > But the point is that we don't have to do the whole thing all at once > (and spend months trying to get the series right). Not exactly correct, we have the whole thing ready right now with libfc running unlocked_qcmd=0 legacy mode. Once I verify START_STOP case in scsi_error.c, I am happy to respin a mergeable tree from linus HEAD for you to pull ASAP. > > > That is what the current patch series already does, in that it makes the > > use of cmd->serial_number optional and requires LLDs who use this for > > anything beyond an informational purpose to explictly call > > scsi_cmd_get_serial(). > > OK, so this patch is a corner case where the error handler is using the > serial number value to deduce something the block layer already knows > (whether the command completed or not). I don't think introducing a > substitute flag is the right way, the information should just be > extracted properly. > Well, according to andmike this is two corner cases that are a result of the drop-host_lock-v4 series using only blk_test_rq_complete() in scsi_try_to_abort_cmd(), which will be true because of: blk_rq_timed_out_timer() -> blk_mark_rq_complete() > But arguments about this don't have to impede the lock push down. > Sure, but I assume you mean the lock push down only for the legacy LLDs that are not already internally unlocking host_lock in SHT->queuecommand() in mainline code right..? --nab