On Thu, 2012-11-08 at 21:06 +0000, Prantis, Kelsey wrote: > 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? > Either one should work fine for your case. The latter one just drops the extra iblock_submit_bios() call to avoid accounting issues during a possible iblock_get_bio() allocation failure. Either one should work fine, as AFAICT from the logs your not getting anywhere close to triggering the rolling iblock_submit_bios() call, or returning non-zero from iblock_execute_rw(). --nab > 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
