Mac updates #384

ahknight · 2017-04-04T00:13:06Z

Updates and fixes for macOS #115

trapexit · 2017-04-04T04:26:19Z

Thanks. Not sure if I'll refactor first and repush or accept and refactor after but this helps a lot. I should probably setup travis-ci to ensure in the future things build.

ahknight · 2017-04-04T14:07:41Z

I'd advise updating it on this branch before merge. One thing I'm noticing with the changes I made is that file creation is broken due to a failure to resolve a file descriptor. I'll keep looking at it, though.

Is there some kind of test suite you use?

trapexit · 2017-04-04T14:26:10Z

Unfortunately not. It's been a TODO for quite a while. It's not all that easy given dealing with the filesystem is nothing but side effects and relying on state held by the fuse library and OS. Would need to mock a lot of things.

I have just added basic Travis-CI integration but that'd only confirm building works right now. (Which for OSX it doesn't in the main branch but if you rebase to master it'll build for you.)

ahknight · 2017-04-04T16:03:24Z

Found the source of the descriptor problem. OS X doesn't like AT_FDCWD in fcntl, but has no problem with opendir(".")/dirfd() to get the same value. ಠ_ಠ

I'll poke around some more.

As the position argument isn’t carried all the way down resource forks won’t work properly (though it easily could be, that would change the internal API for everyone).

OSX doesn’t like AT_FDCWD in it’s fcntl, but getcwd does the job. Also fixed an issue with absolute paths in the process.

After running Clang’s analyzer on it there were quite a few places where there were mis-matched store sizes and signs. I updated them so that the underlying code more closely-matches the OS APIs and the type change happens at the FUSE connection. In this way, if FUSE ever updates the API to actually match the APIs of modern OSes, it’s a fix in one place. Also, I updated read/write to return the total read/write value the OS returned rather than blindly return the request count.

Finder copies are broken without this argument, even if it’s fixed to 0. Upside: it reduces the number of times we platform-check in the xattr code.

ahknight · 2017-04-05T20:11:15Z

Fixed the issues with the missing API (wrote a version of it) and fixed xattrs and Finder copies as well.

A side effect is that the xattr APIs in mergerfs now have a position argument to them, but it's immediately defaulted to zero for non-Apple platforms and omitted in the final call to the OS.

Every other solution I looked at just filled the code with tons of ifdefs and/or foo_osx.icpp files for very small API changes.

trapexit · 2017-04-05T20:18:43Z

Oops. I've been refactoring your last commit into a more abstract interface and aligned to the overall style/layout. Let me know when you're finished and I'll tweak it if necessary.

If you'd like you can give me access to the PR.

trapexit · 2017-04-05T21:06:45Z

src/fs_clonefile.cpp

    }

-  return count;
+  return nwritten;


This shouldn't make a difference so long as count > 0 and it's a successful write/s but I see now that if count == 0 then it won't behave correctly even with your change. I'll need to fix that.

It shouldn't, no. But while making hfsinspect I ran into this issue a few times. There was some kind of situation where the read returned early but I wasn't at the end of the file/device yet and the next read kept going. So, since then, I've just always returned the real count so that client apps' counters are accurate and can handle that.

I get that for the outer function (copyfile_rw) but for writen the only possible bug should be that it doesn't accept zero for byte length. writen should not return till all bytes passed in are written or an error.

I think the issue is actually in copyfile_rw in which a zero return from read isn't handled properly. Should exit the loop. This is a generic bug. I'll probably fix that separately.

I think of it as a data pipe. The OS gives us a total of the bytes written and then we hand that back to FUSE to deliver to the application. If anything goes wrong along the way then returning a fixed value might lead to odd things, especially for a partial write (it's not a fail because data changed, but it's not a success because the whole request wasn't completed). That'd be my only concern.

This function has nothing to do with FUSE. It's for copying data between to underlying drives.

A write, according to standards, does not return 0 unless it's asked to write 0. It can return from 1 to count or -1. That's why writen exists. To make sure that the whole buffer is written.

The problem is the outer function. It should exit on EOF but write should never return anything but 1 - bufsize or -1. And writen should manage being passed a zero bufsize.

ahknight · 2017-04-06T12:49:31Z

The box to allow edits is checked, so feel free to add on here.

ahknight · 2017-04-06T14:27:31Z

The current version should be good. I've tested it with "rsync -aHAX" and with Finder copies and it checks out (I even mixed HFS+ and APFS-CI backing drives).

When you have it looking how you want it, let me know and I'll run those tests again.

trapexit · 2017-04-06T14:55:48Z

Sure. Thanks for the effort. My one concern though is the copying of xattrs. You've hardcoded XATTR_SHOWCOMPRESSION and while I'm not positive yet... from my random readings it appears that you'd need to do with and without it to get the compressed resource forks and more traditional xattrs.

Right now I'm kindof leaning toward having xattr disabled on OSX for a first patch and focus on getting it right later.

…o mac-updates

trapexit · 2017-04-06T15:22:02Z

src/fs_base_futimesat_osx.icpp

@@ -64,6 +64,7 @@ namespace fs
            const char           *path,
            const struct timeval  times[2])
  {
+    int rv;


I'm watching Travis. If I commit something which breaks I'll fix it.

ahknight · 2017-04-06T16:25:06Z

For the xattrs, the Finder uses them a lot for normal operation on the files (things like labels, and display preferences) and other apps keep metadata in them on a semi-regular basis. Not having them limits the usefulness of the resultant mount. :\

The different API with FUSE and the OS is a small pain, but I don't think there's a clean way around having this FS be a superset of functionality in order to pass the right data down.

You're right about the compression flag; that was in there for a test and I missed it. The consumer of the API will pass that if it's expected and there will be side effects if it's there without their knowing/expecting.

…as a size_t)

Clients should get the xattrs they actually asked for, after all.

trapexit · 2017-04-06T16:43:54Z

There are two xattr uses. One is the FUSE interface. The other is the clonepath and clonefile components. Until the clone'ing usage has to be figured out else the copy will be incomplete.

ahknight · 2017-04-06T17:01:03Z

For OSX, if you're copying a file from one backing store to another, use copyfile(3) with COPYFILE_ALL. The OS will handle all the details for you, including xattrs, resource forks, Finder info, etc.

https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man3/copyfile.3.html

trapexit · 2017-04-06T17:05:41Z

Then all fs::xattr stuff used by copyfile aren't needed for Mac. I'll take a look. Thanks.

trapexit · 2017-04-06T23:26:30Z

There isn't a clear separation in these commits. The type stuff, bug fixes, and osx changes.

I'm going to go through it all and pick out the general fixes and abstractions and put them in the main branch first. Leave out the type stuff (what did you run to discover them? clang's c++ analyzer isn't reporting anything.). Then rebase this branch and focus solely on osx changes.

briankendall

Found a potential bug

briankendall · 2017-04-13T05:31:18Z

src/fs_base_setxattr.hpp

    return (errno=ENOTSUP,-1);
-#endif
+  #elif __APPLE__
+    return ::setxattr(path.c_str(),name,value,size,position,flags & XATTR_NOFOLLOW);


Shouldn't flags & XATTR_NOFOLLOW actually be flags | XATTR_NOFOLLOW? I think using bitwise AND means that it will always be 0 as flags will never have the XATTR_NOFOLLOW bit set, effectively discarding the value of flags.

There's a few other similar places where bitwise AND is used where it probably should be a OR.

Yup. Should be ORed.

briankendall · 2017-07-22T19:00:07Z

Any update on this PR? I'm quite interested in trying this out in macOS. If not, is there a particular commit in the PR that I could compile and try out, like the most recent one?

trapexit · 2017-07-22T19:55:50Z

This PR was incomplete and the code has changed quite a bit since it was created. libfuse on OSX is different from Linux (and other platforms) and since I embedded libfuse into mergerfs to simplify installation and normalize behavior its now more complicated to port.

I now have access to an OSX machine but I'm not sure when I'll have time to work on working on a port.

briankendall · 2017-07-24T20:03:13Z

Would it be possible or at all advisable for me to try compiling using the latest commit in this PR's branch?

trapexit · 2017-07-24T20:24:13Z

You can obviously do whatever you like but as I recall this PR was incomplete and IIRC had some bugs.

briankendall · 2018-01-23T20:54:10Z

Just checking in again to see if there's any progress on this. @ahknight is this still something you're working on?

trapexit · 2018-01-23T21:13:39Z

Sorry, I've not had the time to look into this.

briankendall · 2020-02-27T07:02:10Z

I have a renewed interest in trying to get mergerfs working in macOS and was thinking of seeing if I could port it.

I was going through the up-to-date codebase to try and get a sense of what's involved. The way certain low level functions are factored out makes this easier in comparison to when @ahknight was working on it.

But I can see how it's more difficult now that libfuse is included in the project. There's a few places where you're reaching into fuse's internal structures (e.g. fuse_config_set_attr_timeout and fuse_config_get_attr_timeout) and of course those functions aren't available in osxfuse. And osxfuse differs in a few places compared to libfuse; namely there are some constants mergerfs refers to that are missing from osxfuse, and osxfuse is missing some member variables such as fuse_file_info.auto_cache.

If it's not too much trouble, could you share any thoughts on how to proceed?

trapexit · 2020-02-27T11:02:00Z

It's going to be a lot of trouble. Unless OSXFuse is very similar in protocol... you aren't going to be able to just use the high level libfuse API. And my intent is to further hack away at libfuse. If the protocol is similar enough then it might be doable but it's not going to get more API friendly anytime soon. Even if I hadn't vendored libfuse I had intended to move to the low level API which isn't cross platform.

Not knowing anything about osxfuse I really can't comment further.

briankendall · 2020-02-27T19:22:29Z

Ah, I see now that some of the differences I pointed out are in fact significant changes you've made to libfuse, and so in order to port to mac it's necessary to make equivalent changes to osxfuse. That's certainly more work than I'm willing to take on right now.

It does look like osxfuse includes the low-level API, so if and when you transition mergerfs to that then maybe it'd be worth looking into a mac port again. I'd certainly love to see it on the mac platform as this seems to be the most full featured union filesystem currently available.

trapexit · 2020-02-27T19:42:30Z

It's a big change to move to the low level API. My plan is to slowly take out or collapse the abstractions in libfuse to get it down to the LLAPI or maybe even replace libfuse all together. That's why I'm mentioning the protocol and setup. As I understand osxfuse is a fork of the original libfuse so I wouldn't think the protocol has changed too much. Maybe those changes could be incorporated into mergerfs. I should probably just take a look at the osxfuse code.

ahknight added 4 commits April 5, 2017 14:02

Get macOS working again.

d9ae6df

Get xattrs working on macOS.

1aea1d4

As the position argument isn’t carried all the way down resource forks won’t work properly (though it easily could be, that would change the internal API for everyone).

Fix issue with AT_FDCWD

39b7772

OSX doesn’t like AT_FDCWD in it’s fcntl, but getcwd does the job. Also fixed an issue with absolute paths in the process.

ahknight force-pushed the mac-updates branch from 3f7e055 to d1aec14 Compare April 5, 2017 19:03

ahknight added 2 commits April 5, 2017 14:04

Clean up some cpp logic for __APPLE__

6cfd4d5

Adding position arg to xattr APIs

59ece5f

Finder copies are broken without this argument, even if it’s fixed to 0. Upside: it reduces the number of times we platform-check in the xattr code.

trapexit reviewed Apr 5, 2017

View reviewed changes

abstract futimesat and getting of atime/mtime

3b0d371

trapexit force-pushed the mac-updates branch from 1f9080e to 3b0d371 Compare April 6, 2017 14:29

Fix some compile errors for Travis

bca8404

trapexit added 2 commits April 6, 2017 11:18

restructure osx fallocate

b0fb518

Merge branch 'mac-updates' of http://github.com/ahknight/mergerfs int…

a00dc9b

…o mac-updates

trapexit reviewed Apr 6, 2017

View reviewed changes

tweak types

6a67a92

ahknight added 2 commits April 6, 2017 11:40

Silencing an analyzer notice (strlcat returns the size of the string …

3b2fa95

…as a size_t)

Removed stray xattr flag

25fe57f

Clients should get the xattrs they actually asked for, after all.

handle 32bit inode sizes

d0fa9ef

trapexit force-pushed the mac-updates branch from 151bd4a to d0fa9ef Compare April 6, 2017 20:25

remove unnecessary futimesat abstraction

cf567f1

briankendall reviewed Apr 13, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mac updates #384

Mac updates #384

ahknight commented Apr 4, 2017

trapexit commented Apr 4, 2017

ahknight commented Apr 4, 2017

trapexit commented Apr 4, 2017 •

edited

Loading

ahknight commented Apr 4, 2017

ahknight commented Apr 5, 2017

trapexit commented Apr 5, 2017

trapexit Apr 5, 2017

ahknight Apr 6, 2017

trapexit Apr 6, 2017 •

edited

Loading

ahknight Apr 6, 2017

trapexit Apr 6, 2017

ahknight commented Apr 6, 2017

ahknight commented Apr 6, 2017

trapexit commented Apr 6, 2017

trapexit Apr 6, 2017

ahknight commented Apr 6, 2017

trapexit commented Apr 6, 2017

ahknight commented Apr 6, 2017

trapexit commented Apr 6, 2017

trapexit commented Apr 6, 2017

briankendall left a comment

briankendall Apr 13, 2017 •

edited

Loading

trapexit Apr 13, 2017

briankendall commented Jul 22, 2017

trapexit commented Jul 22, 2017

briankendall commented Jul 24, 2017

trapexit commented Jul 24, 2017

briankendall commented Jan 23, 2018

trapexit commented Jan 23, 2018

briankendall commented Feb 27, 2020 •

edited

Loading

trapexit commented Feb 27, 2020 •

edited

Loading

briankendall commented Feb 27, 2020

trapexit commented Feb 27, 2020

Mac updates #384

Are you sure you want to change the base?

Mac updates #384

Conversation

ahknight commented Apr 4, 2017

trapexit commented Apr 4, 2017

ahknight commented Apr 4, 2017

trapexit commented Apr 4, 2017 • edited Loading

ahknight commented Apr 4, 2017

ahknight commented Apr 5, 2017

trapexit commented Apr 5, 2017

trapexit Apr 5, 2017

Choose a reason for hiding this comment

ahknight Apr 6, 2017

Choose a reason for hiding this comment

trapexit Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

ahknight Apr 6, 2017

Choose a reason for hiding this comment

trapexit Apr 6, 2017

Choose a reason for hiding this comment

ahknight commented Apr 6, 2017

ahknight commented Apr 6, 2017

trapexit commented Apr 6, 2017

trapexit Apr 6, 2017

Choose a reason for hiding this comment

ahknight commented Apr 6, 2017

trapexit commented Apr 6, 2017

ahknight commented Apr 6, 2017

trapexit commented Apr 6, 2017

trapexit commented Apr 6, 2017

briankendall left a comment

Choose a reason for hiding this comment

briankendall Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

trapexit Apr 13, 2017

Choose a reason for hiding this comment

briankendall commented Jul 22, 2017

trapexit commented Jul 22, 2017

briankendall commented Jul 24, 2017

trapexit commented Jul 24, 2017

briankendall commented Jan 23, 2018

trapexit commented Jan 23, 2018

briankendall commented Feb 27, 2020 • edited Loading

trapexit commented Feb 27, 2020 • edited Loading

briankendall commented Feb 27, 2020

trapexit commented Feb 27, 2020

trapexit commented Apr 4, 2017 •

edited

Loading

trapexit Apr 6, 2017 •

edited

Loading

briankendall Apr 13, 2017 •

edited

Loading

briankendall commented Feb 27, 2020 •

edited

Loading

trapexit commented Feb 27, 2020 •

edited

Loading