| Message ID | 20220420041053.7927-2-kch@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-blk: small cleanup | expand |
On Tue, Apr 19, 2022 at 09:10:50PM -0700, Chaitanya Kulkarni wrote: > The function virtblk_setup_cmd() calls > virtblk_setup_discard_write_zeroes() once we process the block layer > request operation setup in the switch. Even though it saves duplicate > call for REQ_OP_DISCARD and REQ_OP_WRITE_ZEROES it adds additional check > in the fast path that is redundent since we already have a switch case > for both REQ_OP_DISCARD and REQ_OP_WRITE_ZEROES. > > Move the call virtblk_setup_discard_write_zeroes() into switch case > label of REQ_OP_DISCARD and REQ_OP_WRITE_ZEROES and avoid duplicate > branch in the fast path. > > Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> > --- > drivers/block/virtio_blk.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) Is there data that shows the performance effect of moving the code out of the fast path? > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > index 6ccf15253dee..b77711e73422 100644 > --- a/drivers/block/virtio_blk.c > +++ b/drivers/block/virtio_blk.c > @@ -223,10 +223,14 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev, > break; > case REQ_OP_DISCARD: > type = VIRTIO_BLK_T_DISCARD; > + if (virtblk_setup_discard_write_zeroes(req, unmap)) unmap is never true here. The variable obscures what is going on: s/unmap/false/
On 4/20/22 07:54, Stefan Hajnoczi wrote: > On Tue, Apr 19, 2022 at 09:10:50PM -0700, Chaitanya Kulkarni wrote: >> The function virtblk_setup_cmd() calls >> virtblk_setup_discard_write_zeroes() once we process the block layer >> request operation setup in the switch. Even though it saves duplicate >> call for REQ_OP_DISCARD and REQ_OP_WRITE_ZEROES it adds additional check >> in the fast path that is redundent since we already have a switch case >> for both REQ_OP_DISCARD and REQ_OP_WRITE_ZEROES. >> >> Move the call virtblk_setup_discard_write_zeroes() into switch case >> label of REQ_OP_DISCARD and REQ_OP_WRITE_ZEROES and avoid duplicate >> branch in the fast path. >> >> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> >> --- >> drivers/block/virtio_blk.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) > > Is there data that shows the performance effect of moving the code out > of the fast path? > I don't have a data yet but trying to minimize fast path branches as I can when I was reading the code... >> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c >> index 6ccf15253dee..b77711e73422 100644 >> --- a/drivers/block/virtio_blk.c >> +++ b/drivers/block/virtio_blk.c >> @@ -223,10 +223,14 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev, >> break; >> case REQ_OP_DISCARD: >> type = VIRTIO_BLK_T_DISCARD; >> + if (virtblk_setup_discard_write_zeroes(req, unmap)) > > unmap is never true here. The variable obscures what is going on: > > s/unmap/false/ > yeah, I'll drop this patch from V2. -ck
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 6ccf15253dee..b77711e73422 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -223,10 +223,14 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev, break; case REQ_OP_DISCARD: type = VIRTIO_BLK_T_DISCARD; + if (virtblk_setup_discard_write_zeroes(req, unmap)) + return BLK_STS_RESOURCE; break; case REQ_OP_WRITE_ZEROES: type = VIRTIO_BLK_T_WRITE_ZEROES; unmap = !(req->cmd_flags & REQ_NOUNMAP); + if (virtblk_setup_discard_write_zeroes(req, unmap)) + return BLK_STS_RESOURCE; break; case REQ_OP_DRV_IN: type = VIRTIO_BLK_T_GET_ID; @@ -239,11 +243,6 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev, vbr->out_hdr.type = cpu_to_virtio32(vdev, type); vbr->out_hdr.ioprio = cpu_to_virtio32(vdev, req_get_ioprio(req)); - if (type == VIRTIO_BLK_T_DISCARD || type == VIRTIO_BLK_T_WRITE_ZEROES) { - if (virtblk_setup_discard_write_zeroes(req, unmap)) - return BLK_STS_RESOURCE; - } - return 0; }
The function virtblk_setup_cmd() calls virtblk_setup_discard_write_zeroes() once we process the block layer request operation setup in the switch. Even though it saves duplicate call for REQ_OP_DISCARD and REQ_OP_WRITE_ZEROES it adds additional check in the fast path that is redundent since we already have a switch case for both REQ_OP_DISCARD and REQ_OP_WRITE_ZEROES. Move the call virtblk_setup_discard_write_zeroes() into switch case label of REQ_OP_DISCARD and REQ_OP_WRITE_ZEROES and avoid duplicate branch in the fast path. Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> --- drivers/block/virtio_blk.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)