Refactored lzc_send_resume_redacted with a thread. #11992

rincebrain · 2021-05-03T08:02:59Z

(Creating this PR to get feedback on whether people think this approach is remotely reasonable, or I should abandon it.)

(The FreeBSD test failures don't seem to have anything to do with this change, they just failed to install a package before ever touching this code.)

Motivation and Context

After the changes documented in #11445, writing to certain files
errored out with an opaque error message.

Description

Unfortunately, the path of least resistance to fixing this seemed to be sticking a pipe
into the path of zfs send, along with a thread to copy data to stdout.

As said above, this is mostly to get feedback on whether something like this would remotely be accepted or I should throw it out.

Known caveats include

~~Definitely doesn't work on FreeBSD at the moment due to using splice(); going to correct that next.~~ Works now.
Definitely a better place to put __do_send_output than ~~right above the call.~~ libzfs_core.h
Did not refactor lzc_send_space_resume_redacted the same way yet.
Some of the variable names could definitely be more descriptive.
The buflen used for splice/read+write is entirely pulled out of thin air.

How Has This Been Tested?

Tested on Debian buster and bullseye, with both tiny snapshots and letting a send of a 12T snapshot run to at least 600G.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

pcd1193182

I hate that this is necessary to fix the problem, but I wouldn't object to it landing as long as it received appropriate performance testing first.

Do we know that this is necessary? Would it be possible to submit a patch upstream to handle the "convoluted semantics" associated with FSes that export both write() and write_iter() options? What about the semantics are convoluted, and how hard would it be to fix it? Obviously this is a task that you may not want to take on yourself, but I think it's worth considering if that's a better route to fixing our issue than a ZFS-specific patch.

rincebrain · 2021-05-04T21:48:55Z

I considered writing a patch to fix that instead, since my understanding is that it'd be a pretty simple patch, but quickly concluded that it would just end in tears once they found out why I was submitting it.

edit: I was misunderstanding - tl;dr it is a very simple patch, but satisfying everyone's concerns would be quite difficult.

(Included for anyone who's trying to understand the context)

Longer:
The problem occurs because null_fops, the struct of file operations on /dev/null, has both the "write" and "write_iter" functions defined, and part of the patch explicitly rejects writing to things with both, because "it's complicated".

The original version of the patch dropped the "write" version of the function, but according to the v2 patch's description, that part was removed as it caused a performance regression in writing to /dev/null, and here we are.

So if anyone wanted to write a patch, they'd probably have to do one of:

special-casing /dev/null in those checks (I would expect this to get laughed off LKML, though I am just an outsider peering in)
drop the "write" call and improve the performance of write_iter so there's no performance regression (good luck; the "write" version of the call is just "return [bytecount];")
drop the rejection logic for functions with both write_iter and write (absent a strong argument for why this is reasonable, I would expect this to go as well as the first option)
~~the option I'm most curious about is "what breaks if you drop the write_iter function", and will be experimenting with this when I get tired of debugging ZTS failures in the above patch.~~ I was misunderstanding what the function in question checked. So your only option is really 3, which I think would be safe here since all in-kernel filesystems have dropped their use of write, but I doubt would get acceptance upstream.

nabijaczleweli · 2021-05-05T15:51:02Z

lib/libzfs_core/libzfs_core.c

+{
+	sendargs_t *args = (sendargs_t *)voidargs;
+	sigset_t sigs;
+	int buflen = 131072;


To make this less out-of-thin-air, you might want to steal some logic from libzfs_set_pipe_max() and (a) bump the pipe and (b) transfer in pipe-sized chunks.

Doesn't help on other platforms (according to a forum, FreeBSD pipes under no memory pressure start at 16k and grow up to 64k, or start at 4k and don't grow, or all pipes are shrunk down to 4k if possible, which is a great time), but.

I yanked the logic from libzfs into a new function in libzfs_core so it could be the same code called from both locations.

I find it pretty ugly to do, tbh, but we'll see if anyone else agrees with me...the alternative was duplicating the logic either inline or in a separate function in libzfs_core anyway, so this seemed least bad.

lib/libzfs_core/libzfs_core.c

rincebrain · 2021-05-07T03:01:38Z

Well that's inconvenient. It seems like it might be infeasible to reproduce the exact same error code returns in all cases.

I'm currently diving into the failure of the pyzfs/pyzfs_unittest test, and there are some times where the ioctl got EINTR and the thread got EPIPE, and the test expects EPIPE, and other times where the same thing happens, and the test expects EINTR. (test_send_to_broken_pipe and _2, respectively).

Is it a dealbreaker if I can't make this reproduce the exact error codes in all cases? (And if not, is there any standard guidance for how to decide what the "correct" behavior should be, or just dealer's choice? ex. I special-cased not returning EBADF if the thread hit it because it fixed some of the other test cases, but that breaks some of the pyzfs_unittest cases. Short of that case, though, it's still always going to be erroring when errors are expected and not when not, it just might be a different error code.)

rincebrain · 2021-05-07T05:03:15Z

Or I could be bad at error log reading comprehension and it could be straightforward to fix, that works too.

rincebrain · 2021-05-07T15:05:04Z

Okay, the pyzfs/pyzfs_unittest failure on the FBSD test bots (test_send_bad_fd), I don't see a way to fix at the moment, because it's expecting a return of EBADF, while the send ioctl is returning 0 and the send thread is returning EPIPE, so even if I wanted to special case it, I'd break other cases that are expecting EPIPE. (I'm seeing another test failure in pyzfs/pyzfs_unittest just on FBSD, in test_snaprange_space_same_snap, but since that's also happening on vanilla git too I'm going to write it off as an oddity of my setup and look at it later if I get bored.)

Assuming I'm not completely misreading things and there really isn't an obvious solution, what's the right thing to do here? Changing that one test to permit EPIPE seems like the least painful solution, but is there a reason not to decide to allow that?

Absent anyone's objections or discovering new ZTS failures locally, I think the next commit I push is going to include a change to pyzfs_unittest to do just that.

behlendorf

It's really unfortunate this is needed, but I think it's something we could live with as long as it doesn't have a significant impact on performance.

lib/libzfs_core/libzfs_core.c

rincebrain · 2021-05-09T02:53:39Z

Regarding the test failures:

The failure of removal/removal_condense_export on FreeBSD 13 seems to just be existing flakiness of that test on FBSD, as I can reproduce that failure on a machine running unmodified git master on my FreeBSD 12 VM (and not at all on my 13 VMs, either with git master or this branch).
The failure of redundancy/redundancy_draid_spare1 on FreeBSD 12, I haven't been able to reproduce at all yet, and not for lack of trying. My guess, similar to the below testcase, would be that the VM happened to be slow when running that particular testcase, and timed out.
The failure of fault/auto_spare_multiple on Ubuntu 18.04, I haven't been able to reproduce yet in hundreds of runs between my Ubuntu 18.04, Debian bullseye, and Ubuntu 20.04 VMs. Reading the test, though, it seems A) extremely unlikely to be related to any changes made here, and B) to likely be just a VM running too slowly and timing out? Maybe I should try the slowest VM I can setup...or I've got an RPi 1 around here somewhere...

edit to add: I finally reproduced that specific failure of fault/auto_spare_multiple on Ubuntu 18.04 + vanilla git master, so I'm not going to be concerned about it failing going forward unless someone else disagrees.

The current zloop crash also happens on the same pool files with vanilla git master, and the relevant pool files seem to have originated with a ztest run. Since AFAICT ztest doesn't involve send/recv, I'm reasonably confident its existence is unrelated to these changes, but I welcome someone more knowledgable telling me I'm completely wrong.

nabijaczleweli · 2021-05-09T03:11:57Z

include/libzfs_core.h

@@ -137,6 +137,20 @@ int lzc_wait_fs(const char *, zfs_wait_activity_t, boolean_t *);

 int lzc_set_bootenv(const char *, const nvlist_t *);
 int lzc_get_bootenv(const char *, nvlist_t **);
+
+void* __do_send_output(void* voidargs);


this doesn't need to have this linkage and visibility, does it?

I absolutely agree - I stuck it in there quickly back in the initial commit, as none of the other functions in libzfs_core had prototypes inlined in libzfs_core.c, and have been hoping someone would suggest a better place for both the function definition and prototype ever since.

Absent anyone having proposed brighter ideas, I'll just migrate it to being at the top of libzfs_core.c. If you have a better suggestion for where to put it than that, I'm all ears.

nabijaczleweli · 2021-05-09T03:12:24Z

include/libzfs_core.h

@@ -137,6 +137,20 @@ int lzc_wait_fs(const char *, zfs_wait_activity_t, boolean_t *);

 int lzc_set_bootenv(const char *, const nvlist_t *);
 int lzc_get_bootenv(const char *, nvlist_t **);
+
+void* __do_send_output(void* voidargs);
+#ifdef __linux__


I feel like this definitely shouldn't pollute the global namespace

Seems reasonable. As above, I'll just land it in the top of libzfs_core.c - and if that seems unreasonable, the only other place I can see it going is inside the function using it.

nabijaczleweli · 2021-05-09T11:33:12Z

lib/libzfs_core/libzfs_core.c

@@ -96,6 +97,18 @@ static int g_fd = -1;
 static pthread_mutex_t g_lock = PTHREAD_MUTEX_INITIALIZER;
 static int g_refcount;

+void* __do_send_output(void* voidargs);


this still has external linkage and you declaring this name yields UB (starts with two underscores); is there any reason you can't (drop the declaration and) mark it static?

Absolutely no reason I'm aware of. So I've done it and pushed.

Thanks!

nabijaczleweli · 2021-05-09T11:40:19Z

lib/libzfs_core/libzfs_core.c

@@ -645,6 +658,90 @@ lzc_send_resume(const char *snapname, const char *from, int fd,
 	    resumeoff, NULL));
 }

+int


this can and will overflow

nabijaczleweli@tarta:~/uwu$ cat max.c #include <stdio.h> int lzc_get_pipe_max() { unsigned long max_psize = 65536; #ifdef __linux__ FILE *procf = fopen("/proc/sys/fs/pipe-max-size", "re"); if (procf != NULL) { if (fscanf(procf, "%lu", &max_psize) <= 0) { // safe default max_psize = 65536; } fclose(procf); } #endif return (max_psize); } int main() { int ret = lzc_get_pipe_max(); printf("%d\n", ret); } nabijaczleweli@tarta:~/uwu$ cc max.c -omax nabijaczleweli@tarta:~/uwu$ echo 2147483649 | sudo tee /proc/sys/fs/pipe-max-size 2147483649 nabijaczleweli@tarta:~/uwu$ ./max -2147483648

why not just return an unsigned long (or whichever type makes sense for this)?

Very good point; I overlooked that it was using unsigned longs when I factored out the logic. Corrected.

behlendorf · 2021-05-10T22:44:05Z

lib/libzfs_core/libzfs_core.c

+		fcntl(infd, F_SETPIPE_SZ,
+		    max_psize);
+#endif
+}


It's awkward to split the lzc_get_pipe_max and lzc_set_pipe_max functionality across libraries. I don't see any reason we couldn't move the libzfs_set_pipe_max() call from zfs_receive() in to the recv_impll() function in libzfs_core. Then this code could remain private, just in libzfs_core instead of libzfs.

There; was that change (35bc51c) something like what you had in mind?

Yup, that's what I had in mind. Let just make sure to remove libzfs_set_pipe_max from libzfs_impl.h and make sure the replacement lzc_*_pipe_max functions are static. We should be able to keep those functions private.

nabijaczleweli · 2021-05-13T18:26:47Z

lib/libzfs/os/linux/libzfs_sendrecv_os.c

-
-#ifndef F_GETPIPE_SZ
-#define	F_GETPIPE_SZ (F_GETLEASE + 7)
-#endif /* F_GETPIPE_SZ */

 void
 libzfs_set_pipe_max(int infd)


any reason to keep this unused file?

Nope. I thought I had git rm'd it along with the Makefile update.

nabijaczleweli · 2021-05-13T18:28:23Z

lib/libzfs/os/freebsd/libzfs_compat.c

@@ -39,12 +39,6 @@
 #define	ZFS_KMOD	"openzfs"
 #endif

-void
-libzfs_set_pipe_max(int infd)


you should should probably remove the declaration from libzfs_impl.h if you're removing all definitions

Good idea. Done.

nabijaczleweli · 2021-05-13T18:29:30Z

lib/libzfs_core/libzfs_core.c

+void
+lzc_set_pipe_max(int infd)
+{
+#ifdef __linux__


something like the "FreeBSD automatically resizes" comment from the libzfs impl would be good here as well

actually, a comment along the lines of "FreeBSD grows automatically to 64k" on the unsigned long max_psize = 65536; line above would kill both wrens in one twist maybe?

Sure, done.

nabijaczleweli · 2021-05-13T18:31:13Z

The original libzfs_set_pipe_max() was only declared in libzfs_impl.h and exported due to a (bad) default. Is there a good reason to export lzc_[gs]et_pipe_max()? (AFAICT all you'd need to do would be to mark them static, since they're only used in one TU)

nabijaczleweli · 2021-05-13T18:42:27Z

lib/libzfs_core/libzfs_core.c

+	FILE *procf = fopen("/proc/sys/fs/pipe-max-size", "re");
+
+	if (procf != NULL) {
+		if (fscanf(procf, "%lu", &max_psize) <= 0) {


You're parsing exactly one field here, so any failure means that it remains unchanged. You can drop the if entirely and just do (void) fscanf();, especially since you're setting it back to the initialised value.

Seems reasonable. Sure.

Neat, either I did it incorrectly, or no I can't, as I've not seen that much red in the CI in a while.

I knew --enable-debug turned on -Werror, but I'm surprised I didn't see the warning that's getting promoted to an error without it. Oh well, simple enough...

edit: Oh, I see from the CI that this builds fine on Debian 10. So it's not just me being blind, it really didn't report it. Exciting.

ah, yeah, in that case it'll probably need a dummy variable (and some convincing, potentially?), or to just be reverted to the conditional

Well, the way I put it back worked on Centos Stream 8 and Debian 10, so hopefully it doesn't get rejected by Ubuntu 18.04 or something.

rincebrain · 2021-06-05T12:53:35Z

Okay, fortunately I went through to rebase all my PRs; I completely forgot about some of these outstanding comments.

rincebrain · 2021-06-06T09:38:12Z

Okay, I'm no longer confused about what's going on with the failing pyzfs test case.

You would not think that handing an invalid fd could give back EBADF sometimes and EPIPE others, right?

Well, the precise way the test case functions is:

        with tempfile.TemporaryFile() as tmp:
            bad_fd = tmp.fileno()

        with self.assertRaises(lzc_exc.StreamIOError) as ctx:
            lzc.lzc_send(snap, None, bad_fd)

For those of you unversed in Python - the key point is that the tempfile (and, consequently, its fd which is copied to bad_fd) goes poof when the with statement ends - e.g. tmp.close() happens.

So let's say the now-closed file got fd 5. We call lzc_send(), then on first write to fd 5 in the old send code, EBADF pops out, tada.

But what if you, say, did pipe2() or fopen() or ... before doing a write? Well, it'll assign the lowest unused fd (twice, for pipe2())- which is now 5.

So if we did pipe2() to get a pair of fds back, we now have fds 5 and 6 as ends of a pipe, and an "output" fd value of 5. So we were writing to our own pipe, presumably the wrong end (since I'd expect it to just infinitely loop if it was the correct end), until we eventually errored out with EPIPE.

I'm going to add a quick check at the head of the function after the pipe2() to return EBADF if either of the pipefds equals the out fd, but it feels kind of gross, as I'll be surprised if this function or its wrapper variants ever promised "will not open any fds".

After the changes documented in openzfs#11445, writing to certain files errored out with an opaque error message. Unfortunately, the path of least resistance to fixing this seemed to be sticking a pipe into the path of zfs send, along with a thread to copy data to stdout. Signed-off-by: Rich Ercolani <rincebrain@gmail.com>

rincebrain · 2021-06-08T23:01:54Z

Okay, so I know of two possible things that need to be resolved:

Likewise refactor lzc_send_space_resume_redacted
Some degree of benchmarking

I can figure out the former, though I'll basically just be stealing examples from ZTS to test it; for the latter, does anyone have any examples of what they'd like to see?

I can run time zfs send [...] > /dev/null (or | cat > /dev/null, as the case may be) until the cows come home, but I don't personally have any NVMe pools or huge spinning rust pools to test with, or dedup, or large encrypted datasets, or [...], so knowing what kind of data people would like to see before feeling comfortable with this would be helpful.

By introducing lzc_send_wrapper() and routing all ZFS_IOC_SEND* users through it, we fix a Linux 5.10-introduced bug (see comment) This is all /transparent/ to the users API, ABI, and usage-wise, and disabled on FreeBSD and if the output is already a pipe, and transparently nestable (i.e. zfs_send_one() is wrapped, but so is lzc_send_redacted() it calls to ‒ this wouldn't be strictly necessary if ZFS_IOC_SEND_PROGRESS wasn't strictly denominational w.r.t. the descriptor the send is happening on) Supersedes openzfs#11992 Closes openzfs#11445 Co-authored-by: Rich Ercolani <rincebrain@gmail.com> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

behlendorf · 2022-03-08T17:38:14Z

Closing. A version of this was merged as #13133.

rincebrain force-pushed the pipetest branch from c0bf4c3 to 27139e1 Compare May 3, 2021 08:45

ahrens requested a review from pcd1193182 May 3, 2021 15:31

rincebrain force-pushed the pipetest branch from 27139e1 to d96e2ad Compare May 3, 2021 21:28

pcd1193182 reviewed May 4, 2021

View reviewed changes

nabijaczleweli reviewed May 5, 2021

View reviewed changes

lib/libzfs_core/libzfs_core.c Outdated Show resolved Hide resolved

behlendorf added the Status: Design Review Needed Architecture or design is under discussion label May 6, 2021

behlendorf reviewed May 7, 2021

View reviewed changes

lib/libzfs_core/libzfs_core.c Show resolved Hide resolved

lib/libzfs_core/libzfs_core.c Outdated Show resolved Hide resolved

lib/libzfs_core/libzfs_core.c Outdated Show resolved Hide resolved

nabijaczleweli reviewed May 9, 2021

View reviewed changes

behlendorf reviewed May 10, 2021

View reviewed changes

rincebrain mentioned this pull request May 11, 2021

Fix endianness issues with zstd #12022

Merged

13 tasks

nabijaczleweli reviewed May 13, 2021

View reviewed changes

ahrens assigned jwk404 May 21, 2021

rincebrain force-pushed the pipetest branch from 43ed7c9 to a73c040 Compare June 5, 2021 12:49

rincebrain force-pushed the pipetest branch 2 times, most recently from 033f8fd to 1c2c11e Compare June 5, 2021 13:30

rincebrain force-pushed the pipetest branch from 1c2c11e to cb783fb Compare June 6, 2021 10:57

rincebrain mentioned this pull request Jun 6, 2021

panic on FreeBSD 13-STABLE - VERIFY3(0 == vdev_obsolete_sm_object(vd, &obsolete_sm_object)) failed (0 == 97) #12200

Open

rincebrain marked this pull request as ready for review June 7, 2021 02:35

behlendorf mentioned this pull request Oct 8, 2021

Fail invalid incremental recursive send gracefully #12533

Merged

13 tasks

nabijaczleweli mentioned this pull request Feb 21, 2022

zfs send wrapper thread #13133

Closed

13 tasks

behlendorf closed this Mar 8, 2022

Refactored lzc_send_resume_redacted with a thread. #11992

Refactored lzc_send_resume_redacted with a thread. #11992

Conversation

rincebrain commented May 3, 2021 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

pcd1193182 left a comment

Choose a reason for hiding this comment

rincebrain commented May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rincebrain commented May 7, 2021

rincebrain commented May 7, 2021

rincebrain commented May 7, 2021

behlendorf left a comment

Choose a reason for hiding this comment

rincebrain commented May 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabijaczleweli May 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabijaczleweli May 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabijaczleweli May 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabijaczleweli commented May 13, 2021 • edited Loading

nabijaczleweli May 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rincebrain Jun 5, 2021 • edited Loading

Choose a reason for hiding this comment

nabijaczleweli Jun 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rincebrain commented Jun 5, 2021

rincebrain commented Jun 6, 2021

rincebrain commented Jun 8, 2021

behlendorf commented Mar 8, 2022

rincebrain commented May 3, 2021 •

edited

Loading

rincebrain commented May 4, 2021 •

edited

Loading

rincebrain commented May 9, 2021 •

edited

Loading

nabijaczleweli May 9, 2021 •

edited

Loading

nabijaczleweli May 9, 2021 •

edited

Loading

nabijaczleweli May 13, 2021 •

edited

Loading

nabijaczleweli commented May 13, 2021 •

edited

Loading

nabijaczleweli May 13, 2021 •

edited

Loading

rincebrain Jun 5, 2021 •

edited

Loading

nabijaczleweli Jun 5, 2021 •

edited

Loading