[PATCH] vhost-scsi: Fix log flooding with target does not exist errors

[PATCH] vhost-scsi: Fix log flooding with target does not exist errors

[Mike Christie]

As part of the normal initiator side scanning the guest's scsi layer
will loop over all possible targets and send an inquiry. Since the
max number of targets for virtio-scsi is 256, this results in 255
error messages about targets not existing. When there's more than 1
vhost-scsi device, then you get N * 255 log messages.

It looks like the log message was added by accident in:

commit 09d7583294aa ("vhost/scsi: Use common handling code in request
queue handler")

The patch was just making some coce common but added the vq_err call
and I'm guessing the patch author forgot to enable the vq_err call
(vq_err is implemented by pr_debug which defaults to off). So this
patch removes the call since it's expected to hit this path during
device discovery.

Fixes: 09d7583294aa ("vhost/scsi: Use common handling code in request queue handler")
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/vhost/scsi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index c12a0d4e6386..c9f418a4571a 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1226,10 +1226,8 @@ vhost_scsi_get_req(struct vhost_virtqueue *vq, struct vhost_scsi_ctx *vc,
 			/* validated at handler entry */
 			vs_tpg = vhost_vq_get_backend(vq);
 			tpg = READ_ONCE(vs_tpg[*vc->target]);
-			if (unlikely(!tpg)) {
-				vq_err(vq, "Target 0x%x does not exist\n", *vc->target);
+			if (unlikely(!tpg))
 				goto out;
-			}
 		}
 
 		if (tpgp)
-- 
2.43.0

Re: [PATCH] vhost-scsi: Fix log flooding with target does not exist errors

[Stefan Hajnoczi]

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

On Sat, Jun 07, 2025 at 12:18:15PM -0500, Mike Christie wrote:
> As part of the normal initiator side scanning the guest's scsi layer
> will loop over all possible targets and send an inquiry. Since the
> max number of targets for virtio-scsi is 256, this results in 255
> error messages about targets not existing. When there's more than 1
> vhost-scsi device, then you get N * 255 log messages.
> 
> It looks like the log message was added by accident in:
> 
> commit 09d7583294aa ("vhost/scsi: Use common handling code in request
> queue handler")
> 
> The patch was just making some coce common but added the vq_err call
> and I'm guessing the patch author forgot to enable the vq_err call
> (vq_err is implemented by pr_debug which defaults to off). So this
> patch removes the call since it's expected to hit this path during
> device discovery.
> 
> Fixes: 09d7583294aa ("vhost/scsi: Use common handling code in request queue handler")
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/vhost/scsi.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

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

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

Re: [PATCH] vhost-scsi: Fix log flooding with target does not exist errors

[Stefano Garzarella]

On Sat, Jun 07, 2025 at 12:18:15PM -0500, Mike Christie wrote:
>As part of the normal initiator side scanning the guest's scsi layer
>will loop over all possible targets and send an inquiry. Since the
>max number of targets for virtio-scsi is 256, this results in 255
>error messages about targets not existing. When there's more than 1
>vhost-scsi device, then you get N * 255 log messages.
>
>It looks like the log message was added by accident in:
>
>commit 09d7583294aa ("vhost/scsi: Use common handling code in request
>queue handler")

IIUC that commit just moved the code, but it looks like it was added by
commit 3f8ca2e115e5 ("vhost/scsi: Extract common handling code from
control queue handler")

The rest LGTM!

Thanks,
Stefano

>
>The patch was just making some coce common but added the vq_err call
>and I'm guessing the patch author forgot to enable the vq_err call
>(vq_err is implemented by pr_debug which defaults to off). So this
>patch removes the call since it's expected to hit this path during
>device discovery.
>
>Fixes: 09d7583294aa ("vhost/scsi: Use common handling code in request queue handler")
>Signed-off-by: Mike Christie <michael.christie@oracle.com>
>---
> drivers/vhost/scsi.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
>diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
>index c12a0d4e6386..c9f418a4571a 100644
>--- a/drivers/vhost/scsi.c
>+++ b/drivers/vhost/scsi.c
>@@ -1226,10 +1226,8 @@ vhost_scsi_get_req(struct vhost_virtqueue *vq, struct vhost_scsi_ctx *vc,
> 			/* validated at handler entry */
> 			vs_tpg = vhost_vq_get_backend(vq);
> 			tpg = READ_ONCE(vs_tpg[*vc->target]);
>-			if (unlikely(!tpg)) {
>-				vq_err(vq, "Target 0x%x does not exist\n", *vc->target);
>+			if (unlikely(!tpg))
> 				goto out;
>-			}
> 		}
>
> 		if (tpgp)
>-- 
>2.43.0
>

Re: [PATCH] vhost-scsi: Fix log flooding with target does not exist errors

[michael.christie]

On 6/10/25 9:18 AM, Stefano Garzarella wrote:
> On Sat, Jun 07, 2025 at 12:18:15PM -0500, Mike Christie wrote:
>> As part of the normal initiator side scanning the guest's scsi layer
>> will loop over all possible targets and send an inquiry. Since the
>> max number of targets for virtio-scsi is 256, this results in 255
>> error messages about targets not existing. When there's more than 1
>> vhost-scsi device, then you get N * 255 log messages.
>>
>> It looks like the log message was added by accident in:
>>
>> commit 09d7583294aa ("vhost/scsi: Use common handling code in request
>> queue handler")
> 
> IIUC that commit just moved the code, but it looks like it was added by
> commit 3f8ca2e115e5 ("vhost/scsi: Extract common handling code from
> control queue handler")

I saw that and was not 100% sure what patch to blame.

Your commit adds the new vhost_scsi_get_req function which adds the
vq_err call. It also hooks the control queue handler into it (control ==
SCSI task management like LUN_RESET). With just this patch we don't see
a problem yet because the normal SCSI command handler is not using it yet.

My commit hooks the normal SCSI command handler into the new
vhost_scsi_get_req function. The SCSI command handler calling the new
function is what hits the problem because it's the SCSI INQUIRY command
going through this path during scanning that hits the problem.

Re: [PATCH] vhost-scsi: Fix log flooding with target does not exist errors

[Stefano Garzarella]

On Tue, Jun 10, 2025 at 11:30:58AM -0500, michael.christie@oracle.com wrote:
>On 6/10/25 9:18 AM, Stefano Garzarella wrote:
>> On Sat, Jun 07, 2025 at 12:18:15PM -0500, Mike Christie wrote:
>>> As part of the normal initiator side scanning the guest's scsi layer
>>> will loop over all possible targets and send an inquiry. Since the
>>> max number of targets for virtio-scsi is 256, this results in 255
>>> error messages about targets not existing. When there's more than 1
>>> vhost-scsi device, then you get N * 255 log messages.
>>>
>>> It looks like the log message was added by accident in:
>>>
>>> commit 09d7583294aa ("vhost/scsi: Use common handling code in request
>>> queue handler")
>>
>> IIUC that commit just moved the code, but it looks like it was added by
>> commit 3f8ca2e115e5 ("vhost/scsi: Extract common handling code from
>> control queue handler")
>
>I saw that and was not 100% sure what patch to blame.
>
>Your commit adds the new vhost_scsi_get_req function which adds the
>vq_err call. It also hooks the control queue handler into it (control ==
>SCSI task management like LUN_RESET). With just this patch we don't see
>a problem yet because the normal SCSI command handler is not using it yet.
>
>My commit hooks the normal SCSI command handler into the new
>vhost_scsi_get_req function. The SCSI command handler calling the new
>function is what hits the problem because it's the SCSI INQUIRY command
>going through this path during scanning that hits the problem.
>

I see, thanks for the details. If you want, I'd expand the commit 
description with part of this details, mentioning commit 3f8ca2e115e5, 
but keeping your Fixes tag.

BTW also this version is fine by me, so up to you. In both cases:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks,
Stefano

Re: [PATCH] vhost-scsi: Fix log flooding with target does not exist errors

[Mike Christie]

On 6/11/25 2:50 AM, Stefano Garzarella wrote:
> On Tue, Jun 10, 2025 at 11:30:58AM -0500, michael.christie@oracle.com wrote:
>> On 6/10/25 9:18 AM, Stefano Garzarella wrote:
>>> On Sat, Jun 07, 2025 at 12:18:15PM -0500, Mike Christie wrote:
>>>> As part of the normal initiator side scanning the guest's scsi layer
>>>> will loop over all possible targets and send an inquiry. Since the
>>>> max number of targets for virtio-scsi is 256, this results in 255
>>>> error messages about targets not existing. When there's more than 1
>>>> vhost-scsi device, then you get N * 255 log messages.
>>>>
>>>> It looks like the log message was added by accident in:
>>>>
>>>> commit 09d7583294aa ("vhost/scsi: Use common handling code in request
>>>> queue handler")
>>>
>>> IIUC that commit just moved the code, but it looks like it was added by
>>> commit 3f8ca2e115e5 ("vhost/scsi: Extract common handling code from
>>> control queue handler")
>>
>> I saw that and was not 100% sure what patch to blame.
>>
>> Your commit adds the new vhost_scsi_get_req function which adds the
>> vq_err call. It also hooks the control queue handler into it (control ==
>> SCSI task management like LUN_RESET). With just this patch we don't see
>> a problem yet because the normal SCSI command handler is not using it yet.
>>
>> My commit hooks the normal SCSI command handler into the new
>> vhost_scsi_get_req function. The SCSI command handler calling the new
>> function is what hits the problem because it's the SCSI INQUIRY command
>> going through this path during scanning that hits the problem.
>>
> 
> I see, thanks for the details. If you want, I'd expand the commit description with part of this details, mentioning commit 3f8ca2e115e5, but keeping your Fixes tag.

I'll fix it because the wording in my commit is wrong.