| Message ID | 20190516143314.81302-6-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: > it allows to report it in the error handler > > Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Reviewed-by: Alberto Garcia <berto@igalia.com> > --- > hw/scsi/scsi-disk.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) (Sorry for the late reply :-/) > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c > index e7e865ab3b..b43254103c 100644 > --- a/hw/scsi/scsi-disk.c > +++ b/hw/scsi/scsi-disk.c > @@ -1602,8 +1602,6 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret) > { > SCSIDiskReq *r = data->r; > SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev); > - uint64_t sector_num; > - uint32_t nb_sectors; > > assert(r->req.aiocb == NULL); > if (scsi_disk_req_check_error(r, ret, false)) { > @@ -1611,16 +1609,16 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret) > } > > if (data->count > 0) { > - sector_num = ldq_be_p(&data->inbuf[0]); > - nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL; > - if (!check_lba_range(s, sector_num, nb_sectors)) { > + r->sector = ldq_be_p(&data->inbuf[0]); > + r->sector_count = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL; > + if (!check_lba_range(s, r->sector, r->sector_count)) { > scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE)); > goto done; > } > > r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk, > - sector_num * s->qdev.blocksize, > - nb_sectors * s->qdev.blocksize, > + r->sector * s->qdev.blocksize, > + r->sector_count * s->qdev.blocksize, This looks to me like these are not necessarily in terms of 512-byte sectors. It doesn’t seem to make anything technically wrong, because patch 7 takes that into account. But it’s still weird if everything else in this file treats these fields as being in terms of 512 byte sectors (and they are actually defined this way in SCSIDiskReq). Max > scsi_unmap_complete, data); > data->count--; > data->inbuf += 16; >
On 12/8/2019 8:58 PM, Max Reitz wrote: > On 16.05.19 16:33, Anton Nefedov wrote: >> it allows to report it in the error handler >> >> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Reviewed-by: Alberto Garcia <berto@igalia.com> >> --- >> hw/scsi/scsi-disk.c | 12 +++++------- >> 1 file changed, 5 insertions(+), 7 deletions(-) > > (Sorry for the late reply :-/) > >> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c >> index e7e865ab3b..b43254103c 100644 >> --- a/hw/scsi/scsi-disk.c >> +++ b/hw/scsi/scsi-disk.c >> @@ -1602,8 +1602,6 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret) >> { >> SCSIDiskReq *r = data->r; >> SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev); >> - uint64_t sector_num; >> - uint32_t nb_sectors; >> >> assert(r->req.aiocb == NULL); >> if (scsi_disk_req_check_error(r, ret, false)) { >> @@ -1611,16 +1609,16 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret) >> } >> >> if (data->count > 0) { >> - sector_num = ldq_be_p(&data->inbuf[0]); >> - nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL; >> - if (!check_lba_range(s, sector_num, nb_sectors)) { >> + r->sector = ldq_be_p(&data->inbuf[0]); >> + r->sector_count = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL; >> + if (!check_lba_range(s, r->sector, r->sector_count)) { >> scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE)); >> goto done; >> } >> >> r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk, >> - sector_num * s->qdev.blocksize, >> - nb_sectors * s->qdev.blocksize, >> + r->sector * s->qdev.blocksize, >> + r->sector_count * s->qdev.blocksize, > > This looks to me like these are not necessarily in terms of 512-byte > sectors. It doesn’t seem to make anything technically wrong, because > patch 7 takes that into account. > > But it’s still weird if everything else in this file treats these fields > as being in terms of 512 byte sectors (and they are actually defined > this way in SCSIDiskReq). > Nice that you caught this, thanks! I guess variable names misled me
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index e7e865ab3b..b43254103c 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -1602,8 +1602,6 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret) { SCSIDiskReq *r = data->r; SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev); - uint64_t sector_num; - uint32_t nb_sectors; assert(r->req.aiocb == NULL); if (scsi_disk_req_check_error(r, ret, false)) { @@ -1611,16 +1609,16 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret) } if (data->count > 0) { - sector_num = ldq_be_p(&data->inbuf[0]); - nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL; - if (!check_lba_range(s, sector_num, nb_sectors)) { + r->sector = ldq_be_p(&data->inbuf[0]); + r->sector_count = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL; + if (!check_lba_range(s, r->sector, r->sector_count)) { scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE)); goto done; } r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk, - sector_num * s->qdev.blocksize, - nb_sectors * s->qdev.blocksize, + r->sector * s->qdev.blocksize, + r->sector_count * s->qdev.blocksize, scsi_unmap_complete, data); data->count--; data->inbuf += 16;