Re: [PATCH for-4.2 01/14] block: remove management of bi_remaining when restoring original bi_end_io

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu 14-05-15 17:04:59, 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.
> 
> The bio_inc_remaining() interface has been left exported because it is
> still useful for more elaborate uses of bio_chain() -- it will be used
> in an upcoming DM commit "dm thin: range discard support".
> 
> Fixes: c4cf5261 ("bio: skip atomic inc/dec of ->bi_remaining for non-chains")
> Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> Cc: Jan Kara <jack@xxxxxxx>
> Cc: Chris Mason <clm@xxxxxx>
  One question: What happens if you stack dm-thin on top of e.g. dm-linear?
dm-thin will do it's thing to a bio and passes it to dm-linear. That will
split & chain the bio so BIO_CHAIN will be set. And on IO completion you
will have troubles in dm-thinp as now bi_remaining gets decremented in
bio_endio(). That's the reason why I suggested that we should clear
BIO_CHAIN once bi_remaining hits zero...

								Honza

> ---
>  block/bio-integrity.c        |  4 ++--
>  block/bio.c                  | 20 --------------------
>  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          |  1 -
>  12 files changed, 13 insertions(+), 54 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..3690f1c 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1809,26 +1809,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..fe0a88d 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 *);
>  
> -- 
> 2.3.2 (Apple Git-55)
> 
-- 
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR

--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel




[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux