Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755284AbbESHUi (ORCPT ); Tue, 19 May 2015 03:20:38 -0400 Received: from cantor2.suse.de ([195.135.220.15]:38130 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755253AbbESHUf (ORCPT ); Tue, 19 May 2015 03:20:35 -0400 Date: Tue, 19 May 2015 09:20:29 +0200 From: Jan Kara To: Mike Snitzer Cc: Jan Kara , hch@infradead.org, Jens Axboe , Chris Mason , dm-devel@redhat.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH for-4.2 v2 01/14] block: remove management of bi_remaining when restoring original bi_end_io Message-ID: <20150519072029.GB12901@quack.suse.cz> References: <1431637512-64245-1-git-send-email-snitzer@redhat.com> <1431637512-64245-2-git-send-email-snitzer@redhat.com> <20150518072237.GE26351@quack.suse.cz> <20150518131358.GA13998@redhat.com> <20150518153644.GA10187@quack.suse.cz> <20150518155948.GE13998@redhat.com> <20150518204017.GA17202@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150518204017.GA17202@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12022 Lines: 351 On Mon 18-05-15 16:40:17, Mike Snitzer wrote: > Commit c4cf5261 ("bio: skip atomic inc/dec of ->bi_remaining for > non-chains") regressed all existing callers that followed this pattern: > 1) saving a bio's original bi_end_io > 2) wiring up an intermediate bi_end_io > 3) restoring the original bi_end_io from intermediate bi_end_io > 4) calling bio_endio() to execute the restored original bi_end_io > > The regression was due to BIO_CHAIN only ever getting set if > bio_inc_remaining() is called. For the above pattern it isn't set until > step 3 above (step 2 would've needed to establish BIO_CHAIN). As such > the first bio_endio(), in step 2 above, never decremented __bi_remaining > before calling the intermediate bi_end_io -- leaving __bi_remaining with > the value 1 instead of 0. When bio_inc_remaining() occurred during step > 3 it brought it to a value of 2. When the second bio_endio() was > called, in step 4 above, it should've called the original bi_end_io but > it didn't because there was an extra reference that wasn't dropped (due > to atomic operations being optimized away since BIO_CHAIN wasn't set > upfront). > > Fix this issue by removing the __bi_remaining management complexity for > all callers that use the above pattern -- bio_chain() is the only > interface that _needs_ to be concerned with __bi_remaining. For the > above pattern callers just expect the bi_end_io they set to get called! > Remove bio_endio_nodec() and also remove all bio_inc_remaining() calls > that aren't associated with the bio_chain() interface. > > changes in v2: > Care is now taken to clear the BIO_CHAIN flag once ->__bi_remaining > drops to 0. Also, the bio_inc_remaining() interface has moved local to > bio.c. > > Fixes: c4cf5261 ("bio: skip atomic inc/dec of ->bi_remaining for non-chains") > Signed-off-by: Mike Snitzer The patch looks good to me. You can add: Reviewed-by: Jan Kara Honza > --- > block/bio-integrity.c | 4 ++-- > block/bio.c | 35 ++++++++++++++--------------------- > drivers/md/bcache/io.c | 2 +- > drivers/md/dm-cache-target.c | 6 ------ > drivers/md/dm-raid1.c | 2 -- > drivers/md/dm-snap.c | 1 - > drivers/md/dm-thin.c | 9 +++------ > drivers/md/dm-verity.c | 2 +- > fs/btrfs/disk-io.c | 2 +- > fs/btrfs/volumes.c | 16 +++++----------- > fs/btrfs/volumes.h | 2 -- > include/linux/bio.h | 12 ------------ > 12 files changed, 27 insertions(+), 66 deletions(-) > > diff --git a/block/bio-integrity.c b/block/bio-integrity.c > index 5cbd5d9..0436c21 100644 > --- a/block/bio-integrity.c > +++ b/block/bio-integrity.c > @@ -361,7 +361,7 @@ static void bio_integrity_verify_fn(struct work_struct *work) > > /* Restore original bio completion handler */ > bio->bi_end_io = bip->bip_end_io; > - bio_endio_nodec(bio, error); > + bio_endio(bio, error); > } > > /** > @@ -388,7 +388,7 @@ void bio_integrity_endio(struct bio *bio, int error) > */ > if (error) { > bio->bi_end_io = bip->bip_end_io; > - bio_endio_nodec(bio, error); > + bio_endio(bio, error); > > return; > } > diff --git a/block/bio.c b/block/bio.c > index c2ff8a8..259197d 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -303,6 +303,17 @@ static void bio_chain_endio(struct bio *bio, int error) > bio_put(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->bi_flags |= (1 << BIO_CHAIN); > + smp_mb__before_atomic(); > + atomic_inc(&bio->__bi_remaining); > +} > + > /** > * bio_chain - chain bio completions > * @bio: the target bio > @@ -1756,8 +1767,10 @@ static inline bool bio_remaining_done(struct bio *bio) > > BUG_ON(atomic_read(&bio->__bi_remaining) <= 0); > > - if (atomic_dec_and_test(&bio->__bi_remaining)) > + if (atomic_dec_and_test(&bio->__bi_remaining)) { > + clear_bit(BIO_CHAIN, &bio->bi_flags); > return true; > + } > > return false; > } > @@ -1809,26 +1822,6 @@ void bio_endio(struct bio *bio, int error) > EXPORT_SYMBOL(bio_endio); > > /** > - * bio_endio_nodec - end I/O on a bio, without decrementing bi_remaining > - * @bio: bio > - * @error: error, if any > - * > - * For code that has saved and restored bi_end_io; thing hard before using this > - * function, probably you should've cloned the entire bio. > - **/ > -void bio_endio_nodec(struct bio *bio, int error) > -{ > - /* > - * If it's not flagged as a chain, we are not going to dec the count > - */ > - if (bio_flagged(bio, BIO_CHAIN)) > - bio_inc_remaining(bio); > - > - bio_endio(bio, error); > -} > -EXPORT_SYMBOL(bio_endio_nodec); > - > -/** > * bio_split - split a bio > * @bio: bio to split > * @sectors: number of sectors to split from the front of @bio > diff --git a/drivers/md/bcache/io.c b/drivers/md/bcache/io.c > index fa028fa..cb64e64a 100644 > --- a/drivers/md/bcache/io.c > +++ b/drivers/md/bcache/io.c > @@ -55,7 +55,7 @@ static void bch_bio_submit_split_done(struct closure *cl) > > s->bio->bi_end_io = s->bi_end_io; > s->bio->bi_private = s->bi_private; > - bio_endio_nodec(s->bio, 0); > + bio_endio(s->bio, 0); > > closure_debug_destroy(&s->cl); > mempool_free(s, s->p->bio_split_hook); > diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c > index 705eb7b..41b2594 100644 > --- a/drivers/md/dm-cache-target.c > +++ b/drivers/md/dm-cache-target.c > @@ -86,12 +86,6 @@ static void dm_unhook_bio(struct dm_hook_info *h, struct bio *bio) > { > bio->bi_end_io = h->bi_end_io; > bio->bi_private = h->bi_private; > - > - /* > - * Must bump bi_remaining to allow bio to complete with > - * restored bi_end_io. > - */ > - bio_inc_remaining(bio); > } > > /*----------------------------------------------------------------*/ > diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c > index d6a1c09..743fa9b 100644 > --- a/drivers/md/dm-raid1.c > +++ b/drivers/md/dm-raid1.c > @@ -1254,8 +1254,6 @@ static int mirror_end_io(struct dm_target *ti, struct bio *bio, int error) > dm_bio_restore(bd, bio); > bio_record->details.bi_bdev = NULL; > > - bio_inc_remaining(bio); > - > queue_bio(ms, bio, rw); > return DM_ENDIO_INCOMPLETE; > } > diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c > index 8bfeae2..7c82d3c 100644 > --- a/drivers/md/dm-snap.c > +++ b/drivers/md/dm-snap.c > @@ -1478,7 +1478,6 @@ out: > if (full_bio) { > full_bio->bi_end_io = pe->full_bio_end_io; > full_bio->bi_private = pe->full_bio_private; > - bio_inc_remaining(full_bio); > } > increment_pending_exceptions_done_count(); > > diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c > index 342dbda..e852602c 100644 > --- a/drivers/md/dm-thin.c > +++ b/drivers/md/dm-thin.c > @@ -793,10 +793,9 @@ static void inc_remap_and_issue_cell(struct thin_c *tc, > > static void process_prepared_mapping_fail(struct dm_thin_new_mapping *m) > { > - if (m->bio) { > + if (m->bio) > m->bio->bi_end_io = m->saved_bi_end_io; > - bio_inc_remaining(m->bio); > - } > + > cell_error(m->tc->pool, m->cell); > list_del(&m->list); > mempool_free(m, m->tc->pool->mapping_pool); > @@ -810,10 +809,8 @@ static void process_prepared_mapping(struct dm_thin_new_mapping *m) > int r; > > bio = m->bio; > - if (bio) { > + if (bio) > bio->bi_end_io = m->saved_bi_end_io; > - bio_inc_remaining(bio); > - } > > if (m->err) { > cell_error(pool, m->cell); > diff --git a/drivers/md/dm-verity.c b/drivers/md/dm-verity.c > index 66616db..bb9c6a0 100644 > --- a/drivers/md/dm-verity.c > +++ b/drivers/md/dm-verity.c > @@ -459,7 +459,7 @@ static void verity_finish_io(struct dm_verity_io *io, int error) > bio->bi_end_io = io->orig_bi_end_io; > bio->bi_private = io->orig_bi_private; > > - bio_endio_nodec(bio, error); > + bio_endio(bio, error); > } > > static void verity_work(struct work_struct *w) > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 2ef9a4b..fbdbee4 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -1745,7 +1745,7 @@ static void end_workqueue_fn(struct btrfs_work *work) > bio->bi_private = end_io_wq->private; > bio->bi_end_io = end_io_wq->end_io; > kmem_cache_free(btrfs_end_io_wq_cache, end_io_wq); > - bio_endio_nodec(bio, error); > + bio_endio(bio, error); > } > > static int cleaner_kthread(void *arg) > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 8e8d1d1..dac77d4 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -5585,10 +5585,10 @@ int btrfs_rmap_block(struct btrfs_mapping_tree *map_tree, > > static inline void btrfs_end_bbio(struct btrfs_bio *bbio, struct bio *bio, int err) > { > - if (likely(bbio->flags & BTRFS_BIO_ORIG_BIO_SUBMITTED)) > - bio_endio_nodec(bio, err); > - else > - bio_endio(bio, err); > + bio->bi_private = bbio->private; > + bio->bi_end_io = bbio->end_io; > + bio_endio(bio, err); > + > btrfs_put_bbio(bbio); > } > > @@ -5632,8 +5632,6 @@ static void btrfs_end_bio(struct bio *bio, int err) > bio = bbio->orig_bio; > } > > - bio->bi_private = bbio->private; > - bio->bi_end_io = bbio->end_io; > btrfs_io_bio(bio)->mirror_num = bbio->mirror_num; > /* only send an error to the higher layers if it is > * beyond the tolerance of the btrfs bio > @@ -5815,8 +5813,6 @@ static void bbio_error(struct btrfs_bio *bbio, struct bio *bio, u64 logical) > /* Shoud be the original bio. */ > WARN_ON(bio != bbio->orig_bio); > > - bio->bi_private = bbio->private; > - bio->bi_end_io = bbio->end_io; > btrfs_io_bio(bio)->mirror_num = bbio->mirror_num; > bio->bi_iter.bi_sector = logical >> 9; > > @@ -5897,10 +5893,8 @@ int btrfs_map_bio(struct btrfs_root *root, int rw, struct bio *bio, > if (dev_nr < total_devs - 1) { > bio = btrfs_bio_clone(first_bio, GFP_NOFS); > BUG_ON(!bio); /* -ENOMEM */ > - } else { > + } else > bio = first_bio; > - bbio->flags |= BTRFS_BIO_ORIG_BIO_SUBMITTED; > - } > > submit_stripe_bio(root, bbio, bio, > bbio->stripes[dev_nr].physical, dev_nr, rw, > diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h > index ebc3133..cedae03 100644 > --- a/fs/btrfs/volumes.h > +++ b/fs/btrfs/volumes.h > @@ -292,8 +292,6 @@ struct btrfs_bio_stripe { > struct btrfs_bio; > typedef void (btrfs_bio_end_io_t) (struct btrfs_bio *bio, int err); > > -#define BTRFS_BIO_ORIG_BIO_SUBMITTED (1 << 0) > - > struct btrfs_bio { > atomic_t refs; > atomic_t stripes_pending; > diff --git a/include/linux/bio.h b/include/linux/bio.h > index 7486ea1..f0291cf 100644 > --- a/include/linux/bio.h > +++ b/include/linux/bio.h > @@ -427,7 +427,6 @@ static inline struct bio *bio_clone_kmalloc(struct bio *bio, gfp_t gfp_mask) > } > > extern void bio_endio(struct bio *, int); > -extern void bio_endio_nodec(struct bio *, int); > struct request_queue; > extern int bio_phys_segments(struct request_queue *, struct bio *); > > @@ -659,17 +658,6 @@ 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->bi_flags |= (1 << 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 > -- > 2.3.2 (Apple Git-55) > -- Jan Kara SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/