diff mbox series

tests/nvme: Remove test output for passthrough error logging

Message ID 20221116001234.581003-1-alan.adamson@oracle.com (mailing list archive)
State New, archived
Headers show
Series tests/nvme: Remove test output for passthrough error logging | expand

Commit Message

Alan Adamson Nov. 16, 2022, 12:12 a.m. UTC
Commit d7ac8dca938c ("nvme: quiet user passthrough command errors")
disabled error logging for passthrough commands so the associated
error output in nvme/039.out should be removed.

When an error logging opt-in mechanism for passthrough commands is
provided, the error output can be added back.

Signed-off-by: Alan Adamson <alan.adamson@oracle.com>
---
 tests/nvme/039.out | 2 --
 1 file changed, 2 deletions(-)

Comments

Chaitanya Kulkarni Nov. 16, 2022, 12:54 a.m. UTC | #1
On 11/15/22 16:12, Alan Adamson wrote:
> Commit d7ac8dca938c ("nvme: quiet user passthrough command errors")
> disabled error logging for passthrough commands so the associated
> error output in nvme/039.out should be removed.
> 
> When an error logging opt-in mechanism for passthrough commands is
> provided, the error output can be added back.
> 
> Signed-off-by: Alan Adamson <alan.adamson@oracle.com>
> ---

Shinichiro, I think this should fix the issue.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck
Shin'ichiro Kawasaki Nov. 16, 2022, 4:17 a.m. UTC | #2
On Nov 15, 2022 / 16:12, Alan Adamson wrote:
> Commit d7ac8dca938c ("nvme: quiet user passthrough command errors")
> disabled error logging for passthrough commands so the associated
> error output in nvme/039.out should be removed.
> 
> When an error logging opt-in mechanism for passthrough commands is
> provided, the error output can be added back.

Thanks for this quick action. This two-steps approach looks good for me.

I confirmed the fix avoids the failure with v6.1-rc5 kernel. Also, I observe
this fix makes the test case fail with v6.0 kernel. I suggest to skip the test
case with kernel v6.0 or older, applying the hunk below. Could you repost v2
with this change? Or if you want, I can apply it together with v1. Please let
me know your preference.

diff --git a/tests/nvme/039 b/tests/nvme/039
index e175055..ea626e3 100755
--- a/tests/nvme/039
+++ b/tests/nvme/039
@@ -14,6 +14,7 @@ QUICK=1

 requires() {
        _have_program nvme
+       _have_kver 6 1
        _have_kernel_option FAULT_INJECTION && \
            _have_kernel_option FAULT_INJECTION_DEBUG_FS
 }
Keith Busch Nov. 16, 2022, 4:48 p.m. UTC | #3
On Wed, Nov 16, 2022 at 04:17:03AM +0000, Shinichiro Kawasaki wrote:
> On Nov 15, 2022 / 16:12, Alan Adamson wrote:
> > Commit d7ac8dca938c ("nvme: quiet user passthrough command errors")
> > disabled error logging for passthrough commands so the associated
> > error output in nvme/039.out should be removed.
> > 
> > When an error logging opt-in mechanism for passthrough commands is
> > provided, the error output can be added back.
> 
> Thanks for this quick action. This two-steps approach looks good for me.
> 
> I confirmed the fix avoids the failure with v6.1-rc5 kernel. Also, I observe
> this fix makes the test case fail with v6.0 kernel. I suggest to skip the test
> case with kernel v6.0 or older, applying the hunk below. Could you repost v2
> with this change? Or if you want, I can apply it together with v1. Please let
> me know your preference.

It sounds like some future case might allow these errors to log in some
circumstances, so I'm not sure the test case should be so dependent on
seeing or not seeing these messages. Is there a mechanism to say these
are optional messages that may or may not show up?
Alan Adamson Nov. 16, 2022, 5:22 p.m. UTC | #4
> On Nov 16, 2022, at 8:48 AM, Keith Busch <kbusch@kernel.org> wrote:
> 
> On Wed, Nov 16, 2022 at 04:17:03AM +0000, Shinichiro Kawasaki wrote:
>> On Nov 15, 2022 / 16:12, Alan Adamson wrote:
>>> Commit d7ac8dca938c ("nvme: quiet user passthrough command errors")
>>> disabled error logging for passthrough commands so the associated
>>> error output in nvme/039.out should be removed.
>>> 
>>> When an error logging opt-in mechanism for passthrough commands is
>>> provided, the error output can be added back.
>> 
>> Thanks for this quick action. This two-steps approach looks good for me.
>> 
>> I confirmed the fix avoids the failure with v6.1-rc5 kernel. Also, I observe
>> this fix makes the test case fail with v6.0 kernel. I suggest to skip the test
>> case with kernel v6.0 or older, applying the hunk below. Could you repost v2
>> with this change? Or if you want, I can apply it together with v1. Please let
>> me know your preference.
> 
> It sounds like some future case might allow these errors to log in some
> circumstances, so I'm not sure the test case should be so dependent on
> seeing or not seeing these messages. Is there a mechanism to say these
> are optional messages that may or may not show up?

We could just remove the tests for now which would also work for pre-6.1 and add them back
when the future case shows up (Hopefully soon - I’m working on it).  The test will be changed
to test the opted-in and opted-out mechanism.

Alan
Chaitanya Kulkarni Nov. 16, 2022, 6:41 p.m. UTC | #5
>
>>> I confirmed the fix avoids the failure with v6.1-rc5 kernel. Also, I observe
>>> this fix makes the test case fail with v6.0 kernel. I suggest to skip the test
>>> case with kernel v6.0 or older, applying the hunk below. Could you repost v2
>>> with this change? Or if you want, I can apply it together with v1. Please let
>>> me know your preference.
>>
>> It sounds like some future case might allow these errors to log in some
>> circumstances, so I'm not sure the test case should be so dependent on
>> seeing or not seeing these messages. Is there a mechanism to say these
>> are optional messages that may or may not show up?
> 
> We could just remove the tests for now which would also work for pre-6.1 and add them back
> when the future case shows up (Hopefully soon - I’m working on it).  The test will be changed
> to test the opted-in and opted-out mechanism.
> 

This seems to be the right thing to add the testcase with right feature
upstream than fixing the broken testcase creating compatibility issues.

-ck
diff mbox series

Patch

diff --git a/tests/nvme/039.out b/tests/nvme/039.out
index 162935eb1d7b..139070d22240 100644
--- a/tests/nvme/039.out
+++ b/tests/nvme/039.out
@@ -2,6 +2,4 @@  Running nvme/039
  Read(0x2) @ LBA 0, 1 blocks, Unrecovered Read Error (sct 0x2 / sc 0x81) DNR 
  Read(0x2) @ LBA 0, 1 blocks, Unknown (sct 0x3 / sc 0x75) DNR 
  Write(0x1) @ LBA 0, 1 blocks, Write Fault (sct 0x2 / sc 0x80) DNR 
- Identify(0x6), Access Denied (sct 0x2 / sc 0x86) DNR 
- Unknown(0x96), Invalid Command Opcode (sct 0x0 / sc 0x1) DNR 
 Test complete