copy_file_range() support for cat(1)

[mm]

Summary

With cat(1) utilizing copy_file_range() we can make use of ZFS block cloning with the cat utility the same way as we do with cp(1).

[asomers]

Could you please regenerate the diff with more context? Either use the arcanist-php82 CLI, or generate the diff with diff -U 9999.

[mm]

In D40882#930509, @asomers wrote:

Could you please regenerate the diff with more context? Either use the arcanist-php82 CLI, or generate the diff with diff -U 9999.

I have re-created the diff.

[asomers]

LGTM. I still don't know if it's a good idea to use ZFS block-cloning, but you could take advantage of this change with, say, the NFS client. BTW, have you considered adding copy_file_range support to install(1) ?

[mm]

In D40882#930570, @asomers wrote:

LGTM. I still don't know if it's a good idea to use ZFS block-cloning, but you could take advantage of this change with, say, the NFS client. BTW, have you considered adding copy_file_range support to install(1) ?

Well, with ZFS block cloning we still have a chicken-and-egg problem, we need broader testing. Btw. when someone else reviewed this too, we need an exp-run as this is an elementary tool.

[mm]

Update patch

[asomers]

bin/cat/cat.c

293

Other error codes might require a fallback, too. For example, ENOSYS. The safest thing to do would be to fallback by default and exit only for specific error codes. See D28959 .

296

Why "stdout" in the error message?

[rmacklem]

Btw, zfs_freebsd_copy_file_range() needs to use
vn_rlimit_fsizex() and not vn_rlimit_fsize(),
so that it copies right up to the rlimit.

I recall spotting this, but it seems to have gotten
lost in the block cloning nightmare.

bin/cat/cat.c

283–291

When fd == stdin, copy_file_range() will return
EINVAL, so this works.

However, I think it might be better to check for
fd != stdin before calling copy_file_range() so
that it does not depend on copy_file_range()
returning EINVAL for this case?

I'll leave it up to you.

296

Is there a missing "}" here or am I missing something?

[mm]

In D40882#930822, @rmacklem wrote:

Btw, zfs_freebsd_copy_file_range() needs to use
vn_rlimit_fsizex() and not vn_rlimit_fsize(),
so that it copies right up to the rlimit.

I recall spotting this, but it seems to have gotten
lost in the block cloning nightmare.

Yes, that is the kernel part. Luckily, we are here in userland.

bin/cat/cat.c

283–291

I would like to stay consistent and use the same in cp(1), cat(1) and install(1)

296

fixed

296

The error message should be the same like in raw_cat()

[mm]

bin/cat/cat.c

293

I don't see a ENOSYS return code in man copy_file_range(2).
About the other errors, retry on EIO, ENOSPC, etc. makes no sense and if we retry on EINTR that sounds even undesired to me.
I am took the the code path that was taken cp(1), just take a look into bin/cp/utils.c and look for copy_file_range()

[rmacklem]

Looks ok to me now. I agree that cp(1), cat(1) and any
other consumers of copy_file_range(2) in /usr/src should
follow the same convention w.r.t. falling back upon errors.

bin/cat/cat.c

293

I don't think ENOSYS will ever be returned
by copy_file_range(2). If an fs does not implement
it, vn_generic_copy_file_range() gets called instead.

[imp]

I think it's likely fine as is, but have a suggestion to make it even safer given cat's use in the build process.

bin/cat/cat.c

293

So ENOSYS is possible, if the kernel is too old.
To support *that* (which I kinda think we need to given cat is a build tool), you'll need to mask SIGSYS, either for the full run of the program (Easiest) or around this call (a bit of a pain, and likely not much safer). While we generally don't to a lot of forward/backward compat, this may be a time we should (and I'd recommend just masking (or ignoring) SIGSYS early in main, anything that would have been killed by SIGSYS I think is going to die when we check the return status for any of the even older, but still newly introduced calls... If I'm reading things right...
Though there is bootstrap cat too, which if we placed this behind would also handle things nicely for the light cat use in the build/install paths...

tl;dr: toss #ifndev BOOTSTRAP_CAT around this :)

[mm]

In D40882#930913, @imp wrote:

I think it's likely fine as is, but have a suggestion to make it even safer given cat's use in the build process.

In cp(1) we have no ENOSYS protection or wrapping arount SIGSYS. Is that fine so?

[mm]

Put new code into #ifndef BOOTSTRAP_CAT

[imp]

great!

[rmacklem]

If the return values are changed to ssize_t, it looks
fine to me.

Thanks for doing this, rick

bin/cat/cat.c

86

Probably should change this to
"static ssize_t in_kernel_copy(int);"

397

ret probably should be ssize_t, since that is what
copy_file_range() returns.

[mm]

Use ssize_t for copy_file_range() return