Re: [PATCH 00/20] Error handling fixes

Anand Jain
2024-01-29T23:33:31Z

On 1/30/24 00:13, David Sterba wrote:
> On Fri, Jan 26, 2024 at 08:28:12PM +0800, Anand Jain wrote:
>> On 1/25/24 05:17, David Sterba wrote:
>>> Get rid of some easy BUG_ONs, there are a few groups fixing the same
>>> pattern. At the end there are three patches that move transaction abort
>>> to the right place. I have tested it on my setups only, not the CI, but
>>> given the type of fix we'd never hit any of them without special
>>> instrumentation.
>>>
>>
>> What is our guideline for the error message location in the function
>> stack, leaf function or at the root function? IMO, it should be
>> in the leaf functions(), so that in the event of a serious error,
>> we have substantial logs to pinpoint the issue. Here, despite -EUCLEAN
>> we have no errors logged, in some of the patches. Why not also add
>> error message where it wont' endup with abort(). Otherwise, debugging
>> becomes too difficult when looking at the errors due to corruption.
>
>
> I'll try to sum up suggestions from the whole thread, the proposed end
> result of the error handling should follow this:
>
> - critical errors should be followed by a message at the place where
> they happen, typically next to an EUCLEAN
> - an exception could be something that is called frequently and the
> messages would flood the logs
>
> - transaction abort should be at the place where an error is detected,
> assuming that it's clear from the context that it's unrecoverable and
> no rollback is possible (ie. free structures, undo changes and only
> return an error)
> - this also needs inspection for special cases and exceptions
>
> - tree-checker verifies data at the time of reading from disk, we can
> make some assumptions about structure consistency
>
> - error handling should cover all possible returned values, as an
> example search slot can return < 0, 0 or 1, although some of them
> might not be possible on a consistent filesystem, all should be
> handled and return EUCLEAN + message for the "impossible" ones
>
> There are already examples that can be followed and copied elsewhere,
> however we still lack some helpers for the EUCLEAN and messages
> pattern.

Some of error log might need a rate-limited messages.

Thanks for the holistic error handling in the next series.
For this series:

Reviewed-by: Anand Jain <anand.jain@oracle.com>