On Thu, 2012-11-15 at 05:49 -0500, Christoph Hellwig wrote: > On Thu, Nov 08, 2012 at 07:58:59PM +0000, Nicholas A. Bellinger wrote: > > From: Nicholas Bellinger <[email protected]> > > > > This patch fixes a double completion bug where ibr->pending = 2 usage > > plus the extra callback to iblock_complete_cmd() invoked after bio > > submission in iblock_execute_rw() can interfere with the normal > > bio->bi_done() -> iblock_bio_done() -> iblock_complete_cmd() completion > > path, causing a double target_complete_cmd() call to occur. > > How so exactly? Please provide an explanation, and preferably a test > case. >
So, I was getting a bit confused here while debugging Kelsey's issue with dispatched multiple bios throwing an with a virtio-blk backend. However, setting the default ibr->pending=2 value before dispatch, and making a extra iblock_complete_cmd() call from iblock_execute_rw(), while doing the normal iblock_complete_cmd() calls from iblock_bio_done() is AFAICT a pointless extra atomic_dec_and_test() call per I/O. Was there a reason why you changed ->pending from 1 -> 2 during the se_task removal in commit 5787cacd0bd5ee01..? > > Also drop the bio_cnt >= IBLOCK_MAX_BIO_PER_TASK rolling call to > > iblock_submit_bios() to avoid the exception case where outstanding > > bios completing via iblock_bio_done() are not accounted for when > > returning returning non zero from iblock_execute_rw(). > > This will cause deadlocks when we can't allocate enough bios for > a too large request. > Mmmm.. So the case this patch tries to avoid is where iblock_submit_bio() is called multiple times before the final ibr->pending value is set, this could potentially cause bios completion calls to decrement the value + complete to core before iblock_execute_rw() is done incrementing ibr->pending. How about returning an exception here instead when IBLOCK_MAX_BIOS in reached..? Btw, where did the default of 32 for this come from..? --nab -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
