From: Hongbo Li <lihongbo22@huawei.com>
To: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Brian Foster <bfoster@redhat.com>,
<linux-bcachefs@vger.kernel.org>, <ruanjinjie@huawei.com>,
<chris.zjh@huawei.com>
Subject: Re: [PATCH] bcachefs: intercept mountoption value for bool type
Date: Thu, 22 Feb 2024 11:46:35 +0800 [thread overview]
Message-ID: <6606c887-7bf3-435a-ac6c-5008115607ae@huawei.com> (raw)
In-Reply-To: <sfdxvffrgdjxthrblqqc2vj2zgeuszrmo6muqkicdryx4ulzrp@wfbw4muo54ow>
On 2024/2/22 11:03, Kent Overstreet wrote:
> On Thu, Feb 22, 2024 at 09:30:20AM +0800, Hongbo Li wrote:
>>
>>
>> On 2024/2/22 7:51, Kent Overstreet wrote:
>>> On Wed, Feb 21, 2024 at 08:36:58AM -0500, Brian Foster wrote:
>>>> On Wed, Feb 21, 2024 at 05:37:04PM +0800, Hongbo Li wrote:
>>>>> For mount option with bool type, the value must be 0 or 1 (See
>>>>> bch2_opt_parse). But this seems does not well intercepted cause
>>>>> for other value(like 2...), it returns the unexpect return code
>>>>> with error message printed.
>>>>>
>>>>> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
>>>>> ---
>>>>> fs/bcachefs/opts.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/fs/bcachefs/opts.c b/fs/bcachefs/opts.c
>>>>> index b1ed0b9a20d3..fe1f17830694 100644
>>>>> --- a/fs/bcachefs/opts.c
>>>>> +++ b/fs/bcachefs/opts.c
>>>>> @@ -314,7 +314,7 @@ int bch2_opt_parse(struct bch_fs *c,
>>>>> if (ret < 0 || (*res != 0 && *res != 1)) {
>>>>> if (err)
>>>>> prt_printf(err, "%s: must be bool", opt->attr.name);
>>>>> - return ret;
>>>>> + return ret < 0 ?: -EINVAL;
>>>>
>>>> It might be nice for that error message to be more specific in that the
>>>> bool value must be 0 or 1, but LGTM regardless:
>>>
>>> Please give this a distinct private error code as well (just like we
>>> discussed in the previous patch)
>>>
>> Here, the -EINVAL refers to the return value of other exception branches of
>> this function to keep the consistent error code of this function.
>>
>> In fact, it is a bit confusing to use whether to return a private error code
>> or a system error code in some functions. For example, in bch2_opt_validate,
>> the two exception branches of no sector alignment and power of 2 do not
>> return internal error codes.
>
> bcachefs code should _never_ throw standard error codes; the only case I
> make an exception for, and in some situations is -ENOMEM - because
> generally speaking an allocation failure will emit a backtrace, and most
> allocation failures we never really hit in practice and if we're OOMing,
> the exact allocation is uninteresting.
>
> But even that isn't a hard rule - I've seen bugs where the filesystem
> was failing to open (i.e. mount was failing) due to a failed allocation
> and we did need to know which one, and sometimes there's one allocation
> hiding among the many that's way bigger than you expected - so for all
> the init paths, I try to make sure we have error codes that correspond
> to each allocation.
>
> So in short: always define a distinct error code and give it the best
> name you can think of
>
ok, thank you. This answers my confusion.
>> I don't know what the boundaries are for using private error codes.
>> Generally, internal functions are passed through private error codes, but
>> now I can only refer to this function context to determine the error code
>> type.
>
> The boundary is the bcachefs module boundary - generally speaking the
> last line of code when we're returning to non-bcachefs code should be
> the bch2_err_class() call.
>
> Incidentally, I've been meaning to add a tracepoint so that we can
> recover these error codes even when they're not logged; thanks for
> bringing this up as a reminder. Doing that tonight, and writing the
> following documentation:
>
> diff --git a/Documentation/filesystems/bcachefs/errorcodes.rst b/Documentation/filesystems/bcachefs/errorcodes.rst
> new file mode 100644
> index 000000000000..2cccaa0ba7cd
> --- /dev/null
> +++ b/Documentation/filesystems/bcachefs/errorcodes.rst
> @@ -0,0 +1,30 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +bcachefs private error codes
> +----------------------------
> +
> +In bcachefs, as a hard rule we do not throw or directly use standard error
> +codes (-EINVAL, -EBUSY, etc.). Instead, we define private error codes as needed
> +in fs/bcachefs/errcode.h.
> +
> +This gives us much better error messages and makes debugging much easier. Any
> +direct uses of standard error codes you see in the source code are simply old
> +code that has yet to be converted - feel free to clean it up!
> +
I will refine this patch later. But before that, please help review this
patch first:
https://lore.kernel.org/linux-bcachefs/tcjqfii2emtqwpsgt4doldgg2iwjnsh6fzv7nkdccxt43gpu4g@pfasjkkj3e4k/T/#t
It introduced the parent error code about mount option error in the
first time (x(EINVAL,mount_option)). Later, I will define more private
error code related to mount error in other patch.
Thank you!
> +Private error codes may subtype another error code, this allows for grouping of
> +related errors that should be handled similarly (e.g. transaction restart
> +errors), as well as specifying which standard error code should be returned at
> +the bcachefs module boundary.
> +
> +At the module boundary, we use bch2_err_class() to convert to a standard error
> +code; this also emits a trace event so that the original error code be
> +recovered even if it wasn't logged.
> +
> +Do not reuse error codes! Generally speaking, a private error code should only
> +be thrown in one place. That means that when we see it in a log message we can
> +see, unambiguously, exactly which file and line number it was returned from.
> +
> +Try to give error codes names that are as reasonably descriptive of the error
> +as possible. Frequently, the error will be logged at a place far removed from
> +where the error was generated; good names for error codes mean much more
> +descriptive and useful error messages.
> diff --git a/fs/bcachefs/trace.h b/fs/bcachefs/trace.h
> index 2baba5f9ad3f..cb35789d591e 100644
> --- a/fs/bcachefs/trace.h
> +++ b/fs/bcachefs/trace.h
> @@ -1443,6 +1443,12 @@ DEFINE_EVENT(fs_str, data_update,
> TP_ARGS(c, str)
> );
>
> +TRACE_EVENT(error_downcast,
> + TP_PROTO(struct bch_fs *c, int bch_err, int std_err, unsigned long ip),
> + TP_ARGS(c, str)
> +);
> +
> #endif /* _TRACE_BCACHEFS_H */
>
> /* This part must be outside protection */
prev parent reply other threads:[~2024-02-22 3:46 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-21 9:37 [PATCH] bcachefs: intercept mountoption value for bool type Hongbo Li
2024-02-21 13:36 ` Brian Foster
2024-02-21 23:51 ` Kent Overstreet
2024-02-22 1:30 ` Hongbo Li
2024-02-22 3:03 ` Kent Overstreet
2024-02-22 3:46 ` Hongbo Li [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6606c887-7bf3-435a-ac6c-5008115607ae@huawei.com \
--to=lihongbo22@huawei.com \
--cc=bfoster@redhat.com \
--cc=chris.zjh@huawei.com \
--cc=kent.overstreet@linux.dev \
--cc=linux-bcachefs@vger.kernel.org \
--cc=ruanjinjie@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).