Commit 326e1dbb authored by Mike Snitzer's avatar Mike Snitzer Committed by Jens Axboe
Browse files

block: remove management of bi_remaining when restoring original bi_end_io 



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.

Also, the bio_inc_remaining() interface has been moved local to bio.c.

Fixes: c4cf5261 ("bio: skip atomic inc/dec of ->bi_remaining for non-chains")
Reviewed-by: default avatarChristoph Hellwig <hch@lst.de>
Reviewed-by: default avatarJan Kara <jack@suse.cz>
Signed-off-by: default avatarMike Snitzer <snitzer@redhat.com>
Signed-off-by: default avatarJens Axboe <axboe@fb.com>
parent b04a5636

block/bio-integrity.c

+2 −2
Original line number Diff line number Diff line
@@ -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;

	}

block/bio.c

+14 −21
Original line number Diff line number Diff line
@@ -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;

}
@@ -1808,26 +1821,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

drivers/md/bcache/io.c

+1 −1
Original line number Diff line number Diff line
@@ -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);

drivers/md/dm-cache-target.c

+0 −6
Original line number Diff line number Diff line
@@ -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);

}



/*----------------------------------------------------------------*/

drivers/md/dm-raid1.c

+0 −2
Original line number Diff line number Diff line
@@ -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;

		}

CRA Git | Maintained and supported by SUSTech CRA and CCSE