| Message ID | 9a168461fc4665edffde6d8606920a34312f8932.camel@ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ceph: Introduce CONFIG_CEPH_LIB_DEBUG and CONFIG_CEPH_FS_DEBUG | expand |
On Wed, Jan 15, 2025 at 1:41 AM Viacheslav Dubeyko <Slava.Dubeyko@ibm.com> 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@ibm.com> > --- > 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
Hi Ilya, On Thu, 2025-01-16 at 00:04 +0100, Ilya Dryomov wrote: > On Wed, Jan 15, 2025 at 1:41 AM Viacheslav Dubeyko > <Slava.Dubeyko@ibm.com> wrote: > > > > <skipped> > > > > -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. > First of all, let's split the patch on two parts: (1) CONFIG options suggestion; (2) practical application of CONFIG option. I believe that such CONFIG option is useful for adding pre-condition and post-condition checks in methods that could be executed in debug compilation and it will be excluded from release compilation for production case. Potentially, the first application of this CONFIG option is not good enough. However, the kernel crash is good for the problem investigation (debug compilation, for example), but end-user would like to see working kernel but not crashed one. And returning error is a way to behave in a nice way, from my point of view. > 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. > Yes, I totally see your point. But I believe that as kernel crash as busy loop is wrong behavior. Ideally, we need to report the error and continue to work without kernel crash or busy loop. Would we rework the logic to be more user-friendly and to behave more nicely? I don't quite follow why do we have busy loop even if we know that request is failed? Generally speaking, failed request should be discarded, from the common sense. :) Thanks, Slava.
On Thu, Jan 16, 2025 at 3:01 AM Viacheslav Dubeyko <Slava.Dubeyko@ibm.com> wrote: > > Hi Ilya, > > On Thu, 2025-01-16 at 00:04 +0100, Ilya Dryomov wrote: > > On Wed, Jan 15, 2025 at 1:41 AM Viacheslav Dubeyko > > <Slava.Dubeyko@ibm.com> wrote: > > > > > > > > <skipped> > > > > > > > -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. > > > > First of all, let's split the patch on two parts: > (1) CONFIG options suggestion; > (2) practical application of CONFIG option. > > I believe that such CONFIG option is useful for adding > pre-condition and post-condition checks in methods that > could be executed in debug compilation and it will be > excluded from release compilation for production case. > > Potentially, the first application of this CONFIG option > is not good enough. However, the kernel crash is good for > the problem investigation (debug compilation, for example), > but end-user would like to see working kernel but not crashed one. > And returning error is a way to behave in a nice way, > from my point of view. We can definitely consider such a CONFIG option where there is a good application for it. > > > 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. > > > > Yes, I totally see your point. But I believe that as kernel crash as > busy loop is wrong behavior. Ideally, we need to report the error and > continue to work without kernel crash or busy loop. Would we rework > the logic to be more user-friendly and to behave more nicely? I'm not sure it would be worth the effort in this particular case. > I don't quite follow why do we have busy loop even if we know that > request is failed? Generally speaking, failed request should be > discarded, from the common sense. :) The messenger assumes that most errors are transient, so it simply reestablishes the session and resends outstanding requests. The main reason for this is that depending on how far in the message the error is raised, a corresponding request may not be known yet (consider a scenario where the error pops up before the messenger gets to the fields that identify the request, for example) or there may not be a external request to fail at all. If the request is identified and the error is assumed to be permanent, the request can indeed be failed to the submitter, but currently there is no support for that. There is something more crude where the OSD client can tell the messenger to skip the message and move on -- see get_reply() and @skip parameter in net/ceph/osd_client.c. Normally it's used to skip over duplicate or misdirected messages, but it can also be used to skip a message on a would-be-permanent error that is associated with that particular message. With that, the submitter is never going to see a reply to that request and would likely get stuck due to that at some point, but once again these are almost always basic logic errors. Thanks, Ilya
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 */ cursor->total_resid = length; cursor->data = msg->data; cursor->sr_resid = 0; __ceph_msg_data_cursor_init(cursor); + return 0; } /* diff --git a/net/ceph/messenger_v1.c b/net/ceph/messenger_v1.c index 0cb61c76b9b8..bc2f3a43d572 100644 --- a/net/ceph/messenger_v1.c +++ b/net/ceph/messenger_v1.c @@ -157,12 +157,12 @@ static size_t sizeof_footer(struct ceph_connection *con) sizeof(struct ceph_msg_footer_old); } -static void prepare_message_data(struct ceph_msg *msg, u32 data_len) +static int prepare_message_data(struct ceph_msg *msg, u32 data_len) { /* Initialize data cursor if it's not a sparse read */ u64 len = msg->sparse_read_total ? : data_len; - ceph_msg_data_cursor_init(&msg->cursor, msg, len); + return ceph_msg_data_cursor_init(&msg->cursor, msg, len); } /* @@ -192,10 +192,11 @@ static void prepare_write_message_footer(struct ceph_connection *con) /* * Prepare headers for the next outgoing message. */ -static void prepare_write_message(struct ceph_connection *con) +static int prepare_write_message(struct ceph_connection *con) { struct ceph_msg *m; u32 crc; + int ret; con_out_kvec_reset(con); con->v1.out_msg_done = false; @@ -251,7 +252,10 @@ static void prepare_write_message(struct ceph_connection *con) /* is there a data payload? */ con->out_msg->footer.data_crc = 0; if (m->data_length) { - prepare_message_data(con->out_msg, m->data_length); + ret = prepare_message_data(con->out_msg, m- >data_length); + if (ret) + return ret; + con->v1.out_more = 1; /* data + footer will follow */ } else { /* no, queue up footer too and be done */ @@ -259,6 +263,7 @@ static void prepare_write_message(struct ceph_connection *con) } ceph_con_flag_set(con, CEPH_CON_F_WRITE_PENDING); + return 0; } /* @@ -1230,8 +1235,11 @@ static int read_partial_message(struct ceph_connection *con) /* prepare for data payload, if any */ - if (data_len) - prepare_message_data(con->in_msg, data_len); + if (data_len) { + ret = prepare_message_data(con->in_msg, data_len); + if (ret) + return ret; + } } /* front */ @@ -1546,8 +1554,11 @@ int ceph_con_v1_try_write(struct ceph_connection *con) } /* is anything else pending? */ if (!list_empty(&con->out_queue)) { - prepare_write_message(con); - goto more; + ret = prepare_write_message(con); + if (ret) + goto out; + else + goto more; } if (con->in_seq > con->in_seq_acked) { prepare_write_ack(con); diff --git a/net/ceph/messenger_v2.c b/net/ceph/messenger_v2.c index bd608ffa0627..0904821c8dfa 100644 --- a/net/ceph/messenger_v2.c +++ b/net/ceph/messenger_v2.c @@ -1026,7 +1026,10 @@ static int setup_message_sgs(struct sg_table *sgt, struct ceph_msg *msg, if (need_padding(dlen)) sg_cnt++; } else { - ceph_msg_data_cursor_init(&cursor, msg, dlen); + ret = ceph_msg_data_cursor_init(&cursor, msg, dlen); + if (ret) + return ret; + sg_cnt += calc_sg_cnt_cursor(&cursor);
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@ibm.com> --- 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(-) } } @@ -1046,7 +1049,10 @@ static int setup_message_sgs(struct sg_table *sgt, struct ceph_msg *msg, if (pages) { init_sgs_pages(&cur_sg, pages, dpos, dlen, data_pad); } else { - ceph_msg_data_cursor_init(&cursor, msg, dlen); + ret = ceph_msg_data_cursor_init(&cursor, msg, dlen); + if (ret) + return ret; + init_sgs_cursor(&cur_sg, &cursor, data_pad); } } @@ -1860,10 +1866,13 @@ static int prepare_read_control_remainder(struct ceph_connection *con) static int prepare_read_data(struct ceph_connection *con) { struct bio_vec bv; + int ret; con->in_data_crc = -1; - ceph_msg_data_cursor_init(&con->v2.in_cursor, con->in_msg, - data_len(con->in_msg)); + ret = ceph_msg_data_cursor_init(&con->v2.in_cursor, con- >in_msg, + data_len(con->in_msg)); + if (ret) + return ret; get_bvec_at(&con->v2.in_cursor, &bv); if (ceph_test_opt(from_msgr(con->msgr), RXBOUNCE)) { @@ -2025,6 +2034,7 @@ static int prepare_sparse_read_cont(struct ceph_connection *con) static int prepare_sparse_read_data(struct ceph_connection *con) { struct ceph_msg *msg = con->in_msg; + int ret; dout("%s: starting sparse read\n", __func__); @@ -2034,8 +2044,10 @@ static int prepare_sparse_read_data(struct ceph_connection *con) if (!con_secure(con)) con->in_data_crc = -1; - ceph_msg_data_cursor_init(&con->v2.in_cursor, msg, - msg->sparse_read_total); + ret = ceph_msg_data_cursor_init(&con->v2.in_cursor, msg, + msg->sparse_read_total); + if (ret) + return ret; reset_in_kvecs(con); con->v2.in_state = IN_S_PREPARE_SPARSE_DATA_CONT; @@ -3184,17 +3196,21 @@ int ceph_con_v2_try_read(struct ceph_connection *con) } } -static void queue_data(struct ceph_connection *con) +static int queue_data(struct ceph_connection *con) { struct bio_vec bv; + int ret; con->v2.out_epil.data_crc = -1; - ceph_msg_data_cursor_init(&con->v2.out_cursor, con->out_msg, - data_len(con->out_msg)); + ret = ceph_msg_data_cursor_init(&con->v2.out_cursor, con- >out_msg, + data_len(con->out_msg)); + if (ret) + return ret; get_bvec_at(&con->v2.out_cursor, &bv); set_out_bvec(con, &bv, true); con->v2.out_state = OUT_S_QUEUE_DATA_CONT; + return 0; } static void queue_data_cont(struct ceph_connection *con) @@ -3309,8 +3325,11 @@ static int populate_out_iter(struct ceph_connection *con) switch (con->v2.out_state) { case OUT_S_QUEUE_DATA: WARN_ON(!con->out_msg); - queue_data(con); - goto populated; + ret = queue_data(con); + if (ret) + return ret; + else + goto populated; case OUT_S_QUEUE_DATA_CONT: WARN_ON(!con->out_msg); queue_data_cont(con);