Re: [PATCH] btrfs: make inode ref log recovery faster

[liubo <liubo2009@cn.fujitsu.com>]

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 <liubo2009@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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>