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

[Sami Tolvanen]

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?

> +
>  /*
>   * 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?

Sami