| Message ID | 1404353196-10914-1-git-send-email-guihc.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Jul 03, 2014 at 10:06:33AM +0800, Gui Hecheng wrote: > Clear the seeding flag may cause the original filesystem to be writable, > which is dangerous. Can you please describe the dangerous scenario a bit more? This would also go to the documentation so it's not only to satisfy my curiosity. Dropping the seeding flag could be dangerous if the filesystem starts in seeding mode, a new device is added, some writes are done, then filesystem is unmounted. Now it's a 2 device filesystem, where the orignal holds some data and without the seeding flag it would accept new writes. Still ok for me, though this is probably the time where some user assumptions may break. > In this case, add user confirmation check when clearing seeding flag. > Also warn the user that the fs is in a dangerous condition when > the seeding flag is cleared if it it forced to. The -y option is tied only to the seeding option, but it should IMO be more general and called --force. > Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com> > --- > btrfstune.c | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/btrfstune.c b/btrfstune.c > index 3f2f0cd..0e18088 100644 > --- a/btrfstune.c > +++ b/btrfstune.c > @@ -103,6 +104,7 @@ static void print_usage(void) > fprintf(stderr, "\t-S value\tpositive value will enable seeding, zero to disable, negative is not allowed\n"); > fprintf(stderr, "\t-r \t\tenable extended inode refs\n"); > fprintf(stderr, "\t-x \t\tenable skinny metadata extent refs\n"); > + fprintf(stderr, "\t-y \t\tsay yes to clear the seeding flag, make sure that you are aware of the danger\n"); The help text could say someting like "--force\tallow dangerous changes\n" btrfstune only allows setting the bit for extref and skinny-metadata, unsetting would be dangerous as well. -- 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 Thu, 2014-07-03 at 18:51 +0200, David Sterba wrote: > On Thu, Jul 03, 2014 at 10:06:33AM +0800, Gui Hecheng wrote: > > Clear the seeding flag may cause the original filesystem to be writable, > > which is dangerous. > > Can you please describe the dangerous scenario a bit more? This would > also go to the documentation so it's not only to satisfy my curiosity. Yes, I'll include a certain scenario in the changelog of a v2 patch. > Dropping the seeding flag could be dangerous if the filesystem starts in > seeding mode, a new device is added, some writes are done, then > filesystem is unmounted. > > Now it's a 2 device filesystem, where the orignal holds some data and > without the seeding flag it would accept new writes. Still ok for me, > though this is probably the time where some user assumptions may break. > > > In this case, add user confirmation check when clearing seeding flag. > > Also warn the user that the fs is in a dangerous condition when > > the seeding flag is cleared if it it forced to. > > The -y option is tied only to the seeding option, but it should IMO be > more general and called --force. I agree. > > Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com> > > --- > > btrfstune.c | 24 +++++++++++++++++++++++- > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > diff --git a/btrfstune.c b/btrfstune.c > > index 3f2f0cd..0e18088 100644 > > --- a/btrfstune.c > > +++ b/btrfstune.c > > @@ -103,6 +104,7 @@ static void print_usage(void) > > fprintf(stderr, "\t-S value\tpositive value will enable seeding, zero to disable, negative is not allowed\n"); > > fprintf(stderr, "\t-r \t\tenable extended inode refs\n"); > > fprintf(stderr, "\t-x \t\tenable skinny metadata extent refs\n"); > > + fprintf(stderr, "\t-y \t\tsay yes to clear the seeding flag, make sure that you are aware of the danger\n"); > > The help text could say someting like > > "--force\tallow dangerous changes\n" > > btrfstune only allows setting the bit for extref and skinny-metadata, > unsetting would be dangerous as well. On my part, I don't find any scenarioes for these two, could you please remind me more? -- 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 Fri, Jul 04, 2014 at 09:59:13AM +0800, Gui Hecheng wrote: > > > --- a/btrfstune.c > > > +++ b/btrfstune.c > > > @@ -103,6 +104,7 @@ static void print_usage(void) > > > fprintf(stderr, "\t-S value\tpositive value will enable seeding, zero to disable, negative is not allowed\n"); > > > fprintf(stderr, "\t-r \t\tenable extended inode refs\n"); > > > fprintf(stderr, "\t-x \t\tenable skinny metadata extent refs\n"); > > > + fprintf(stderr, "\t-y \t\tsay yes to clear the seeding flag, make sure that you are aware of the danger\n"); > > > > The help text could say someting like > > > > "--force\tallow dangerous changes\n" > > > > btrfstune only allows setting the bit for extref and skinny-metadata, > > unsetting would be dangerous as well. > On my part, I don't find any scenarioes for these two, could you please > remind me more? For example the extended refs add a new key type representing the hardlinks that do not fit into the leaf. Dropping the bit from incompat bits will hide that. Mounting the filesystem on kernels < 3.7 will not work as expected then. The extref key is not necessarily used even if the bit is set, only if the hardlinks do not fit, so dropping the bit will not automatically cause a disaster. -- 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/btrfstune.c b/btrfstune.c index 3f2f0cd..0e18088 100644 --- a/btrfstune.c +++ b/btrfstune.c @@ -56,6 +56,7 @@ static int update_seeding_flag(struct btrfs_root *root, int set_flag) return 1; } super_flags &= ~BTRFS_SUPER_FLAG_SEEDING; + fprintf(stderr, "Warning: Seeding flag cleared. This could be dangerous!\n"); } trans = btrfs_start_transaction(root, 1); @@ -103,6 +104,7 @@ static void print_usage(void) fprintf(stderr, "\t-S value\tpositive value will enable seeding, zero to disable, negative is not allowed\n"); fprintf(stderr, "\t-r \t\tenable extended inode refs\n"); fprintf(stderr, "\t-x \t\tenable skinny metadata extent refs\n"); + fprintf(stderr, "\t-y \t\tsay yes to clear the seeding flag, make sure that you are aware of the danger\n"); } int main(int argc, char *argv[]) @@ -113,11 +115,12 @@ int main(int argc, char *argv[]) int seeding_flag = 0; u64 seeding_value = 0; int skinny_flag = 0; + int force_clear = 0; int ret; optind = 1; while(1) { - int c = getopt(argc, argv, "S:rx"); + int c = getopt(argc, argv, "S:rxy"); if (c < 0) break; switch(c) { @@ -131,6 +134,9 @@ int main(int argc, char *argv[]) case 'x': skinny_flag = 1; break; + case 'y': + force_clear = 1; + break; default: print_usage(); return 1; @@ -151,6 +157,13 @@ int main(int argc, char *argv[]) return 1; } + if (!seeding_flag && force_clear) { + fprintf(stderr, + "ERROR: -y option only work with -S option.\n"); + print_usage(); + return 1; + } + ret = check_mounted(device); if (ret < 0) { fprintf(stderr, "Could not check mount status: %s\n", @@ -169,6 +182,15 @@ int main(int argc, char *argv[]) } if (seeding_flag) { + if (!seeding_value && !force_clear) { + fprintf(stderr, "Warning: This is dangerous, clearing the seeding flag will make the original filesystem writable.\n"); + ret = ask_user("We are going to clear the seeding flag, are you sure?"); + if (!ret) { + fprintf(stderr, "Clear seeding flag canceled\n"); + return 1; + } + } + ret = update_seeding_flag(root, seeding_value); if (!ret) success++;
Clear the seeding flag may cause the original filesystem to be writable, which is dangerous. In this case, add user confirmation check when clearing seeding flag. Also warn the user that the fs is in a dangerous condition when the seeding flag is cleared if it it forced to. Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com> --- btrfstune.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)