[PATH]Some fix for batching extent insert.

[PATH]Some fix for batching extent insert.

[Liu Hui]

Hi,
When I review the code about batching extent insert, I found some code
could result in problems in some corner cases.
1)In finish_current_insert(), when it finds nothing to insert and it
has skipped some locked extents, it should try again to see if the
locked extent is unlocked. So, it will re-search the extent_ins
extent_io_tree by reseting 'search' to 0. There is one place forget to
reset the 'search' pointer to 0 which will lead BTRFS not to finish
*all* current insert.

2)In insert_extents(), when ret==1 and last is not zero, it should
check if the current inserted item is the last item in this batching
inserts. If so, it should just break from loop. If not, 'cur =
insert_list->next' will make no sense because the list is empty now,
and 'op' will point to an unexpectable place.

3)There are also some trivial fixs in this patch including one comment
typo error and deleting two redundant lines.

--
Thanks & Best Regards
Hui
--

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 8bb4524..0d0e2e7 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -3033,7 +3033,6 @@ int btrfs_insert_some_items(struct
btrfs_trans_handle *trans,
 	struct btrfs_item *item;
 	int ret = 0;
 	int slot;
-	int slot_orig;
 	int i;
 	u32 nritems;
 	u32 total_data = 0;
@@ -3056,7 +3055,6 @@ int btrfs_insert_some_items(struct
btrfs_trans_handle *trans,
 	if (ret < 0)
 		goto out;

-	slot_orig = path->slots[0];
 	leaf = path->nodes[0];

 	nritems = btrfs_header_nritems(leaf);
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index e785f0a..be5b376 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -783,7 +783,7 @@ static int noinline insert_extents(struct
btrfs_trans_handle *trans,
 			i = total - 1;
 			cur = insert_list->prev;
 			op = list_entry(cur, struct pending_extent_op, list);
-		} else if (last) {
+		} else if (last && (i + ret < total)) {
 			/*
 			 * ok we successfully inserted the last item on the
 			 * list, lets reset everything
@@ -2145,6 +2145,7 @@ again:
 		if (ret) {
 			if (skipped && all && !num_inserts) {
 				skipped = 0;
+				search = 0;
 				continue;
 			}
 			mutex_unlock(&info->extent_ins_mutex);
@@ -2184,7 +2185,7 @@ again:
 	}

 	/*
-	 * process teh update list, clear the writeback bit for it, and if
+	 * process the update list, clear the writeback bit for it, and if
 	 * somebody marked this thing for deletion then just unlock it and be
 	 * done, the free_extents will handle it
 	 */

Re: [PATH]Some fix for batching extent insert.

[Chris Mason]

On Mon, 2008-11-17 at 23:48 +0800, Liu Hui wrote:
> Hi,
> When I review the code about batching extent insert, I found some code
> could result in problems in some corner cases.
> 1)In finish_current_insert(), when it finds nothing to insert and it
> has skipped some locked extents, it should try again to see if the
> locked extent is unlocked. So, it will re-search the extent_ins
> extent_io_tree by reseting 'search' to 0. There is one place forget to
> reset the 'search' pointer to 0 which will lead BTRFS not to finish
> *all* current insert.
> 

Thanks for spending time on this, but I'm afraid this patch leads to
infinite looping in finish_current_insert under stress testing.

-chris

Re: [PATH]Some fix for batching extent insert.

[Chris Mason]

On Tue, 2008-11-18 at 07:17 -0500, Chris Mason wrote:
> On Mon, 2008-11-17 at 23:48 +0800, Liu Hui wrote:
> > Hi,
> > When I review the code about batching extent insert, I found some code
> > could result in problems in some corner cases.
> > 1)In finish_current_insert(), when it finds nothing to insert and it
> > has skipped some locked extents, it should try again to see if the
> > locked extent is unlocked. So, it will re-search the extent_ins
> > extent_io_tree by reseting 'search' to 0. There is one place forget to
> > reset the 'search' pointer to 0 which will lead BTRFS not to finish
> > *all* current insert.
> > 
> 
> Thanks for spending time on this, but I'm afraid this patch leads to
> infinite looping in finish_current_insert under stress testing.

Ah, I see there is a check later in finish_current_insert to make sure
we don't miss anything.  I'm retesting with just that hunk removed.

-chris

Re: [PATH]Some fix for batching extent insert.

[yanhai zhu]

hello,
   In fact, Before Liu's change, this function maybe don't wait and
insert ALL extents when all=1 in some cases,
please think about the following conditions:

when run to Line 2146
(1) all is set to 1
(2) we have skipped some extents in previous loops (skipped = 1)
(3) we have found nothing yet (num_inserts = 0)
According to Line 2145, we just simplely set skipped=0, continue loop
keeping @search unchanged. In the next loop, find_first_extent_bit()
still checks the @search position and returns 1, but at this time,
since skipped==0
Line 2146 will not be triggered, we break the loop at Line 2152,with
(1) all is 1 (2) skipped is 0 (3) num_inserts is 0
The following procedures with list will be ok since they can survive
from empty list.
But, Line 2220 will not be triggered either because skipped is 0 now.
Line 2285, also not work.
At last, we exit from this function, with (1) skipped some locked
extents (2) insert none extents

Liu Hui's patch did the right thing to fix this problem, and the more
simple way to fix it is just remove Line 2146-2149, so the if
statement at Line 2220 will do right things now.

In fact when we fix it like this, the funtion it self is right in
theory but appears to loop forever.
As I have no test suite, I guess *maybe* we encounter some unlock
extents(which may be forever locked),
this fix function insist on its duty -- which is to make sure all
extents are checked -- so trapped into a
forever loop.

So I think there may be another hidden error triggered by this fix(a
dead lock or a lock leak?)

thanks,
Zhu Yanhai
--

2008/11/18 Chris Mason <chris.mason@oracle.com>:
> On Tue, 2008-11-18 at 07:17 -0500, Chris Mason wrote:
>> On Mon, 2008-11-17 at 23:48 +0800, Liu Hui wrote:
>> > Hi,
>> > When I review the code about batching extent insert, I found some code
>> > could result in problems in some corner cases.
>> > 1)In finish_current_insert(), when it finds nothing to insert and it
>> > has skipped some locked extents, it should try again to see if the
>> > locked extent is unlocked. So, it will re-search the extent_ins
>> > extent_io_tree by reseting 'search' to 0. There is one place forget to
>> > reset the 'search' pointer to 0 which will lead BTRFS not to finish
>> > *all* current insert.
>> >
>>
>> Thanks for spending time on this, but I'm afraid this patch leads to
>> infinite looping in finish_current_insert under stress testing.
>
> Ah, I see there is a check later in finish_current_insert to make sure
> we don't miss anything.  I'm retesting with just that hunk removed.
>
> -chris
>
>
> --
> 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
>



-- 
Zhu Yanhai

Re: [PATH]Some fix for batching extent insert.

[Chris Mason]

On Tue, 2008-11-18 at 22:35 +0800, yanhai zhu wrote:
> hello,
>    In fact, Before Liu's change, this function maybe don't wait and
> insert ALL extents when all=1 in some cases,
> please think about the following conditions:
> 

Ok, I see what you mean.  It turned out it was actually the other hunk
that was breaking things.  I think Liu is right about that bug as well,
but I changed it slightly to preserve the counter updating but avoid
touching the list if we're really done.

Thanks!

-chris