Regression tests for CIFS client

Hi everyone,

The CIFS client never had proper regression tests and we try to tweak the connectathon tests a little bit and run to avoid regressions creeping in. I think a testsuite that checks for regressions would be useful.

I spent sometime hacking up a set of tests. These tests use Python and PyUnit framework which I think might help quickly adding newer tests. Tests are written based on the past bug reports and experiences.

The primary intent of the tests is to provide some basic infrastructure upon which tests can be added easily in the future and these tests are by no means comprehensive. I have tried to avoid duplicating tests already done by Connectathon and other tests but there could still be a few duplicates in there. The tests are only lightly tested.

Currently cifstests is hosted here:

https://github.com/sureshjayaram/cifstests

Feel free to try out and let me know your feedback or any comments and suggestions.

Here's are the failures seen with 3.1 based kernel. I think the open() with O_DIRECT is expected to fail since cifs doesn't support it (I'd be interesting in knowing exact details). But, I've not dig into the xattr tests, not sure why setattr is failing (even if CIFS_XATTR is set, fs mounted with user_xattr).

Test Output
============

======================================================================
ERROR: test_directIO (__main__.OpenTests) open a file with O_DIRECT
----------------------------------------------------------------------
Traceback (most recent call last):
File "./testcifs.py", line 144, in test_directIO
raise e
OSError: [Errno 22] Invalid argument: 'testfile'

======================================================================
ERROR: test_dir_attr (__main__.XattrTests) set attrs, get attrs and remove attrs for a dir
----------------------------------------------------------------------
Traceback (most recent call last):
File "./testcifs.py", line 364, in test_dir_attr
raise e
IOError: [Errno 95] Operation not supported: 'test'

======================================================================
ERROR: test_file_attr (__main__.XattrTests) set attrs, get attrs and remove attrs for a file
----------------------------------------------------------------------
Traceback (most recent call last):
File "./testcifs.py", line 343, in test_file_attr
raise e
IOError: [Errno 95] Operation not supported: 'testfile'

----------------------------------------------------------------------
Ran 22 tests in 0.072s

FAILED (errors=3)

What do you think? Is it be a good idea?

I know I have barely scratched the surface, but any suggestion on having a working regression test is welcome. And of course, regression tests are supposed to evolve over time. Would this be a convenient way to add more tests?

Quoted 33 lines · On Wed, 08 Aug 2012 17:31:09 +0530 Suresh Jayaraman…

Hi everyone,

The CIFS client never had proper regression tests and we try to tweak the connectathon tests a little bit and run to avoid regressions creeping in. I think a testsuite that checks for regressions would be useful.

I spent sometime hacking up a set of tests. These tests use Python and PyUnit framework which I think might help quickly adding newer tests. Tests are written based on the past bug reports and experiences.

The primary intent of the tests is to provide some basic infrastructure upon which tests can be added easily in the future and these tests are by no means comprehensive. I have tried to avoid duplicating tests already done by Connectathon and other tests but there could still be a few duplicates in there. The tests are only lightly tested.

Currently cifstests is hosted here:

https://github.com/sureshjayaram/cifstests

Feel free to try out and let me know your feedback or any comments and suggestions.

Here's are the failures seen with 3.1 based kernel. I think the open() with O_DIRECT is expected to fail since cifs doesn't support it (I'd be interesting in knowing exact details). But, I've not dig into the xattr tests, not sure why setattr is failing (even if CIFS_XATTR is set, fs mounted with user_xattr).

Test Output
============

Quoted 9 lines

======================================================================
ERROR: test_directIO (__main__.OpenTests) open a file with O_DIRECT
----------------------------------------------------------------------
Traceback (most recent call last):
File "./testcifs.py", line 144, in test_directIO
raise e
OSError: [Errno 22] Invalid argument: 'testfile'

With all of the recent changes to the read/write code, I think we can reasonably do O_DIRECT now. Just make sure that you don't request an oplock on open and ensure that you're not using cache=loose codepaths.

In point of fact, we do some bounce buffering under the covers, but it does avoid the pagecache.

Quoted 18 lines

======================================================================
ERROR: test_dir_attr (__main__.XattrTests) set attrs, get attrs and remove attrs for a dir
----------------------------------------------------------------------
Traceback (most recent call last):
File "./testcifs.py", line 364, in test_dir_attr
raise e
IOError: [Errno 95] Operation not supported: 'test'

======================================================================
ERROR: test_file_attr (__main__.XattrTests) set attrs, get attrs and remove attrs for a file
----------------------------------------------------------------------
Traceback (most recent call last):
File "./testcifs.py", line 343, in test_file_attr
raise e
IOError: [Errno 95] Operation not supported: 'testfile'

Quoted 5 lines

----------------------------------------------------------------------
Ran 22 tests in 0.072s

FAILED (errors=3)

The above I'm not sure about -- maybe depends on which attributes you're trying to set?

Quoted 8 lines

What do you think? Is it be a good idea?

I know I have barely scratched the surface, but any suggestion on having a working regression test is welcome. And of course, regression tests are supposed to evolve over time. Would this be a convenient way to add more tests?

Sounds like a very worthwhile endeavor. The key to any testing infrastructure is to make it very easy to run the tests. Any hassle in setting it up is a reason not to do so, so you want to make sure there are no such barriers.

You also want to make sure that you have the ability to drill down into a single test failure without needing to run a bunch of goop around it. The cthon suite is good for this since most of the tests are written in C. That makes it easy to strace them to track down problems. I assume we'll be able to write tests in C and just have the python framework call them?

Quoted 20 lines · Jeff Layton

Quoted 11 lines · On Wed, 08 Aug 2012 17:31:09 +0530 Suresh Jayaraman…

The primary intent of the tests is to provide some basic infrastructure upon which tests can be added easily in the future and these tests are by no means comprehensive. I have tried to avoid duplicating tests already done by Connectathon and other tests but there could still be a few duplicates in there. The tests are only lightly tested.

Currently cifstests is hosted here:

https://github.com/sureshjayaram/cifstests

Sounds like a very worthwhile endeavor. The key to any testing infrastructure is to make it very easy to run the tests. Any hassle in setting it up is a reason not to do so, so you want to make sure there are no such barriers.

Quoted 6 lines

You also want to make sure that you have the ability to drill down into a single test failure without needing to run a bunch of goop around it. The cthon suite is good for this since most of the tests are written in C. That makes it easy to strace them to track down problems. I assume we'll be able to write tests in C and just have the python framework call them?

I like the idea of something expandable that we can add tests to (e.g. for cifs specific mount options, or to add tests to cover recently reported bugs).

I have patches to run xfstests over cifs, but I mostly run connectathon and dbench and fsx. Ideally running regression tests could be triggered by commits to a staging tree (or branch of cifs-2.6.git) on git.samba.org although not certain how easy this

Quoted 9 lines · On Wed, 08 Aug 2012 17:31:09 +0530 Suresh Jayaraman…

Here's are the failures seen with 3.1 based kernel. I think the open() with O_DIRECT is expected to fail since cifs doesn't support it (I'd be interesting in knowing exact details). But, I've not dig into the xattr tests, not sure why setattr is failing (even if CIFS_XATTR is set, fs mounted with user_xattr).

Test Output
============

Quoted 9 lines

======================================================================
ERROR: test_directIO (__main__.OpenTests) open a file with O_DIRECT
----------------------------------------------------------------------
Traceback (most recent call last):
File "./testcifs.py", line 144, in test_directIO
raise e
OSError: [Errno 22] Invalid argument: 'testfile'

With all of the recent changes to the read/write code, I think we can reasonably do O_DIRECT now. Just make sure that you don't request an oplock on open and ensure that you're not using cache=loose codepaths.

In point of fact, we do some bounce buffering under the covers, but it does avoid the pagecache.

Quoted 18 lines

======================================================================
ERROR: test_dir_attr (__main__.XattrTests) set attrs, get attrs and remove attrs for a dir
----------------------------------------------------------------------
Traceback (most recent call last):
File "./testcifs.py", line 364, in test_dir_attr
raise e
IOError: [Errno 95] Operation not supported: 'test'

======================================================================
ERROR: test_file_attr (__main__.XattrTests) set attrs, get attrs and remove attrs for a file
----------------------------------------------------------------------
Traceback (most recent call last):
File "./testcifs.py", line 343, in test_file_attr
raise e
IOError: [Errno 95] Operation not supported: 'testfile'

Quoted 5 lines

----------------------------------------------------------------------
Ran 22 tests in 0.072s

FAILED (errors=3)

The above I'm not sure about -- maybe depends on which attributes you're trying to set?

Quoted 8 lines

What do you think? Is it be a good idea?

I know I have barely scratched the surface, but any suggestion on having a working regression test is welcome. And of course, regression tests are supposed to evolve over time. Would this be a convenient way to add more tests?

Sounds like a very worthwhile endeavor. The key to any testing infrastructure is to make it very easy to run the tests. Any hassle in setting it up is a reason not to do so, so you want to make sure there are no such barriers.

You also want to make sure that you have the ability to drill down into a single test failure without needing to run a bunch of goop around it. The cthon suite is good for this since most of the tests are written in C. That makes it easy to strace them to track down problems. I assume

we'll be able to write tests in C and just have the python framework call them?

Quoted 22 lines · Jeff Layton

Quoted 9 lines · On Wed, 08 Aug 2012 17:31:09 +0530 Suresh Jayaraman…

Here's are the failures seen with 3.1 based kernel. I think the open() with O_DIRECT is expected to fail since cifs doesn't support it (I'd be interesting in knowing exact details). But, I've not dig into the xattr tests, not sure why setattr is failing (even if CIFS_XATTR is set, fs mounted with user_xattr).

Test Output
============

Quoted 9 lines

======================================================================
ERROR: test_directIO (__main__.OpenTests) open a file with O_DIRECT
----------------------------------------------------------------------
Traceback (most recent call last):
File "./testcifs.py", line 144, in test_directIO
raise e
OSError: [Errno 22] Invalid argument: 'testfile'

Quoted 4 lines

With all of the recent changes to the read/write code, I think we can reasonably do O_DIRECT now. Just make sure that you don't request an oplock on open and ensure that you're not using cache=loose codepaths.

Hmm, I'm not sure yet what is going on wrong.. I disabled oplocks by doing

echo N > /sys/module/cifs/parameters/enable_oplocks

(Is there a way for an application to not request oplocks?) and mounted with cache=strict and ran the directIO test alone.

strace output (snip)
--------------
stat("/mnt/cifstests", 0x7fff317e68f0) = -1 ENOENT (No such file or directory)
mkdir("cifstests", 0755) = 0
chdir("cifstests") = 0
open("testfile", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 3 fstat(3, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
close(3) = 0
open("testfile", O_RDONLY|O_DIRECT) = -1 EINVAL (Invalid argument)
write(1, "[Errno 22] Invalid argument: 'te"..., 40) = 40

But I see from dmesg oplocks are being granted.

Quoted 27 lines

In point of fact, we do some bounce buffering under the covers, but it does avoid the pagecache.

Quoted 23 lines

======================================================================
ERROR: test_dir_attr (__main__.XattrTests) set attrs, get attrs and remove attrs for a dir
----------------------------------------------------------------------
Traceback (most recent call last):
File "./testcifs.py", line 364, in test_dir_attr
raise e
IOError: [Errno 95] Operation not supported: 'test'

======================================================================
ERROR: test_file_attr (__main__.XattrTests) set attrs, get attrs and remove attrs for a file
----------------------------------------------------------------------
Traceback (most recent call last):
File "./testcifs.py", line 343, in test_file_attr
raise e
IOError: [Errno 95] Operation not supported: 'testfile'

----------------------------------------------------------------------
Ran 22 tests in 0.072s

FAILED (errors=3)

Quoted 2 lines

The above I'm not sure about -- maybe depends on which attributes you're trying to set?

I was trying to set something like:

user.mime_type = 'text/plain'

(the same worked on ext4)

Quoted 8 lines

Quoted 8 lines

What do you think? Is it be a good idea?

I know I have barely scratched the surface, but any suggestion on having a working regression test is welcome. And of course, regression tests are supposed to evolve over time. Would this be a convenient way to add more tests?

Quoted 4 lines

Sounds like a very worthwhile endeavor. The key to any testing infrastructure is to make it very easy to run the tests. Any hassle in setting it up is a reason not to do so, so you want to make sure there are no such barriers.

One such hassle I see with the current tests is that the dependencies on external modules such as xattr and acls. This could be overcome by writing C extensions that could use the C apis. Any other hassles you are noticing?

Quoted 4 lines

You also want to make sure that you have the ability to drill down into a single test failure without needing to run a bunch of goop around it. The cthon suite is good for this since most of the tests are written in C. That makes it easy to strace them to track down problems. I assume

It is possible to run strace the same way with these tests (like for e.g. the above direct IO strace output). The output does seem a little more verbose. But, any other problems you are seeing with these tests written in python?

Quoted 3 lines

we'll be able to write tests in C and just have the python framework call them?

I initially thought of writing these tests in C. But, later decided in favor of Python because of ease of use, smaller code size and availability of plenty of modules. Making it simpler (simpler here is subjective) means that should be little or no excuse for not writing an regression test when submitting a bug fix for a regression.

But, if the consensus is that we would prefer tests in C, I can redo the tests in C. If we do it in C, I think we could use CUnit framework for writing tests as it would be more natural.

Quoted 24 lines · On Thu, 09 Aug 2012 20:07:56 +0530 Suresh Jayaraman…

Quoted 5 lines

With all of the recent changes to the read/write code, I think we can reasonably do O_DIRECT now. Just make sure that you don't request an oplock on open and ensure that you're not using cache=loose codepaths.

Hmm, I'm not sure yet what is going on wrong.. I disabled oplocks by doing

echo N > /sys/module/cifs/parameters/enable_oplocks

(Is there a way for an application to not request oplocks?) and mounted with cache=strict and ran the directIO test alone.

strace output (snip)
--------------
stat("/mnt/cifstests", 0x7fff317e68f0) = -1 ENOENT (No such file or directory)
mkdir("cifstests", 0755) = 0
chdir("cifstests") = 0
open("testfile", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 3 fstat(3, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
close(3) = 0
open("testfile", O_RDONLY|O_DIRECT) = -1 EINVAL (Invalid argument)
write(1, "[Errno 22] Invalid argument: 'te"..., 40) = 40

Quoted 2 lines

But I see from dmesg oplocks are being granted.

Sorry, I might not have been clear. I meant that in principle, we could probably fix the cifs client to do O_DIRECT properly now that we have the strictcache code. It doesn't do that currently, of course...

Quoted 7 lines

I was trying to set something like:

user.mime_type = 'text/plain'

(the same worked on ext4)

Are you testing against samba? You might need to set "ea support = yes".

Quoted 38 lines

Quoted 8 lines

Quoted 8 lines

What do you think? Is it be a good idea?

I know I have barely scratched the surface, but any suggestion on having a working regression test is welcome. And of course, regression tests are supposed to evolve over time. Would this be a convenient way to add more tests?

Quoted 4 lines

Sounds like a very worthwhile endeavor. The key to any testing infrastructure is to make it very easy to run the tests. Any hassle in setting it up is a reason not to do so, so you want to make sure there are no such barriers.

One such hassle I see with the current tests is that the dependencies on external modules such as xattr and acls. This could be overcome by writing C extensions that could use the C apis. Any other hassles you are noticing?

Quoted 4 lines

You also want to make sure that you have the ability to drill down into a single test failure without needing to run a bunch of goop around it. The cthon suite is good for this since most of the tests are written in C. That makes it easy to strace them to track down problems. I assume

It is possible to run strace the same way with these tests (like for e.g. the above direct IO strace output). The output does seem a little more verbose. But, any other problems you are seeing with these tests written in python?

Quoted 3 lines

we'll be able to write tests in C and just have the python framework call them?

I initially thought of writing these tests in C. But, later decided in favor of Python because of ease of use, smaller code size and availability of plenty of modules. Making it simpler (simpler here is subjective) means that should be little or no excuse for not writing an regression test when submitting a bug fix for a regression.

Quoted 4 lines

But, if the consensus is that we would prefer tests in C, I can redo the tests in C. If we do it in C, I think we could use CUnit framework for writing tests as it would be more natural.

I'm fine with them in python, but eventually there may be need to write certain tests in C. As long as the python framework can execute those (and I'm fairly sure it can), I don't think it'll be a problem...

Quoted 26 lines · Jeff Layton

Quoted 24 lines · On Thu, 09 Aug 2012 20:07:56 +0530 Suresh Jayaraman…

Quoted 5 lines

With all of the recent changes to the read/write code, I think we can reasonably do O_DIRECT now. Just make sure that you don't request an oplock on open and ensure that you're not using cache=loose codepaths.

Hmm, I'm not sure yet what is going on wrong.. I disabled oplocks by doing

echo N > /sys/module/cifs/parameters/enable_oplocks

(Is there a way for an application to not request oplocks?) and mounted with cache=strict and ran the directIO test alone.

strace output (snip)
--------------
stat("/mnt/cifstests", 0x7fff317e68f0) = -1 ENOENT (No such file or directory)
mkdir("cifstests", 0755) = 0
chdir("cifstests") = 0
open("testfile", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 3 fstat(3, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
close(3) = 0
open("testfile", O_RDONLY|O_DIRECT) = -1 EINVAL (Invalid argument)
write(1, "[Errno 22] Invalid argument: 'te"..., 40) = 40

Quoted 5 lines

Quoted 2 lines

But I see from dmesg oplocks are being granted.

Sorry, I might not have been clear. I meant that in principle, we could probably fix the cifs client to do O_DIRECT properly now that we have the strictcache code. It doesn't do that currently, of course...

Ah, ok. What does it take to fix the cifs client? I could take a look. Are there pointers to discussion threads on this topic?