[PATCH] target/iblock: Fix double iblock_complete_cmd callback bug

[PATCH] target/iblock: Fix double iblock_complete_cmd callback bug

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

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.

This bug was introduced during v3.4-rc2 code with:

commit 5787cacd0bd5ee016ad807b244550d34fe2beebe
Author: Christoph Hellwig <hch@infradead.org>
Date:   Tue Apr 24 00:25:06 2012 -0400

    target: remove struct se_task

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 bug was originally reported by Kelsey when a single se_cmd
descriptor required more than one bio to complete.

Reported-by: Kelsey Prantis <kelsey.prantis@intel.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: stable@vger.kernel.org
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_iblock.c |   11 +----------
 1 files changed, 1 insertions(+), 10 deletions(-)

diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 57d7674..d066932 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -47,7 +47,6 @@
 
 #include "target_core_iblock.h"
 
-#define IBLOCK_MAX_BIO_PER_TASK	 32	/* max # of bios to submit at a time */
 #define IBLOCK_BIO_POOL_SIZE	128
 
 static struct se_subsystem_api iblock_template;
@@ -602,7 +601,6 @@ static int iblock_execute_rw(struct se_cmd *cmd)
 	struct scatterlist *sg;
 	u32 sg_num = sgl_nents;
 	sector_t block_lba;
-	unsigned bio_cnt;
 	int rw;
 	int i;
 
@@ -658,8 +656,7 @@ static int iblock_execute_rw(struct se_cmd *cmd)
 	bio_list_init(&list);
 	bio_list_add(&list, bio);
 
-	atomic_set(&ibr->pending, 2);
-	bio_cnt = 1;
+	atomic_set(&ibr->pending, 1);
 
 	for_each_sg(sgl, sg, sgl_nents, i) {
 		/*
@@ -669,10 +666,6 @@ static int iblock_execute_rw(struct se_cmd *cmd)
 		 */
 		while (bio_add_page(bio, sg_page(sg), sg->length, sg->offset)
 				!= sg->length) {
-			if (bio_cnt >= IBLOCK_MAX_BIO_PER_TASK) {
-				iblock_submit_bios(&list, rw);
-				bio_cnt = 0;
-			}
 
 			bio = iblock_get_bio(cmd, block_lba, sg_num);
 			if (!bio)
@@ -680,7 +673,6 @@ static int iblock_execute_rw(struct se_cmd *cmd)
 
 			atomic_inc(&ibr->pending);
 			bio_list_add(&list, bio);
-			bio_cnt++;
 		}
 
 		/* Always in 512 byte units for Linux/Block */
@@ -689,7 +681,6 @@ static int iblock_execute_rw(struct se_cmd *cmd)
 	}
 
 	iblock_submit_bios(&list, rw);
-	iblock_complete_cmd(cmd);
 	return 0;
 
 fail_put_bios:
-- 
1.7.2.5

Re: [PATCH] target/iblock: Fix double iblock_complete_cmd callback bug

[Christoph Hellwig]

On Thu, Nov 08, 2012 at 07:58:59PM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> 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.

> 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.

Re: [PATCH] target/iblock: Fix double iblock_complete_cmd callback bug

[Nicholas A. Bellinger]

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 <nab@linux-iscsi.org>
> > 
> > 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

Re: [PATCH] target/iblock: Fix double iblock_complete_cmd callback bug

[Nicholas A. Bellinger]

On Thu, 2012-11-15 at 14:36 -0800, Nicholas A. Bellinger wrote:
> 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 <nab@linux-iscsi.org>
> > > 
> > > 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.
> 

Err, nevermind..  Dropping this patch as v3.7 material for the moment
while debugging the Kelsey's multiple-bio IBLOCK+virtio-blk bug..

> How about returning an exception here instead when IBLOCK_MAX_BIOS in
> reached..?  Btw, where did the default of 32 for this come from..?
> 

Still curious where this value comes from..?  :)

--nab

Re: [PATCH] target/iblock: Fix double iblock_complete_cmd callback bug

[Christoph Hellwig]

On Thu, Nov 15, 2012 at 02:36:30PM -0800, Nicholas A. Bellinger wrote:
> 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..?

It is to avoid having the request completed before even submitting all
bios.  As soon as one is submitted it could on a very fast device
complete before we've incremented the count for the next bio.

It's a very common scheme used all over the kernel when submitting
I/O in smaller subdivisions.

> 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.

That's exacltly what we try to avoid here.

> How about returning an exception here instead when IBLOCK_MAX_BIOS in
> reached..?

Why?  As soon as we kicked the first batch off we're guaranteed to make
progress allocating more as sson as the first of the submitted ones
completes.

> Btw, where did the default of 32 for this come from..?

It's a random number, with the important property that it's considerably
smaller than IBLOCK_BIO_POOL_SIZE.