| Message ID | 20200826131910.1879079-1-liangpeng10@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/mirror: fix core when using iothreads | expand |
On 8/26/20 9:19 AM, Peng Liang wrote: > We found an issue when doing block-commit with iothreads, which tries to > dereference a NULL pointer. > I'm clearing out my patch backlog. I am a bit out of the loop on block issues at the moment, did this issue get addressed in a later patch that I am not seeing, or does it still need attention? --js > <main thread> |<IO thread> > | > mirror_start_job | > 1. bdrv_ref(mirror_top_bs); | > bdrv_drained_begin(bs); | > bdrv_append(mirror_top_bs, | > bs, &local_err); | > bdrv_drained_end(bs); | > | > 2. s = block_job_create(...); | > |bdrv_mirror_top_pwritev > | MirrorBDSOpaque *s = bs->opaque; > | bool copy_to_target; > | copy_to_target = s->job->ret >= 0 && > | s->job->copy_mode == > | MIRROR_COPY_MODE_WRITE_BLOCKING; > | (s->job is not NULL until 3!) > 3. bs_opaque->job = s; | > > Just moving step 2 & 3 before 1 can avoid this. > > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: Peng Liang <liangpeng10@huawei.com> > --- > block/mirror.c | 21 ++++++++++----------- > 1 file changed, 10 insertions(+), 11 deletions(-) > > diff --git a/block/mirror.c b/block/mirror.c > index e8e8844afc40..7c872be71149 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -1600,6 +1600,16 @@ static BlockJob *mirror_start_job( > mirror_top_bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED | > BDRV_REQ_NO_FALLBACK; > bs_opaque = g_new0(MirrorBDSOpaque, 1); > + /* Make sure that the source is not resized while the job is running */ > + s = block_job_create(job_id, driver, NULL, bs, > + BLK_PERM_CONSISTENT_READ, > + BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED | > + BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed, > + creation_flags, cb, opaque, errp); > + if (!s) { > + goto fail; > + } > + bs_opaque->job = s; > mirror_top_bs->opaque = bs_opaque; > > /* bdrv_append takes ownership of the mirror_top_bs reference, need to keep > @@ -1612,19 +1622,8 @@ static BlockJob *mirror_start_job( > if (local_err) { > bdrv_unref(mirror_top_bs); > error_propagate(errp, local_err); > - return NULL; > - } > - > - /* Make sure that the source is not resized while the job is running */ > - s = block_job_create(job_id, driver, NULL, mirror_top_bs, > - BLK_PERM_CONSISTENT_READ, > - BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED | > - BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed, > - creation_flags, cb, opaque, errp); > - if (!s) { > goto fail; > } > - bs_opaque->job = s; > > /* The block job now has a reference to this node */ > bdrv_unref(mirror_top_bs); >
On 9/23/2020 10:50 PM, John Snow wrote: > On 8/26/20 9:19 AM, Peng Liang wrote: >> We found an issue when doing block-commit with iothreads, which tries to >> dereference a NULL pointer. >> > > I'm clearing out my patch backlog. I am a bit out of the loop on block > issues at the moment, did this issue get addressed in a later patch that > I am not seeing, or does it still need attention? > > --js > Hi John, I'm very sorry for missing your reply. Michael encountered the problem too and gave a reproducer script. Thanks, Peng
diff --git a/block/mirror.c b/block/mirror.c index e8e8844afc40..7c872be71149 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1600,6 +1600,16 @@ static BlockJob *mirror_start_job( mirror_top_bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED | BDRV_REQ_NO_FALLBACK; bs_opaque = g_new0(MirrorBDSOpaque, 1); + /* Make sure that the source is not resized while the job is running */ + s = block_job_create(job_id, driver, NULL, bs, + BLK_PERM_CONSISTENT_READ, + BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED | + BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed, + creation_flags, cb, opaque, errp); + if (!s) { + goto fail; + } + bs_opaque->job = s; mirror_top_bs->opaque = bs_opaque; /* bdrv_append takes ownership of the mirror_top_bs reference, need to keep @@ -1612,19 +1622,8 @@ static BlockJob *mirror_start_job( if (local_err) { bdrv_unref(mirror_top_bs); error_propagate(errp, local_err); - return NULL; - } - - /* Make sure that the source is not resized while the job is running */ - s = block_job_create(job_id, driver, NULL, mirror_top_bs, - BLK_PERM_CONSISTENT_READ, - BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED | - BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed, - creation_flags, cb, opaque, errp); - if (!s) { goto fail; } - bs_opaque->job = s; /* The block job now has a reference to this node */ bdrv_unref(mirror_top_bs);
We found an issue when doing block-commit with iothreads, which tries to dereference a NULL pointer. <main thread> |<IO thread> | mirror_start_job | 1. bdrv_ref(mirror_top_bs); | bdrv_drained_begin(bs); | bdrv_append(mirror_top_bs, | bs, &local_err); | bdrv_drained_end(bs); | | 2. s = block_job_create(...); | |bdrv_mirror_top_pwritev | MirrorBDSOpaque *s = bs->opaque; | bool copy_to_target; | copy_to_target = s->job->ret >= 0 && | s->job->copy_mode == | MIRROR_COPY_MODE_WRITE_BLOCKING; | (s->job is not NULL until 3!) 3. bs_opaque->job = s; | Just moving step 2 & 3 before 1 can avoid this. Reported-by: Euler Robot <euler.robot@huawei.com> Signed-off-by: Peng Liang <liangpeng10@huawei.com> --- block/mirror.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)