Hi there, I have been using FreeNAS for some time now to host my Hyper-V virtual machine files in a CIFS share. I have been using verion FreeNAS-9.3-STABLE-201602031011 for some time. I recently tried to upgrade to FreeNAS 9.10 and found Hyper-V was no longer able to run my VMs. FreeNAS 9.3 uses Samba version 4.1.21. I think FreeNAS 9.10 uses 4.3.6. I used WireShark to see if I could see a difference between the two and found that Hyper-V would make a FSCTL_SET_COMPRESSION on a .vsv file. In FreeNAS 9.3, the response would be STATUS_INVALID_DEVICE_REQUEST but in FreeNAS 9.10 the response would be STATUS_NOT_SUPPORTED. From looking at https://msdn.microsoft.com/en-us/library/cc232043.aspx I believe that STATUS_NOT_SUPPORTED is not a valid response and it should still be returning STATUS_INVALID_DEVICE_REQUEST. While I am a software developer, I'm totally out of my depth with this so if I've got it all wrong, then my humblest apologies. If not, then I this is able to be resolved. Please let me know if I can provide any more information. Cheers, Nick Barrett
I've been digging a bit deeper into this. I had a look at the source at https://github.com/samba-team/samba and did a search for FSCTL_SET_COMPRESSION. This lead me to https://github.com/samba-team/samba/blob/82801f9ec895deb9536a2b0a4e0ce4b3d5853220/source3/smbd/smb2_ioctl_filesys.c. In smb2_ioctl_filesys.c on line 450, there's a case of FSCTL_SET_COMPRESSION in a switch statement. This calls a method called fsctl_set_cmprn. In fsctl_set_cmprn on line 102, there appears to be a check to see if the file system supports compression. If it doesn't it returns NT_STATUS_NOT_SUPPORTED. I believe this is the problem. I believe it should return STATUS_INVALID_DEVICE_REQUEST instead. I'm hoping someone can verify this =)
Thanks a lot for your thorough investigation here Nick! Your analysis looks correct. The only thing I'd like to check before committing the NT_STATUS_NOT_SUPPORTED->STATUS_INVALID_DEVICE_REQUEST change is test against ReFS on Server 2016 (TP5), which doesn't support compression.
Created attachment 12539 [details] proposed fix - as send to samba-technical ML
Comment on attachment 12539 [details] proposed fix - as send to samba-technical ML @Jeremy: please review if you have a spare moment. I think this change is correct, based on the ReFS responses to the newly added tests.
Comment on attachment 12539 [details] proposed fix - as send to samba-technical ML LGTM. Pushing to master.
(In reply to Jeremy Allison from comment #5) > LGTM. Pushing to master. Thank you Jeremy, and thanks for the ntvfs knownfail addition (I missed that in local testing). I'll attach v4-4-test and v4-5-test based patches for maintenance.
Created attachment 12548 [details] patchset for 4.4, cherry picked from master
Created attachment 12549 [details] patchset for 4.5, cherry picked from master
Is there a patch for 4.3.x? We aren't planning to use 4.4 until late November.
(In reply to Josh Paetzel from comment #9) > Is there a patch for 4.3.x? We aren't planning to use 4.4 until late > November. 4.3.x takes security fixes only now that 4.4 and 4.5 are available: https://wiki.samba.org/index.php/Samba_Release_Planning That said, the patches should apply cleanly to 4.3.x, if you care to carry them downstream. The smbtorture test changes need not be applied.
Re-assigning to Karolin for inclusion in 4.5.next, 4.4.next.
(In reply to Jeremy Allison from comment #11) Pushed to autobuild-v4-{5,4}-test.
(In reply to Karolin Seeger from comment #12) Pushed to both branches. Closing out bug report. Thanks!
My thanks to you all David, Jeremy and Karolin for getting this sorted. It's very much appreciated =)
Hello again, Thanks again for everyones effort on this one. It's my understanding that the team at FreeNas have back ported the fix to the version that is currently used in FreeNas. I have tested a build that includes this fix and I am still experiencing the same issue. I've had a read over the code to the best of my ability. I believe the GET_COMPRESSION has been dealt with, but the SET_COMPRESSION is still returning NT_STATUS_NOT_SUPPORTED instead of NT_STATUS_INVALID_DEVICE_REQUEST. I believe the line of code that needs to be changed is here https://github.com/samba-team/samba/blob/master/source3/smbd/smb2_ioctl_filesys.c#L109 As such, I think the test test_ioctl_compress_notsup_set here (https://github.com/samba-team/samba/blob/f6f6263f1f03db965b64b5d7858e44ab5ffb0aeb/source4/torture/smb2/ioctl.c#L2609) needs to check for the return of NT_STATUS_INVALID_DEVICE_REQUEST as well.
(In reply to Nick Barrett from comment #15) > Hello again, > > Thanks again for everyones effort on this one. Thanks for your persistence :) > It's my understanding that the team at FreeNas have back ported the fix to > the version that is currently used in FreeNas. > > I have tested a build that includes this fix and I am still experiencing the > same issue. > > I've had a read over the code to the best of my ability. I believe the > GET_COMPRESSION has been dealt with, but the SET_COMPRESSION is still > returning NT_STATUS_NOT_SUPPORTED instead of > NT_STATUS_INVALID_DEVICE_REQUEST. Yes, that's correct. > I believe the line of code that needs to be changed is here > https://github.com/samba-team/samba/blob/master/source3/smbd/ > smb2_ioctl_filesys.c#L109 > > As such, I think the test test_ioctl_compress_notsup_set here > (https://github.com/samba-team/samba/blob/ > f6f6263f1f03db965b64b5d7858e44ab5ffb0aeb/source4/torture/smb2/ioctl.c#L2609) > needs to check for the return of NT_STATUS_INVALID_DEVICE_REQUEST as well. test_ioctl_compress_notsup_set() passes against a Windows Server 2016 ReFS share, which doesn't support compression. Would you possibly be able to test your application against a Windows Server 2016 ReFS share and provide a network trace?
(In reply to David Disseldorp from comment #16) ... > test_ioctl_compress_notsup_set() passes against a Windows Server 2016 ReFS > share, which doesn't support compression. > > Would you possibly be able to test your application against a Windows Server > 2016 ReFS share and provide a network trace? Hi Nick, did you get a chance to test this?
Hi David, Thanks for your patience. I haven't had the chance yet. So far I have upgraded my failover cluster to Server 2016. Next step is to add some storage so I can try an ReFS share. I'll let you know how I get on.
I've done some further investigation. Setup: I have upgraded all my Hyper-V servers to Windows Server 2016. For my test, I added a spare hard drive to one of the servers and formatted it using ReFS. I then shared this ReFS formatted drive. One thing that I noticed this time around, was that when Hyper-V is calling FSCTL_SET_COMPRESSION on the share, it is asking for COMPRESSION_FORMAT_NONE. Unfortunately I have lost the wireshark dumps from last time, so I can't confirm that they also asked for COMPRESSION_FORMAT_NONE but I presume they did. Tests: Looking through the WireShark dumps, the ReFS formatted share returns STATUS_SUCCESS. You mentioned that ReFS doesn't support compression, so I'm guessing it responds with STATUS_SUCCESS because the request is asking for COMPRESSION_FORMAT_NONE. I don't really have a way to test for requesting other compression formats as I am just using Hyper-V for my tests. I took a WireShark dump again of Samba version 4.1.21 (which is the version found in FreeNAS-9.3-STABLE-201602031011 that I am successfully running) and I can confirm that it returns STATUS_INVALID_DEVICE_REQUEST for the same request. Thoughts: In my case it appears that Hyper-V is trying to ensure there isn't any compression going on. My FreeNas server has compression turned off on the volume that's hosting the VM files. I don't know how this would all work for a volume that does have compression enabled. For a non-compressed volume though, here's my guess as to what the response values should be to a FSCTL_SET_COMPRESSION request: Request Response COMPRESSION_FORMAT_NONE STATUS_SUCCESS COMPRESSION_FORMAT_DEFAULT STATUS_INVALID_DEVICE_REQUEST COMPRESSION_FORMAT_LZNT1 STATUS_INVALID_DEVICE_REQUEST If you need any other specific test run then let me know. =)
It took me a while but I created a VM running FreeNas using FreeNAS-9.10.2 (a476f16) which uses Samba 4.4.5. I can confirm that a FSCTL_SET_COMPRESSION request for COMPRESSION_FORMAT_NONE results in a STATUS_NOT_SUPPORTED response which causes Hyper-V to fail.
Thanks for the update, Nick! I'll work on a test + fix for this behaviour, and will post here once I have something working against Windows 2016 (ReFS) and Samba.
(In reply to Nick Barrett from comment #19) ... > Request Response > COMPRESSION_FORMAT_NONE STATUS_SUCCESS > COMPRESSION_FORMAT_DEFAULT STATUS_INVALID_DEVICE_REQUEST > COMPRESSION_FORMAT_LZNT1 STATUS_INVALID_DEVICE_REQUEST As mentioned earlier, f6f6263f1f03db965b64b5d7858e44ab5ffb0aeb confirmed that STATUS_NOT_SUPPORTED is the correct response, if we're to use Windows 2016 ReFS behaviour as the benchmark. What I did miss in the previous change was the STATUS_SUCCESS response to set(COMPRESSION_FORMAT_NONE) requests - thanks for doing the investigation on this. The latest set of patches (sent to samba-technical with you in cc) fix this on the server side.
Hi David, After rereading your responses a couple of times, I now understand what you are saying about the result to requesting actual file compression being STATUS_NOT_SUPPORTED instead of STATUS_INVALID_DEVICE_REQUEST. For some reason I got it stuck in my head. My apologies for keep bringing it up. Thanks again for sorting this. =)
Created attachment 12805 [details] Follow up patch-set fixing set-compression(format=NONE) Same patch-set, as sent to the technical ML: https://lists.samba.org/archive/samba-technical/2017-January/117945.html Behaviour confirmed against WS2016 ReFS.
Comment on attachment 12805 [details] Follow up patch-set fixing set-compression(format=NONE) LGTM. Pushed to master.
Created attachment 12806 [details] git-am fix for 4.5.next, 4.4.next. Cherry-pick from what went into master. Applies cleanly to 4.5.next, 4.4.next.
Comment on attachment 12806 [details] git-am fix for 4.5.next, 4.4.next. Thanks Jeremy! @Karo: please pull for 4.5.next and 4.4.next.
(In reply to David Disseldorp from comment #27) Pushed to autobuild-v4-{5,4}-test.
(In reply to Karolin Seeger from comment #28) Pushed to both branches. Closing out bug report. Thanks!
Just tried version 4.5.4 and it worked. Yay! Thanks for sorting this out =)
(In reply to Nick Barrett from comment #30) > Just tried version 4.5.4 and it worked. Yay! \o/ Thanks for the feedback, Nick!
(In reply to David Disseldorp from comment #27) > Comment on attachment 12806 [details] > git-am fix for 4.5.next, 4.4.next. > > Thanks Jeremy! > @Karo: please pull for 4.5.next and 4.4.next. Reopening again, sorry. As reported by Hodur in bug#13028, 4.6 appears to have missed the 28cc347876b97b7409d6efd377f031fc6df0c5f3 backport here - it's carried in 4.4, 4.5 and 4.7/master only.
Created attachment 13583 [details] cherry picked fix for 4.6.next
*** Bug 13028 has been marked as a duplicate of this bug. ***
Comment on attachment 13583 [details] cherry picked fix for 4.6.next LGTM, but do we also need the test fix ? Jeremy.
Pushed to autobuild-v4-6-test.
(In reply to Jeremy Allison from comment #35) > Comment on attachment 13583 [details] > cherry picked fix for 4.6.next > > LGTM, but do we also need the test fix ? The other branches are carrying the test, so I'll attach the 4.6.next test backport here too. @Karo: low priority, but please push the (pending) test-for-4.6.next patch when reviewed, and then feel free to close this bug.
Created attachment 13615 [details] cherry-picked test for 4.6.next Test backport for 4.6, which I overlooked earlier.
(In reply to David Disseldorp from comment #38) Pushed test to autobuild-v4-6-test.
Pushed to v4-6-test. Closing out bug report. Thanks!