On Wed 13-05-15 23:29:53, Mike Snitzer wrote: > On Wed, May 13 2015 at 10:49pm -0400, > Mike Snitzer <snitzer@xxxxxxxxxx> wrote: > > > As you know commit c4cf5261 avoids the atomic operations during endio > > unless BIO_CHAIN is set. It only gets set if bio_inc_remaining() is > > used. But bio_inc_remaining() is only used _after_ the initial endio -- > > we needed BIO_CHAIN set earlier for these cases. Seems we need a new > > function to register the intent to cascade endios (to allow for proper > > accounting)? > > > > We could then switch callers to something like: > > > > bio_set_chain(bio); // establishes BIO_CHAIN iff __bio_remaining is 1? > > old = io->bi_end_io; > > bio->bi_end_io = new; > > > > in new(): > > bio->bi_end_io = old; > > bio_inc_remaining(bio) > > > > Anyway, not sure about the proper fix -- I do know this commit is a > > definite regression (but it does nicely avoid the costly atomics ;) > > Just adding this to dm-thin.c:save_and_set_endio() fixed it: > bio->bi_flags |= (1 << BIO_CHAIN); > > (setting a bit doesn't need a bio_set_chain fn like I suggested above). > > But, all relevant callers would need this treatment (I'd imagine > bio_endio_nodec callers need help too? -- either that or __bi_remaining > is completely irrelevant for all old users except for bio_chain()). From my mostly outsider view of block layer and dm, it seems that dm-thinp isn't really interested in bi_remaining value. It just needs bio_endio() to call the original completion function and to achieve that dm-thinp increments bi_remaining. What we could do is the following: 1) bio_remaining_done() would clear BIO_CHAIN flag when __bi_remaining() drops to 0. That way bio_endio() will end up calling ->bi_end_io() always as soon as __bi_remaining drops to 0 or if bio was never chained. This wouldn't be needed if nobody can chain on bios previously modified by e.g. dm-thinp but AFAIU we cannot assume that. 2) remove bio_inc_remaining() from dm-thinp, dm-snap, dm-raid1, dm-cache-target as they all look like situations where we only want newly set bi_end_io function to be called by bio_endio(). 3) as a bonus bi_remaining handling is now fully inside block/bio.c and bio_inc_remaining() can be static there. Thoughts? Honza -- Jan Kara <jack@xxxxxxx> SUSE Labs, CR -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
