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

FUSE Improvements #85

Closed
9 tasks done
billziss-gh opened this issue May 13, 2017 · 11 comments
Closed
9 tasks done

FUSE Improvements #85

billziss-gh opened this issue May 13, 2017 · 11 comments
Assignees
Milestone

Comments

@billziss-gh
Copy link
Collaborator

billziss-gh commented May 13, 2017

Based on some feedback from porting rar2fs.

  • fuse_exited is missing. See commit f685311.

    • Turns out that fuse_exited was deprecated at the time of the FUSE 2.8 API, which is why it was not originally added to WinFsp-FUSE. Adding nevertheless.
  • Fix the warning below. See commit ed58b7a.

      /usr/include/fuse/winfsp_fuse.h: In function ‘fsp_fuse_env’:
      /usr/include/fuse/winfsp_fuse.h:367:19: warning: missing initializer for field  reserved’ of ‘struct fsp_fuse_env’ [-Wmissing-field-initializers]
          static struct fsp_fuse_env env = FSP_FUSE_ENV_INIT;
                      ^
      /usr/include/fuse/winfsp_fuse.h:251:12: note: ‘reserved’ declared here
          void (*reserved[3])();
                  ^
    
  • Some of the --options are inconvenient to use. Equivalent -o options should be added (e.g. -o VolumePrefix=...). See commit 4ea9c6e.

  • Turns out that --options use backslash as an escape character. Allow slash for -o VolumePrefix as in -o VolumePrefix=/server/share. Commit c7d720e.

  • Some FUSE programs depend on a fuse.h file in /usr/include (vs /usr/include/fuse). Although WinFsp-FUSE implements FUSE 2.8 this should probably be added for compatibility purposes. [WONTFIX: The reason is that including the fuse.h file directly from /usr/include is deprecated on LIBFUSE and that compiling with cygfuse on Cygwin requires -DCYGFUSE. ]

  • Cygfuse is currently only built for 64-bit. It should also be built for 32-bit. See commit 6d5401d.

  • @hasse69 reports that the 32-bit cygfuse fails with STATUS_NOT_IMPLEMENTED. [UNRESOLVED: See issue Creation of a disk file system may fail #88.]

  • Improve cygfuse initialization by only acquiring the cygfuse_mutex when strictly necessary. See 18a77d6.

  • Output error message to stderr when cygfuse_init fails. See commit 9d77c19.

See hasse69/rar2fs#75

@hasse69
Copy link

hasse69 commented May 18, 2017

Great job!

@hasse69 reports that the 32-bit cygfuse fails with STATUS_NOT_IMPLEMENTED.

Let's do as you say and wait for an official package of both WinFSP and cygfuse.

fuse_exited is missing. See commit f685311.

Right, but these APIs are flagged for removal in 3.0 and with that a lot of functions that rar2fs use will be removed. Moving from 2.x to 3.x version is a bit of a undertaking. This function is used since I discovered a problem in native fuse for the SIGINT signal that sometimes failed on me. If that is now fixed in later version of fuse I do not know, maybe, maybe not.

That brings me to the SIGTERM issue you saw. Killing a daemonized process using SIGTERM works fine using legacy fuse. So it seems related to the cygfuse layer. Have you been able to test the same procedure on e.g. sshfs running on top of cygfuse? Does it behave the same? If not then I need to revisit the rar2fs implementation and see how sshfs is handling this.

@billziss-gh
Copy link
Collaborator Author

Let's do as you say and wait for an official package of both WinFSP and cygfuse.

Agreed.

fuse_exited is missing. See commit f685311.

Right, but these APIs are flagged for removal in 3.0 and with that a lot of functions that rar2fs use will be removed.

That was my rationale for not adding it as I recall. However since WinFsp-FUSE emulates FUSE 2.8 functionality, I think it makes sense to add it for now. If FUSE 3 functionality ever gets added to WinFsp, then we could remove/hide it in the FUSE 3 interface.

That brings me to the SIGTERM issue you saw. Killing a daemonized process using SIGTERM works fine using legacy fuse. So it seems related to the cygfuse layer. Have you been able to test the same procedure on e.g. sshfs running on top of cygfuse? Does it behave the same? If not then I need to revisit the rar2fs implementation and see how sshfs is handling this.

As far as I can remember signals work correctly on all FUSE file systems ported to Cygwin. Sshfs definitely works without any problems, if you want to test with it.

BTW, are signals in rar2fs set on the main thread or elsewhere?

The WinFsp-FUSE signals handling is quite different from the one in LIBFUSE. It uses a separate thread that does a sigwait for the appropriate signals. The reason for this design is that Cygwin signals cannot interrupt Win32 calls and I needed to be able to do so in WinFsp-FUSE; the sigwait design allows me to "interrupt" a waiting Win32 call in a way that Cygwin signals cannot. See https://github.com/billziss-gh/winfsp/wiki/SSHFS-Port-Case-Study#some-additional-challenges

So it is possible that the WinFsp-FUSE signals do not behave as on other platforms. And this is already on top of Cygwin's signal mechanism which is of course an emulation running on top of Win32.

@hasse69
Copy link

hasse69 commented May 18, 2017

As far as I can remember signals work correctly on all FUSE file systems ported to Cygwin. Sshfs definitely works without any problems, if you want to test with it.

Ok, maybe that is because sshfs does not use a split fuse main loop but use the default one. Need to check.

BTW, are signals in rar2fs set on the main thread or elsewhere?

As I stated before (not here though) is that rar2fs does not catch nor block SIGTERM. And it works when running in the foreground.

EDIT: I simply tried to replace the split fuse_main() loop and use the default one provided by fuse itself and now SIGTERM seems to be handled properly. I need the more advanced split loop for the fuse issue mentioned above, but for cygfuse it might not be needed. Will fix that.

@billziss-gh
Copy link
Collaborator Author

EDIT: I simply tried to replace the split fuse_main() loop and use the default one provided by fuse itself and now SIGTERM seems to be handled properly. I need the more advanced split loop for the fuse issue mentioned above, but for cygfuse it might not be needed. Will fix that.

Hmmm... it would be good if we understood what the difference is. There is be a bug somewhere here and it would be good if we pinpointed it.

The code for fuse_main is here:
https://github.com/billziss-gh/winfsp/blob/master/src/dll/fuse/fuse_main.c#L121

It's getting late for me and I am off to bed.

@hasse69
Copy link

hasse69 commented May 18, 2017

Agreed. But if it is a bug or simply a limitation is hard to say. But since it works in foreground mode I would say there is something in the split fuse_main() function set that rar2fs use. SSHFS also use the split functions if FUSE version is reported >= 26 so it might be that rar2fs is doing something wrong, even though I cannot spot it right now.

@hasse69
Copy link

hasse69 commented May 18, 2017

Ok, the difference is that fuse_loop()/_mt() is not called from the main thread in rar2fs. Will check if that affects the behavior. This works on Linux but maybe might trick the slightly different signal handling you describe into a corner.

@billziss-gh
Copy link
Collaborator Author

Good detective work! Signals and threads have always been fraught with problems. I think you have found the original cause of the problem.

BTW, the code that sets the signals on WinFsp-FUSE is here:

https://github.com/billziss-gh/winfsp/blob/master/inc/fuse/winfsp_fuse.h#L288-L353

@hasse69
Copy link

hasse69 commented May 18, 2017 via email

@hasse69
Copy link

hasse69 commented May 19, 2017

The SIGTERM issue for rar2fs should be fixed in commit hasse69/rar2fs@882ed3a

The change was even more simple than I first imagined.

@billziss-gh
Copy link
Collaborator Author

Great work. Thank you for hunting this down!

My plan is to have a Beta 3 release with most/all the FUSE improvements listed in this issue by the end of the weekend.

@billziss-gh
Copy link
Collaborator Author

Closing this. The STATUS_NOT_IMPLEMENTED issue is being tracked in issue #88.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants