| Message ID | 1462463665-85312-3-git-send-email-snitzer@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 05, 2016 at 11:54:22AM -0400, Mike Snitzer wrote: > Commit 326e1dbb57 ("block: remove management of bi_remaining when > restoring original bi_end_io") made bio_inc_remaining() private to bio.c > because the only use-case that made sense was confined to the > bio_chain() interface. > > Since that time DM thinp went on to use bio_chain() in its relatively > complex implementation of async discard support. That implementation, > even when converted over to use the new async __blkdev_issue_discard() > interface, depends on deferred completion of the original discard bio -- > which is most appropriately implemented using bio_inc_remaining(). Can you explain that code flow to me? I still fail to why exactly dm-thinp (and only dm-thinp) needs this. Maybe start by pointing me to the additional bio_endio that pairs with the bio_inc_remaining call. > All said, bio_inc_remaining() should really only be used in conjunction > with bio_chain(). It isn't intended for generic bio reference counting. It's obviously used outside bio_chain in dm-thinp, so this sentence doesn't make too much sense to me. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 06 2016 at 11:25am -0400, Christoph Hellwig <hch@lst.de> wrote: > On Thu, May 05, 2016 at 11:54:22AM -0400, Mike Snitzer wrote: > > Commit 326e1dbb57 ("block: remove management of bi_remaining when > > restoring original bi_end_io") made bio_inc_remaining() private to bio.c > > because the only use-case that made sense was confined to the > > bio_chain() interface. > > > > Since that time DM thinp went on to use bio_chain() in its relatively > > complex implementation of async discard support. That implementation, > > even when converted over to use the new async __blkdev_issue_discard() > > interface, depends on deferred completion of the original discard bio -- > > which is most appropriately implemented using bio_inc_remaining(). > > Can you explain that code flow to me? I still fail to why exactly > dm-thinp (and only dm-thinp) needs this. Maybe start by pointing me > to the additional bio_endio that pairs with the bio_inc_remaining > call. If you look at the latest code here: https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/log/?h=dm-4.7 dm-thin.c:break_up_discard_bio()'s bio_inc_remaining() is paired with dm-thin.c:end_discard()'s bio_endio() But the flow is: -> process_discard_cell_passdown -> break_up_discard_bio (per-mapping reference taken on original discard bio) -> bio_endio(original discard bio) -- cleanest place to complete it.. but doing so here requires extra references on that discard bio because we must wait for the N mappings and associated sub-discard bios to be chained, issued, and completed ... wait for associated sub-discard mappings to quiesce and become "prepared"... -> process_prepared_discard_passdown (bio-chains aren't constructed until here) -> passdown_double_checking_shared_status (iterates over each mappings' subset of the original discard) -> __blkdev_issue_discard -> bio_endio (per-mapping reference, taken in break_up_discard_bio, is dropped) > > All said, bio_inc_remaining() should really only be used in conjunction > > with bio_chain(). It isn't intended for generic bio reference counting. > > It's obviously used outside bio_chain in dm-thinp, so this sentence > doesn't make too much sense to me. It is used in conjunction with supporting thinp's use of bio_chain(), via __blkdev_issue_discard, but yeah I can see why you think they aren't related. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/block/bio.c b/block/bio.c index 807d25e..0e4aa42 100644 --- a/block/bio.c +++ b/block/bio.c @@ -311,17 +311,6 @@ static void bio_chain_endio(struct bio *bio) bio_endio(__bio_chain_endio(bio)); } -/* - * Increment chain count for the bio. Make sure the CHAIN flag update - * is visible before the raised count. - */ -static inline void bio_inc_remaining(struct bio *bio) -{ - bio_set_flag(bio, BIO_CHAIN); - smp_mb__before_atomic(); - atomic_inc(&bio->__bi_remaining); -} - /** * bio_chain - chain bio completions * @bio: the target bio diff --git a/include/linux/bio.h b/include/linux/bio.h index 6b7481f..9faebf7 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -703,6 +703,17 @@ static inline struct bio *bio_list_get(struct bio_list *bl) } /* + * Increment chain count for the bio. Make sure the CHAIN flag update + * is visible before the raised count. + */ +static inline void bio_inc_remaining(struct bio *bio) +{ + bio_set_flag(bio, BIO_CHAIN); + smp_mb__before_atomic(); + atomic_inc(&bio->__bi_remaining); +} + +/* * bio_set is used to allow other portions of the IO system to * allocate their own private memory pools for bio and iovec structures. * These memory pools in turn all allocate from the bio_slab
Commit 326e1dbb57 ("block: remove management of bi_remaining when restoring original bi_end_io") made bio_inc_remaining() private to bio.c because the only use-case that made sense was confined to the bio_chain() interface. Since that time DM thinp went on to use bio_chain() in its relatively complex implementation of async discard support. That implementation, even when converted over to use the new async __blkdev_issue_discard() interface, depends on deferred completion of the original discard bio -- which is most appropriately implemented using bio_inc_remaining(). DM thinp foolishly duplicated bio_inc_remaining(), local to dm-thin.c as __bio_inc_remaining(), so re-exporting bio_inc_remaining() allows us to put an end to that foolishness. All said, bio_inc_remaining() should really only be used in conjunction with bio_chain(). It isn't intended for generic bio reference counting. Signed-off-by: Mike Snitzer <snitzer@redhat.com> Acked-by: Joe Thornber <ejt@redhat.com> --- block/bio.c | 11 ----------- include/linux/bio.h | 11 +++++++++++ 2 files changed, 11 insertions(+), 11 deletions(-)