[PATCH 0/4] virtio-blk: small cleanup

[PATCH 0/4] virtio-blk: small cleanup

[Chaitanya Kulkarni]

Hi,

This has some nit fixes and code cleanups along with removing
deprecated API fir ida_simple_XXX().

-ck

Chaitanya Kulkarni (4):
  virtio-blk: remove additional check in fast path
  virtio-blk: don't add a new line
  virtio-blk: avoid function call in the fast path
  virtio-blk: remove deprecated ida_simple_XXX()

 drivers/block/virtio_blk.c | 38 +++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 21 deletions(-)

-- 
2.29.0

[PATCH 1/4] virtio-blk: remove additional check in fast path

[Chaitanya Kulkarni]

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(-)

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;
 }
 
-- 
2.29.0

[PATCH 2/4] virtio-blk: don't add a new line

[Chaitanya Kulkarni]

Don't split sector assignment line for REQ_OP_READ and REQ_OP_WRITE in
the virtblk_setup_cmd() which fits in one line perfectly.

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/block/virtio_blk.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index b77711e73422..d038800474c2 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -210,13 +210,11 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev,
 	switch (req_op(req)) {
 	case REQ_OP_READ:
 		type = VIRTIO_BLK_T_IN;
-		vbr->out_hdr.sector = cpu_to_virtio64(vdev,
-						      blk_rq_pos(req));
+		vbr->out_hdr.sector = cpu_to_virtio64(vdev, blk_rq_pos(req));
 		break;
 	case REQ_OP_WRITE:
 		type = VIRTIO_BLK_T_OUT;
-		vbr->out_hdr.sector = cpu_to_virtio64(vdev,
-						      blk_rq_pos(req));
+		vbr->out_hdr.sector = cpu_to_virtio64(vdev, blk_rq_pos(req));
 		break;
 	case REQ_OP_FLUSH:
 		type = VIRTIO_BLK_T_FLUSH;
-- 
2.29.0

[PATCH 3/4] virtio-blk: avoid function call in the fast path

[Chaitanya Kulkarni]

We can avoid a function call virtblk_map_data() in the fast path if
block layer request has no physical segments by moving the call
blk_rq_nr_phys_segments() from virtblk_map_data() to virtio_queue_rq().

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/block/virtio_blk.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index d038800474c2..74c3a48cd1e5 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -178,9 +178,6 @@ static int virtblk_map_data(struct blk_mq_hw_ctx *hctx, struct request *req,
 {
 	int err;
 
-	if (!blk_rq_nr_phys_segments(req))
-		return 0;
-
 	vbr->sg_table.sgl = vbr->sg;
 	err = sg_alloc_table_chained(&vbr->sg_table,
 				     blk_rq_nr_phys_segments(req),
@@ -303,7 +300,7 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
 	struct request *req = bd->rq;
 	struct virtblk_req *vbr = blk_mq_rq_to_pdu(req);
 	unsigned long flags;
-	int num;
+	int num = 0;
 	int qid = hctx->queue_num;
 	bool notify = false;
 	blk_status_t status;
@@ -315,10 +312,12 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
 
 	blk_mq_start_request(req);
 
-	num = virtblk_map_data(hctx, req, vbr);
-	if (unlikely(num < 0)) {
-		virtblk_cleanup_cmd(req);
-		return BLK_STS_RESOURCE;
+	if (blk_rq_nr_phys_segments(req)) {
+		num = virtblk_map_data(hctx, req, vbr);
+		if (unlikely(num < 0)) {
+			virtblk_cleanup_cmd(req);
+			return BLK_STS_RESOURCE;
+		}
 	}
 
 	spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
-- 
2.29.0

[PATCH 4/4] virtio-blk: remove deprecated ida_simple_XXX()

[Chaitanya Kulkarni]

Replace deprecated ida_simple_get() and ida_simple_remove() with
ida_alloc_max() and ida_free().

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/block/virtio_blk.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 74c3a48cd1e5..e05748337dd1 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -411,7 +411,7 @@ static void virtblk_free_disk(struct gendisk *disk)
 {
 	struct virtio_blk *vblk = disk->private_data;
 
-	ida_simple_remove(&vd_index_ida, vblk->index);
+	ida_free(&vd_index_ida, vblk->index);
 	mutex_destroy(&vblk->vdev_mutex);
 	kfree(vblk);
 }
@@ -720,8 +720,8 @@ static int virtblk_probe(struct virtio_device *vdev)
 		return -EINVAL;
 	}
 
-	err = ida_simple_get(&vd_index_ida, 0, minor_to_index(1 << MINORBITS),
-			     GFP_KERNEL);
+	err = ida_alloc_max(&vd_index_ida, minor_to_index(1 << MINORBITS),
+			    GFP_KERNEL);
 	if (err < 0)
 		goto out;
 	index = err;
@@ -911,7 +911,7 @@ static int virtblk_probe(struct virtio_device *vdev)
 out_free_vblk:
 	kfree(vblk);
 out_free_index:
-	ida_simple_remove(&vd_index_ida, index);
+	ida_free(&vd_index_ida, index);
 out:
 	return err;
 }
-- 
2.29.0

Re: [PATCH 1/4] virtio-blk: remove additional check in fast path

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]

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/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] virtio-blk: remove additional check in fast path

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 1386 bytes --]

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/

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/4] virtio-blk: don't add a new line

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

On Tue, Apr 19, 2022 at 09:10:51PM -0700, Chaitanya Kulkarni wrote:
> Don't split sector assignment line for REQ_OP_READ and REQ_OP_WRITE in
> the virtblk_setup_cmd() which fits in one line perfectly.
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>  drivers/block/virtio_blk.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

There is a cost to patches: humans spend time reviewing them, CI systems
consume energy running tests, downstream maintainers deal with
backports, and git-blame(1) becomes harder to use when code changes.

What constitutes code churn is subjective. For me personally I prefer it
when patches have a clear benefit that outweighs the costs.
Nevertheless:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/4] virtio-blk: don't add a new line

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 779 bytes --]

On Tue, Apr 19, 2022 at 09:10:51PM -0700, Chaitanya Kulkarni wrote:
> Don't split sector assignment line for REQ_OP_READ and REQ_OP_WRITE in
> the virtblk_setup_cmd() which fits in one line perfectly.
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>  drivers/block/virtio_blk.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

There is a cost to patches: humans spend time reviewing them, CI systems
consume energy running tests, downstream maintainers deal with
backports, and git-blame(1) becomes harder to use when code changes.

What constitutes code churn is subjective. For me personally I prefer it
when patches have a clear benefit that outweighs the costs.
Nevertheless:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/4] virtio-blk: avoid function call in the fast path

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2178 bytes --]

On Tue, Apr 19, 2022 at 09:10:52PM -0700, Chaitanya Kulkarni wrote:
> We can avoid a function call virtblk_map_data() in the fast path if
> block layer request has no physical segments by moving the call
> blk_rq_nr_phys_segments() from virtblk_map_data() to virtio_queue_rq().
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>  drivers/block/virtio_blk.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)

virtblk_map_data() is a static function that is not called by anything
else in virtio_blk.c. The disassembly on my x86_64 machine shows it has
been inlined into virtio_queue_rq(). The compiler has already done what
this patch is trying to do (and more).

Can you share performance data or some more background on why this
code change is necessary?

> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index d038800474c2..74c3a48cd1e5 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -178,9 +178,6 @@ static int virtblk_map_data(struct blk_mq_hw_ctx *hctx, struct request *req,
>  {
>  	int err;
>  
> -	if (!blk_rq_nr_phys_segments(req))
> -		return 0;
> -
>  	vbr->sg_table.sgl = vbr->sg;
>  	err = sg_alloc_table_chained(&vbr->sg_table,
>  				     blk_rq_nr_phys_segments(req),
> @@ -303,7 +300,7 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
>  	struct request *req = bd->rq;
>  	struct virtblk_req *vbr = blk_mq_rq_to_pdu(req);
>  	unsigned long flags;
> -	int num;
> +	int num = 0;
>  	int qid = hctx->queue_num;
>  	bool notify = false;
>  	blk_status_t status;
> @@ -315,10 +312,12 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
>  
>  	blk_mq_start_request(req);
>  
> -	num = virtblk_map_data(hctx, req, vbr);
> -	if (unlikely(num < 0)) {
> -		virtblk_cleanup_cmd(req);
> -		return BLK_STS_RESOURCE;
> +	if (blk_rq_nr_phys_segments(req)) {
> +		num = virtblk_map_data(hctx, req, vbr);
> +		if (unlikely(num < 0)) {
> +			virtblk_cleanup_cmd(req);
> +			return BLK_STS_RESOURCE;
> +		}
>  	}
>  
>  	spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
> -- 
> 2.29.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] virtio-blk: avoid function call in the fast path

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 2178 bytes --]

On Tue, Apr 19, 2022 at 09:10:52PM -0700, Chaitanya Kulkarni wrote:
> We can avoid a function call virtblk_map_data() in the fast path if
> block layer request has no physical segments by moving the call
> blk_rq_nr_phys_segments() from virtblk_map_data() to virtio_queue_rq().
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>  drivers/block/virtio_blk.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)

virtblk_map_data() is a static function that is not called by anything
else in virtio_blk.c. The disassembly on my x86_64 machine shows it has
been inlined into virtio_queue_rq(). The compiler has already done what
this patch is trying to do (and more).

Can you share performance data or some more background on why this
code change is necessary?

> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index d038800474c2..74c3a48cd1e5 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -178,9 +178,6 @@ static int virtblk_map_data(struct blk_mq_hw_ctx *hctx, struct request *req,
>  {
>  	int err;
>  
> -	if (!blk_rq_nr_phys_segments(req))
> -		return 0;
> -
>  	vbr->sg_table.sgl = vbr->sg;
>  	err = sg_alloc_table_chained(&vbr->sg_table,
>  				     blk_rq_nr_phys_segments(req),
> @@ -303,7 +300,7 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
>  	struct request *req = bd->rq;
>  	struct virtblk_req *vbr = blk_mq_rq_to_pdu(req);
>  	unsigned long flags;
> -	int num;
> +	int num = 0;
>  	int qid = hctx->queue_num;
>  	bool notify = false;
>  	blk_status_t status;
> @@ -315,10 +312,12 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
>  
>  	blk_mq_start_request(req);
>  
> -	num = virtblk_map_data(hctx, req, vbr);
> -	if (unlikely(num < 0)) {
> -		virtblk_cleanup_cmd(req);
> -		return BLK_STS_RESOURCE;
> +	if (blk_rq_nr_phys_segments(req)) {
> +		num = virtblk_map_data(hctx, req, vbr);
> +		if (unlikely(num < 0)) {
> +			virtblk_cleanup_cmd(req);
> +			return BLK_STS_RESOURCE;
> +		}
>  	}
>  
>  	spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
> -- 
> 2.29.0
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 4/4] virtio-blk: remove deprecated ida_simple_XXX()

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 387 bytes --]

On Tue, Apr 19, 2022 at 09:10:53PM -0700, Chaitanya Kulkarni wrote:
> Replace deprecated ida_simple_get() and ida_simple_remove() with
> ida_alloc_max() and ida_free().
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>  drivers/block/virtio_blk.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 4/4] virtio-blk: remove deprecated ida_simple_XXX()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 387 bytes --]

On Tue, Apr 19, 2022 at 09:10:53PM -0700, Chaitanya Kulkarni wrote:
> Replace deprecated ida_simple_get() and ida_simple_remove() with
> ida_alloc_max() and ida_free().
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>  drivers/block/virtio_blk.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] virtio-blk: remove additional check in fast path

[Chaitanya Kulkarni]

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

Re: [PATCH 2/4] virtio-blk: don't add a new line

[Chaitanya Kulkarni]

On 4/20/22 08:04, Stefan Hajnoczi wrote:
> On Tue, Apr 19, 2022 at 09:10:51PM -0700, Chaitanya Kulkarni wrote:
>> Don't split sector assignment line for REQ_OP_READ and REQ_OP_WRITE in
>> the virtblk_setup_cmd() which fits in one line perfectly.
>>
>> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
>> ---
>>   drivers/block/virtio_blk.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> There is a cost to patches: humans spend time reviewing them, CI systems
> consume energy running tests, downstream maintainers deal with
> backports, and git-blame(1) becomes harder to use when code changes.
> 
> What constitutes code churn is subjective. For me personally I prefer it
> when patches have a clear benefit that outweighs the costs.
> Nevertheless:
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 

maybe we can just drop it then, for naked eye it looked a bit odd
to add a new line..

-ck

Re: [PATCH 3/4] virtio-blk: avoid function call in the fast path

[Chaitanya Kulkarni]

On 4/20/22 08:16, Stefan Hajnoczi wrote:
> On Tue, Apr 19, 2022 at 09:10:52PM -0700, Chaitanya Kulkarni wrote:
>> We can avoid a function call virtblk_map_data() in the fast path if
>> block layer request has no physical segments by moving the call
>> blk_rq_nr_phys_segments() from virtblk_map_data() to virtio_queue_rq().
>>
>> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
>> ---
>>   drivers/block/virtio_blk.c | 15 +++++++--------
>>   1 file changed, 7 insertions(+), 8 deletions(-)
> 
> virtblk_map_data() is a static function that is not called by anything
> else in virtio_blk.c. The disassembly on my x86_64 machine shows it has
> been inlined into virtio_queue_rq(). The compiler has already done what
> this patch is trying to do (and more).
> 
> Can you share performance data or some more background on why this
> code change is necessary?
> 
>

I don't have performance numbers, but when reading the code it
takes to a function call especially in the fast path where
read/write requests has phys segments and those are probably
most frequent requests...

I'll drop it from V2, if I find performance numbers then I'll
repost is with the quantitative data ...

-ck

Re: [PATCH 0/4] virtio-blk: small cleanup

[Chaitanya Kulkarni]

On 4/19/22 21:10, Chaitanya Kulkarni wrote:
> Hi,
> 
> This has some nit fixes and code cleanups along with removing
> deprecated API fir ida_simple_XXX().
> 
> -ck
> 
> Chaitanya Kulkarni (4):
>    virtio-blk: remove additional check in fast path
>    virtio-blk: don't add a new line
>    virtio-blk: avoid function call in the fast path
>    virtio-blk: remove deprecated ida_simple_XXX()
> 
>   drivers/block/virtio_blk.c | 38 +++++++++++++++++---------------------
>   1 file changed, 17 insertions(+), 21 deletions(-)
> 

Thanks for the review, I'll send out a V2 and drop patches that
lacks the quantitative data..

-ck

Re: [PATCH 0/4] virtio-blk: small cleanup

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 775 bytes --]

On Thu, Apr 21, 2022 at 09:56:28PM +0000, Chaitanya Kulkarni wrote:
> On 4/19/22 21:10, Chaitanya Kulkarni wrote:
> > Hi,
> > 
> > This has some nit fixes and code cleanups along with removing
> > deprecated API fir ida_simple_XXX().
> > 
> > -ck
> > 
> > Chaitanya Kulkarni (4):
> >    virtio-blk: remove additional check in fast path
> >    virtio-blk: don't add a new line
> >    virtio-blk: avoid function call in the fast path
> >    virtio-blk: remove deprecated ida_simple_XXX()
> > 
> >   drivers/block/virtio_blk.c | 38 +++++++++++++++++---------------------
> >   1 file changed, 17 insertions(+), 21 deletions(-)
> > 
> 
> Thanks for the review, I'll send out a V2 and drop patches that
> lacks the quantitative data..

Thank you!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/4] virtio-blk: small cleanup

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 775 bytes --]

On Thu, Apr 21, 2022 at 09:56:28PM +0000, Chaitanya Kulkarni wrote:
> On 4/19/22 21:10, Chaitanya Kulkarni wrote:
> > Hi,
> > 
> > This has some nit fixes and code cleanups along with removing
> > deprecated API fir ida_simple_XXX().
> > 
> > -ck
> > 
> > Chaitanya Kulkarni (4):
> >    virtio-blk: remove additional check in fast path
> >    virtio-blk: don't add a new line
> >    virtio-blk: avoid function call in the fast path
> >    virtio-blk: remove deprecated ida_simple_XXX()
> > 
> >   drivers/block/virtio_blk.c | 38 +++++++++++++++++---------------------
> >   1 file changed, 17 insertions(+), 21 deletions(-)
> > 
> 
> Thanks for the review, I'll send out a V2 and drop patches that
> lacks the quantitative data..

Thank you!

Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization