| Message ID | 1520010495-58172-1-git-send-email-anton.nefedov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | ide: fix invalid TRIM range abortion for macio | expand |
On 02/03/18 17:08, Anton Nefedov wrote: > commit 947858b0 "ide: abort TRIM operation for invalid range" > is incorrect for macio; just ide_dma_error() without doing a callback > is not enough for that errorpath. > > Instead, pass -EINVAL to the callback and handle it there > (see related motivation for read/write in 58ac32113). > > It will however catch possible EINVAL from the block layer too. > > Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> > --- > hw/ide/core.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/hw/ide/core.c b/hw/ide/core.c > index 257b429..54510d4 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -402,7 +402,6 @@ typedef struct TrimAIOCB { > QEMUIOVector *qiov; > BlockAIOCB *aiocb; > int i, j; > - bool is_invalid; > } TrimAIOCB; > > static void trim_aio_cancel(BlockAIOCB *acb) > @@ -430,11 +429,8 @@ static void ide_trim_bh_cb(void *opaque) > { > TrimAIOCB *iocb = opaque; > > - if (iocb->is_invalid) { > - ide_dma_error(iocb->s); > - } else { > - iocb->common.cb(iocb->common.opaque, iocb->ret); > - } > + iocb->common.cb(iocb->common.opaque, iocb->ret); > + > qemu_bh_delete(iocb->bh); > iocb->bh = NULL; > qemu_aio_unref(iocb); > @@ -462,7 +458,7 @@ static void ide_issue_trim_cb(void *opaque, int ret) > } > > if (!ide_sect_range_ok(s, sector, count)) { > - iocb->is_invalid = true; > + iocb->ret = -EINVAL; > goto done; > } > > @@ -502,7 +498,6 @@ BlockAIOCB *ide_issue_trim( > iocb->qiov = qiov; > iocb->i = -1; > iocb->j = 0; > - iocb->is_invalid = false; > ide_issue_trim_cb(iocb, 0); > return &iocb->common; > } > @@ -848,6 +843,12 @@ static void ide_dma_cb(void *opaque, int ret) > if (ret == -ECANCELED) { > return; > } > + > + if (ret == -EINVAL) { > + ide_dma_error(s); > + return; > + } > + > if (ret < 0) { > if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) { > s->bus->dma->aiocb = NULL; > Hi Anton, Thanks for this. I applied this patch to git master with my macio patch at https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg06076.html and it allowed to continue all the way through the Debian installer for the PPC g3beige machine so I believe it is working. Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> ATB, Mark.
On 05/03/18 21:54, Mark Cave-Ayland wrote: > On 02/03/18 17:08, Anton Nefedov wrote: > >> commit 947858b0 "ide: abort TRIM operation for invalid range" >> is incorrect for macio; just ide_dma_error() without doing a callback >> is not enough for that errorpath. >> >> Instead, pass -EINVAL to the callback and handle it there >> (see related motivation for read/write in 58ac32113). >> >> It will however catch possible EINVAL from the block layer too. >> >> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> >> --- >> hw/ide/core.c | 17 +++++++++-------- >> 1 file changed, 9 insertions(+), 8 deletions(-) >> >> diff --git a/hw/ide/core.c b/hw/ide/core.c >> index 257b429..54510d4 100644 >> --- a/hw/ide/core.c >> +++ b/hw/ide/core.c >> @@ -402,7 +402,6 @@ typedef struct TrimAIOCB { >> QEMUIOVector *qiov; >> BlockAIOCB *aiocb; >> int i, j; >> - bool is_invalid; >> } TrimAIOCB; >> static void trim_aio_cancel(BlockAIOCB *acb) >> @@ -430,11 +429,8 @@ static void ide_trim_bh_cb(void *opaque) >> { >> TrimAIOCB *iocb = opaque; >> - if (iocb->is_invalid) { >> - ide_dma_error(iocb->s); >> - } else { >> - iocb->common.cb(iocb->common.opaque, iocb->ret); >> - } >> + iocb->common.cb(iocb->common.opaque, iocb->ret); >> + >> qemu_bh_delete(iocb->bh); >> iocb->bh = NULL; >> qemu_aio_unref(iocb); >> @@ -462,7 +458,7 @@ static void ide_issue_trim_cb(void *opaque, int ret) >> } >> if (!ide_sect_range_ok(s, sector, count)) { >> - iocb->is_invalid = true; >> + iocb->ret = -EINVAL; >> goto done; >> } >> @@ -502,7 +498,6 @@ BlockAIOCB *ide_issue_trim( >> iocb->qiov = qiov; >> iocb->i = -1; >> iocb->j = 0; >> - iocb->is_invalid = false; >> ide_issue_trim_cb(iocb, 0); >> return &iocb->common; >> } >> @@ -848,6 +843,12 @@ static void ide_dma_cb(void *opaque, int ret) >> if (ret == -ECANCELED) { >> return; >> } >> + >> + if (ret == -EINVAL) { >> + ide_dma_error(s); >> + return; >> + } >> + >> if (ret < 0) { >> if (ide_handle_rw_error(s, -ret, >> ide_dma_cmd_to_retry(s->dma_cmd))) { >> s->bus->dma->aiocb = NULL; >> > > Hi Anton, > > Thanks for this. I applied this patch to git master with my macio patch > at https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg06076.html > and it allowed to continue all the way through the Debian installer for > the PPC g3beige machine so I believe it is working. > > Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Hi John, I know that you're busy, but just wondering if you have managed to review these 2 TRIM patches? They are a candidate for 2.12 since they prevent a qemu-system-ppc segfault on Mac machines when issuing IDE TRIM commands. ATB, Mark.
On 03/16/2018 07:20 AM, Mark Cave-Ayland wrote: > On 05/03/18 21:54, Mark Cave-Ayland wrote: > >> On 02/03/18 17:08, Anton Nefedov wrote: >> >>> commit 947858b0 "ide: abort TRIM operation for invalid range" >>> is incorrect for macio; just ide_dma_error() without doing a callback >>> is not enough for that errorpath. >>> >>> Instead, pass -EINVAL to the callback and handle it there >>> (see related motivation for read/write in 58ac32113). >>> >>> It will however catch possible EINVAL from the block layer too. >>> >>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> >>> --- >>> hw/ide/core.c | 17 +++++++++-------- >>> 1 file changed, 9 insertions(+), 8 deletions(-) >>> >>> diff --git a/hw/ide/core.c b/hw/ide/core.c >>> index 257b429..54510d4 100644 >>> --- a/hw/ide/core.c >>> +++ b/hw/ide/core.c >>> @@ -402,7 +402,6 @@ typedef struct TrimAIOCB { >>> QEMUIOVector *qiov; >>> BlockAIOCB *aiocb; >>> int i, j; >>> - bool is_invalid; >>> } TrimAIOCB; >>> static void trim_aio_cancel(BlockAIOCB *acb) >>> @@ -430,11 +429,8 @@ static void ide_trim_bh_cb(void *opaque) >>> { >>> TrimAIOCB *iocb = opaque; >>> - if (iocb->is_invalid) { >>> - ide_dma_error(iocb->s); >>> - } else { >>> - iocb->common.cb(iocb->common.opaque, iocb->ret); >>> - } >>> + iocb->common.cb(iocb->common.opaque, iocb->ret); >>> + >>> qemu_bh_delete(iocb->bh); >>> iocb->bh = NULL; >>> qemu_aio_unref(iocb); >>> @@ -462,7 +458,7 @@ static void ide_issue_trim_cb(void *opaque, int ret) >>> } >>> if (!ide_sect_range_ok(s, sector, count)) { >>> - iocb->is_invalid = true; >>> + iocb->ret = -EINVAL; >>> goto done; >>> } >>> @@ -502,7 +498,6 @@ BlockAIOCB *ide_issue_trim( >>> iocb->qiov = qiov; >>> iocb->i = -1; >>> iocb->j = 0; >>> - iocb->is_invalid = false; >>> ide_issue_trim_cb(iocb, 0); >>> return &iocb->common; >>> } >>> @@ -848,6 +843,12 @@ static void ide_dma_cb(void *opaque, int ret) >>> if (ret == -ECANCELED) { >>> return; >>> } >>> + >>> + if (ret == -EINVAL) { >>> + ide_dma_error(s); >>> + return; >>> + } >>> + >>> if (ret < 0) { >>> if (ide_handle_rw_error(s, -ret, >>> ide_dma_cmd_to_retry(s->dma_cmd))) { >>> s->bus->dma->aiocb = NULL; >>> >> >> Hi Anton, >> >> Thanks for this. I applied this patch to git master with my macio >> patch at >> https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg06076.html >> and it allowed to continue all the way through the Debian installer >> for the PPC g3beige machine so I believe it is working. >> >> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > Hi John, > > I know that you're busy, but just wondering if you have managed to > review these 2 TRIM patches? They are a candidate for 2.12 since they > prevent a qemu-system-ppc segfault on Mac machines when issuing IDE TRIM > commands. > > > ATB, > > Mark. Will send a PR once it looks as if Peter Maydell has caught up with all of the last-minute PR panic for soft freeze -- they're not forgotten, I promise! Thank you for checking in on me. --John
On 03/02/2018 12:08 PM, Anton Nefedov wrote: > commit 947858b0 "ide: abort TRIM operation for invalid range" > is incorrect for macio; just ide_dma_error() without doing a callback > is not enough for that errorpath. > > Instead, pass -EINVAL to the callback and handle it there > (see related motivation for read/write in 58ac32113). > > It will however catch possible EINVAL from the block layer too. > > Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> > --- > hw/ide/core.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/hw/ide/core.c b/hw/ide/core.c > index 257b429..54510d4 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -402,7 +402,6 @@ typedef struct TrimAIOCB { > QEMUIOVector *qiov; > BlockAIOCB *aiocb; > int i, j; > - bool is_invalid; > } TrimAIOCB; > > static void trim_aio_cancel(BlockAIOCB *acb) > @@ -430,11 +429,8 @@ static void ide_trim_bh_cb(void *opaque) > { > TrimAIOCB *iocb = opaque; > > - if (iocb->is_invalid) { > - ide_dma_error(iocb->s); > - } else { > - iocb->common.cb(iocb->common.opaque, iocb->ret); > - } > + iocb->common.cb(iocb->common.opaque, iocb->ret); > + > qemu_bh_delete(iocb->bh); > iocb->bh = NULL; > qemu_aio_unref(iocb); > @@ -462,7 +458,7 @@ static void ide_issue_trim_cb(void *opaque, int ret) > } > > if (!ide_sect_range_ok(s, sector, count)) { > - iocb->is_invalid = true; > + iocb->ret = -EINVAL; > goto done; > } > > @@ -502,7 +498,6 @@ BlockAIOCB *ide_issue_trim( > iocb->qiov = qiov; > iocb->i = -1; > iocb->j = 0; > - iocb->is_invalid = false; > ide_issue_trim_cb(iocb, 0); > return &iocb->common; > } > @@ -848,6 +843,12 @@ static void ide_dma_cb(void *opaque, int ret) > if (ret == -ECANCELED) { > return; > } > + > + if (ret == -EINVAL) { > + ide_dma_error(s); > + return; > + } > + > if (ret < 0) { > if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) { > s->bus->dma->aiocb = NULL; > Thanks, applied to my IDE tree: https://github.com/jnsnow/qemu/commits/ide https://github.com/jnsnow/qemu.git --js
diff --git a/hw/ide/core.c b/hw/ide/core.c index 257b429..54510d4 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -402,7 +402,6 @@ typedef struct TrimAIOCB { QEMUIOVector *qiov; BlockAIOCB *aiocb; int i, j; - bool is_invalid; } TrimAIOCB; static void trim_aio_cancel(BlockAIOCB *acb) @@ -430,11 +429,8 @@ static void ide_trim_bh_cb(void *opaque) { TrimAIOCB *iocb = opaque; - if (iocb->is_invalid) { - ide_dma_error(iocb->s); - } else { - iocb->common.cb(iocb->common.opaque, iocb->ret); - } + iocb->common.cb(iocb->common.opaque, iocb->ret); + qemu_bh_delete(iocb->bh); iocb->bh = NULL; qemu_aio_unref(iocb); @@ -462,7 +458,7 @@ static void ide_issue_trim_cb(void *opaque, int ret) } if (!ide_sect_range_ok(s, sector, count)) { - iocb->is_invalid = true; + iocb->ret = -EINVAL; goto done; } @@ -502,7 +498,6 @@ BlockAIOCB *ide_issue_trim( iocb->qiov = qiov; iocb->i = -1; iocb->j = 0; - iocb->is_invalid = false; ide_issue_trim_cb(iocb, 0); return &iocb->common; } @@ -848,6 +843,12 @@ static void ide_dma_cb(void *opaque, int ret) if (ret == -ECANCELED) { return; } + + if (ret == -EINVAL) { + ide_dma_error(s); + return; + } + if (ret < 0) { if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) { s->bus->dma->aiocb = NULL;
commit 947858b0 "ide: abort TRIM operation for invalid range" is incorrect for macio; just ide_dma_error() without doing a callback is not enough for that errorpath. Instead, pass -EINVAL to the callback and handle it there (see related motivation for read/write in 58ac32113). It will however catch possible EINVAL from the block layer too. Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> --- hw/ide/core.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)