On Tue, Feb 25 2020 at 5:02pm -0500, Christoph Hellwig <hch@xxxxxxxxxxxxx> 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
