[dm-devel] [PATCH] dm verity: skip verity work on I/O errors when system is shutting down

[Hyeongseok Kim]

Hi Mike,

How do you think about this patch?



On 12/7/20 8:18 AM, hyeongseok wrote:
> On 12/5/20 8:03 AM, Sami Tolvanen wrote:
>> On Thu, Dec 3, 2020 at 3:46 PM hyeongseok <hyeongseok at gmail.com> wrote:
>>> On 12/4/20 2:22 AM, Sami Tolvanen wrote:
>>>> Hi,
>>>>
>>>> On Wed, Dec 2, 2020 at 4:48 PM Hyeongseok Kim 
>>>> <hyeongseok at gmail.com> wrote:
>>>>> If emergency system shutdown is called, like by thermal shutdown,
>>>>> dm device could be alive when the block device couldn't process
>>>>> I/O requests anymore. In this status, the handling of I/O errors
>>>>> by new dm I/O requests or by those already in-flight can lead to
>>>>> a verity corruption state, which is misjudgment.
>>>>> So, skip verity work for I/O error when system is shutting down.
>>>> Thank you for the patch. I agree that attempting to correct I/O errors
>>>> when the system is shutting down, and thus generating more I/O that's
>>>> likely going to fail, is not a good idea.
>>>>
>>>>> Signed-off-by: Hyeongseok Kim <hyeongseok at gmail.com>
>>>>> ---
>>>>>    drivers/md/dm-verity-target.c | 12 +++++++++++-
>>>>>    1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/md/dm-verity-target.c 
>>>>> b/drivers/md/dm-verity-target.c
>>>>> index f74982dcbea0..ba62c537798b 100644
>>>>> --- a/drivers/md/dm-verity-target.c
>>>>> +++ b/drivers/md/dm-verity-target.c
>>>>> @@ -64,6 +64,15 @@ struct buffer_aux {
>>>>>           int hash_verified;
>>>>>    };
>>>>>
>>>>> +/*
>>>>> + * While system shutdown, skip verity work for I/O error.
>>>>> + */
>>>>> +static inline bool verity_is_system_shutting_down(void)
>>>>> +{
>>>>> +       return system_state == SYSTEM_HALT || system_state == 
>>>>> SYSTEM_POWER_OFF
>>>>> +               || system_state == SYSTEM_RESTART;
>>>>> +}
>>>> Which of these states does the system get to during an emergency
>>>> shutdown? Can we simplify this by changing the test to system_state >
>>>> SYSTEM_RUNNING?
>>> I only saw that it was SYSTEM_POWER_OFF or SYSTEM_RESTART, I wonder if
>>> I/O error can occur in SYSTEM_SUSPEND state.
>> OK, so think the current version is fine then.
>>
>>> As far as I know, this could be happen in emergency shutdown case,
>>> can you explain if you have a case when I/O error can occur by
>>> SYSTEM_SUSPEND?
>> No, I don't have a case where that would happen.
>>
>>>> Otherwise, this looks good to me. However, I'm now wondering if an I/O
>>>> error should ever result in verity_handle_err() being called. Without
>>>> FEC, dm-verity won't call verity_handle_err() when I/O fails, but with
>>>> FEC enabled, it currently does, assuming error correction fails. Any
>>>> thoughts?
>>> Yes, I have thought about this, and to be honest, I think verity or FEC
>>> should not call verity_handle_error() in case of I/O errors.
>> I tend to agree. We could simply check the original bio->bi_status in
>> verity_verify_io() and if we failed to correct an I/O error, return an
>> error instead of going into verity_handle_err(). Any thoughts?
>>
>>> But, because I couldn't know the ability of FEC, I only focused on not
>>> breaking curent logics other than system shutdown && I/O errors case.
>> Sure, makes sense. We can addrwess that separately.
> Sounds reasonable. I hope the next improvements.
>>
>> Reviewed-by: Sami Tolvanen <samitolvanen at google.com>
>>
>> Sami
>>
> Thank you for the review.
>
> Hyeongseok
>
>