| Message ID | 20190516143314.81302-5-anton.nefedov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | discard blockstats | expand |
On 16.05.19 16:33, Anton Nefedov wrote: > Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > hw/ide/core.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/hw/ide/core.c b/hw/ide/core.c > index 6afadf894f..3a7ac93777 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -441,6 +441,14 @@ static void ide_issue_trim_cb(void *opaque, int ret) > TrimAIOCB *iocb = opaque; > IDEState *s = iocb->s; > > + if (iocb->i >= 0) { > + if (ret >= 0) { > + block_acct_done(blk_get_stats(s->blk), &s->acct); > + } else { > + block_acct_failed(blk_get_stats(s->blk), &s->acct); Hmm, in other places (ide_handle_rw_error() here or scsi_handle_rw_error() in scsi-disk) only report this with BLOCK_ERROR_ACTION_REPORT. So I’m wondering whether the same should be done here. Max > + } > + } > + > if (ret >= 0) { > while (iocb->j < iocb->qiov->niov) { > int j = iocb->j; > @@ -458,10 +466,14 @@ static void ide_issue_trim_cb(void *opaque, int ret) > } > > if (!ide_sect_range_ok(s, sector, count)) { > + block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_UNMAP); > iocb->ret = -EINVAL; > goto done; > } > > + block_acct_start(blk_get_stats(s->blk), &s->acct, > + count << BDRV_SECTOR_BITS, BLOCK_ACCT_UNMAP); > + > /* Got an entry! Submit and exit. */ > iocb->aiocb = blk_aio_pdiscard(s->blk, > sector << BDRV_SECTOR_BITS, >
On 12/8/2019 9:16 PM, Max Reitz wrote: > On 16.05.19 16:33, Anton Nefedov wrote: >> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> hw/ide/core.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/hw/ide/core.c b/hw/ide/core.c >> index 6afadf894f..3a7ac93777 100644 >> --- a/hw/ide/core.c >> +++ b/hw/ide/core.c >> @@ -441,6 +441,14 @@ static void ide_issue_trim_cb(void *opaque, int ret) >> TrimAIOCB *iocb = opaque; >> IDEState *s = iocb->s; >> >> + if (iocb->i >= 0) { >> + if (ret >= 0) { >> + block_acct_done(blk_get_stats(s->blk), &s->acct); >> + } else { >> + block_acct_failed(blk_get_stats(s->blk), &s->acct); > > Hmm, in other places (ide_handle_rw_error() here or > scsi_handle_rw_error() in scsi-disk) only report this with > BLOCK_ERROR_ACTION_REPORT. > > So I’m wondering whether the same should be done here. > Many other places do not check the action: scsi_{dma|read|write}_complete(), hw/ide/atapi.c calls That feels somewhat weird to me, to account the operation complete when it's not. (But I don't really know the use cases of BLOCK_ERROR_ACTION_IGNORE). Can it be that the error action is only checked so that the operation is not accounted twice (as there might be block_acct_done() later in the common path with ACTION_IGNORE) /Anton
diff --git a/hw/ide/core.c b/hw/ide/core.c index 6afadf894f..3a7ac93777 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -441,6 +441,14 @@ static void ide_issue_trim_cb(void *opaque, int ret) TrimAIOCB *iocb = opaque; IDEState *s = iocb->s; + if (iocb->i >= 0) { + if (ret >= 0) { + block_acct_done(blk_get_stats(s->blk), &s->acct); + } else { + block_acct_failed(blk_get_stats(s->blk), &s->acct); + } + } + if (ret >= 0) { while (iocb->j < iocb->qiov->niov) { int j = iocb->j; @@ -458,10 +466,14 @@ static void ide_issue_trim_cb(void *opaque, int ret) } if (!ide_sect_range_ok(s, sector, count)) { + block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_UNMAP); iocb->ret = -EINVAL; goto done; } + block_acct_start(blk_get_stats(s->blk), &s->acct, + count << BDRV_SECTOR_BITS, BLOCK_ACCT_UNMAP); + /* Got an entry! Submit and exit. */ iocb->aiocb = blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS,