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

Message ID 1402539901-22779-2-git-send-email-guihc.fnst@cn.fujitsu.com
State Accepted
Headers show

Commit Message

Gui Hecheng June 12, 2014, 2:25 a.m. UTC
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(-)

Comments

Eric Sandeen June 25, 2014, 2:17 a.m. UTC | #1
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;
> 

--
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
Wang Shilong June 25, 2014, 2:20 a.m. UTC | #2
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
>

--
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
Gui Hecheng June 25, 2014, 2:22 a.m. UTC | #3
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;
> > 
> 


--
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
Eric Sandeen June 25, 2014, 5:14 a.m. UTC | #4
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
> 

--
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
Eric Sandeen June 25, 2014, 5:25 a.m. UTC | #5
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)
--
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
Gui Hecheng June 25, 2014, 6:51 a.m. UTC | #6
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

--
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

Patch
diff mbox

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;