Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CIFS client support for Linux #103

Closed
wants to merge 1 commit into from

Conversation

shibumi
Copy link

@shibumi shibumi commented Jul 16, 2018

Supported are any client statistics in SMB1 and SMB2. This should fix #95

@shibumi shibumi force-pushed the shibumi/cifs-support branch 2 times, most recently from 2f5136d to d774d6a Compare July 16, 2018 20:22
@SuperQ SuperQ requested review from mdlayher and SuperQ July 16, 2018 21:57
Copy link
Contributor

@mdlayher mdlayher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. I think this needs some work in the readability department, and I can't say I'm comfortable with the heavy use of regex for string parsing.

Is there any better way? Is there a more machine-readable CIFS interface?

I believe some improvements can be made here that will make this code more maintainable in the long run.

cifs/parse_cifs.go Outdated Show resolved Hide resolved
cifs/parse_cifs.go Outdated Show resolved Hide resolved
cifs/parse_cifs.go Outdated Show resolved Hide resolved
@shibumi
Copy link
Author

shibumi commented Jul 19, 2018

@mdlayher Sure. I am full with you about the part about readability. About the regex part: I don't think there is an easier way for this. You can have a look at the example files in the issue: #95 Any other idea is more than welcome :)

@shibumi
Copy link
Author

shibumi commented Jul 21, 2018

I will overwork this and open a new PR, when it's ready.

@shibumi shibumi closed this Jul 21, 2018
@mdlayher
Copy link
Contributor

No need to close it, just following up with more commits is totally fine!

@shibumi shibumi reopened this Sep 14, 2018
@shibumi
Copy link
Author

shibumi commented Sep 14, 2018

@mdlayher I have finally some time to code further on it. So I have reopen the pull Request and did further research. I've found another solution for getting CIFS statistics. The cifsiostat command provides the following output:

Linux 3.10.0-862.3.3.el7.x86_64 (SERVER) 	14.09.2018 	_x86_64_	(16 CPU)

Filesystem:                    rB/s         wB/s    rops/s    wops/s         fo/s         fc/s         fd/s
\\SERVER\SHARE         0,00         0,00      0,00      0,00         0,00         0,00         0,00 

This is much less output as we get via /proc/fs/cifs/Stats:

Resources in use
CIFS Session: 1
Share (unique mount targets): 1
SMB Request/Response Buffer: 1 Pool size: 5
SMB Small Req/Resp Buffer: 1 Pool size: 30
Operations (MIDs): 0

0 session 0 share reconnects
Total vfs operations: 85 maximum at one time: 2

1) \\SERVER\SHARE
SMBs: 81 Oplocks breaks: 0
Reads:  0 Bytes: 0
Writes: 0 Bytes: 0
Flushes: 0
Locks: 0 HardLinks: 0 Symlinks: 0
Opens: 0 Closes: 0 Deletes: 0
Posix Opens: 0 Posix Mkdirs: 0
Mkdirs: 0 Rmdirs: 0
Renames: 0 T2 Renames 0
FindFirst: 0 FNext 0 FClose 0

So I would prefer for writing a parser for /proc/fs/cifs/Stats (although it's far more complicated with a lot of context switches and other nasty things. I can't understand that there is no easy machine readable output in the kernel at the moment)

@shibumi
Copy link
Author

shibumi commented Sep 16, 2018

@mdlayher @SuperQ Can one of you have a look at the newest commit? If this commit is okay I would rebase, squash and commit with sign-off message.

I am unhappy with a few things at the moment:

  • I need to move the flush regex line for SMB2 very early in the Regex Array otherwise we will have a mismatch and we will pollute our SMB2 statistics with a SMB1 flush statistic
  • I need to check for a null pointer before calling parseSMBStats
  • I am using the currentSMBSessionIDs variable just as check for a nullpointer.. I am pretty sure there is a more convenient way to do this.

EDIT: I have changed the datastructure. I am using the same hashmap for SMB1 and SMB2 now. This way the code is a lot easier to read and it should be easier to access the data as well.

@shibumi shibumi changed the title This commit adds support for CIFS to procfs CIFS client support for Linux Jan 15, 2019
@shibumi shibumi force-pushed the shibumi/cifs-support branch 2 times, most recently from 441d371 to 1f1c397 Compare January 15, 2019 13:41
@shibumi
Copy link
Author

shibumi commented Jan 15, 2019

@mdlayher @SuperQ Could you review again? It builds fine now, but it still uses the regex approach to get the statistics. A better solution would be to write an own parser for cifs statistics (maybe in a seperate project?). However this approach works fine, feedback to my code is welcome. I am still a golang newbie and need to learn a lot.


// parseHeader parses our SMB header
func parseHeader(line string, header map[string]uint64) error {
for _, regexpHeader := range regexpHeaders {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a single multline regex instead of this array? I think it would make the code be more readable.

Renames: 0 T2 Renames 0
FindFirst: 1 FNext 0 FClose 0

2) \\server2\share2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use different numbers in the second fixture? I think there might be an issue with the parsing.

@discordianfish
Copy link
Member

Sorry for not responding earlier! So this looks really hard to parse.. :-/

I don't think the loop over the file is doing the right thing. After it finds the 'new smb block', it always run the parse block function. Either way, I'd suggest you first try to split the file into the 'global' and per mount blocks and parse these with big multiline regex. I think that would make the code much more readable.

@shibumi
Copy link
Author

shibumi commented Jan 31, 2019

Hi @discordianfish,
Thanks for the cool review :) I am totally with you. A multiline regex makes sense here.
Let me see if I got your points corrrectly. You want to split the file into blocks + split the regex into three multiline regex blocks. And then I should just parse all blocks? What about building one giant multiline regex for the whole file and firing it on the whole file? Never done multiline regex in golang before.

I don't think the loop over the file is doing the right thing. After it finds the 'new smb block', it always run the parse block function.

The tests that I have attached are working fine.

@discordianfish
Copy link
Member

Try using different numbers in the different fixtures, that should expose the issue. Or I'm wrong, quite possible :)
One giant regexp for the whole file probably doesn't work because the sections are repeating but dunno, maybe there is a way.

@shibumi
Copy link
Author

shibumi commented Feb 9, 2019

Ok I tried using different numbers in the second fixture and you are correct. The tests will fail, but I have no idea why. When I introspect the two stats want and have they are the same. I compared them. I guess you are talking about the stats hashmap for the second fixture, correct? They have the same values. I will upload a new version with new values in the second fixtures.

About parsing it as one file:
Yes, it's correct. The sections can repeat. So you can either have only SMB1 blocks or only SMB2 blocks or a mixed file with SMB1 and SMB2 blocks in no particular order. It's also possible to have 20 SMB2 blocks and just 2 SMB1 blocks after the 5th SMB2 block..

this makes parsing of the cifsstats really difficult. It's strange that no kernel developer tried changing this already. I mean human readable output is nice, but it would be nice for us to have more machine readable output (the NFS stats are awesome for example)

This commit adds CIFS/SMB client support for procfs.
It support SMB version 1,2 and 3.
We parse /proc/fs/cifs/stats for CIFS and SMB statistics.

Signed-off-by: Christian Rebischke <[email protected]>
@discordianfish
Copy link
Member

I'm going to close this for now. Let's re-open when the changes are addressed.

bobrik pushed a commit to bobrik/procfs that referenced this pull request Jan 14, 2023
Add two new fields to Meminfo struct
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add parser for CIFS client stats
3 participants