| Message ID | 1401276041-18349-1-git-send-email-wangsl.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Sterba |
| Headers | show |
The subject and the comment say what this change does, but that's obvious from reading the code. Nothing says *why* the change has been made. What does this fix, and how does it fix it? Can you add/update the commit log so that some reader in the future (or for that matter, a reviewer in the present) will have an idea about the reason for this change? What was the failure case, what was the failure mode, why does this change fix it, etc. Thanks, -Eric On 5/28/14, 6:20 AM, Wang Shilong wrote: > Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> > --- > cmds-check.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/cmds-check.c b/cmds-check.c > index db7df80..0e4e042 100644 > --- a/cmds-check.c > +++ b/cmds-check.c > @@ -6810,8 +6810,7 @@ int cmd_check(int argc, char **argv) > int option_index = 0; > int init_csum_tree = 0; > int qgroup_report = 0; > - enum btrfs_open_ctree_flags ctree_flags = > - OPEN_CTREE_PARTIAL | OPEN_CTREE_EXCLUSIVE; > + enum btrfs_open_ctree_flags ctree_flags = OPEN_CTREE_EXCLUSIVE; > > while(1) { > int c; > @@ -6877,6 +6876,10 @@ int cmd_check(int argc, char **argv) > goto err_out; > } > > + /* only allow partial opening under repair mode */ > + if (repair) > + ctree_flags |= OPEN_CTREE_PARTIAL; > + > info = open_ctree_fs_info(argv[optind], bytenr, 0, ctree_flags); > if (!info) { > fprintf(stderr, "Couldn't open file system\n"); > -- 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
2014-05-28 21:56 GMT+08:00 Eric Sandeen <sandeen@redhat.com>: > The subject and the comment say what this change does, but > that's obvious from reading the code. Nothing says *why* > the change has been made. What does this fix, and how does > it fix it? Yup, the reason that we allow partial opening is that sometimes, we may have a corrupted extent tree(for example), but for fsck repair case, the broken tree maybe rebuilt. So if users only want to do check but not repaired, this patch will make fsck return failure as soon as possible and tell users that some critial roots have been corrupted... Let's come to your comments, Eric, you are absolutely right, i was a little lazy sometimes... I would add necessay changelog to describe why we need this patch, thanks for your comments. Regards, Wang > > Can you add/update the commit log so that some reader in the future > (or for that matter, a reviewer in the present) will have an idea > about the reason for this change? What was the failure case, what > was the failure mode, why does this change fix it, etc. > > Thanks, > -Eric > > On 5/28/14, 6:20 AM, Wang Shilong wrote: >> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> >> --- >> cmds-check.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/cmds-check.c b/cmds-check.c >> index db7df80..0e4e042 100644 >> --- a/cmds-check.c >> +++ b/cmds-check.c >> @@ -6810,8 +6810,7 @@ int cmd_check(int argc, char **argv) >> int option_index = 0; >> int init_csum_tree = 0; >> int qgroup_report = 0; >> - enum btrfs_open_ctree_flags ctree_flags = >> - OPEN_CTREE_PARTIAL | OPEN_CTREE_EXCLUSIVE; >> + enum btrfs_open_ctree_flags ctree_flags = OPEN_CTREE_EXCLUSIVE; >> >> while(1) { >> int c; >> @@ -6877,6 +6876,10 @@ int cmd_check(int argc, char **argv) >> goto err_out; >> } >> >> + /* only allow partial opening under repair mode */ >> + if (repair) >> + ctree_flags |= OPEN_CTREE_PARTIAL; >> + >> info = open_ctree_fs_info(argv[optind], bytenr, 0, ctree_flags); >> if (!info) { >> fprintf(stderr, "Couldn't open file system\n"); >> > > -- > 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
On 5/28/14, 9:10 AM, Shilong Wang wrote: > 2014-05-28 21:56 GMT+08:00 Eric Sandeen <sandeen@redhat.com>: >> The subject and the comment say what this change does, but >> that's obvious from reading the code. Nothing says *why* >> the change has been made. What does this fix, and how does >> it fix it? > > Yup, the reason that we allow partial opening is that sometimes, > we may have a corrupted extent tree(for example), but for fsck repair case, > the broken tree maybe rebuilt. > > So if users only want to do check but not repaired, this patch will make fsck > return failure as soon as possible and tell users that some critial roots have > been corrupted... > > Let's come to your comments, Eric, you are absolutely right, i was a little > lazy sometimes... Sometimes I am too. ;) > I would add necessay changelog to describe why we need this patch, > thanks for your comments. thanks. Perhaps it's obvious to people more familiar with the code, but I think it's always helpful to be a bit more descriptive; things that make perfect sense right now can sometimes be confusing a year or so later. Thanks, -Eric > Regards, > Wang >> >> Can you add/update the commit log so that some reader in the future >> (or for that matter, a reviewer in the present) will have an idea >> about the reason for this change? What was the failure case, what >> was the failure mode, why does this change fix it, etc. >> >> Thanks, >> -Eric >> >> On 5/28/14, 6:20 AM, Wang Shilong wrote: >>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> >>> --- >>> cmds-check.c | 7 +++++-- >>> 1 file changed, 5 insertions(+), 2 deletions(-) >>> >>> diff --git a/cmds-check.c b/cmds-check.c >>> index db7df80..0e4e042 100644 >>> --- a/cmds-check.c >>> +++ b/cmds-check.c >>> @@ -6810,8 +6810,7 @@ int cmd_check(int argc, char **argv) >>> int option_index = 0; >>> int init_csum_tree = 0; >>> int qgroup_report = 0; >>> - enum btrfs_open_ctree_flags ctree_flags = >>> - OPEN_CTREE_PARTIAL | OPEN_CTREE_EXCLUSIVE; >>> + enum btrfs_open_ctree_flags ctree_flags = OPEN_CTREE_EXCLUSIVE; >>> >>> while(1) { >>> int c; >>> @@ -6877,6 +6876,10 @@ int cmd_check(int argc, char **argv) >>> goto err_out; >>> } >>> >>> + /* only allow partial opening under repair mode */ >>> + if (repair) >>> + ctree_flags |= OPEN_CTREE_PARTIAL; >>> + >>> info = open_ctree_fs_info(argv[optind], bytenr, 0, ctree_flags); >>> if (!info) { >>> fprintf(stderr, "Couldn't open file system\n"); >>> >> >> -- >> 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 > -- 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
On Wed, May 28, 2014 at 07:20:38PM +0800, Wang Shilong wrote: > --- a/cmds-check.c > +++ b/cmds-check.c > @@ -6810,8 +6810,7 @@ int cmd_check(int argc, char **argv) > int option_index = 0; > int init_csum_tree = 0; > int qgroup_report = 0; > - enum btrfs_open_ctree_flags ctree_flags = > - OPEN_CTREE_PARTIAL | OPEN_CTREE_EXCLUSIVE; > + enum btrfs_open_ctree_flags ctree_flags = OPEN_CTREE_EXCLUSIVE; > > while(1) { > int c; > @@ -6877,6 +6876,10 @@ int cmd_check(int argc, char **argv) > goto err_out; > } > > + /* only allow partial opening under repair mode */ > + if (repair) > + ctree_flags |= OPEN_CTREE_PARTIAL; I'm curious why. The usual way is to run fsck, look at errors and call with --repair eventually, expecting the repair mode do fix what's fixable. Now this would not return the same set of errors in the non-repair mode? This of course depends on the damage of the filesystem, but I think we should try to let it continue as far as possible and then stop. This probably means extra checks of the data structures before use, but this a good pattern for fsck anyway. > + > info = open_ctree_fs_info(argv[optind], bytenr, 0, ctree_flags); > if (!info) { > fprintf(stderr, "Couldn't open file system\n"); -- 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
diff --git a/cmds-check.c b/cmds-check.c index db7df80..0e4e042 100644 --- a/cmds-check.c +++ b/cmds-check.c @@ -6810,8 +6810,7 @@ int cmd_check(int argc, char **argv) int option_index = 0; int init_csum_tree = 0; int qgroup_report = 0; - enum btrfs_open_ctree_flags ctree_flags = - OPEN_CTREE_PARTIAL | OPEN_CTREE_EXCLUSIVE; + enum btrfs_open_ctree_flags ctree_flags = OPEN_CTREE_EXCLUSIVE; while(1) { int c; @@ -6877,6 +6876,10 @@ int cmd_check(int argc, char **argv) goto err_out; } + /* only allow partial opening under repair mode */ + if (repair) + ctree_flags |= OPEN_CTREE_PARTIAL; + info = open_ctree_fs_info(argv[optind], bytenr, 0, ctree_flags); if (!info) { fprintf(stderr, "Couldn't open file system\n");
Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com> --- cmds-check.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)