| Message ID | 20200225170744.10485-1-dg@emlix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dm integrity: reinitialize __bi_remaining when reusing bio | expand |
On Tue, Feb 25, 2020 at 06:07:44PM +0100, Daniel Glöckner wrote: > In cases where dec_in_flight has to requeue the integrity_bio_wait work > to transfer the rest of the data, the __bi_remaining field of the bio > might already have been decremented to zero. Reusing the bio without > reinitializing that counter to 1 can then result in integrity_end_io > being called too early when the BIO_CHAIN flag is set, f.ex. due to > blk_queue_split. In our case this triggered the BUG() in > blk_mq_end_request when the hardware signalled completion of the bio > after integrity_end_io had modified it. > > Signed-off-by: Daniel Glöckner <dg@emlix.com> Drivers have no business poking into these internals. If a bio is reused the caller needs to use bio_reset instead.
Hello Christoph, Am 02/25/20 um 20:12 schrieb Christoph Hellwig: > On Tue, Feb 25, 2020 at 06:07:44PM +0100, Daniel Glöckner wrote: >> In cases where dec_in_flight has to requeue the integrity_bio_wait work >> to transfer the rest of the data, the __bi_remaining field of the bio >> might already have been decremented to zero. Reusing the bio without >> reinitializing that counter to 1 can then result in integrity_end_io >> being called too early when the BIO_CHAIN flag is set, f.ex. due to >> blk_queue_split. In our case this triggered the BUG() in >> blk_mq_end_request when the hardware signalled completion of the bio >> after integrity_end_io had modified it. >> >> Signed-off-by: Daniel Glöckner <dg@emlix.com> > > Drivers have no business poking into these internals. If a bio is > reused the caller needs to use bio_reset instead. bio_reset will reset too many fields. As you can see in the context of the diff, dm-integrity expects f.ex. the values modified by bio_advance to stay intact and the transfer should of course use the same disk and operation. How about doing the atomic_set in bio_remaining_done (in block/bio.c) where the BIO_CHAIN flag is cleared once __bi_remaining hits zero? Or is requeuing a bio without bio_reset really a no-go? In that case a one-liner won't do... Best regards, Daniel
On Tue, Feb 25, 2020 at 08:54:07PM +0100, Daniel Glöckner wrote: > bio_reset will reset too many fields. As you can see in the context of > the diff, dm-integrity expects f.ex. the values modified by bio_advance > to stay intact and the transfer should of course use the same disk and > operation. > > How about doing the atomic_set in bio_remaining_done (in block/bio.c) > where the BIO_CHAIN flag is cleared once __bi_remaining hits zero? > Or is requeuing a bio without bio_reset really a no-go? In that case a > one-liner won't do... That tends to add a overhead to the fast path for a rather exotic case. I'm having a bit of a hard time understanding the dm-integrity code due to it's annoyingly obsfucated code, but it seems like it tries to submit a bio again after it came out of a ->end_io handler. That might have some other problems, but if we only want to paper over the remaining count a isngle call to bio_inc_remaining might be all you need.
On Tue, Feb 25 2020 at 5:02pm -0500, Christoph Hellwig <hch@infradead.org> wrote: > On Tue, Feb 25, 2020 at 08:54:07PM +0100, Daniel Glöckner wrote: > > bio_reset will reset too many fields. As you can see in the context of > > the diff, dm-integrity expects f.ex. the values modified by bio_advance > > to stay intact and the transfer should of course use the same disk and > > operation. > > > > How about doing the atomic_set in bio_remaining_done (in block/bio.c) > > where the BIO_CHAIN flag is cleared once __bi_remaining hits zero? > > Or is requeuing a bio without bio_reset really a no-go? In that case a > > one-liner won't do... > > That tends to add a overhead to the fast path for a rather exotic > case. I'm having a bit of a hard time understanding the dm-integrity > code due to it's annoyingly obsfucated code, but it seems like it > tries to submit a bio again after it came out of a ->end_io handler. Yeah, the dm-integrity code has always been complex and it has only gotten moreso. I'm struggling with it too. This case that Daniel has seen a BUG_ON with is when there is a need to finish a partially completed bio (as reflected by the per-bio-data's internal accounting managed by dm-integrity). > That might have some other problems, but if we only want to paper > over the remaining count a isngle call to bio_inc_remaining might be all > you need. bio_inc_remaining() is really meant to be paired with bio_chain(). They are pretty tightly coupled these days. We removed __bio_inc_remaining() once we weened all (ab)users many releases ago. Definitely don't want an open-coded equivalent buried in an obscure dm-integrity usecase. Could be bio_split() + generic_make_request() recursion is a way forward -- but that'd likely require some extra work in dm-integrity. All the additional code in dm_integrity_map_continue() makes me think I could easily be missing something. Mikulas, if you could look closer at this issue and see what your best suggestion would be that'd be appreciated. Thanks, Mike
diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c index b225b3e445fa..8cea2978fc24 100644 --- a/drivers/md/dm-integrity.c +++ b/drivers/md/dm-integrity.c @@ -1438,6 +1438,7 @@ static void dec_in_flight(struct dm_integrity_io *dio) if (likely(!bio->bi_status) && unlikely(bio_sectors(bio) != dio->range.n_sectors)) { dio->range.logical_sector += dio->range.n_sectors; bio_advance(bio, dio->range.n_sectors << SECTOR_SHIFT); + atomic_set(&bio->__bi_remaining, 1); INIT_WORK(&dio->work, integrity_bio_wait); queue_work(ic->wait_wq, &dio->work); return;
In cases where dec_in_flight has to requeue the integrity_bio_wait work to transfer the rest of the data, the __bi_remaining field of the bio might already have been decremented to zero. Reusing the bio without reinitializing that counter to 1 can then result in integrity_end_io being called too early when the BIO_CHAIN flag is set, f.ex. due to blk_queue_split. In our case this triggered the BUG() in blk_mq_end_request when the hardware signalled completion of the bio after integrity_end_io had modified it. Signed-off-by: Daniel Glöckner <dg@emlix.com> --- drivers/md/dm-integrity.c | 1 + 1 file changed, 1 insertion(+)