cp(1) could use sendfile

Review Request #1311 - Created Dec. 2, 2018 and updated

Information
Sebastian Wiedenroth
illumos-gate
Reviewers
general

Change to libcmdutils so that cp(1) uses the sendfile(3ext) interface.
This avoids copying the data between kernel and userspace.

My testing so far includes using this cp on trees with various file
sizes. I've tested that checksums still match. I've tested error handling
is unchanged when reaching the fs quota. I also booted into a system with this change.
I have not tested this with NFS because I currently don't have such a setup.

Issues

  • 2
  • 0
  • 1
  • 3
Description From Last Updated
By using sendfile(), you've changed the state in which fi will be left. sendfile() does not modify the file pointer ... Andy Fiddaman Andy Fiddaman
You still need to free srcbuf and targbuf here. I'd actually be tempted to add an err: label near the ... Andy Fiddaman Andy Fiddaman
Andy Fiddaman

libcmdutils:writefile() is also used by mv in some cases (cross-filesystem move for example) so that should be included in the tests.

Not important, but I'd prefer this be a global variable to allow easier manipulation with mdb.

By using sendfile(), you've changed the state in which fi will be left. sendfile() does not modify the file pointer of the input file unlike the previous code.

Do any consumers of writefile() care? I suspect not but you should check.

Why continue here if a signal is received?

You still need to free srcbuf and targbuf here.
I'd actually be tempted to add an err: label near the bottom of the function and consolidate

    (void) close(fi);
    (void) close(fo);
    if (S_ISREG(s2p->st_mode))
        (void) unlink(targbuf);
    if (srcbuf != NULL)
        free(srcbuf);
    if (targbuf != NULL)
        free(targbuf);
    return (1);

which appears three times in this function

Andrew Stormont

   

You should be able to send the entire file with a single call to sendfile(). Ditch the loop.

  1. Without the loop a user would not be able to cancel a cp process sending a very large file. It will appear hanging until everything has been written.

  2. Interesting. I sort of assumed that SIGINT would cause it to return EINTR.

Loading...