On 02/23/2011 09:30 AM, Josef Bacik wrote: > 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 <liubo2...@cn.fujitsu.com> >>>> --- >>>> 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,
ahh, my fault. I'll fix it, thanks a lot, :) liubo > > Josef > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html