linux-kernel - Re: [PATCH v3] xfs: introduce object readahead to log recovery

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 14 Aug 2013 16:42:39 +1000
From:	Dave Chinner <david@...morbit.com>
To:	Zhi Yong Wu <zwu.kernel@...il.com>
Cc:	xfstests <xfs@....sgi.com>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	Zhi Yong Wu <wuzhy@...ux.vnet.ibm.com>,
	linux-kernel mlist <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] xfs: introduce object readahead to log recovery

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 <david@...morbit.com> wrote:
> > On Wed, Jul 31, 2013 at 04:42:45PM +0800, zwu.kernel@...il.com wrote:
> >> From: Zhi Yong Wu <wuzhy@...ux.vnet.ibm.com>
> >>
> >>   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@...morbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/