Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752787Ab3HNGmp (ORCPT ); Wed, 14 Aug 2013 02:42:45 -0400 Received: from ipmail07.adl2.internode.on.net ([150.101.137.131]:57317 "EHLO ipmail07.adl2.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751841Ab3HNGmo (ORCPT ); Wed, 14 Aug 2013 02:42:44 -0400 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: AogHAJslC1J5LCJR/2dsb2JhbABbgwaDNLZ/hTSBJhd0giQBAQUnExwjEAgDFQMJJQ8FDRgDIROHfgMOsBgNiF4WjS+CdweDG3YDlXuBaIwrhSeDLSo Date: Wed, 14 Aug 2013 16:42:39 +1000 From: Dave Chinner To: Zhi Yong Wu Cc: xfstests , "linux-fsdevel@vger.kernel.org" , Zhi Yong Wu , linux-kernel mlist Subject: Re: [PATCH v3] xfs: introduce object readahead to log recovery Message-ID: <20130814064239.GE12779@dastard> References: <1375260165-8773-1-git-send-email-zwu.kernel@gmail.com> <20130814053523.GY12779@dastard> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2633 Lines: 69 On Wed, Aug 14, 2013 at 01:59:02PM +0800, Zhi Yong Wu wrote: > On Wed, Aug 14, 2013 at 1:35 PM, Dave Chinner wrote: > > On Wed, Jul 31, 2013 at 04:42:45PM +0800, zwu.kernel@gmail.com wrote: > >> From: Zhi Yong Wu > >> > >> It can take a long time to run log recovery operation because it is > >> single threaded and is bound by read latency. We can find that it took > >> most of the time to wait for the read IO to occur, so if one object > >> readahead is introduced to log recovery, it will obviously reduce the > >> log recovery time. > >> > >> Log recovery time stat: > >> > >> w/o this patch w/ this patch > >> > >> real: 0m15.023s 0m7.802s > >> user: 0m0.001s 0m0.001s > >> sys: 0m0.246s 0m0.107s > > > > This version works as advertised as well. > > > >> @@ -3216,6 +3351,18 @@ xlog_recover_commit_trans( > >> goto out; > >> } > >> > >> + if (!list_empty(&ra_list)) { > >> + error = xlog_recover_items_pass2(log, trans, > >> + &buffer_list, &ra_list); > >> + if (error) > >> + goto out; > >> + > >> + list_splice_tail_init(&ra_list, &done_list); > >> + } > >> + > >> + if (!list_empty(&done_list)) > >> + list_splice_init(&done_list, &trans->r_itemq); > >> + > >> xlog_recover_free_trans(trans); > > > > I think this still leaks the trans structure when an error occurs. > > Indeed, I think this is a pre-existing leak, as the current code > > will skip freeing the trans structure on item recovery failure and > > nothing else frees it. So it appears to me to be busted before this > > patch is added. > Yes, i also found this and think so. > > > > Hence on a xlog_recover_items_pass2() error we need to splice the > > ra-list to the done_list and free trans. i.e. the "if (error) goto > > out;" lines in the above hunk do not need to be there, and the > > "out:" label moved to above the call to xlog_recover_free_trans() so > > the main loop does the right thing when an error occurs. > Do you need to draft one patch to fix trans leaking? or can it be > fixed in this patch? Just fix it in this patch - the above paragraph explains how to fix it.... Cheers, Dave. -- Dave Chinner david@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/