[PATCH 1/3] btrfs-progs: fix missing parity stripe for raid6 in chunk-recover

[PATCH 1/3] btrfs-progs: fix missing parity stripe for raid6 in chunk-recover

[Gui Hecheng]

When deal with the p & q stripes for data profile raid6, chunk-recover
forgets to insert them into the chunk record. Just insert them back
freely.
Also wrap the inert procedure into a new function, fill_chunk_up.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 chunk-recover.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/chunk-recover.c b/chunk-recover.c
index dfa7ff6..9b46b0b 100644
--- a/chunk-recover.c
+++ b/chunk-recover.c
@@ -1785,6 +1785,23 @@ static inline int count_devext_records(struct list_head *record_list)
 	return num_of_records;
 }
 
+static int fill_chunk_up(struct chunk_record *chunk, struct list_head *devexts,
+			 struct recover_control *rc)
+{
+	int ret = 0;
+	int i;
+
+	for (i = 0; i < chunk->num_stripes; i++) {
+		if (!chunk->stripes[i].devid) {
+			ret = insert_stripe(devexts, rc, chunk, i);
+			if (ret)
+				break;
+		}
+	}
+
+	return ret;
+}
+
 #define EQUAL_STRIPE (1 << 0)
 
 static int rebuild_raid_data_chunk_stripes(struct recover_control *rc,
@@ -1919,9 +1936,9 @@ next_csum:
 		num_unordered = count_devext_records(&unordered);
 		if (chunk->type_flags & BTRFS_BLOCK_GROUP_RAID6
 					&& num_unordered == 2) {
-			list_splice_init(&unordered, &chunk->dextents);
 			btrfs_release_path(&path);
-			return 0;
+			ret = fill_chunk_up(chunk, &unordered, rc);
+			return ret;
 		}
 
 		goto next_stripe;
@@ -1966,14 +1983,7 @@ out:
 			& BTRFS_BLOCK_GROUP_RAID5)
 		 || (num_unordered == 3 && chunk->type_flags
 			& BTRFS_BLOCK_GROUP_RAID6)) {
-			for (i = 0; i < chunk->num_stripes; i++) {
-				if (!chunk->stripes[i].devid) {
-					ret = insert_stripe(&unordered, rc,
-							chunk, i);
-					if (ret)
-						break;
-				}
-			}
+			ret = fill_chunk_up(chunk, &unordered, rc);
 		}
 	}
 fail_out:
-- 
1.8.1.4

[PATCH 2/3] btrfs-progs: fix max mirror number error for chunk-recover

[Gui Hecheng]

When run chunk-recover on a health btrfs(data profile raid0, with
plenty of data), the program has a chance to abort on the number
of mirrors of an extent.

According to the kernel code, the max mirror number of an extent
is 3 not 2:
	ctree.h: 		BTRFS_MAX_MIRRORS	3
	chunk-recover.c :	BTRFS_NUM_MIRRORS	2
just change BTRFS_NUM_MIRRORS to 3, and everything goes well.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 chunk-recover.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/chunk-recover.c b/chunk-recover.c
index 9b46b0b..d5a688e 100644
--- a/chunk-recover.c
+++ b/chunk-recover.c
@@ -42,7 +42,7 @@
 #include "btrfsck.h"
 #include "commands.h"
 
-#define BTRFS_NUM_MIRRORS			2
+#define BTRFS_NUM_MIRRORS			3
 
 struct recover_control {
 	int verbose;
-- 
1.8.1.4

Re: [PATCH 2/3] btrfs-progs: fix max mirror number error for chunk-recover

[Eric Sandeen]

On 6/11/14, 9:25 PM, Gui Hecheng wrote:
> When run chunk-recover on a health btrfs(data profile raid0, with
> plenty of data), the program has a chance to abort on the number
> of mirrors of an extent.
> 
> According to the kernel code, the max mirror number of an extent
> is 3 not 2:
> 	ctree.h: 		BTRFS_MAX_MIRRORS	3
> 	chunk-recover.c :	BTRFS_NUM_MIRRORS	2
> just change BTRFS_NUM_MIRRORS to 3, and everything goes well.

Wouldn't it make a lot more sense, then, to change the userspace
macro to be called BTRFS_MAX_MIRRORS as well?

-Eric

> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> ---
>  chunk-recover.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/chunk-recover.c b/chunk-recover.c
> index 9b46b0b..d5a688e 100644
> --- a/chunk-recover.c
> +++ b/chunk-recover.c
> @@ -42,7 +42,7 @@
>  #include "btrfsck.h"
>  #include "commands.h"
>  
> -#define BTRFS_NUM_MIRRORS			2
> +#define BTRFS_NUM_MIRRORS			3
>  
>  struct recover_control {
>  	int verbose;
> 

Re: [PATCH 2/3] btrfs-progs: fix max mirror number error for chunk-recover

[Wang Shilong]

On 06/25/2014 10:17 AM, Eric Sandeen wrote:
> On 6/11/14, 9:25 PM, Gui Hecheng wrote:
>> When run chunk-recover on a health btrfs(data profile raid0, with
>> plenty of data), the program has a chance to abort on the number
>> of mirrors of an extent.
>>
>> According to the kernel code, the max mirror number of an extent
>> is 3 not 2:
>> 	ctree.h: 		BTRFS_MAX_MIRRORS	3
>> 	chunk-recover.c :	BTRFS_NUM_MIRRORS	2
>> just change BTRFS_NUM_MIRRORS to 3, and everything goes well.
> Wouldn't it make a lot more sense, then, to change the userspace
> macro to be called BTRFS_MAX_MIRRORS as well?
Yeah, agree,  Nice review. :-)

Wang
>
> -Eric
>
>> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
>> ---
>>   chunk-recover.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/chunk-recover.c b/chunk-recover.c
>> index 9b46b0b..d5a688e 100644
>> --- a/chunk-recover.c
>> +++ b/chunk-recover.c
>> @@ -42,7 +42,7 @@
>>   #include "btrfsck.h"
>>   #include "commands.h"
>>   
>> -#define BTRFS_NUM_MIRRORS			2
>> +#define BTRFS_NUM_MIRRORS			3
>>   
>>   struct recover_control {
>>   	int verbose;
>>
> --
> 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
>

Re: [PATCH 2/3] btrfs-progs: fix max mirror number error for chunk-recover

[Gui Hecheng]

On Tue, 2014-06-24 at 21:17 -0500, Eric Sandeen wrote:
> On 6/11/14, 9:25 PM, Gui Hecheng wrote:
> > When run chunk-recover on a health btrfs(data profile raid0, with
> > plenty of data), the program has a chance to abort on the number
> > of mirrors of an extent.
> > 
> > According to the kernel code, the max mirror number of an extent
> > is 3 not 2:
> > 	ctree.h: 		BTRFS_MAX_MIRRORS	3
> > 	chunk-recover.c :	BTRFS_NUM_MIRRORS	2
> > just change BTRFS_NUM_MIRRORS to 3, and everything goes well.
> 
> Wouldn't it make a lot more sense, then, to change the userspace
> macro to be called BTRFS_MAX_MIRRORS as well?
> 
> -Eric
> 
Yes, Eric, unify the names between userspace and kernelspace is really a
good point. Also, I plan to move the macro into ctree.h, what do you
think?

-Gui

> > Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> > ---
> >  chunk-recover.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/chunk-recover.c b/chunk-recover.c
> > index 9b46b0b..d5a688e 100644
> > --- a/chunk-recover.c
> > +++ b/chunk-recover.c
> > @@ -42,7 +42,7 @@
> >  #include "btrfsck.h"
> >  #include "commands.h"
> >  
> > -#define BTRFS_NUM_MIRRORS			2
> > +#define BTRFS_NUM_MIRRORS			3
> >  
> >  struct recover_control {
> >  	int verbose;
> > 
> 

Re: [PATCH 2/3] btrfs-progs: fix max mirror number error for chunk-recover

[Eric Sandeen]

On 6/24/14, 9:22 PM, Gui Hecheng wrote:
> On Tue, 2014-06-24 at 21:17 -0500, Eric Sandeen wrote:
>> On 6/11/14, 9:25 PM, Gui Hecheng wrote:
>>> When run chunk-recover on a health btrfs(data profile raid0, with
>>> plenty of data), the program has a chance to abort on the number
>>> of mirrors of an extent.
>>>
>>> According to the kernel code, the max mirror number of an extent
>>> is 3 not 2:
>>> 	ctree.h: 		BTRFS_MAX_MIRRORS	3
>>> 	chunk-recover.c :	BTRFS_NUM_MIRRORS	2
>>> just change BTRFS_NUM_MIRRORS to 3, and everything goes well.
>>
>> Wouldn't it make a lot more sense, then, to change the userspace
>> macro to be called BTRFS_MAX_MIRRORS as well?
>>
>> -Eric
>>
> Yes, Eric, unify the names between userspace and kernelspace is really a
> good point. Also, I plan to move the macro into ctree.h, what do you
> think?

It's only used in chunk-recover.c, so I don't see much point to moving it
to a new file.

To be honest, I haven't paid a lot of attention to the code.  The macro
*is* actually used to limit the same thing in userspace as in kernelspace,
right? ;)

-Eric

> -Gui
> 
>>> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
>>> ---
>>>  chunk-recover.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/chunk-recover.c b/chunk-recover.c
>>> index 9b46b0b..d5a688e 100644
>>> --- a/chunk-recover.c
>>> +++ b/chunk-recover.c
>>> @@ -42,7 +42,7 @@
>>>  #include "btrfsck.h"
>>>  #include "commands.h"
>>>  
>>> -#define BTRFS_NUM_MIRRORS			2
>>> +#define BTRFS_NUM_MIRRORS			3
>>>  
>>>  struct recover_control {
>>>  	int verbose;
>>>
>>
> 
> 
> --
> 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
> 

Re: [PATCH 2/3] btrfs-progs: fix max mirror number error for chunk-recover

[Eric Sandeen]

On 6/25/14, 12:14 AM, Eric Sandeen wrote:
> On 6/24/14, 9:22 PM, Gui Hecheng wrote:
>> > On Tue, 2014-06-24 at 21:17 -0500, Eric Sandeen wrote:
>>> >> On 6/11/14, 9:25 PM, Gui Hecheng wrote:
>>>> >>> When run chunk-recover on a health btrfs(data profile raid0, with
>>>> >>> plenty of data), the program has a chance to abort on the number
>>>> >>> of mirrors of an extent.
>>>> >>>
>>>> >>> According to the kernel code, the max mirror number of an extent
>>>> >>> is 3 not 2:
>>>> >>> 	ctree.h: 		BTRFS_MAX_MIRRORS	3
>>>> >>> 	chunk-recover.c :	BTRFS_NUM_MIRRORS	2
>>>> >>> just change BTRFS_NUM_MIRRORS to 3, and everything goes well.
>>> >>
>>> >> Wouldn't it make a lot more sense, then, to change the userspace
>>> >> macro to be called BTRFS_MAX_MIRRORS as well?
>>> >>
>>> >> -Eric
>>> >>
>> > Yes, Eric, unify the names between userspace and kernelspace is really a
>> > good point. Also, I plan to move the macro into ctree.h, what do you
>> > think?
> It's only used in chunk-recover.c, so I don't see much point to moving it
> to a new file.

Sorry, I take that back.  Actually -

Yes, I think it does make sense, just so that userspace moves slightly closer to
kernelspace.

-Eric (who said long ago that he wanted to try to sync things up, but
found himself daunted by the task, and failed)

Re: [PATCH 2/3] btrfs-progs: fix max mirror number error for chunk-recover

[Gui Hecheng]

On Wed, 2014-06-25 at 00:25 -0500, Eric Sandeen wrote:
> On 6/25/14, 12:14 AM, Eric Sandeen wrote:
> > On 6/24/14, 9:22 PM, Gui Hecheng wrote:
> >> > On Tue, 2014-06-24 at 21:17 -0500, Eric Sandeen wrote:
> >>> >> On 6/11/14, 9:25 PM, Gui Hecheng wrote:
> >>>> >>> When run chunk-recover on a health btrfs(data profile raid0, with
> >>>> >>> plenty of data), the program has a chance to abort on the number
> >>>> >>> of mirrors of an extent.
> >>>> >>>
> >>>> >>> According to the kernel code, the max mirror number of an extent
> >>>> >>> is 3 not 2:
> >>>> >>> 	ctree.h: 		BTRFS_MAX_MIRRORS	3
> >>>> >>> 	chunk-recover.c :	BTRFS_NUM_MIRRORS	2
> >>>> >>> just change BTRFS_NUM_MIRRORS to 3, and everything goes well.
> >>> >>
> >>> >> Wouldn't it make a lot more sense, then, to change the userspace
> >>> >> macro to be called BTRFS_MAX_MIRRORS as well?
> >>> >>
> >>> >> -Eric
> >>> >>
> >> > Yes, Eric, unify the names between userspace and kernelspace is really a
> >> > good point. Also, I plan to move the macro into ctree.h, what do you
> >> > think?
> > It's only used in chunk-recover.c, so I don't see much point to moving it
> > to a new file.
> 
> Sorry, I take that back.  Actually -
> 
> Yes, I think it does make sense, just so that userspace moves slightly closer to
> kernelspace.
> 
> -Eric (who said long ago that he wanted to try to sync things up, but
> found himself daunted by the task, and failed)

Aha，it's really a huge work for one person to do the sync things.
But Rome was not built in one day by one guy. We have a long way to go
and let's take one more step now :)

-Gui

[PATCH 3/3] btrfs-progs: cleanup unused assignment for chunk-recover

[Gui Hecheng]

The 'num_unordered' will be recounted after 'goto out',
just remove it.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 chunk-recover.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/chunk-recover.c b/chunk-recover.c
index d5a688e..8bc5bc3 100644
--- a/chunk-recover.c
+++ b/chunk-recover.c
@@ -1905,7 +1905,6 @@ next_csum:
 		fprintf(stderr, "Fetch csum failed\n");
 		goto fail_out;
 	} else if (ret == 1) {
-		num_unordered = count_devext_records(&unordered);
 		if (!(*flags & EQUAL_STRIPE))
 			*flags |= EQUAL_STRIPE;
 		goto out;
-- 
1.8.1.4

Re: [PATCH 1/3] btrfs-progs: fix missing parity stripe for raid6 in chunk-recover

[Duncan]

Gui Hecheng posted on Thu, 12 Jun 2014 10:24:59 +0800 as excerpted:

> When deal with the p & q stripes for data profile raid6, chunk-recover
> forgets to insert them into the chunk record. Just insert them back
> freely.
> Also wrap the inert procedure into a new function, fill_chunk_up.

If there's a v2 anyway: s/inert/insert/ ?

(For a moment I was contemplating the reasons one might wrap a stub, 
which is how I was translating inert procedure. Then it hit me... =:^)

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman

Re: [PATCH 1/3] btrfs-progs: fix missing parity stripe for raid6 in chunk-recover

[Gui Hecheng]

On Thu, 2014-06-12 at 07:45 +0000, Duncan wrote:
> Gui Hecheng posted on Thu, 12 Jun 2014 10:24:59 +0800 as excerpted:
> 
> > When deal with the p & q stripes for data profile raid6, chunk-recover
> > forgets to insert them into the chunk record. Just insert them back
> > freely.
> > Also wrap the inert procedure into a new function, fill_chunk_up.
> 
> If there's a v2 anyway: s/inert/insert/ ?
Oh, yes, 'insert' is the right thing. Thanks very much! I'll send a v2.

-Gui
> (For a moment I was contemplating the reasons one might wrap a stub, 
> which is how I was translating inert procedure. Then it hit me... =:^)
>