* [dm-devel] [PATCH] dm verity: skip verity work on I/O errors when system is shutting down @ 2020-12-03 0:46 Hyeongseok Kim 2020-12-03 17:22 ` Sami Tolvanen 0 siblings, 1 reply; 6+ messages in thread From: Hyeongseok Kim @ 2020-12-03 0:46 UTC (permalink / raw) To: agk, snitzer; +Cc: Hyeongseok Kim, dm-devel, samitolvanen, hyeongseok.kim 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. Signed-off-by: Hyeongseok Kim <hyeongseok@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; +} + /* * 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; } -- 2.27.0.83.g0313f36 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [dm-devel] [PATCH] dm verity: skip verity work on I/O errors when system is shutting down 2020-12-03 0:46 [dm-devel] [PATCH] dm verity: skip verity work on I/O errors when system is shutting down Hyeongseok Kim @ 2020-12-03 17:22 ` Sami Tolvanen 2020-12-03 23:46 ` hyeongseok 0 siblings, 1 reply; 6+ messages in thread From: Sami Tolvanen @ 2020-12-03 17:22 UTC (permalink / raw) To: Hyeongseok Kim Cc: device-mapper development, 김형석, Mike Snitzer, Alasdair Kergon Hi, On Wed, Dec 2, 2020 at 4:48 PM Hyeongseok Kim <hyeongseok@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@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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dm-devel] [PATCH] dm verity: skip verity work on I/O errors when system is shutting down 2020-12-03 17:22 ` Sami Tolvanen @ 2020-12-03 23:46 ` hyeongseok 2020-12-04 23:03 ` Sami Tolvanen 0 siblings, 1 reply; 6+ messages in thread From: hyeongseok @ 2020-12-03 23:46 UTC (permalink / raw) To: Sami Tolvanen Cc: device-mapper development, 김형석, Mike Snitzer, Alasdair Kergon On 12/4/20 2:22 AM, Sami Tolvanen wrote: > Hi, > > On Wed, Dec 2, 2020 at 4:48 PM Hyeongseok Kim <hyeongseok@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@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 > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dm-devel] [PATCH] dm verity: skip verity work on I/O errors when system is shutting down 2020-12-03 23:46 ` hyeongseok @ 2020-12-04 23:03 ` Sami Tolvanen 2020-12-06 23:18 ` hyeongseok 0 siblings, 1 reply; 6+ messages in thread From: Sami Tolvanen @ 2020-12-04 23:03 UTC (permalink / raw) To: hyeongseok Cc: device-mapper development, 김형석, Mike Snitzer, Alasdair Kergon On Thu, Dec 3, 2020 at 3:46 PM hyeongseok <hyeongseok@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@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@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 address that separately. Reviewed-by: Sami Tolvanen <samitolvanen@google.com> Sami -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dm-devel] [PATCH] dm verity: skip verity work on I/O errors when system is shutting down 2020-12-04 23:03 ` Sami Tolvanen @ 2020-12-06 23:18 ` hyeongseok 2020-12-10 1:03 ` Hyeongseok Kim 0 siblings, 1 reply; 6+ messages in thread From: hyeongseok @ 2020-12-06 23:18 UTC (permalink / raw) To: Sami Tolvanen Cc: device-mapper development, 김형석, Mike Snitzer, Alasdair Kergon On 12/5/20 8:03 AM, Sami Tolvanen wrote: > On Thu, Dec 3, 2020 at 3:46 PM hyeongseok <hyeongseok@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@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@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@google.com> > > Sami > Thank you for the review. Hyeongseok -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dm-devel] [PATCH] dm verity: skip verity work on I/O errors when system is shutting down 2020-12-06 23:18 ` hyeongseok @ 2020-12-10 1:03 ` Hyeongseok Kim 0 siblings, 0 replies; 6+ messages in thread From: Hyeongseok Kim @ 2020-12-10 1:03 UTC (permalink / raw) To: Mike Snitzer Cc: device-mapper development, 김형석, Alasdair Kergon, Sami Tolvanen 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@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@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@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@google.com> >> >> Sami >> > Thank you for the review. > > Hyeongseok > > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-10 14:45 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-12-03 0:46 [dm-devel] [PATCH] dm verity: skip verity work on I/O errors when system is shutting down Hyeongseok Kim 2020-12-03 17:22 ` Sami Tolvanen 2020-12-03 23:46 ` hyeongseok 2020-12-04 23:03 ` Sami Tolvanen 2020-12-06 23:18 ` hyeongseok 2020-12-10 1:03 ` Hyeongseok Kim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).