| Message ID | 541A979D.8060409@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
-------- Original Message -------- Subject: [PATCH 2/5] btrfs: correct a message on setting nodatacow From: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com> To: linux-btrfs@vger.kernel.org <linux-btrfs@vger.kernel.org> Date: 2014?09?18? 16:28 > From: Naohiro Aota <naota@elisp.net> > > If we set nodatacow mount option after compress-force option, > we don't get compression disabling message. > > === > $ sudo mount -o remount,compress-force,nodatacow /; dmesg|tail -n 3 > [ 3845.719047] BTRFS info (device vda2): force zlib compression > [ 3845.719052] BTRFS info (device vda2): setting nodatacow > [ 3845.719055] BTRFS info (device vda2): disk space caching is enabled > === > > Signed-off-by: Naohiro Aota <naota@elisp.net> > Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com> > --- > fs/btrfs/super.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index d1c5b6d..d131098 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -462,8 +462,7 @@ int btrfs_parse_options(struct btrfs_root *root, char *options) > break; > case Opt_nodatacow: > if (!btrfs_test_opt(root, NODATACOW)) { > - if (!btrfs_test_opt(root, COMPRESS) || > - !btrfs_test_opt(root, FORCE_COMPRESS)) { > + if (btrfs_test_opt(root, COMPRESS)) { > btrfs_info(root->fs_info, > "setting nodatacow, compression disabled"); > } else { > -- 1.8.3.1 > > -- > 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 Although the patch makes the output ok, the core problem is missing conflict options check. compress-force mount options implies datacow and datasum, but following nodatasum will disable datasum and compress, in fact they are conflicting mount option... Even the current behavior(later mount option will override previous ones) provides great tolerance, IMO there should better be some conflicting check for mount options. For example, we first save all the mount options passed in into a temporary bitmaps to finds out the conflicting and only when they contains no conflicts, set the mount options to fs_info. (Maybe bitmap is not enough for this case, since we can't distinguish default value and value to be set?) What do you think about this idea ? Thanks, Qu -- 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
Hi Qu, Thank you for your comment. (2014/09/19 11:03), Qu Wenruo wrote: > > -------- Original Message -------- > Subject: [PATCH 2/5] btrfs: correct a message on setting nodatacow > From: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com> > To: linux-btrfs@vger.kernel.org <linux-btrfs@vger.kernel.org> > Date: 2014?09?18? 16:28 >> From: Naohiro Aota <naota@elisp.net> >> >> If we set nodatacow mount option after compress-force option, >> we don't get compression disabling message. >> >> === >> $ sudo mount -o remount,compress-force,nodatacow /; dmesg|tail -n 3 >> [ 3845.719047] BTRFS info (device vda2): force zlib compression >> [ 3845.719052] BTRFS info (device vda2): setting nodatacow >> [ 3845.719055] BTRFS info (device vda2): disk space caching is enabled >> === >> >> Signed-off-by: Naohiro Aota <naota@elisp.net> >> Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com> >> --- >> fs/btrfs/super.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c >> index d1c5b6d..d131098 100644 >> --- a/fs/btrfs/super.c >> +++ b/fs/btrfs/super.c >> @@ -462,8 +462,7 @@ int btrfs_parse_options(struct btrfs_root *root, char *options) >> break; >> case Opt_nodatacow: >> if (!btrfs_test_opt(root, NODATACOW)) { >> - if (!btrfs_test_opt(root, COMPRESS) || >> - !btrfs_test_opt(root, FORCE_COMPRESS)) { >> + if (btrfs_test_opt(root, COMPRESS)) { >> btrfs_info(root->fs_info, >> "setting nodatacow, compression disabled"); >> } else { >> -- 1.8.3.1 >> >> -- >> 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 > Although the patch makes the output ok, the core problem is missing conflict options check. > > compress-force mount options implies datacow and datasum, but following nodatasum will disable datasum and compress, in fact they are conflicting mount option... > > Even the current behavior(later mount option will override previous ones) provides great tolerance, > IMO there should better be some conflicting check for mount options. > > For example, we first save all the mount options passed in into a temporary bitmaps to finds out the conflicting > and only when they contains no conflicts, set the mount options to fs_info. > (Maybe bitmap is not enough for this case, since we can't distinguish default value and value to be set?) > > What do you think about this idea ? I'm against your idea for two reasons and it's better to stay in current behavior though it's a bit complex. First, the rule "last one wins" is not only a conventional rule, but also is what mount(8) says. https://git.kernel.org/cgit/utils/util-linux/util-linux.git/tree/sys-utils/mount.8#n253 ====== The usual behavior is that the last option wins if there are conflicting ones. ====== Second, if we change the behavior, we would break existing systems. At worst case, users would fail to boot their system after updating kernel, because of the failure of mounting Btrfs at the init process. Thanks, Satoru > > Thanks, > Qu > -- > 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
-------- Original Message -------- Subject: Re: [PATCH 2/5] btrfs: correct a message on setting nodatacow From: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com> To: Qu Wenruo <quwenruo@cn.fujitsu.com>, linux-btrfs@vger.kernel.org <linux-btrfs@vger.kernel.org> Date: 2014?09?19? 14:45 > Hi Qu, > > Thank you for your comment. > > (2014/09/19 11:03), Qu Wenruo wrote: >> >> -------- Original Message -------- >> Subject: [PATCH 2/5] btrfs: correct a message on setting nodatacow >> From: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com> >> To: linux-btrfs@vger.kernel.org <linux-btrfs@vger.kernel.org> >> Date: 2014?09?18? 16:28 >>> From: Naohiro Aota <naota@elisp.net> >>> >>> If we set nodatacow mount option after compress-force option, >>> we don't get compression disabling message. >>> >>> === >>> $ sudo mount -o remount,compress-force,nodatacow /; dmesg|tail -n 3 >>> [ 3845.719047] BTRFS info (device vda2): force zlib compression >>> [ 3845.719052] BTRFS info (device vda2): setting nodatacow >>> [ 3845.719055] BTRFS info (device vda2): disk space caching is enabled >>> === >>> >>> Signed-off-by: Naohiro Aota <naota@elisp.net> >>> Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com> >>> --- >>> fs/btrfs/super.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c >>> index d1c5b6d..d131098 100644 >>> --- a/fs/btrfs/super.c >>> +++ b/fs/btrfs/super.c >>> @@ -462,8 +462,7 @@ int btrfs_parse_options(struct btrfs_root *root, >>> char *options) >>> break; >>> case Opt_nodatacow: >>> if (!btrfs_test_opt(root, NODATACOW)) { >>> - if (!btrfs_test_opt(root, COMPRESS) || >>> - !btrfs_test_opt(root, FORCE_COMPRESS)) { >>> + if (btrfs_test_opt(root, COMPRESS)) { >>> btrfs_info(root->fs_info, >>> "setting nodatacow, compression >>> disabled"); >>> } else { >>> -- 1.8.3.1 >>> >>> -- >>> 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 >> Although the patch makes the output ok, the core problem is missing >> conflict options check. >> >> compress-force mount options implies datacow and datasum, but >> following nodatasum will disable datasum and compress, in fact they >> are conflicting mount option... >> >> Even the current behavior(later mount option will override previous >> ones) provides great tolerance, >> IMO there should better be some conflicting check for mount options. >> >> For example, we first save all the mount options passed in into a >> temporary bitmaps to finds out the conflicting >> and only when they contains no conflicts, set the mount options to >> fs_info. >> (Maybe bitmap is not enough for this case, since we can't distinguish >> default value and value to be set?) >> >> What do you think about this idea ? > > I'm against your idea for two reasons and it's better to > stay in current behavior though it's a bit complex. > > First, the rule "last one wins" is not only a conventional rule, > but also is what mount(8) says. > > https://git.kernel.org/cgit/utils/util-linux/util-linux.git/tree/sys-utils/mount.8#n253 > > > ====== > The usual behavior is that the last option wins if there are conflicting > ones. > ====== > > Second, if we change the behavior, we would break existing > systems. At worst case, users would fail to boot their system > after updating kernel, because of the failure of mounting > Btrfs at the init process. > > Thanks, > Satoru > It really makes sense. So I'm OK to keep things and it's true that the conflict check is somewhat overkilled. Thanks, Qu >> >> Thanks, >> Qu >> -- >> 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
=== $ sudo mount -o remount,compress-force,nodatacow /; dmesg|tail -n 3 [ 3845.719047] BTRFS info (device vda2): force zlib compression [ 3845.719052] BTRFS info (device vda2): setting nodatacow [ 3845.719055] BTRFS info (device vda2): disk space caching is enabled === Signed-off-by: Naohiro Aota <naota@elisp.net> Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com> --- fs/btrfs/super.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index d1c5b6d..d131098 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -462,8 +462,7 @@ int btrfs_parse_options(struct btrfs_root *root, char *options) break; case Opt_nodatacow: if (!btrfs_test_opt(root, NODATACOW)) { - if (!btrfs_test_opt(root, COMPRESS) || - !btrfs_test_opt(root, FORCE_COMPRESS)) { + if (btrfs_test_opt(root, COMPRESS)) { btrfs_info(root->fs_info, "setting nodatacow, compression disabled"); } else {
From: Naohiro Aota <naota@elisp.net> If we set nodatacow mount option after compress-force option, we don't get compression disabling message. -- 1.8.3.1 -- 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