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 <[email protected]> > >> --- > >> 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
