* [PATCH resend v2] bcachefs: reject unknown mount options
@ 2024-09-17 1:39 David Disseldorp
2024-09-21 23:13 ` Kent Overstreet
2024-10-10 18:33 ` Janpieter Sollie
0 siblings, 2 replies; 6+ messages in thread
From: David Disseldorp @ 2024-09-17 1:39 UTC (permalink / raw)
To: linux-bcachefs; +Cc: David Disseldorp, Martin Doucha, Hongbo Li
This is effectively a revert of commit 03ef80b469d5 ("bcachefs: Ignore
unknown mount options"), rebased atop commit 929d954330142 ("bcachefs:
use new mount API").
Contrary to the 03ef80b469d5 commit message, unknown mount options do
result in failure on other filesystems such as xfs, btrfs, ext4, etc.
Ignoring unknown options may result in unexpected behaviour: e.g. if
an application requires future-workload-specific-mount-option to work
and is run on a system without future-workload-specific-mount-option
kernel support, then it may fail without indication of the missing
kernel functionality.
This reversion may cause failures for existing users reliant on the
ignore-unknown behaviour, but it should be bearable given bcachefs is
still flagged experimental.
Fixes: 03ef80b469d5 ("bcachefs: Ignore unknown mount options")
Reported-by: Martin Doucha <martin.doucha@suse.com>
Closes: https://bugzilla.opensuse.org/show_bug.cgi?id=1230065
Signed-off-by: David Disseldorp <ddiss@suse.de>
Reviewed-by: Hongbo Li <lihongbo22@huawei.com>
---
fs/bcachefs/opts.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/bcachefs/opts.c b/fs/bcachefs/opts.c
index e10fc1da71b19..bc5c180878212 100644
--- a/fs/bcachefs/opts.c
+++ b/fs/bcachefs/opts.c
@@ -482,9 +482,8 @@ int bch2_parse_one_mount_opt(struct bch_fs *c, struct bch_opts *opts,
val = "0";
}
- /* Unknown options are ignored: */
if (id < 0)
- return 0;
+ goto bad_opt;
if (!(bch2_opt_table[id].flags & OPT_MOUNT))
goto bad_opt;
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH resend v2] bcachefs: reject unknown mount options
2024-09-17 1:39 [PATCH resend v2] bcachefs: reject unknown mount options David Disseldorp
@ 2024-09-21 23:13 ` Kent Overstreet
2024-09-23 3:59 ` David Disseldorp
2024-09-23 9:05 ` Hongbo Li
2024-10-10 18:33 ` Janpieter Sollie
1 sibling, 2 replies; 6+ messages in thread
From: Kent Overstreet @ 2024-09-21 23:13 UTC (permalink / raw)
To: David Disseldorp; +Cc: linux-bcachefs, Martin Doucha, Hongbo Li
On Tue, Sep 17, 2024 at 01:39:57AM GMT, David Disseldorp wrote:
> This is effectively a revert of commit 03ef80b469d5 ("bcachefs: Ignore
> unknown mount options"), rebased atop commit 929d954330142 ("bcachefs:
> use new mount API").
>
> Contrary to the 03ef80b469d5 commit message, unknown mount options do
> result in failure on other filesystems such as xfs, btrfs, ext4, etc.
>
> Ignoring unknown options may result in unexpected behaviour: e.g. if
> an application requires future-workload-specific-mount-option to work
> and is run on a system without future-workload-specific-mount-option
> kernel support, then it may fail without indication of the missing
> kernel functionality.
>
> This reversion may cause failures for existing users reliant on the
> ignore-unknown behaviour, but it should be bearable given bcachefs is
> still flagged experimental.
>
> Fixes: 03ef80b469d5 ("bcachefs: Ignore unknown mount options")
> Reported-by: Martin Doucha <martin.doucha@suse.com>
> Closes: https://bugzilla.opensuse.org/show_bug.cgi?id=1230065
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> Reviewed-by: Hongbo Li <lihongbo22@huawei.com>
I applied this and got some new failures in the CI - it seems something
is broken in remount.
00162 ========= TEST ec_small
00162
00162 WATCHDOG 60
00163 bcachefs (857683e6-ef4f-41f5-abdd-dca191c38fff): starting version 1.12: rebalance_work_acct_fix opts=errors=panic,metadata_replicas=2,data_replicas=2,erasure_code
00163 bcachefs (857683e6-ef4f-41f5-abdd-dca191c38fff): initializing new filesystem
00163 bcachefs (857683e6-ef4f-41f5-abdd-dca191c38fff): going read-write
00163 bcachefs (857683e6-ef4f-41f5-abdd-dca191c38fff): marking superblocks
00163 bcachefs (857683e6-ef4f-41f5-abdd-dca191c38fff): initializing freespace
00163 bcachefs (857683e6-ef4f-41f5-abdd-dca191c38fff): done initializing freespace
00163 bcachefs (857683e6-ef4f-41f5-abdd-dca191c38fff): reading snapshots table
00163 bcachefs (857683e6-ef4f-41f5-abdd-dca191c38fff): reading snapshots done
00163 bcachefs (857683e6-ef4f-41f5-abdd-dca191c38fff): done starting filesystem
00163 1+0 records in
00163 1+0 records out
00163 1048576 bytes (1.0 MB, 1.0 MiB) copied, 0.0115145 s, 91.1 MB/s
00163 bcachefs: bch2_parse_one_mount_opt() Bad mount option rwerrors
00163 Error: Invalid argument
00163 Error 1 at /host//home/testdashboard/ktest/tests/fs/bcachefs/ec.ktest 32 from: mount -o remount,ro /mnt, exiting
00163 Error 1 at /host/home/testdashboard/ktest/tests/prelude.sh 269 from: ( set -e; run_test $i ), exiting
00163
00163 ========= FAILED ec_small in 2s
The test is just doing
mount -o remount,ro /mnt
Thomas, this is likely something you introduced, think you could take a
look?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH resend v2] bcachefs: reject unknown mount options
2024-09-21 23:13 ` Kent Overstreet
@ 2024-09-23 3:59 ` David Disseldorp
2024-09-23 6:07 ` Hongbo Li
2024-09-23 9:05 ` Hongbo Li
1 sibling, 1 reply; 6+ messages in thread
From: David Disseldorp @ 2024-09-23 3:59 UTC (permalink / raw)
To: Kent Overstreet; +Cc: linux-bcachefs, Martin Doucha, Hongbo Li
On Sat, 21 Sep 2024 19:13:31 -0400, Kent Overstreet wrote:
> I applied this and got some new failures in the CI - it seems something
> is broken in remount.
...
> 00163 bcachefs: bch2_parse_one_mount_opt() Bad mount option rwerrors
> 00163 Error: Invalid argument
> 00163 Error 1 at /host//home/testdashboard/ktest/tests/fs/bcachefs/ec.ktest 32 from: mount -o remount,ro /mnt, exiting
> 00163 Error 1 at /host/home/testdashboard/ktest/tests/prelude.sh 269 from: ( set -e; run_test $i ), exiting
> 00163
> 00163 ========= FAILED ec_small in 2s
>
> The test is just doing
> mount -o remount,ro /mnt
>
> Thomas, this is likely something you introduced, think you could take a
> look?
Thanks for the details. "mount -o remount,ro <mnt>" works fine for me,
and I'm not aware of anything that could be providing the "rwerrors"
parameter. Which version of mount / util-linux is used here?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH resend v2] bcachefs: reject unknown mount options
2024-09-23 3:59 ` David Disseldorp
@ 2024-09-23 6:07 ` Hongbo Li
0 siblings, 0 replies; 6+ messages in thread
From: Hongbo Li @ 2024-09-23 6:07 UTC (permalink / raw)
To: David Disseldorp, Kent Overstreet; +Cc: linux-bcachefs, Martin Doucha
On 2024/9/23 11:59, David Disseldorp wrote:
> On Sat, 21 Sep 2024 19:13:31 -0400, Kent Overstreet wrote:
>
>> I applied this and got some new failures in the CI - it seems something
>> is broken in remount.
> ...
>> 00163 bcachefs: bch2_parse_one_mount_opt() Bad mount option rwerrors
>> 00163 Error: Invalid argument
>> 00163 Error 1 at /host//home/testdashboard/ktest/tests/fs/bcachefs/ec.ktest 32 from: mount -o remount,ro /mnt, exiting
>> 00163 Error 1 at /host/home/testdashboard/ktest/tests/prelude.sh 269 from: ( set -e; run_test $i ), exiting
>> 00163
>> 00163 ========= FAILED ec_small in 2s
>>
>> The test is just doing
>> mount -o remount,ro /mnt
>>
>> Thomas, this is likely something you introduced, think you could take a
>> look?
>
> Thanks for the details. "mount -o remount,ro <mnt>" works fine for me,
> and I'm not aware of anything that could be providing the "rwerrors"
It seems rw, errors=xxx which misses a comma.
> parameter. Which version of mount / util-linux is used here?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH resend v2] bcachefs: reject unknown mount options
2024-09-21 23:13 ` Kent Overstreet
2024-09-23 3:59 ` David Disseldorp
@ 2024-09-23 9:05 ` Hongbo Li
1 sibling, 0 replies; 6+ messages in thread
From: Hongbo Li @ 2024-09-23 9:05 UTC (permalink / raw)
To: Kent Overstreet, David Disseldorp; +Cc: linux-bcachefs, Martin Doucha
On 2024/9/22 7:13, Kent Overstreet wrote:
> On Tue, Sep 17, 2024 at 01:39:57AM GMT, David Disseldorp wrote:
>> This is effectively a revert of commit 03ef80b469d5 ("bcachefs: Ignore
>> unknown mount options"), rebased atop commit 929d954330142 ("bcachefs:
>> use new mount API").
>>
>> Contrary to the 03ef80b469d5 commit message, unknown mount options do
>> result in failure on other filesystems such as xfs, btrfs, ext4, etc.
>>
>> Ignoring unknown options may result in unexpected behaviour: e.g. if
>> an application requires future-workload-specific-mount-option to work
>> and is run on a system without future-workload-specific-mount-option
>> kernel support, then it may fail without indication of the missing
>> kernel functionality.
>>
>> This reversion may cause failures for existing users reliant on the
>> ignore-unknown behaviour, but it should be bearable given bcachefs is
>> still flagged experimental.
>>
>> Fixes: 03ef80b469d5 ("bcachefs: Ignore unknown mount options")
>> Reported-by: Martin Doucha <martin.doucha@suse.com>
>> Closes: https://bugzilla.opensuse.org/show_bug.cgi?id=1230065
>> Signed-off-by: David Disseldorp <ddiss@suse.de>
>> Reviewed-by: Hongbo Li <lihongbo22@huawei.com>
>
> I applied this and got some new failures in the CI - it seems something
> is broken in remount.
>
> 00162 ========= TEST ec_small
> 00162
> 00162 WATCHDOG 60
> 00163 bcachefs (857683e6-ef4f-41f5-abdd-dca191c38fff): starting version 1.12: rebalance_work_acct_fix opts=errors=panic,metadata_replicas=2,data_replicas=2,erasure_code
> 00163 bcachefs (857683e6-ef4f-41f5-abdd-dca191c38fff): initializing new filesystem
> 00163 bcachefs (857683e6-ef4f-41f5-abdd-dca191c38fff): going read-write
> 00163 bcachefs (857683e6-ef4f-41f5-abdd-dca191c38fff): marking superblocks
> 00163 bcachefs (857683e6-ef4f-41f5-abdd-dca191c38fff): initializing freespace
> 00163 bcachefs (857683e6-ef4f-41f5-abdd-dca191c38fff): done initializing freespace
> 00163 bcachefs (857683e6-ef4f-41f5-abdd-dca191c38fff): reading snapshots table
> 00163 bcachefs (857683e6-ef4f-41f5-abdd-dca191c38fff): reading snapshots done
> 00163 bcachefs (857683e6-ef4f-41f5-abdd-dca191c38fff): done starting filesystem
> 00163 1+0 records in
> 00163 1+0 records out
> 00163 1048576 bytes (1.0 MB, 1.0 MiB) copied, 0.0115145 s, 91.1 MB/s
> 00163 bcachefs: bch2_parse_one_mount_opt() Bad mount option rwerrors
> 00163 Error: Invalid argument
> 00163 Error 1 at /host//home/testdashboard/ktest/tests/fs/bcachefs/ec.ktest 32 from: mount -o remount,ro /mnt, exiting
> 00163 Error 1 at /host/home/testdashboard/ktest/tests/prelude.sh 269 from: ( set -e; run_test $i ), exiting
> 00163
> 00163 ========= FAILED ec_small in 2s
>
> The test is just doing
> mount -o remount,ro /mnt
ah, this is indeed a bug in show_options. It misses a comma when fill
options buffer to seq. The following code should be added.
diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
index e774cb3e6b1a..79c3a650b714 100644
--- a/fs/bcachefs/fs.c
+++ b/fs/bcachefs/fs.c
@@ -1927,7 +1927,7 @@ static int bch2_show_options(struct seq_file *seq,
struct dentry *root)
bch2_opts_to_text(&buf, c->opts, c, c->disk_sb.sb,
OPT_MOUNT, OPT_HIDDEN, OPT_SHOW_MOUNT_STYLE);
printbuf_nul_terminate(&buf);
- seq_puts(seq, buf.buf);
+ seq_printf(seq, ",%s", buf.buf);
int ret = buf.allocation_failure ? -ENOMEM : 0;
printbuf_exit(&buf);
I will send this later. The test passed in my environment with this v2
patch.
Thanks,
Hongbo
>
> Thomas, this is likely something you introduced, think you could take a
> look?
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH resend v2] bcachefs: reject unknown mount options
2024-09-17 1:39 [PATCH resend v2] bcachefs: reject unknown mount options David Disseldorp
2024-09-21 23:13 ` Kent Overstreet
@ 2024-10-10 18:33 ` Janpieter Sollie
1 sibling, 0 replies; 6+ messages in thread
From: Janpieter Sollie @ 2024-10-10 18:33 UTC (permalink / raw)
To: David Disseldorp, linux-bcachefs
Cc: Martin Doucha, Hongbo Li, Kent Overstreet
Op 17/09/2024 om 03:39 schreef David Disseldorp:
> This is effectively a revert of commit 03ef80b469d5 ("bcachefs: Ignore
> unknown mount options"), rebased atop commit 929d954330142 ("bcachefs:
> use new mount API").
>
> Contrary to the 03ef80b469d5 commit message, unknown mount options do
> result in failure on other filesystems such as xfs, btrfs, ext4, etc.
>
> Ignoring unknown options may result in unexpected behaviour: e.g. if
> an application requires future-workload-specific-mount-option to work
> and is run on a system without future-workload-specific-mount-option
> kernel support, then it may fail without indication of the missing
> kernel functionality.
>
> This reversion may cause failures for existing users reliant on the
> ignore-unknown behaviour, but it should be bearable given bcachefs is
> still flagged experimental.
>
> Fixes: 03ef80b469d5 ("bcachefs: Ignore unknown mount options")
> Reported-by: Martin Doucha <martin.doucha@suse.com>
> Closes: https://bugzilla.opensuse.org/show_bug.cgi?id=1230065
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> Reviewed-by: Hongbo Li <lihongbo22@huawei.com>
> ---
> fs/bcachefs/opts.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/bcachefs/opts.c b/fs/bcachefs/opts.c
> index e10fc1da71b19..bc5c180878212 100644
> --- a/fs/bcachefs/opts.c
> +++ b/fs/bcachefs/opts.c
> @@ -482,9 +482,8 @@ int bch2_parse_one_mount_opt(struct bch_fs *c, struct bch_opts *opts,
> val = "0";
> }
>
> - /* Unknown options are ignored: */
> if (id < 0)
> - return 0;
> + goto bad_opt;
>
> if (!(bch2_opt_table[id].flags & OPT_MOUNT))
> goto bad_opt;
Testing this patch caused my bcachefs mount command to fail:
the "user" and "nofail" attributes in /etc/fstab are not recognised.
I'd love the "nofail" attribute on a "experimental" filesystem. IMO it is even a soft requirement
Janpieter Sollie
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-10-10 18:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-17 1:39 [PATCH resend v2] bcachefs: reject unknown mount options David Disseldorp
2024-09-21 23:13 ` Kent Overstreet
2024-09-23 3:59 ` David Disseldorp
2024-09-23 6:07 ` Hongbo Li
2024-09-23 9:05 ` Hongbo Li
2024-10-10 18:33 ` Janpieter Sollie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox