[PATCH] bcachefs: fix incorrect show_options results

[PATCH] bcachefs: fix incorrect show_options results

[Hongbo Li]

When call show_options in bcachefs, the options buffer is appeneded
to the seq variable. In fact, it requires an additional comma to be
appended first. This will affect the remount process when reading
existing mount options.

Fixes: 9305cf91d05e ("bcachefs: bch2_opts_to_text()")
Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 fs/bcachefs/fs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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);
-- 
2.34.1

Re: [PATCH] bcachefs: fix incorrect show_options results

[David Disseldorp]

On Mon, 23 Sep 2024 17:50:03 +0800, Hongbo Li wrote:

> When call show_options in bcachefs, the options buffer is appeneded
> to the seq variable. In fact, it requires an additional comma to be
> appended first. This will affect the remount process when reading
> existing mount options.
> 
> Fixes: 9305cf91d05e ("bcachefs: bch2_opts_to_text()")

Ah, that'd explain why I wasn't seeing this alongside the
fail-on-unknown-opt fix. FWIW, this first appears in
bcachefs-2024-09-21 as 3621ecc10f831f4fd27784083dfaf5b8481098b5 .

> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
> ---
>  fs/bcachefs/fs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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);

Looks okay, although it might be simpler to just drop the !first
conditionals from bch2_opts_to_text() as things were prior to the
regression.

>  	int ret = buf.allocation_failure ? -ENOMEM : 0;

Should the copy from buf into seq occur if allocation_failure
is already flagged? I'd expect seq_puts() to have oopsed.

Re: [PATCH] bcachefs: fix incorrect show_options results

[Kent Overstreet]

On Mon, Sep 23, 2024 at 02:39:31PM GMT, David Disseldorp wrote:
> On Mon, 23 Sep 2024 17:50:03 +0800, Hongbo Li wrote:
> 
> > When call show_options in bcachefs, the options buffer is appeneded
> > to the seq variable. In fact, it requires an additional comma to be
> > appended first. This will affect the remount process when reading
> > existing mount options.
> > 
> > Fixes: 9305cf91d05e ("bcachefs: bch2_opts_to_text()")
> 
> Ah, that'd explain why I wasn't seeing this alongside the
> fail-on-unknown-opt fix. FWIW, this first appears in
> bcachefs-2024-09-21 as 3621ecc10f831f4fd27784083dfaf5b8481098b5 .
> 
> > Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
> > ---
> >  fs/bcachefs/fs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 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);
> 
> Looks okay, although it might be simpler to just drop the !first
> conditionals from bch2_opts_to_text() as things were prior to the
> regression.
> 
> >  	int ret = buf.allocation_failure ? -ENOMEM : 0;
> 
> Should the copy from buf into seq occur if allocation_failure
> is already flagged? I'd expect seq_puts() to have oopsed.

That's a good point; it might be the first allocation that fails, not a
realloc. It won't be in practice, buuuut....

Re: [PATCH] bcachefs: fix incorrect show_options results

[David Disseldorp]

On Mon, 23 Sep 2024 17:40:05 -0400, Kent Overstreet wrote:
> On Mon, Sep 23, 2024 at 02:39:31PM GMT, David Disseldorp wrote:
...
> > >  	int ret = buf.allocation_failure ? -ENOMEM : 0;  
> > 
> > Should the copy from buf into seq occur if allocation_failure
> > is already flagged? I'd expect seq_puts() to have oopsed.
> 
> That's a good point; it might be the first allocation that fails, not a
> realloc. It won't be in practice, buuuut....

I've sent a follow-up patch to avoid the seq_puts(seq, NULL). Feel free
to squash it in with this if you like.

Re: [PATCH] bcachefs: fix incorrect show_options results

[Hongbo Li]

On 2024/9/23 22:39, David Disseldorp wrote:
> On Mon, 23 Sep 2024 17:50:03 +0800, Hongbo Li wrote:
> 
>> When call show_options in bcachefs, the options buffer is appeneded
>> to the seq variable. In fact, it requires an additional comma to be
>> appended first. This will affect the remount process when reading
>> existing mount options.
>>
>> Fixes: 9305cf91d05e ("bcachefs: bch2_opts_to_text()")
> 
> Ah, that'd explain why I wasn't seeing this alongside the
> fail-on-unknown-opt fix. FWIW, this first appears in
> bcachefs-2024-09-21 as 3621ecc10f831f4fd27784083dfaf5b8481098b5 .
> 
>> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
>> ---
>>   fs/bcachefs/fs.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> 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);
> 
> Looks okay, although it might be simpler to just drop the !first
> conditionals from bch2_opts_to_text() as things were prior to the
> regression.
> 

The bch2_opts_to_text may be intended to provide a unified format(by 
joining options into a string with commas), although currently ther is 
only one call to it.

>>   	int ret = buf.allocation_failure ? -ENOMEM : 0;
> 
> Should the copy from buf into seq occur if allocation_failure
> is already flagged? I'd expect seq_puts() to have oopsed.

Or to be more rigorous, copy buf only when allocation_failure is false?

Thanks,
Hongbo