* Re: [f2fs-dev] [PATCH] fsck.f2fs: correct return value
2020-08-05 8:09 [f2fs-dev] [PATCH] fsck.f2fs: correct return value Chao Yu
@ 2020-08-05 9:38 ` Norbert Lange
2020-08-05 21:59 ` Norbert Lange
2020-08-18 6:52 ` Jaegeuk Kim
2 siblings, 0 replies; 6+ messages in thread
From: Norbert Lange @ 2020-08-05 9:38 UTC (permalink / raw)
To: Chao Yu; +Cc: jaegeuk, linux-f2fs-devel
Am Mi., 5. Aug. 2020 um 10:10 Uhr schrieb Chao Yu <yuchao0@huawei.com>:
>
> As Norbert Lange reported:
>
> "
> $ fsck.f2fs -a /dev/mmcblk0p5; echo $?
> Info: Fix the reported corruption.
> Info: Mounted device!
> Info: Check FS only on RO mounted device
> Error: Failed to open the device!
> 255
> "
>
> Michael Laß reminds:
>
> "
> I think the return value is exactly the problem here. See fsck(8) (
> https://linux.die.net/man/8/fsck) which specifies the return values.
> Systemd looks at these and decides how to proceed:
>
> https://github.com/systemd/systemd/blob/a859abf062cef1511e4879c4ee39c6036ebeaec8/src/fsck/fsck.c#L407
>
> That means, if fsck.f2fs returns 255, then
> the FSCK_SYSTEM_SHOULD_REBOOT bit is set and systemd will reboot.
> "
>
> So the problem here is fsck.f2fs didn't return correct value to userspace
> apps, result in later unexpected behavior of rebooting, let's fix this.
>
> Reported-by: Norbert Lange <nolange79@gmail.com>
> Reported-by: Michael Laß <bevan@bi-co.net>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
> fsck/fsck.h | 11 +++++++++++
> fsck/main.c | 27 +++++++++++++++++++++------
> 2 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/fsck/fsck.h b/fsck/fsck.h
> index e86730c34fc4..c5e85fefa07b 100644
> --- a/fsck/fsck.h
> +++ b/fsck/fsck.h
> @@ -13,6 +13,17 @@
>
> #include "f2fs.h"
>
> +enum {
> + FSCK_SUCCESS = 0,
> + FSCK_ERROR_CORRECTED = 1 << 0,
> + FSCK_SYSTEM_SHOULD_REBOOT = 1 << 1,
> + FSCK_ERRORS_LEFT_UNCORRECTED = 1 << 2,
> + FSCK_OPERATIONAL_ERROR = 1 << 3,
> + FSCK_USAGE_OR_SYNTAX_ERROR = 1 << 4,
> + FSCK_USER_CANCELLED = 1 << 5,
> + FSCK_SHARED_LIB_ERROR = 1 << 7,
> +};
> +
> struct quota_ctx;
>
> #define FSCK_UNMATCHED_EXTENT 0x00000001
> diff --git a/fsck/main.c b/fsck/main.c
> index 9a1596f35e79..11d472b9941c 100644
> --- a/fsck/main.c
> +++ b/fsck/main.c
> @@ -630,7 +630,7 @@ void f2fs_parse_options(int argc, char *argv[])
> error_out(prog);
> }
>
> -static void do_fsck(struct f2fs_sb_info *sbi)
> +static int do_fsck(struct f2fs_sb_info *sbi)
> {
> struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
> u32 flag = le32_to_cpu(ckpt->ckpt_flags);
> @@ -655,7 +655,7 @@ static void do_fsck(struct f2fs_sb_info *sbi)
> } else {
> MSG(0, "[FSCK] F2FS metadata [Ok..]");
> fsck_free(sbi);
> - return;
> + return FSCK_SUCCESS;
> }
>
> if (!c.ro)
> @@ -687,7 +687,7 @@ static void do_fsck(struct f2fs_sb_info *sbi)
> ret = quota_init_context(sbi);
> if (ret) {
> ASSERT_MSG("quota_init_context failure: %d", ret);
> - return;
> + return FSCK_OPERATIONAL_ERROR;
> }
> }
> fsck_chk_orphan_node(sbi);
> @@ -695,8 +695,14 @@ static void do_fsck(struct f2fs_sb_info *sbi)
> F2FS_FT_DIR, TYPE_INODE, &blk_cnt, NULL);
> fsck_chk_quota_files(sbi);
>
> - fsck_verify(sbi);
> + ret = fsck_verify(sbi);
> fsck_free(sbi);
> +
> + if (!c.bug_on)
> + return FSCK_SUCCESS;
> + if (!ret)
> + return FSCK_ERROR_CORRECTED;
> + return FSCK_ERRORS_LEFT_UNCORRECTED;
> }
>
> static void do_dump(struct f2fs_sb_info *sbi)
> @@ -833,11 +839,15 @@ int main(int argc, char **argv)
> if (c.func != DUMP && f2fs_devs_are_umounted() < 0) {
> if (errno == EBUSY) {
> ret = -1;
> + if (c.func == FSCK)
> + ret = FSCK_OPERATIONAL_ERROR;
> goto quick_err;
> }
> if (!c.ro || c.func == DEFRAG) {
> MSG(0, "\tError: Not available on mounted device!\n");
> ret = -1;
> + if (c.func == FSCK)
> + ret = FSCK_OPERATIONAL_ERROR;
> goto quick_err;
> }
>
> @@ -854,6 +864,8 @@ int main(int argc, char **argv)
> /* Get device */
> if (f2fs_get_device_info() < 0) {
> ret = -1;
> + if (c.func == FSCK)
> + ret = FSCK_OPERATIONAL_ERROR;
> goto quick_err;
> }
>
> @@ -873,7 +885,7 @@ fsck_again:
>
> switch (c.func) {
> case FSCK:
> - do_fsck(sbi);
> + ret = do_fsck(sbi);
> break;
> #ifdef WITH_DUMP
> case DUMP:
> @@ -935,8 +947,11 @@ retry:
> }
> }
> ret = f2fs_finalize_device();
> - if (ret < 0)
> + if (ret) {
> + if (c.func == FSCK)
> + return FSCK_OPERATIONAL_ERROR;
> return ret;
> + }
>
> printf("\nDone: %lf secs\n", (get_boottime_ns() - start) / 1000000000.0);
> return 0;
> --
> 2.26.2
>
Applied this to 1.13.0, resolving a few simple conflicts. This fixes
the issue with systemd noot booting.
Thanks, Norbert
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [f2fs-dev] [PATCH] fsck.f2fs: correct return value
2020-08-05 8:09 [f2fs-dev] [PATCH] fsck.f2fs: correct return value Chao Yu
2020-08-05 9:38 ` Norbert Lange
@ 2020-08-05 21:59 ` Norbert Lange
2020-08-07 2:03 ` Chao Yu
2020-08-18 6:52 ` Jaegeuk Kim
2 siblings, 1 reply; 6+ messages in thread
From: Norbert Lange @ 2020-08-05 21:59 UTC (permalink / raw)
To: Chao Yu; +Cc: jaegeuk, linux-f2fs-devel
Am Mi., 5. Aug. 2020 um 10:10 Uhr schrieb Chao Yu <yuchao0@huawei.com>:
>
> As Norbert Lange reported:
>
> "
> $ fsck.f2fs -a /dev/mmcblk0p5; echo $?
> Info: Fix the reported corruption.
> Info: Mounted device!
> Info: Check FS only on RO mounted device
> Error: Failed to open the device!
> 255
> "
>
> Michael Laß reminds:
>
> "
> I think the return value is exactly the problem here. See fsck(8) (
> https://linux.die.net/man/8/fsck) which specifies the return values.
> Systemd looks at these and decides how to proceed:
>
> https://github.com/systemd/systemd/blob/a859abf062cef1511e4879c4ee39c6036ebeaec8/src/fsck/fsck.c#L407
>
> That means, if fsck.f2fs returns 255, then
> the FSCK_SYSTEM_SHOULD_REBOOT bit is set and systemd will reboot.
> "
>
> So the problem here is fsck.f2fs didn't return correct value to userspace
> apps, result in later unexpected behavior of rebooting, let's fix this.
>
> Reported-by: Norbert Lange <nolange79@gmail.com>
> Reported-by: Michael Laß <bevan@bi-co.net>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
> fsck/fsck.h | 11 +++++++++++
> fsck/main.c | 27 +++++++++++++++++++++------
> 2 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/fsck/fsck.h b/fsck/fsck.h
> index e86730c34fc4..c5e85fefa07b 100644
> --- a/fsck/fsck.h
> +++ b/fsck/fsck.h
> @@ -13,6 +13,17 @@
>
> #include "f2fs.h"
>
> +enum {
> + FSCK_SUCCESS = 0,
> + FSCK_ERROR_CORRECTED = 1 << 0,
> + FSCK_SYSTEM_SHOULD_REBOOT = 1 << 1,
> + FSCK_ERRORS_LEFT_UNCORRECTED = 1 << 2,
> + FSCK_OPERATIONAL_ERROR = 1 << 3,
> + FSCK_USAGE_OR_SYNTAX_ERROR = 1 << 4,
> + FSCK_USER_CANCELLED = 1 << 5,
> + FSCK_SHARED_LIB_ERROR = 1 << 7,
> +};
> +
> struct quota_ctx;
>
> #define FSCK_UNMATCHED_EXTENT 0x00000001
> diff --git a/fsck/main.c b/fsck/main.c
> index 9a1596f35e79..11d472b9941c 100644
> --- a/fsck/main.c
> +++ b/fsck/main.c
> @@ -630,7 +630,7 @@ void f2fs_parse_options(int argc, char *argv[])
> error_out(prog);
> }
>
> -static void do_fsck(struct f2fs_sb_info *sbi)
> +static int do_fsck(struct f2fs_sb_info *sbi)
> {
> struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
> u32 flag = le32_to_cpu(ckpt->ckpt_flags);
> @@ -655,7 +655,7 @@ static void do_fsck(struct f2fs_sb_info *sbi)
> } else {
> MSG(0, "[FSCK] F2FS metadata [Ok..]");
> fsck_free(sbi);
> - return;
> + return FSCK_SUCCESS;
> }
>
> if (!c.ro)
> @@ -687,7 +687,7 @@ static void do_fsck(struct f2fs_sb_info *sbi)
> ret = quota_init_context(sbi);
> if (ret) {
> ASSERT_MSG("quota_init_context failure: %d", ret);
> - return;
> + return FSCK_OPERATIONAL_ERROR;
> }
> }
> fsck_chk_orphan_node(sbi);
> @@ -695,8 +695,14 @@ static void do_fsck(struct f2fs_sb_info *sbi)
> F2FS_FT_DIR, TYPE_INODE, &blk_cnt, NULL);
> fsck_chk_quota_files(sbi);
>
> - fsck_verify(sbi);
> + ret = fsck_verify(sbi);
> fsck_free(sbi);
> +
> + if (!c.bug_on)
> + return FSCK_SUCCESS;
> + if (!ret)
> + return FSCK_ERROR_CORRECTED;
> + return FSCK_ERRORS_LEFT_UNCORRECTED;
> }
>
> static void do_dump(struct f2fs_sb_info *sbi)
> @@ -833,11 +839,15 @@ int main(int argc, char **argv)
> if (c.func != DUMP && f2fs_devs_are_umounted() < 0) {
> if (errno == EBUSY) {
> ret = -1;
> + if (c.func == FSCK)
> + ret = FSCK_OPERATIONAL_ERROR;
> goto quick_err;
> }
> if (!c.ro || c.func == DEFRAG) {
> MSG(0, "\tError: Not available on mounted device!\n");
> ret = -1;
> + if (c.func == FSCK)
> + ret = FSCK_OPERATIONAL_ERROR;
> goto quick_err;
> }
>
> @@ -854,6 +864,8 @@ int main(int argc, char **argv)
> /* Get device */
> if (f2fs_get_device_info() < 0) {
> ret = -1;
> + if (c.func == FSCK)
> + ret = FSCK_OPERATIONAL_ERROR;
> goto quick_err;
> }
>
> @@ -873,7 +885,7 @@ fsck_again:
>
> switch (c.func) {
> case FSCK:
> - do_fsck(sbi);
> + ret = do_fsck(sbi);
This returncode will never be used.
> break;
> #ifdef WITH_DUMP
> case DUMP:
> @@ -935,8 +947,11 @@ retry:
> }
> }
> ret = f2fs_finalize_device();
> - if (ret < 0)
> + if (ret) {
> + if (c.func == FSCK)
> + return FSCK_OPERATIONAL_ERROR;
> return ret;
> + }
>
> printf("\nDone: %lf secs\n", (get_boottime_ns() - start) / 1000000000.0);
> return 0;
> --
> 2.26.2
>
Norbert
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [f2fs-dev] [PATCH] fsck.f2fs: correct return value
2020-08-05 21:59 ` Norbert Lange
@ 2020-08-07 2:03 ` Chao Yu
0 siblings, 0 replies; 6+ messages in thread
From: Chao Yu @ 2020-08-07 2:03 UTC (permalink / raw)
To: Norbert Lange; +Cc: jaegeuk, linux-f2fs-devel
Norbert, fixed, thanks for pointing it out. :)
Thanks,
On 2020/8/6 5:59, Norbert Lange wrote:
> Am Mi., 5. Aug. 2020 um 10:10 Uhr schrieb Chao Yu <yuchao0@huawei.com>:
>>
>> As Norbert Lange reported:
>>
>> "
>> $ fsck.f2fs -a /dev/mmcblk0p5; echo $?
>> Info: Fix the reported corruption.
>> Info: Mounted device!
>> Info: Check FS only on RO mounted device
>> Error: Failed to open the device!
>> 255
>> "
>>
>> Michael Laß reminds:
>>
>> "
>> I think the return value is exactly the problem here. See fsck(8) (
>> https://linux.die.net/man/8/fsck) which specifies the return values.
>> Systemd looks at these and decides how to proceed:
>>
>> https://github.com/systemd/systemd/blob/a859abf062cef1511e4879c4ee39c6036ebeaec8/src/fsck/fsck.c#L407
>>
>> That means, if fsck.f2fs returns 255, then
>> the FSCK_SYSTEM_SHOULD_REBOOT bit is set and systemd will reboot.
>> "
>>
>> So the problem here is fsck.f2fs didn't return correct value to userspace
>> apps, result in later unexpected behavior of rebooting, let's fix this.
>>
>> Reported-by: Norbert Lange <nolange79@gmail.com>
>> Reported-by: Michael Laß <bevan@bi-co.net>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>> fsck/fsck.h | 11 +++++++++++
>> fsck/main.c | 27 +++++++++++++++++++++------
>> 2 files changed, 32 insertions(+), 6 deletions(-)
>>
>> diff --git a/fsck/fsck.h b/fsck/fsck.h
>> index e86730c34fc4..c5e85fefa07b 100644
>> --- a/fsck/fsck.h
>> +++ b/fsck/fsck.h
>> @@ -13,6 +13,17 @@
>>
>> #include "f2fs.h"
>>
>> +enum {
>> + FSCK_SUCCESS = 0,
>> + FSCK_ERROR_CORRECTED = 1 << 0,
>> + FSCK_SYSTEM_SHOULD_REBOOT = 1 << 1,
>> + FSCK_ERRORS_LEFT_UNCORRECTED = 1 << 2,
>> + FSCK_OPERATIONAL_ERROR = 1 << 3,
>> + FSCK_USAGE_OR_SYNTAX_ERROR = 1 << 4,
>> + FSCK_USER_CANCELLED = 1 << 5,
>> + FSCK_SHARED_LIB_ERROR = 1 << 7,
>> +};
>> +
>> struct quota_ctx;
>>
>> #define FSCK_UNMATCHED_EXTENT 0x00000001
>> diff --git a/fsck/main.c b/fsck/main.c
>> index 9a1596f35e79..11d472b9941c 100644
>> --- a/fsck/main.c
>> +++ b/fsck/main.c
>> @@ -630,7 +630,7 @@ void f2fs_parse_options(int argc, char *argv[])
>> error_out(prog);
>> }
>>
>> -static void do_fsck(struct f2fs_sb_info *sbi)
>> +static int do_fsck(struct f2fs_sb_info *sbi)
>> {
>> struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
>> u32 flag = le32_to_cpu(ckpt->ckpt_flags);
>> @@ -655,7 +655,7 @@ static void do_fsck(struct f2fs_sb_info *sbi)
>> } else {
>> MSG(0, "[FSCK] F2FS metadata [Ok..]");
>> fsck_free(sbi);
>> - return;
>> + return FSCK_SUCCESS;
>> }
>>
>> if (!c.ro)
>> @@ -687,7 +687,7 @@ static void do_fsck(struct f2fs_sb_info *sbi)
>> ret = quota_init_context(sbi);
>> if (ret) {
>> ASSERT_MSG("quota_init_context failure: %d", ret);
>> - return;
>> + return FSCK_OPERATIONAL_ERROR;
>> }
>> }
>> fsck_chk_orphan_node(sbi);
>> @@ -695,8 +695,14 @@ static void do_fsck(struct f2fs_sb_info *sbi)
>> F2FS_FT_DIR, TYPE_INODE, &blk_cnt, NULL);
>> fsck_chk_quota_files(sbi);
>>
>> - fsck_verify(sbi);
>> + ret = fsck_verify(sbi);
>> fsck_free(sbi);
>> +
>> + if (!c.bug_on)
>> + return FSCK_SUCCESS;
>> + if (!ret)
>> + return FSCK_ERROR_CORRECTED;
>> + return FSCK_ERRORS_LEFT_UNCORRECTED;
>> }
>>
>> static void do_dump(struct f2fs_sb_info *sbi)
>> @@ -833,11 +839,15 @@ int main(int argc, char **argv)
>> if (c.func != DUMP && f2fs_devs_are_umounted() < 0) {
>> if (errno == EBUSY) {
>> ret = -1;
>> + if (c.func == FSCK)
>> + ret = FSCK_OPERATIONAL_ERROR;
>> goto quick_err;
>> }
>> if (!c.ro || c.func == DEFRAG) {
>> MSG(0, "\tError: Not available on mounted device!\n");
>> ret = -1;
>> + if (c.func == FSCK)
>> + ret = FSCK_OPERATIONAL_ERROR;
>> goto quick_err;
>> }
>>
>> @@ -854,6 +864,8 @@ int main(int argc, char **argv)
>> /* Get device */
>> if (f2fs_get_device_info() < 0) {
>> ret = -1;
>> + if (c.func == FSCK)
>> + ret = FSCK_OPERATIONAL_ERROR;
>> goto quick_err;
>> }
>>
>> @@ -873,7 +885,7 @@ fsck_again:
>>
>> switch (c.func) {
>> case FSCK:
>> - do_fsck(sbi);
>> + ret = do_fsck(sbi);
>
> This returncode will never be used.
>
>> break;
>> #ifdef WITH_DUMP
>> case DUMP:
>> @@ -935,8 +947,11 @@ retry:
>> }
>> }
>> ret = f2fs_finalize_device();
>> - if (ret < 0)
>> + if (ret) {
>> + if (c.func == FSCK)
>> + return FSCK_OPERATIONAL_ERROR;
>> return ret;
>> + }
>>
>> printf("\nDone: %lf secs\n", (get_boottime_ns() - start) / 1000000000.0);
>> return 0;
>> --
>> 2.26.2
>>
>
> Norbert
> .
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [f2fs-dev] [PATCH] fsck.f2fs: correct return value
2020-08-05 8:09 [f2fs-dev] [PATCH] fsck.f2fs: correct return value Chao Yu
2020-08-05 9:38 ` Norbert Lange
2020-08-05 21:59 ` Norbert Lange
@ 2020-08-18 6:52 ` Jaegeuk Kim
2020-08-18 9:04 ` Chao Yu
2 siblings, 1 reply; 6+ messages in thread
From: Jaegeuk Kim @ 2020-08-18 6:52 UTC (permalink / raw)
To: Chao Yu; +Cc: nolange79, linux-f2fs-devel
On 08/05, Chao Yu wrote:
> As Norbert Lange reported:
>
> "
> $ fsck.f2fs -a /dev/mmcblk0p5; echo $?
> Info: Fix the reported corruption.
> Info: Mounted device!
> Info: Check FS only on RO mounted device
> Error: Failed to open the device!
> 255
> "
>
> Michael Laß reminds:
>
> "
> I think the return value is exactly the problem here. See fsck(8) (
> https://linux.die.net/man/8/fsck) which specifies the return values.
> Systemd looks at these and decides how to proceed:
>
> https://github.com/systemd/systemd/blob/a859abf062cef1511e4879c4ee39c6036ebeaec8/src/fsck/fsck.c#L407
>
> That means, if fsck.f2fs returns 255, then
> the FSCK_SYSTEM_SHOULD_REBOOT bit is set and systemd will reboot.
> "
>
> So the problem here is fsck.f2fs didn't return correct value to userspace
> apps, result in later unexpected behavior of rebooting, let's fix this.
>
> Reported-by: Norbert Lange <nolange79@gmail.com>
> Reported-by: Michael Laß <bevan@bi-co.net>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
> fsck/fsck.h | 11 +++++++++++
> fsck/main.c | 27 +++++++++++++++++++++------
> 2 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/fsck/fsck.h b/fsck/fsck.h
> index e86730c34fc4..c5e85fefa07b 100644
> --- a/fsck/fsck.h
> +++ b/fsck/fsck.h
> @@ -13,6 +13,17 @@
>
> #include "f2fs.h"
>
> +enum {
> + FSCK_SUCCESS = 0,
> + FSCK_ERROR_CORRECTED = 1 << 0,
> + FSCK_SYSTEM_SHOULD_REBOOT = 1 << 1,
> + FSCK_ERRORS_LEFT_UNCORRECTED = 1 << 2,
> + FSCK_OPERATIONAL_ERROR = 1 << 3,
> + FSCK_USAGE_OR_SYNTAX_ERROR = 1 << 4,
> + FSCK_USER_CANCELLED = 1 << 5,
> + FSCK_SHARED_LIB_ERROR = 1 << 7,
> +};
> +
> struct quota_ctx;
>
> #define FSCK_UNMATCHED_EXTENT 0x00000001
> diff --git a/fsck/main.c b/fsck/main.c
> index 9a1596f35e79..11d472b9941c 100644
> --- a/fsck/main.c
> +++ b/fsck/main.c
> @@ -630,7 +630,7 @@ void f2fs_parse_options(int argc, char *argv[])
> error_out(prog);
> }
>
> -static void do_fsck(struct f2fs_sb_info *sbi)
> +static int do_fsck(struct f2fs_sb_info *sbi)
> {
> struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
> u32 flag = le32_to_cpu(ckpt->ckpt_flags);
> @@ -655,7 +655,7 @@ static void do_fsck(struct f2fs_sb_info *sbi)
> } else {
> MSG(0, "[FSCK] F2FS metadata [Ok..]");
> fsck_free(sbi);
> - return;
> + return FSCK_SUCCESS;
> }
>
> if (!c.ro)
> @@ -687,7 +687,7 @@ static void do_fsck(struct f2fs_sb_info *sbi)
> ret = quota_init_context(sbi);
> if (ret) {
> ASSERT_MSG("quota_init_context failure: %d", ret);
> - return;
> + return FSCK_OPERATIONAL_ERROR;
> }
> }
> fsck_chk_orphan_node(sbi);
> @@ -695,8 +695,14 @@ static void do_fsck(struct f2fs_sb_info *sbi)
> F2FS_FT_DIR, TYPE_INODE, &blk_cnt, NULL);
> fsck_chk_quota_files(sbi);
>
> - fsck_verify(sbi);
> + ret = fsck_verify(sbi);
> fsck_free(sbi);
> +
> + if (!c.bug_on)
> + return FSCK_SUCCESS;
> + if (!ret)
> + return FSCK_ERROR_CORRECTED;
I applied this to get FSCK_ERROR_CORRECTED.
--- a/fsck/fsck.c
+++ b/fsck/fsck.c
@@ -3165,6 +3165,8 @@ int fsck_verify(struct f2fs_sb_info *sbi)
is_set_ckpt_flags(cp, CP_QUOTA_NEED_FSCK_FLAG)) {
write_checkpoints(sbi);
}
+ /* to return FSCK_ERROR_CORRECTED */
+ ret = 0;
}
return ret;
}
> + return FSCK_ERRORS_LEFT_UNCORRECTED;
> }
>
> static void do_dump(struct f2fs_sb_info *sbi)
> @@ -833,11 +839,15 @@ int main(int argc, char **argv)
> if (c.func != DUMP && f2fs_devs_are_umounted() < 0) {
> if (errno == EBUSY) {
> ret = -1;
> + if (c.func == FSCK)
> + ret = FSCK_OPERATIONAL_ERROR;
> goto quick_err;
> }
> if (!c.ro || c.func == DEFRAG) {
> MSG(0, "\tError: Not available on mounted device!\n");
> ret = -1;
> + if (c.func == FSCK)
> + ret = FSCK_OPERATIONAL_ERROR;
> goto quick_err;
> }
>
> @@ -854,6 +864,8 @@ int main(int argc, char **argv)
> /* Get device */
> if (f2fs_get_device_info() < 0) {
> ret = -1;
> + if (c.func == FSCK)
> + ret = FSCK_OPERATIONAL_ERROR;
> goto quick_err;
> }
>
> @@ -873,7 +885,7 @@ fsck_again:
>
> switch (c.func) {
> case FSCK:
> - do_fsck(sbi);
> + ret = do_fsck(sbi);
> break;
> #ifdef WITH_DUMP
> case DUMP:
> @@ -935,8 +947,11 @@ retry:
> }
> }
> ret = f2fs_finalize_device();
> - if (ret < 0)
> + if (ret) {
> + if (c.func == FSCK)
> + return FSCK_OPERATIONAL_ERROR;
> return ret;
> + }
>
> printf("\nDone: %lf secs\n", (get_boottime_ns() - start) / 1000000000.0);
> return 0;
> --
> 2.26.2
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [f2fs-dev] [PATCH] fsck.f2fs: correct return value
2020-08-18 6:52 ` Jaegeuk Kim
@ 2020-08-18 9:04 ` Chao Yu
0 siblings, 0 replies; 6+ messages in thread
From: Chao Yu @ 2020-08-18 9:04 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: nolange79, linux-f2fs-devel
On 2020/8/18 14:52, Jaegeuk Kim wrote:
> On 08/05, Chao Yu wrote:
>> As Norbert Lange reported:
>>
>> "
>> $ fsck.f2fs -a /dev/mmcblk0p5; echo $?
>> Info: Fix the reported corruption.
>> Info: Mounted device!
>> Info: Check FS only on RO mounted device
>> Error: Failed to open the device!
>> 255
>> "
>>
>> Michael Laß reminds:
>>
>> "
>> I think the return value is exactly the problem here. See fsck(8) (
>> https://linux.die.net/man/8/fsck) which specifies the return values.
>> Systemd looks at these and decides how to proceed:
>>
>> https://github.com/systemd/systemd/blob/a859abf062cef1511e4879c4ee39c6036ebeaec8/src/fsck/fsck.c#L407
>>
>> That means, if fsck.f2fs returns 255, then
>> the FSCK_SYSTEM_SHOULD_REBOOT bit is set and systemd will reboot.
>> "
>>
>> So the problem here is fsck.f2fs didn't return correct value to userspace
>> apps, result in later unexpected behavior of rebooting, let's fix this.
>>
>> Reported-by: Norbert Lange <nolange79@gmail.com>
>> Reported-by: Michael Laß <bevan@bi-co.net>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>> fsck/fsck.h | 11 +++++++++++
>> fsck/main.c | 27 +++++++++++++++++++++------
>> 2 files changed, 32 insertions(+), 6 deletions(-)
>>
>> diff --git a/fsck/fsck.h b/fsck/fsck.h
>> index e86730c34fc4..c5e85fefa07b 100644
>> --- a/fsck/fsck.h
>> +++ b/fsck/fsck.h
>> @@ -13,6 +13,17 @@
>>
>> #include "f2fs.h"
>>
>> +enum {
>> + FSCK_SUCCESS = 0,
>> + FSCK_ERROR_CORRECTED = 1 << 0,
>> + FSCK_SYSTEM_SHOULD_REBOOT = 1 << 1,
>> + FSCK_ERRORS_LEFT_UNCORRECTED = 1 << 2,
>> + FSCK_OPERATIONAL_ERROR = 1 << 3,
>> + FSCK_USAGE_OR_SYNTAX_ERROR = 1 << 4,
>> + FSCK_USER_CANCELLED = 1 << 5,
>> + FSCK_SHARED_LIB_ERROR = 1 << 7,
>> +};
>> +
>> struct quota_ctx;
>>
>> #define FSCK_UNMATCHED_EXTENT 0x00000001
>> diff --git a/fsck/main.c b/fsck/main.c
>> index 9a1596f35e79..11d472b9941c 100644
>> --- a/fsck/main.c
>> +++ b/fsck/main.c
>> @@ -630,7 +630,7 @@ void f2fs_parse_options(int argc, char *argv[])
>> error_out(prog);
>> }
>>
>> -static void do_fsck(struct f2fs_sb_info *sbi)
>> +static int do_fsck(struct f2fs_sb_info *sbi)
>> {
>> struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
>> u32 flag = le32_to_cpu(ckpt->ckpt_flags);
>> @@ -655,7 +655,7 @@ static void do_fsck(struct f2fs_sb_info *sbi)
>> } else {
>> MSG(0, "[FSCK] F2FS metadata [Ok..]");
>> fsck_free(sbi);
>> - return;
>> + return FSCK_SUCCESS;
>> }
>>
>> if (!c.ro)
>> @@ -687,7 +687,7 @@ static void do_fsck(struct f2fs_sb_info *sbi)
>> ret = quota_init_context(sbi);
>> if (ret) {
>> ASSERT_MSG("quota_init_context failure: %d", ret);
>> - return;
>> + return FSCK_OPERATIONAL_ERROR;
>> }
>> }
>> fsck_chk_orphan_node(sbi);
>> @@ -695,8 +695,14 @@ static void do_fsck(struct f2fs_sb_info *sbi)
>> F2FS_FT_DIR, TYPE_INODE, &blk_cnt, NULL);
>> fsck_chk_quota_files(sbi);
>>
>> - fsck_verify(sbi);
>> + ret = fsck_verify(sbi);
>> fsck_free(sbi);
>> +
>> + if (!c.bug_on)
>> + return FSCK_SUCCESS;
>> + if (!ret)
>> + return FSCK_ERROR_CORRECTED;
>
> I applied this to get FSCK_ERROR_CORRECTED.
>
> --- a/fsck/fsck.c
> +++ b/fsck/fsck.c
> @@ -3165,6 +3165,8 @@ int fsck_verify(struct f2fs_sb_info *sbi)
> is_set_ckpt_flags(cp, CP_QUOTA_NEED_FSCK_FLAG)) {
> write_checkpoints(sbi);
> }
> + /* to return FSCK_ERROR_CORRECTED */
> + ret = 0;
Correct, thanks. :)
Thanks,
> }
> return ret;
> }
>
>> + return FSCK_ERRORS_LEFT_UNCORRECTED;
>> }
>>
>> static void do_dump(struct f2fs_sb_info *sbi)
>> @@ -833,11 +839,15 @@ int main(int argc, char **argv)
>> if (c.func != DUMP && f2fs_devs_are_umounted() < 0) {
>> if (errno == EBUSY) {
>> ret = -1;
>> + if (c.func == FSCK)
>> + ret = FSCK_OPERATIONAL_ERROR;
>> goto quick_err;
>> }
>> if (!c.ro || c.func == DEFRAG) {
>> MSG(0, "\tError: Not available on mounted device!\n");
>> ret = -1;
>> + if (c.func == FSCK)
>> + ret = FSCK_OPERATIONAL_ERROR;
>> goto quick_err;
>> }
>>
>> @@ -854,6 +864,8 @@ int main(int argc, char **argv)
>> /* Get device */
>> if (f2fs_get_device_info() < 0) {
>> ret = -1;
>> + if (c.func == FSCK)
>> + ret = FSCK_OPERATIONAL_ERROR;
>> goto quick_err;
>> }
>>
>> @@ -873,7 +885,7 @@ fsck_again:
>>
>> switch (c.func) {
>> case FSCK:
>> - do_fsck(sbi);
>> + ret = do_fsck(sbi);
>> break;
>> #ifdef WITH_DUMP
>> case DUMP:
>> @@ -935,8 +947,11 @@ retry:
>> }
>> }
>> ret = f2fs_finalize_device();
>> - if (ret < 0)
>> + if (ret) {
>> + if (c.func == FSCK)
>> + return FSCK_OPERATIONAL_ERROR;
>> return ret;
>> + }
>>
>> printf("\nDone: %lf secs\n", (get_boottime_ns() - start) / 1000000000.0);
>> return 0;
>> --
>> 2.26.2
> .
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 6+ messages in thread