On Wed, Jan 15, 2025 at 1:41 AM Viacheslav Dubeyko
<Slava.Dubeyko@xxxxxxx> wrote:
>
> Hello,
>
> There are multiple cases of using BUG_ON() in the main logic of
> CephFS kernel code. For example, ceph_msg_data_cursor_init() is
> one of the example:
>
> void ceph_msg_data_cursor_init(struct ceph_msg_data_cursor *cursor,
> struct ceph_msg *msg, size_t length)
> {
> BUG_ON(!length);
> BUG_ON(length > msg->data_length);
> BUG_ON(!msg->num_data_items);
>
> <skipped>
> }
>
> Such approach is good for the case of debugging an issue.
> But it is not user friendly approach because returning
> and processing an error is more preferable than crashing
> the kernel.
>
> This patch introduces a special debug configuration option
> for CephFS subsystems with the goal of error processing
> in the case of release build and kernel crash in the case
> of debug build:
>
> if CONFIG_CEPH_LIB_DEBUG
> BUG_ON();
> else
> return <error code>;
> endif
>
> Signed-off-by: Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx>
> ---
> fs/ceph/Kconfig | 13 +++++++++++
> include/linux/ceph/messenger.h | 2 +-
> net/ceph/Kconfig | 13 +++++++++++
> net/ceph/messenger.c | 16 +++++++++++--
> net/ceph/messenger_v1.c | 27 +++++++++++++++-------
> net/ceph/messenger_v2.c | 41 +++++++++++++++++++++++++---------
> 6 files changed, 90 insertions(+), 22 deletions(-)
>
> diff --git a/fs/ceph/Kconfig b/fs/ceph/Kconfig
> index 7249d70e1a43..203fb5d1cdd4 100644
> --- a/fs/ceph/Kconfig
> +++ b/fs/ceph/Kconfig
> @@ -50,3 +50,16 @@ config CEPH_FS_SECURITY_LABEL
>
> If you are not using a security module that requires using
> extended attributes for file security labels, say N.
> +
> +config CEPH_FS_DEBUG
> + bool "Ceph client debugging"
> + depends on CEPH_FS
> + default n
> + help
> + If you say Y here, this option enables additional pre-
> condition
> + and post-condition checks in functions. Also it could enable
> + BUG_ON() instead of returning the error code. This option
> could
> + save more messages in system log and execute additional
> computation.
> +
> + If you are going to debug the code, then chose Y here.
> + If unsure, say N.
> diff --git a/include/linux/ceph/messenger.h
> b/include/linux/ceph/messenger.h
> index 1717cc57cdac..acfab9052046 100644
> --- a/include/linux/ceph/messenger.h
> +++ b/include/linux/ceph/messenger.h
> @@ -532,7 +532,7 @@ u32 ceph_get_global_seq(struct ceph_messenger
> *msgr, u32 gt);
> void ceph_con_discard_sent(struct ceph_connection *con, u64 ack_seq);
> void ceph_con_discard_requeued(struct ceph_connection *con, u64
> reconnect_seq);
>
> -void ceph_msg_data_cursor_init(struct ceph_msg_data_cursor *cursor,
> +int ceph_msg_data_cursor_init(struct ceph_msg_data_cursor *cursor,
> struct ceph_msg *msg, size_t length);
> struct page *ceph_msg_data_next(struct ceph_msg_data_cursor *cursor,
> size_t *page_offset, size_t *length);
> diff --git a/net/ceph/Kconfig b/net/ceph/Kconfig
> index c5c4eef3a9ff..4248661669bd 100644
> --- a/net/ceph/Kconfig
> +++ b/net/ceph/Kconfig
> @@ -45,3 +45,16 @@ config CEPH_LIB_USE_DNS_RESOLVER
> Documentation/networking/dns_resolver.rst
>
> If unsure, say N.
> +
> +config CEPH_LIB_DEBUG
> + bool "Ceph core library debugging"
> + depends on CEPH_LIB
> + default n
> + help
> + If you say Y here, this option enables additional pre-
> condition
> + and post-condition checks in functions. Also it could enable
> + BUG_ON() instead of returning the error code. This option
> could
> + save more messages in system log and execute additional
> computation.
> +
> + If you are going to debug the code, then chose Y here.
> + If unsure, say N.
> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> index d1b5705dc0c6..42db34345572 100644
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -1063,18 +1063,30 @@ static void __ceph_msg_data_cursor_init(struct
> ceph_msg_data_cursor *cursor)
> cursor->need_crc = true;
> }
>
> -void ceph_msg_data_cursor_init(struct ceph_msg_data_cursor *cursor,
> - struct ceph_msg *msg, size_t length)
> +int ceph_msg_data_cursor_init(struct ceph_msg_data_cursor *cursor,
> + struct ceph_msg *msg, size_t length)
> {
> +#ifdef CONFIG_CEPH_LIB_DEBUG
> BUG_ON(!length);
> BUG_ON(length > msg->data_length);
> BUG_ON(!msg->num_data_items);
> +#else
> + if (!length)
> + return -EINVAL;
> +
> + if (length > msg->data_length)
> + return -EINVAL;
> +
> + if (!msg->num_data_items)
> + return -EINVAL;
> +#endif /* CONFIG_CEPH_LIB_DEBUG */
Hi Slava,
I don't think this is a good idea. I'm all for returning errors where
it makes sense and is possible and such cases don't actually need to be
conditioned on a CONFIG option. Here, this EINVAL error would be
raised very far away from the cause -- potentially seconds later and in
a different thread or even a different kernel module. It would still
(eventually) hang the client because the messenger wouldn't be able to
make progress for that connection/session.
With this patch in place, in the scenario that you have been chasing
where CephFS apparently asks to read X bytes but sets up a reply
message with a data buffer that is smaller than X bytes, the messenger
would enter a busy loop, endlessly reporting the new error, "faulting",
reestablishing the session, resending the outstanding read request and
attempting to fit the reply into the same (short) reply message. I'd
argue that an endless loop is worse than an easily identifiable BUG_ON
in one of the kworker threads.
There is no good way to process the new error, at least not with the
current structure of the messenger. In theory, the read request could
be failed, but that would require wider changes and a bunch of special
case code that would be there just to recover from what could have been
a BUG_ON for an obvious programming error.
Thanks,
Ilya
