UNIX cp multithreaded implementation in C

Question

I recently finished a high school project of mine for a class revolving around creating my own multithreaded implementation of the "cp" terminal command in UNIX systems.

I am seeking for help by experienced people in C to help me make my code better, offering constructive criticism all while improving myself at the same time.

Specifically:

• Can my code become faster than it currently is?
• Are there any potential runtime errors in my code? ( I have tested it with multiple examples and all would work fine, I'm just trying to find the ones I didn't think of)
• Does my code have a good overall structure?

Points of interest:

src/crawler.c

#include "../headers/crawler.h"

int dircrawler(char* path, char* dest, int pathsize)
{
   // dirent variables and structures
   DIR* dp;
   struct dirent* entry;
   // buffers
   size_t bufsize;
   char *buf1, *buf2;
   buf1 = buf2 = NULL;
   struct cp_thr_args* args = NULL;

   if ((dp = opendir(path)) != NULL) {
      while ((entry = readdir(dp)) != NULL) {
         // skip over . and ..
         if (!(strcmp(entry->d_name, ".")) || !(strcmp(entry->d_name, "..")))
            continue;

         if (buf1 != NULL)
            free(buf1);
         if (buf2 != NULL)
            free(buf2);

         // printf("%s\n",entry->d_name);

         // buf1: absolute new path, buf2: relative destination path
         bufsize = snprintf(NULL, 0, "%s/%s", path, entry->d_name) + 1;
         mmalloc(&buf1, bufsize); // buf1
         snprintf(buf1, bufsize, "%s/%s", path, entry->d_name);

         bufsize = snprintf(NULL, 0, "%s%s/%s", dest, (path + pathsize - 1), entry->d_name) + 1;
         mmalloc(&buf2, bufsize); // buf2
         snprintf(buf2, bufsize, "%s%s/%s", dest, (path + pathsize), entry->d_name);

         if (entry->d_type == DT_DIR) {
            // make new directory in dest
            mkdir(buf2, 0755);
            if (dircrawler(buf1, dest, pathsize))
               closedir(dp);

         } else {
            // thread copying section
            int found = 1;
            while (1) { // ((Point of interest))
               // trying to find a non occupied thread
               // using status[]
               for (int i = 0; i < THREADCOUNT; ++i) {
                  if (status[i] == 1) {
                     cp_thr_init(&args, buf1, buf2, &status[i]);
                     pthread_create(&cpthr[i], NULL, cp_thr, (void*)args);
                     pthread_barrier_wait(&can_free);
                     cp_thr_free(args);
                     found = 0;
                     break;
                  }
               }
               if (!found)
                  break;
            }
         }
      }
      if (buf1 != NULL)
         free(buf1);
      if (buf2 != NULL)
         free(buf2);
      closedir(dp);
      return 0;
   }
}

src/threads.c

#include "../headers/threads.h"

int status[THREADCOUNT] = { 1, 1, 1, 1 };
pthread_mutex_t cp_lock = PTHREAD_MUTEX_INITIALIZER;

void* cp_thr(void* args)
{
   // extract args data before it gets rewritten
   struct cp_thr_args* temp = args;
   char* from;
   mmalloc(&from, return_size(temp->from) + 1);
   char* dest;
   mmalloc(&dest, return_size(temp->dest) + 1);
   int* status = temp->status;
   strcpy(from, temp->from);
   strcpy(dest, temp->dest);
   // send signal to release barrier and free args from crawl
   pthread_barrier_wait(&can_free); // ((Point of interest))

   // assign status to 0 to mark thread as occupied
   pthread_mutex_lock(&cp_lock);
   *status = 0;
   pthread_mutex_unlock(&cp_lock);

   copy_file(from, dest);

   free(from);
   free(dest);

   // assign status to 1 to mark thread as free to use
   pthread_mutex_lock(&cp_lock);
   *status = 1;
   pthread_mutex_unlock(&cp_lock);

   return (void*)0;
}

Answer 1

Use ftw() to crawl directories

ftw() is a POSIX function that will recursively crawl directories, and calls a callback function for every directory entry found.

Consider using strdup() and asprintf()

Instead of copying strings using some kind of strlen()+malloc()+strcpy(), use the POSIX function strdup() if possible, which takes care of all this for you.

Linux and most BSDs also implement asprintf(), which takes care of allocating a correctly sized buffer for you. Be aware that this is definitely not cross-platform.

Don't throw away threads

While thread creation and destruction is quite fast on Linux, it still has an overhead. If you are copying many small files, that overhead might start dominating. It would be much better to have a fixed number of threads that are waiting for work to do.

A typical implementation would use a thread-safe queue for this. Have the directory crawler fill a queue of filenames to copy. The queue is guarded by a mutex, and has an associated condition variable. Whenever it pushes an item to the queue, it signals the condition variable. The threads wait for elements to be pushed, and when woken up, they will pop one entry from the queue and start copying it.

However, assuming the directory crawler is much faster than files can be copied, it would be a waste of memory to fill a queue with all filenames encountered, especially if the directory tree is large. You only need a few entries in the queue for worker threads to pick up work. You can make the crawler block itself if the queue gets too large. You should add a second condition variable then that gets signalled whenever a worker dequeues a filename to copy.

Find the right number of threads to use

It's hard to predict how many threads will be optimal. It could be that one thread is optimal, depending on how fast the storage is compared to the overhead of having multiple threads. Having as many threads as files to copy might also not be ideal; even if they don't use much CPU time, performing I/O on multiple files at the same time might result in more disk seeks (which is mostly an issue on harddisks, but some solid state drives also have some performance penalty for doing non-sequential access). You could benchmark your code and optimize the number of threads for your system, but the performance might be different on other systems.

Alternatives for threads

Threads are a solution to the problem that there is some implicit serialization when doing regular I/O operations on files in a single thread. However, there are also other techniques to have multiple I/O operations submitted to the kernel simultaneously. POSIX AIO is one way to do this, however it has its limitations. In particular, I don't think you can easily hand off a file copy operation to it. What is more promising is Linux's io_uring, although this is of course not a cross-platform solution to your problem. If you are interested in the latter, then also see this LWN article and this excellent guide. Most BSD flavors have something similar called kqueue.