Illumos 4368 4369

[dweeezil]

4369 implement zfs bookmarks
4368 zfs send filesystems from readonly pools
Reviewed by: Christopher Siden [email protected]
Reviewed by: George Wilson [email protected]
Approved by: Garrett D'Amore [email protected]
Ported by: Tim Chase [email protected]

Porting notes:

Added spa_feat_refcount_cache_lock to struct spa to protext
spa_feat_refcount_cache since zfsonlinux doesn't have an atomic_swap_64
to update it.

[dweeezil]

If someone could give me a clue as to how to get atomic_swap_64 into spl (or zfs), I'd like to get rid of the mutex I added and switch the updating of spa_feat_refcount_cache back to using it.

[behlendorf]

@dweeezil You should be able to map atomic_swap_64 to Linux's atomic_xchg. Check out include/sys/atomic.h for the other atomic implementations. You're also going to need to add it to libspl on the zfs side if needs to be compiled in user space.

[dweeezil]

@behlendorf I think openzfs/spl#377 does the Right Thing.

[behlendorf]

@dweeezil Yes, openzfs/spl#377 looks correct. If you don't mind I'll add something equivalent in libspl and refresh your patch stack as a different pull request for review. This way we can avoid adding doing something Linux specific here.

[behlendorf]

@dweeezil I've merged #2529 after updating it to use atomic_swap_64 like Illumos and merging the needed spl support. I'd forgotten support for this was already added to libspl some time ago. That was a nice surprise.

b0bc7a8 Illumos 4370, 4371

However, making that change means you're going to need to refresh this pull request. You should no longer need to do anything Linux specific here such as add the spa_feat_refcount_cache_lock mutex.

I've spent a lot of time simply reviewing code over the last few days and I appreciate how much time you must have spent porting these changes. Thank you again for working on this!

[dweeezil]

@behlendorf I thought I had removed spa_feat_refcount_cache_lock already but now I need to check my patch stack . I'll get everything refreshed shortly with it removed. This stack has been very tricky to keep track of and I'm using a handful of my own tools/scripts to help me.

[dweeezil]

@behlendorf I see what's going on here. Unfortunately there must have been some "commit creep" or something of that sort. The need for a 64-bit atomic swap was introduced by the bookmark feature which is/was in dweeezil/zfs:illumos-4368-4369 (#2530) but apparently some of that code must have leaked into dweeezil/zfs:illumos-4370-4371 (#2529). It's not at all clear to me how that happened. I will get #2530 refreshed ASAP based on current master to make things right and will post another note here when it's ready (along with its commit SHA).

[behlendorf]

That helps explain things. I'll watch for the update so I can get it tested and merged. I'm hoping we can just work through you're entire stack over the next few weeks and get basically caught up.

[dweeezil]

@behlendorf This pull request has been updated as dweeezil/zfs@3be8308. I just ran a few tests on a build of that commit and it appears to work properly (including typical bookmark-using operations).

[dweeezil]

AFAIK, the rest of the stack is OK. I'm going to keep pressing forward now. I still have no idea how part of a later commit leaked into an earlier commit. I keep all the patches in "git am" format and replace the "diff" part of the patch file as required when I have to tweak a patch and use a simple script to re-apply all the "am" format files and force-push them to github. In any case, the dweeezil/zfs@3be8308 commit should hopefully represent a stable state corresponding to the illumos code base as of their issue 4369 (w.r.t. the bookmark feature).

[dweeezil]

@behlendorf I figured out what happened and it's not at all what I expected nor what I described above. The need for atomic_swap_64() was indeed added in the "4371 DMU code clean up" patch in illumos (illumos/illumos-gate@43466aae). The actual problem is that I removed spa_feat_refcount_cache_lock and associated code in the wrong branch. In other words, everything is OK now except that this pull request's branch (dweeezil:illumos-4368-4369) should not have any commentary in it regarding the need for atomic_swap_64(). As mentioned above, I have fixed the code in this branch so that it applies properly as of dweeezil/zfs@3be8308 (but it should have the commit comment updated).

[behlendorf]

@dweeezil OK, that makes sense and I think explains everything. I'll work on getting the bookmarks in next.

[dweeezil]

@behlendorf I just refreshed the rest of the branches. This is the refreshed stack:

$ git log HEAD ^da53684^ --oneline
1a22477 4897 Space accounting mismatch in L2ARC/zpool Reviewed by: Matthew Ahrens <[email protected]> Reviewe
c9a5151 Illumos 4881 - zfs send performance regression with embedded data
beaa026 Illumos 4390 - I/O errors can corrupt space map when deleting fs/vol
be444a7 Illumos 4757 - embedded blkptrs, 4913 - zfs release no space check
7917a37 Illumos 3835 zfs need not store 2 copies of all metadata
8177d52 zed needs libzfs_core
d1dbebc Illumos #4754, #4755.
1fcacb8 Illumos 4752
49f4660 Illumos #4374
da53684 Illumos 4368, 4369.