* [PATCH 1/4] btrfs-progs: adjust the return values for scrub
@ 2014-07-17 2:40 Gui Hecheng
2014-07-17 2:40 ` [PATCH 2/4] btrfs-progs: remove unnecessary judgment for fd in scrub Gui Hecheng
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Gui Hecheng @ 2014-07-17 2:40 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/4] btrfs-progs: remove unnecessary judgment for fd in scrub
2014-07-17 2:40 [PATCH 1/4] btrfs-progs: adjust the return values for scrub Gui Hecheng
@ 2014-07-17 2:40 ` Gui Hecheng
2014-07-30 6:07 ` Satoru Takeuchi
2014-07-17 2:40 ` [PATCH 3/4] btrfs-progs: replace a confusing raw number with a macro Gui Hecheng
` (2 subsequent siblings)
3 siblings, 1 reply; 18+ messages in thread
From: Gui Hecheng @ 2014-07-17 2:40 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/4] btrfs-progs: replace a confusing raw number with a macro
2014-07-17 2:40 [PATCH 1/4] btrfs-progs: adjust the return values for scrub Gui Hecheng
2014-07-17 2:40 ` [PATCH 2/4] btrfs-progs: remove unnecessary judgment for fd in scrub Gui Hecheng
@ 2014-07-17 2:40 ` Gui Hecheng
2014-07-29 12:16 ` David Sterba
2014-07-17 2:40 ` [PATCH 4/4] btrfs-progs: correct manpage option description for scrub Gui Hecheng
2014-07-30 6:03 ` [PATCH 1/4] btrfs-progs: adjust the return values " Satoru Takeuchi
3 siblings, 1 reply; 18+ messages in thread
From: Gui Hecheng @ 2014-07-17 2:40 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 4/4] btrfs-progs: correct manpage option description for scrub
2014-07-17 2:40 [PATCH 1/4] btrfs-progs: adjust the return values for scrub Gui Hecheng
2014-07-17 2:40 ` [PATCH 2/4] btrfs-progs: remove unnecessary judgment for fd in scrub Gui Hecheng
2014-07-17 2:40 ` [PATCH 3/4] btrfs-progs: replace a confusing raw number with a macro Gui Hecheng
@ 2014-07-17 2:40 ` Gui Hecheng
2014-07-30 5:20 ` Satoru Takeuchi
2014-07-30 6:03 ` [PATCH 1/4] btrfs-progs: adjust the return values " Satoru Takeuchi
3 siblings, 1 reply; 18+ messages in thread
From: Gui Hecheng @ 2014-07-17 2:40 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] btrfs-progs: replace a confusing raw number with a macro
2014-07-17 2:40 ` [PATCH 3/4] btrfs-progs: replace a confusing raw number with a macro Gui Hecheng
@ 2014-07-29 12:16 ` David Sterba
2014-07-29 12:19 ` David Sterba
0 siblings, 1 reply; 18+ messages in thread
From: David Sterba @ 2014-07-29 12:16 UTC (permalink / raw)
To: Gui Hecheng; +Cc: linux-btrfs
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] btrfs-progs: replace a confusing raw number with a macro
2014-07-29 12:16 ` David Sterba
@ 2014-07-29 12:19 ` David Sterba
2014-07-30 1:17 ` Gui Hecheng
0 siblings, 1 reply; 18+ messages in thread
From: David Sterba @ 2014-07-29 12:19 UTC (permalink / raw)
To: dsterba, Gui Hecheng, linux-btrfs
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] btrfs-progs: replace a confusing raw number with a macro
2014-07-29 12:19 ` David Sterba
@ 2014-07-30 1:17 ` Gui Hecheng
0 siblings, 0 replies; 18+ messages in thread
From: Gui Hecheng @ 2014-07-30 1:17 UTC (permalink / raw)
To: dsterba; +Cc: linux-btrfs
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 :)
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] btrfs-progs: correct manpage option description for scrub
2014-07-17 2:40 ` [PATCH 4/4] btrfs-progs: correct manpage option description for scrub Gui Hecheng
@ 2014-07-30 5:20 ` Satoru Takeuchi
2014-07-30 5:39 ` Gui Hecheng
0 siblings, 1 reply; 18+ messages in thread
From: Satoru Takeuchi @ 2014-07-30 5:20 UTC (permalink / raw)
To: Gui Hecheng, linux-btrfs
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>::
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] btrfs-progs: correct manpage option description for scrub
2014-07-30 5:20 ` Satoru Takeuchi
@ 2014-07-30 5:39 ` Gui Hecheng
2014-07-30 6:09 ` Satoru Takeuchi
0 siblings, 1 reply; 18+ messages in thread
From: Gui Hecheng @ 2014-07-30 5:39 UTC (permalink / raw)
To: Satoru Takeuchi; +Cc: linux-btrfs
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>::
> >
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] btrfs-progs: adjust the return values for scrub
2014-07-17 2:40 [PATCH 1/4] btrfs-progs: adjust the return values for scrub Gui Hecheng
` (2 preceding siblings ...)
2014-07-17 2:40 ` [PATCH 4/4] btrfs-progs: correct manpage option description for scrub Gui Hecheng
@ 2014-07-30 6:03 ` Satoru Takeuchi
2014-07-30 6:36 ` Satoru Takeuchi
3 siblings, 1 reply; 18+ messages in thread
From: Satoru Takeuchi @ 2014-07-30 6:03 UTC (permalink / raw)
To: Gui Hecheng, linux-btrfs
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;
> }
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] btrfs-progs: remove unnecessary judgment for fd in scrub
2014-07-17 2:40 ` [PATCH 2/4] btrfs-progs: remove unnecessary judgment for fd in scrub Gui Hecheng
@ 2014-07-30 6:07 ` Satoru Takeuchi
0 siblings, 0 replies; 18+ messages in thread
From: Satoru Takeuchi @ 2014-07-30 6:07 UTC (permalink / raw)
To: Gui Hecheng, linux-btrfs
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);
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] btrfs-progs: correct manpage option description for scrub
2014-07-30 5:39 ` Gui Hecheng
@ 2014-07-30 6:09 ` Satoru Takeuchi
2014-07-30 7:32 ` [PATCH v2] " Gui Hecheng
0 siblings, 1 reply; 18+ messages in thread
From: Satoru Takeuchi @ 2014-07-30 6:09 UTC (permalink / raw)
To: Gui Hecheng; +Cc: linux-btrfs
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] btrfs-progs: adjust the return values for scrub
2014-07-30 6:03 ` [PATCH 1/4] btrfs-progs: adjust the return values " Satoru Takeuchi
@ 2014-07-30 6:36 ` Satoru Takeuchi
2014-07-30 7:27 ` Gui Hecheng
0 siblings, 1 reply; 18+ messages in thread
From: Satoru Takeuchi @ 2014-07-30 6:36 UTC (permalink / raw)
To: Gui Hecheng, linux-btrfs
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] btrfs-progs: adjust the return values for scrub
2014-07-30 6:36 ` Satoru Takeuchi
@ 2014-07-30 7:27 ` Gui Hecheng
0 siblings, 0 replies; 18+ messages in thread
From: Gui Hecheng @ 2014-07-30 7:27 UTC (permalink / raw)
To: Satoru Takeuchi; +Cc: linux-btrfs
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
> >
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2] btrfs-progs: correct manpage option description for scrub
2014-07-30 6:09 ` Satoru Takeuchi
@ 2014-07-30 7:32 ` Gui Hecheng
2014-07-30 10:26 ` Duncan
0 siblings, 1 reply; 18+ messages in thread
From: Gui Hecheng @ 2014-07-30 7:32 UTC (permalink / raw)
To: linux-btrfs; +Cc: takeuchi_satoru, dsterba, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2] btrfs-progs: correct manpage option description for scrub
2014-07-30 7:32 ` [PATCH v2] " Gui Hecheng
@ 2014-07-30 10:26 ` Duncan
2014-07-31 1:25 ` Gui Hecheng
0 siblings, 1 reply; 18+ messages in thread
From: Duncan @ 2014-07-30 10:26 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] btrfs-progs: correct manpage option description for scrub
2014-07-30 10:26 ` Duncan
@ 2014-07-31 1:25 ` Gui Hecheng
2014-07-31 12:24 ` David Sterba
0 siblings, 1 reply; 18+ messages in thread
From: Gui Hecheng @ 2014-07-31 1:25 UTC (permalink / raw)
To: Duncan; +Cc: linux-btrfs
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] btrfs-progs: correct manpage option description for scrub
2014-07-31 1:25 ` Gui Hecheng
@ 2014-07-31 12:24 ` David Sterba
0 siblings, 0 replies; 18+ messages in thread
From: David Sterba @ 2014-07-31 12:24 UTC (permalink / raw)
To: Gui Hecheng; +Cc: Duncan, linux-btrfs
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2014-07-31 12:24 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-17 2:40 [PATCH 1/4] btrfs-progs: adjust the return values for scrub Gui Hecheng
2014-07-17 2:40 ` [PATCH 2/4] btrfs-progs: remove unnecessary judgment for fd in scrub Gui Hecheng
2014-07-30 6:07 ` Satoru Takeuchi
2014-07-17 2:40 ` [PATCH 3/4] btrfs-progs: replace a confusing raw number with a macro Gui Hecheng
2014-07-29 12:16 ` David Sterba
2014-07-29 12:19 ` David Sterba
2014-07-30 1:17 ` Gui Hecheng
2014-07-17 2:40 ` [PATCH 4/4] btrfs-progs: correct manpage option description for scrub Gui Hecheng
2014-07-30 5:20 ` Satoru Takeuchi
2014-07-30 5:39 ` Gui Hecheng
2014-07-30 6:09 ` Satoru Takeuchi
2014-07-30 7:32 ` [PATCH v2] " Gui Hecheng
2014-07-30 10:26 ` Duncan
2014-07-31 1:25 ` Gui Hecheng
2014-07-31 12:24 ` David Sterba
2014-07-30 6:03 ` [PATCH 1/4] btrfs-progs: adjust the return values " Satoru Takeuchi
2014-07-30 6:36 ` Satoru Takeuchi
2014-07-30 7:27 ` Gui Hecheng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).