On Wed, Feb 23, 2011 at 09:12:36AM +0800, liubo wrote:
> On 02/22/2011 10:32 PM, David Sterba wrote:
> > Hi,
> >
> > no deeper analysis done, but the double free error was obvious :)
> >
> > On Tue, Feb 22, 2011 at 07:42:25PM +0800, liubo wrote:
> >> When we recover from crash via write-ahead log tree and process
> >> the inode refs, for each btrfs_inode_ref item, we will
> >> 1) check if we already have a perfect match in fs/file tree, if
> >> we have, then we're done.
> >> 2) search the corresponding back reference in fs/file tree, and
> >> check all the names in this back reference to see if they are
> >> also in the log to avoid conflict corners.
> >> 3) recover the logged inode refs to fs/file tree.
> >>
> >> In current btrfs, however,
> >> - for 2)'s check, once is enough, since the checked back references
> >> will remain unchanged after processing all the inode refs belonged
> >> to the key.
> >> - it has no need to do another 1) between 2) and 3).
> >>
> >> This patch focus on the above problems and
> >> I've made a small test to show how it improves,
> >>
> >> $dd if=/dev/zero of=foobar bs=4K count=1
> >> $sync
> >> $make 100 hard links continuously, like ln foobar link_i
> >> $fsync foobar
> >> $echo b > /proc/sysrq-trigger
> >> after reboot
> >> $time mount DEV PATH
> >>
> >> without patch:
> >> real 0m0.285s
> >> user 0m0.001s
> >> sys 0m0.009s
> >>
> >> with patch:
> >> real 0m0.123s
> >> user 0m0.000s
> >> sys 0m0.010s
> >>
> >> Signed-off-by: Liu Bo <liubo2009@xxxxxxxxxxxxxx>
> >> ---
> >> fs/btrfs/tree-log.c | 33 +++++++++++----------------------
> >> 1 files changed, 11 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> >> index a4bbb85..8f2a9f3 100644
> >> --- a/fs/btrfs/tree-log.c
> >> +++ b/fs/btrfs/tree-log.c
> >> @@ -799,12 +799,12 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans,
> >> struct inode *dir;
> >> int ret;
> >> struct btrfs_inode_ref *ref;
> >> - struct btrfs_dir_item *di;
> >> struct inode *inode;
> >> char *name;
> >> int namelen;
> >> unsigned long ref_ptr;
> >> unsigned long ref_end;
> >> + int search_done = 0;
> >>
> >> /*
> >> * it is possible that we didn't log all the parent directories
> >> @@ -845,7 +845,10 @@ again:
> >> * existing back reference, and we don't want to create
> >> * dangling pointers in the directory.
> >> */
> >> -conflict_again:
> >> +
> >> + if (search_done)
> >> + goto insert;
> >> +
> >> ret = btrfs_search_slot(NULL, root, key, path, 0, 0);
> >> if (ret == 0) {
> >> char *victim_name;
> >> @@ -888,35 +891,21 @@ conflict_again:
> >> victim_name_len);
> >> kfree(victim_name);
> > ^^^^^^^^^^^^^^^^^^^
> >> btrfs_release_path(root, path);
> >> - goto conflict_again;
> >> }
> >> kfree(victim_name);
> > ^^^^^^^^^^^^^^^^^^^
> > double free
>
> thanks for reviewing, but the first one is followed by a goto phrase, so IMO it is ok.
>
Your patch removes that goto, so it's not ok. Thanks,
Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
