[PATCH 1/4] btrfs-progs: adjust the return values for scrub

[PATCH 1/4] btrfs-progs: adjust the return values for scrub

[Gui Hecheng]

o Return 0 to indicate success,
  when detected errors were corrected during scrubbing.
  P.s. This is also to facilitate scripting when return value
       is to be checked.
o Warn the users if there are uncorrectable errors detected.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 cmds-scrub.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/cmds-scrub.c b/cmds-scrub.c
index 5265a2b..f9e2b40 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -1514,14 +1514,17 @@ out:
 	}
 	close_file_or_dir(fdmnt, dirstream);
 
-	if (nothing_to_resume)
-		return 2;
 	if (err)
 		return 1;
-	if (e_correctable)
+	if (nothing_to_resume)
+		return 2;
+	if (e_uncorrectable) {
+		ERR(!do_quiet, "ERROR: There are uncorrectable errors.\n");
 		return 3;
-	if (e_uncorrectable)
-		return 4;
+	}
+	if (e_correctable)
+		ERR(!do_quiet, "WARNING: errors detected during scrubbing, corrected.\n");
+
 	return 0;
 }
 
-- 
1.8.1.4

[PATCH 2/4] btrfs-progs: remove unnecessary judgment for fd in scrub

[Gui Hecheng]

The scrub_read_file function is always on a branch,
which has (fd >= 0), so there is not need to judgment
the pasted in arg.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 cmds-scrub.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/cmds-scrub.c b/cmds-scrub.c
index f9e2b40..a604b25 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -474,9 +474,6 @@ static struct scrub_file_record **scrub_read_file(int fd, int report_errors)
 	char empty_uuid[BTRFS_FSID_SIZE] = {0};
 	struct scrub_file_record **p = NULL;
 
-	if (fd < 0)
-		return ERR_PTR(-EINVAL);
-
 again:
 	old_avail = avail - i;
 	BUG_ON(old_avail < 0);
-- 
1.8.1.4

Re: [PATCH 2/4] btrfs-progs: remove unnecessary judgment for fd in scrub

[Satoru Takeuchi]

Hi Gui,

(2014/07/17 11:40), Gui Hecheng wrote:
> The scrub_read_file function is always on a branch,
> which has (fd >= 0), so there is not need to judgment
> the pasted in arg.
> 
> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> ---
>   cmds-scrub.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/cmds-scrub.c b/cmds-scrub.c
> index f9e2b40..a604b25 100644
> --- a/cmds-scrub.c
> +++ b/cmds-scrub.c
> @@ -474,9 +474,6 @@ static struct scrub_file_record **scrub_read_file(int fd, int report_errors)
>   	char empty_uuid[BTRFS_FSID_SIZE] = {0};
>   	struct scrub_file_record **p = NULL;
>   
> -	if (fd < 0)
> -		return ERR_PTR(-EINVAL);
> -

It's correct. In addition, adding "'fd' should point to a opened file"
like comment is better for potential users of this function.

Thanks,
Satoru


>   again:
>   	old_avail = avail - i;
>   	BUG_ON(old_avail < 0);
> 

[PATCH 3/4] btrfs-progs: replace a confusing raw number with a macro

[Gui Hecheng]

The raw number 36 for the uuid string length is somewhat confusing,
use a macro to define replace it.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 cmds-scrub.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/cmds-scrub.c b/cmds-scrub.c
index a604b25..03eb9ba 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -48,6 +48,8 @@ static const char * const scrub_cmd_group_usage[] = {
 	NULL
 };
 
+#define UUID_LEN_STR 36
+
 #define SCRUB_DATA_FILE "/var/lib/btrfs/scrub.status"
 #define SCRUB_PROGRESS_SOCKET_PATH "/var/lib/btrfs/scrub.progress"
 #define SCRUB_FILE_VERSION_PREFIX "scrub status"
@@ -550,7 +552,7 @@ again:
 				;
 			if (i + j + 1 >= avail)
 				_SCRUB_INVALID;
-			if (j != 36)
+			if (j != UUID_LEN_STR)
 				_SCRUB_INVALID;
 			l[i + j] = '\0';
 			ret = uuid_parse(l + i, p[curr]->fsid);
-- 
1.8.1.4

Re: [PATCH 3/4] btrfs-progs: replace a confusing raw number with a macro

[David Sterba]

On Thu, Jul 17, 2014 at 10:40:38AM +0800, Gui Hecheng wrote:
> The raw number 36 for the uuid string length is somewhat confusing,
> use a macro to define replace it.

There's the BTRFS_UUID_UNPARSED_SIZE macro, please use it instead to
avoid duplicate definitions.

Re: [PATCH 3/4] btrfs-progs: replace a confusing raw number with a macro

[David Sterba]

On Tue, Jul 29, 2014 at 02:16:14PM +0200, David Sterba wrote:
> On Thu, Jul 17, 2014 at 10:40:38AM +0800, Gui Hecheng wrote:
> > The raw number 36 for the uuid string length is somewhat confusing,
> > use a macro to define replace it.
> 
> There's the BTRFS_UUID_UNPARSED_SIZE macro, please use it instead to
> avoid duplicate definitions.

Or I'll do that myself as it's a really a small change.

Re: [PATCH 3/4] btrfs-progs: replace a confusing raw number with a macro

[Gui Hecheng]

On Tue, 2014-07-29 at 14:19 +0200, David Sterba wrote:
> On Tue, Jul 29, 2014 at 02:16:14PM +0200, David Sterba wrote:
> > On Thu, Jul 17, 2014 at 10:40:38AM +0800, Gui Hecheng wrote:
> > > The raw number 36 for the uuid string length is somewhat confusing,
> > > use a macro to define replace it.
> > 
> > There's the BTRFS_UUID_UNPARSED_SIZE macro, please use it instead to
> > avoid duplicate definitions.
> 
> Or I'll do that myself as it's a really a small change.
Oh, yes, it is really kind of you to do so :)

[PATCH 4/4] btrfs-progs: correct manpage option description for scrub

[Gui Hecheng]

The -f option of scrub means to skip checking running scrub,
not to force checking.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 Documentation/btrfs-scrub.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/btrfs-scrub.txt b/Documentation/btrfs-scrub.txt
index 7b27d63..1af9b9f 100644
--- a/Documentation/btrfs-scrub.txt
+++ b/Documentation/btrfs-scrub.txt
@@ -47,7 +47,7 @@ manpage).
 -n <ioprio_classdata>::::
 Set IO priority classdata (see `ionice`(1) manpage).
 -f::::
-force to check whether scrub has started or resumed in userspace.
+force to skip checking whether scrub has started or resumed in userspace.
 this is useful when scrub stat record file is damaged.
 
 *cancel* <path>|<device>::
-- 
1.8.1.4

Re: [PATCH 4/4] btrfs-progs: correct manpage option description for scrub

[Satoru Takeuchi]

Hi Gui,

(2014/07/17 11:40), Gui Hecheng wrote:
> The -f option of scrub means to skip checking running scrub,
> not to force checking.
> 
> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> ---
>   Documentation/btrfs-scrub.txt | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/btrfs-scrub.txt b/Documentation/btrfs-scrub.txt
> index 7b27d63..1af9b9f 100644
> --- a/Documentation/btrfs-scrub.txt
> +++ b/Documentation/btrfs-scrub.txt
> @@ -47,7 +47,7 @@ manpage).
>   -n <ioprio_classdata>::::
>   Set IO priority classdata (see `ionice`(1) manpage).
>   -f::::
> -force to check whether scrub has started or resumed in userspace.
> +force to skip checking whether scrub has started or resumed in userspace.

I consider "scrub has started and resumed" is not user-friendly
expression. First, it can be replaced with more easy one,
"scrub is running". Second, there in no explanation about
this checking behavior before "-f" option's description.

So, how about the following idea?

 Fix 1. Add "If scrub is already running running, it fails."
        to the description before `Options` section
 Fix 2. Replace "force to check ..." with
        "force starting new scrub even if scrub is already running."
 Fix 3. Fix cmd_scrub_start_usage too.

Thanks,
Satoru

>   this is useful when scrub stat record file is damaged.
>   
>   *cancel* <path>|<device>::
> 

Re: [PATCH 4/4] btrfs-progs: correct manpage option description for scrub

[Gui Hecheng]

On Wed, 2014-07-30 at 14:20 +0900, Satoru Takeuchi wrote:
> Hi Gui,
> 
> (2014/07/17 11:40), Gui Hecheng wrote:
> > The -f option of scrub means to skip checking running scrub,
> > not to force checking.
> > 
> > Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> > ---
> >   Documentation/btrfs-scrub.txt | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/btrfs-scrub.txt b/Documentation/btrfs-scrub.txt
> > index 7b27d63..1af9b9f 100644
> > --- a/Documentation/btrfs-scrub.txt
> > +++ b/Documentation/btrfs-scrub.txt
> > @@ -47,7 +47,7 @@ manpage).
> >   -n <ioprio_classdata>::::
> >   Set IO priority classdata (see `ionice`(1) manpage).
> >   -f::::
> > -force to check whether scrub has started or resumed in userspace.
> > +force to skip checking whether scrub has started or resumed in userspace.

Hi Satoru,
Thanks for your comments first. My opinions are as follows:

> I consider "scrub has started and resumed" is not user-friendly
> expression. First, it can be replaced with more easy one,
> "scrub is running". Second, there in no explanation about
> this checking behavior before "-f" option's description.

Yes, "Scrub is running" is more precise.

> So, how about the following idea?
> 
>  Fix 1. Add "If scrub is already running running, it fails."
>         to the description before `Options` section
  This is really a valuable idea. 

>  Fix 2. Replace "force to check ..." with
>         "force starting new scrub even if scrub is already running."
  This is more precise.

>  Fix 3. Fix cmd_scrub_start_usage too.
  Of course, thanks for reminding me.

So please let me rework this patch, could I add your sign-off-by then?

Thanks,
Gui
> 
> Thanks,
> Satoru
> 
> >   this is useful when scrub stat record file is damaged.
> >   
> >   *cancel* <path>|<device>::
> > 
> 

Re: [PATCH 4/4] btrfs-progs: correct manpage option description for scrub

[Satoru Takeuchi]

Hi Gui,

(2014/07/30 14:39), Gui Hecheng wrote:
> On Wed, 2014-07-30 at 14:20 +0900, Satoru Takeuchi wrote:
>> Hi Gui,
>>
>> (2014/07/17 11:40), Gui Hecheng wrote:
>>> The -f option of scrub means to skip checking running scrub,
>>> not to force checking.
>>>
>>> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
>>> ---
>>>    Documentation/btrfs-scrub.txt | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/btrfs-scrub.txt b/Documentation/btrfs-scrub.txt
>>> index 7b27d63..1af9b9f 100644
>>> --- a/Documentation/btrfs-scrub.txt
>>> +++ b/Documentation/btrfs-scrub.txt
>>> @@ -47,7 +47,7 @@ manpage).
>>>    -n <ioprio_classdata>::::
>>>    Set IO priority classdata (see `ionice`(1) manpage).
>>>    -f::::
>>> -force to check whether scrub has started or resumed in userspace.
>>> +force to skip checking whether scrub has started or resumed in userspace.
>
> Hi Satoru,
> Thanks for your comments first. My opinions are as follows:
>
>> I consider "scrub has started and resumed" is not user-friendly
>> expression. First, it can be replaced with more easy one,
>> "scrub is running". Second, there in no explanation about
>> this checking behavior before "-f" option's description.
>
> Yes, "Scrub is running" is more precise.
>
>> So, how about the following idea?
>>
>>   Fix 1. Add "If scrub is already running running, it fails."
>>          to the description before `Options` section
>    This is really a valuable idea.
>
>>   Fix 2. Replace "force to check ..." with
>>          "force starting new scrub even if scrub is already running."
>    This is more precise.
>
>>   Fix 3. Fix cmd_scrub_start_usage too.
>    Of course, thanks for reminding me.
>
> So please let me rework this patch,

Thank you for reworking.

>could I add your sign-off-by then?

Yes, please.

Thanks,
Satoru

>
> Thanks,
> Gui
>>
>> Thanks,
>> Satoru
>>
>>>    this is useful when scrub stat record file is damaged.
>>>
>>>    *cancel* <path>|<device>::
>>>
>>
>
>
> --
> 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 v2] btrfs-progs: correct manpage option description for scrub

[Gui Hecheng]

The -f option of scrub means to
    "force starting new scrub even if a scrub is already running"
*not*
    "force to check whether scrub has started or resumed in userspace"
as described originally.

So replace the orignal description in the manpage and code.

Also, add description of the potential failure as follows
    "If a scrub is already running running, it fails."

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
Cc: David Sterba <dsterba@suse.cz>
---
changelog
	v1->v2: adopt more precise expression of the checking behavior
		add description of potential failure
---
 Documentation/btrfs-scrub.txt | 6 +++---
 cmds-scrub.c                  | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/btrfs-scrub.txt b/Documentation/btrfs-scrub.txt
index 7b27d63..b8d3b65 100644
--- a/Documentation/btrfs-scrub.txt
+++ b/Documentation/btrfs-scrub.txt
@@ -18,7 +18,7 @@ SUBCOMMAND
 ----------
 *start* [-BdqrRf] [-c <ioprio_class> -n <ioprio_classdata>] <path>|<device>::
 Start a scrub on all devices of the filesystem identified by <path> or on
-a single <device>.
+a single <device>. If a scrub is already running, the new one fails.
 +
 Without options, scrub is started as a background process.
 Progress can be obtained with the *scrub status* command. Scrubbing
@@ -47,8 +47,8 @@ manpage).
 -n <ioprio_classdata>::::
 Set IO priority classdata (see `ionice`(1) manpage).
 -f::::
-force to check whether scrub has started or resumed in userspace.
-this is useful when scrub stat record file is damaged.
+Force starting new scrub even if a scrub is already running.
+This is useful when scrub stat record file is damaged.
 
 *cancel* <path>|<device>::
 If a scrub is running on the filesystem identified by <path>, cancel it.
diff --git a/cmds-scrub.c b/cmds-scrub.c
index 03eb9ba..8c3d61c 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -1529,7 +1529,7 @@ out:
 
 static const char * const cmd_scrub_start_usage[] = {
 	"btrfs scrub start [-BdqrRf] [-c ioprio_class -n ioprio_classdata] <path>|<device>",
-	"Start a new scrub",
+	"Start a new scrub. If a scrub is already running, the new one fails.",
 	"",
 	"-B     do not background",
 	"-d     stats per device (-B only)",
@@ -1538,7 +1538,7 @@ static const char * const cmd_scrub_start_usage[] = {
 	"-R     raw print mode, print full data instead of summary"
 	"-c     set ioprio class (see ionice(1) manpage)",
 	"-n     set ioprio classdata (see ionice(1) manpage)",
-	"-f     force to skip checking whether scrub has started/resumed in userspace ",
+	"-f     force starting new scrub even if a scrub is already running.",
 	"       this is useful when scrub stats record file is damaged",
 	NULL
 };
-- 
1.8.1.4

Re: [PATCH v2] btrfs-progs: correct manpage option description for scrub

[Duncan]

Gui Hecheng posted on Wed, 30 Jul 2014 15:32:05 +0800 as excerpted:

> Also, add description of the potential failure as follows
>     "If a scrub is already running running, it fails."

Just a typo (running running?) in the description...

-- 
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 v2] btrfs-progs: correct manpage option description for scrub

[Gui Hecheng]

On Wed, 2014-07-30 at 10:26 +0000, Duncan wrote:
> Gui Hecheng posted on Wed, 30 Jul 2014 15:32:05 +0800 as excerpted:
> 
> > Also, add description of the potential failure as follows
> >     "If a scrub is already running running, it fails."
> 
> Just a typo (running running?) in the description...
> 
Hmmm, that's my fault. Thanks very much, I will correct it soon.

Re: [PATCH v2] btrfs-progs: correct manpage option description for scrub

[David Sterba]

On Thu, Jul 31, 2014 at 09:25:07AM +0800, Gui Hecheng wrote:
> On Wed, 2014-07-30 at 10:26 +0000, Duncan wrote:
> > Gui Hecheng posted on Wed, 30 Jul 2014 15:32:05 +0800 as excerpted:
> > 
> > > Also, add description of the potential failure as follows
> > >     "If a scrub is already running running, it fails."
> > 
> > Just a typo (running running?) in the description...
> > 
> Hmmm, that's my fault. Thanks very much, I will correct it soon.

I'll fix the changelog, no need to resend the patch.

Re: [PATCH 1/4] btrfs-progs: adjust the return values for scrub

[Satoru Takeuchi]

Hi Gui,

(2014/07/17 11:40), Gui Hecheng wrote:
> o Return 0 to indicate success,
>    when detected errors were corrected during scrubbing.
>    P.s. This is also to facilitate scripting when return value
>         is to be checked.
> o Warn the users if there are uncorrectable errors detected.
> 
> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> ---
>   cmds-scrub.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/cmds-scrub.c b/cmds-scrub.c
> index 5265a2b..f9e2b40 100644
> --- a/cmds-scrub.c
> +++ b/cmds-scrub.c
> @@ -1514,14 +1514,17 @@ out:
>   	}
>   	close_file_or_dir(fdmnt, dirstream);
>   
> -	if (nothing_to_resume)
> -		return 2;
>   	if (err)
>   		return 1;
> -	if (e_correctable)
> +	if (nothing_to_resume)
> +		return 2;
> +	if (e_uncorrectable) {
> +		ERR(!do_quiet, "ERROR: There are uncorrectable errors.\n");
>   		return 3;
> -	if (e_uncorrectable)
> -		return 4;
> +	}
> +	if (e_correctable)
> +		ERR(!do_quiet, "WARNING: errors detected during scrubbing, corrected.\n");
> +

1. ERR() messages are not necessary since start command
   reports its status in the middle of scrub_start().
   It includes both the number of correctable/uncorrectable errors.

2. It prints messages even if this program runs as foreground mode.
   Checking do_print is necessary.

3. Whether correctable/uncorrectable error exists in a file system
   is not related to the exist status of scrub. The role of this
   program is

    a) reading all data from all devices and verify checksums, and
    b) correct checksum errors "if possible".

   So, I consider scrub only returns error if this program itself
   failed to finish.

Thanks,
Satoru

>   	return 0;
>   }
>   
> 

Re: [PATCH 1/4] btrfs-progs: adjust the return values for scrub

[Satoru Takeuchi]

Hi Gui,

(2014/07/30 15:03), Satoru Takeuchi wrote:
> Hi Gui,
> 
> (2014/07/17 11:40), Gui Hecheng wrote:
>> o Return 0 to indicate success,
>>     when detected errors were corrected during scrubbing.
>>     P.s. This is also to facilitate scripting when return value
>>          is to be checked.
>> o Warn the users if there are uncorrectable errors detected.
>>
>> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
>> ---
>>    cmds-scrub.c | 13 ++++++++-----
>>    1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/cmds-scrub.c b/cmds-scrub.c
>> index 5265a2b..f9e2b40 100644
>> --- a/cmds-scrub.c
>> +++ b/cmds-scrub.c
>> @@ -1514,14 +1514,17 @@ out:
>>    	}
>>    	close_file_or_dir(fdmnt, dirstream);
>>    
>> -	if (nothing_to_resume)
>> -		return 2;
>>    	if (err)
>>    		return 1;
>> -	if (e_correctable)
>> +	if (nothing_to_resume)
>> +		return 2;
>> +	if (e_uncorrectable) {
>> +		ERR(!do_quiet, "ERROR: There are uncorrectable errors.\n");
>>    		return 3;
>> -	if (e_uncorrectable)
>> -		return 4;
>> +	}
>> +	if (e_correctable)
>> +		ERR(!do_quiet, "WARNING: errors detected during scrubbing, corrected.\n");
>> +
> 
> 1. ERR() messages are not necessary since start command
>     reports its status in the middle of scrub_start().
>     It includes both the number of correctable/uncorrectable errors.
> 
> 2. It prints messages even if this program runs as foreground mode.
>     Checking do_print is necessary.
> 
> 3. Whether correctable/uncorrectable error exists in a file system
>     is not related to the exist status of scrub. The role of this
>     program is
> 
>      a) reading all data from all devices and verify checksums, and
>      b) correct checksum errors "if possible".
> 
>     So, I consider scrub only returns error if this program itself
>     failed to finish.

Sorry, I understood what you said. It's no problem to exit with
error codes to detect whether correctable/uncorrectable errors happen
or not. Sorry for noise.

Thanks,
Satoru

> 
> Thanks,
> Satoru
> 
>>    	return 0;
>>    }
>>    
>>
> 
> --
> 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 1/4] btrfs-progs: adjust the return values for scrub

[Gui Hecheng]

Hi Satoru, I just give a reply to the former 2 mails together here.

On Wed, 2014-07-30 at 15:36 +0900, Satoru Takeuchi wrote:
> Hi Gui,
> 
> (2014/07/30 15:03), Satoru Takeuchi wrote:
> > Hi Gui,
> > 
> > (2014/07/17 11:40), Gui Hecheng wrote:
> >> o Return 0 to indicate success,
> >>     when detected errors were corrected during scrubbing.
> >>     P.s. This is also to facilitate scripting when return value
> >>          is to be checked.
> >> o Warn the users if there are uncorrectable errors detected.
> >>
> >> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> >> ---
> >>    cmds-scrub.c | 13 ++++++++-----
> >>    1 file changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/cmds-scrub.c b/cmds-scrub.c
> >> index 5265a2b..f9e2b40 100644
> >> --- a/cmds-scrub.c
> >> +++ b/cmds-scrub.c
> >> @@ -1514,14 +1514,17 @@ out:
> >>    	}
> >>    	close_file_or_dir(fdmnt, dirstream);
> >>    
> >> -	if (nothing_to_resume)
> >> -		return 2;
> >>    	if (err)
> >>    		return 1;
> >> -	if (e_correctable)
> >> +	if (nothing_to_resume)
> >> +		return 2;
> >> +	if (e_uncorrectable) {
> >> +		ERR(!do_quiet, "ERROR: There are uncorrectable errors.\n");
> >>    		return 3;
> >> -	if (e_uncorrectable)
> >> -		return 4;
> >> +	}
> >> +	if (e_correctable)
> >> +		ERR(!do_quiet, "WARNING: errors detected during scrubbing, corrected.\n");
> >> +
> > 
> > 1. ERR() messages are not necessary since start command
> >     reports its status in the middle of scrub_start().
> >     It includes both the number of correctable/uncorrectable errors.
	Ok, it may not seems "neccessary", but these ERR()s serve as
conclusions at the end to the users. Because users care whether a scrub
is able to detect errors and if yes, whether the errors are corrected.
So it adds to user experience.

> > 2. It prints messages even if this program runs as foreground mode.
> >     Checking do_print is necessary.
	The do_print serves to control whether "sub procedures(such as
pthread_create/join, scrub_write_progress etc.)" failed or not in
foreground mode, so do_print should not effect conclusion outputs since
all the "sub procedures" succeeded.
 
> > 3. Whether correctable/uncorrectable error exists in a file system
> >     is not related to the exist status of scrub. The role of this
> >     program is
> > 
> >      a) reading all data from all devices and verify checksums, and
> >      b) correct checksum errors "if possible".
> > 
> >     So, I consider scrub only returns error if this program itself
> >     failed to finish.
> 
> Sorry, I understood what you said. It's no problem to exit with
> error codes to detect whether correctable/uncorrectable errors happen
> or not. Sorry for noise.
> 
> Thanks,
> Satoru

Oh, it is really kind of you to spend time reviewing. Thanks very much.

-Gui
> > 
> > Thanks,
> > Satoru
> > 
> >>    	return 0;
> >>    }
> >>    
> >>
> > 
> > --
> > 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
> > 
>