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

[hyeongseok]

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.
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?

>> +
>>   /*
>>    * Initialize struct buffer_aux for a freshly created buffer.
>>    */
>> @@ -564,7 +573,8 @@ static void verity_end_io(struct bio *bio)
>>   {
>>          struct dm_verity_io *io = bio->bi_private;
>>
>> -       if (bio->bi_status && !verity_fec_is_enabled(io->v)) {
>> +       if (bio->bi_status &&
>> +               (!verity_fec_is_enabled(io->v) || verity_is_system_shutting_down())) {
>>                  verity_finish_io(io, bio->bi_status);
>>                  return;
>>          }
> 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.
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.

>
> Sami
>