Hi Nicholas, I notice that the patch you submitted for this issue ([PATCH] target/iblock: Fix double iblock_complete_cmd callback bug) is a bit different from the latest included in this thread. Should we change over to that patch, or stick with what you provided us in this thread? Kelsey On 11/8/12 11:31 AM, "Nicholas A. Bellinger" <nab@xxxxxxxxxxxxxxx> wrote: >On Thu, 2012-11-08 at 10:10 -0800, Nicholas A. Bellinger wrote: >> On Thu, 2012-11-08 at 14:25 +0000, Prantis, Kelsey wrote: >> > On 11/7/12 4:26 PM, "Prantis, Kelsey" <kelsey.prantis@xxxxxxxxx> >>wrote: >> > >>So I'll try to reproduce using some different combinations of >> > >>fabric_max_sectors, but for the moment please apply the following >>debug >> > >>patch to dump struct scatterlist + struct bio members and try to >> > >>reproduce. >> > > >> > >Am working on applying the patch now, and will get back to you when >>I have >> > >a result. >> > >> > There was quite a bit of output, which can be fetched from: >> > ftp://ftp.whamcloud.com/uploads/lio-debug.txt >> > >> >> Hi Kelsey, >> >> Thanks for the log. After an quick look, here's what I see so far: >> > ><SNIP> > >> So AFAICT this looks like a iblock bio completion callback is being >> invoked more than once, that is being triggered when using multiple BIOs >> to complete a single se_cmd descriptor. >> >> Looking at the code in question from target_core_iblock.c, I think I see >> what's going on now. There is a double completion happening between the >> iblock_complete_cmd call in iblock_execute_rw() and iblock_bio_done() >> that appears to have been introduced with: >> >> commit 5787cacd0bd5ee016ad807b244550d34fe2beebe >> Author: Christoph Hellwig <hch@xxxxxxxxxxxxx> >> Date: Tue Apr 24 00:25:06 2012 -0400 >> >> target: remove struct se_task >> >> I'm currently testing the following patch to address this bug. >> >> Please give it a shot with your setup. >> >> Thanks! >> >> --nab >> >> diff --git a/drivers/target/target_core_iblock.c >>b/drivers/target/target_core_iblock.c >> index 57d7674..305aa41 100644 >> --- a/drivers/target/target_core_iblock.c >> +++ b/drivers/target/target_core_iblock.c >> @@ -658,7 +658,7 @@ static int iblock_execute_rw(struct se_cmd *cmd) >> bio_list_init(&list); >> bio_list_add(&list, bio); >> >> - atomic_set(&ibr->pending, 2); >> + atomic_set(&ibr->pending, 1); >> bio_cnt = 1; >> >> for_each_sg(sgl, sg, sgl_nents, i) { >> @@ -689,7 +689,6 @@ static int iblock_execute_rw(struct se_cmd *cmd) >> } >> >> iblock_submit_bios(&list, rw); >> - iblock_complete_cmd(cmd); >> return 0; >> >> fail_put_bios: >> >> -- > >Ok, a slightly updated version of this patch to make sure that >ibr->pending in incremented ahead of the possible call to >iblock_submit_bios() when bio_cnt >= IBLOCK_MAX_BIO_PER_TASK. > >This is not happening in your particular case, but is still required >to ensure that iblock_bio_done() waits for all outstanding bios >to return before completing to target core. > >--nab > >diff --git a/drivers/target/target_core_iblock.c >b/drivers/target/target_core_iblock.c >index 57d7674..70fd5b5 100644 >--- a/drivers/target/target_core_iblock.c >+++ b/drivers/target/target_core_iblock.c >@@ -658,7 +658,7 @@ static int iblock_execute_rw(struct se_cmd *cmd) > bio_list_init(&list); > bio_list_add(&list, bio); > >- atomic_set(&ibr->pending, 2); >+ atomic_set(&ibr->pending, 1); > bio_cnt = 1; > > for_each_sg(sgl, sg, sgl_nents, i) { >@@ -669,16 +669,23 @@ static int iblock_execute_rw(struct se_cmd *cmd) > */ > while (bio_add_page(bio, sg_page(sg), sg->length, >sg->offset) > != sg->length) { >+ /* >+ * Increment pending now before possible call to >+ * iblock_submit_bios() -> async iblock_bio_done() >+ */ >+ atomic_inc(&ibr->pending); >+ > 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) >+ if (!bio) { >+ atomic_dec(&ibr->pending); > goto fail_put_bios; >+ } > >- atomic_inc(&ibr->pending); > bio_list_add(&list, bio); > bio_cnt++; > } >@@ -689,7 +696,6 @@ static int iblock_execute_rw(struct se_cmd *cmd) > } > > iblock_submit_bios(&list, rw); >- iblock_complete_cmd(cmd); > return 0; > > fail_put_bios: > > -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
