Repository: valkey-io/valkey
File: src/bio.c (C)

Code before:
#include "bio.h"
#include <stdatomic.h>

static unsigned int bio_job_to_worker[] = {
    [BIO_CLOSE_FILE] = 0,
    [BIO_AOF_FSYNC] = 1,
    [BIO_CLOSE_AOF] = 1,
    [BIO_LAZY_FREE] = 2,
    [BIO_RDB_SAVE] = 3,
};

typedef struct {
    const char *const bio_worker_title;
    pthread_t bio_thread_id;
    pthread_mutex_t bio_mutex;
    pthread_cond_t bio_newjob_cond;
    list *bio_jobs;
} bio_worker_data;

static bio_worker_data bio_workers[] = {
    {"bio_close_file"},
    {"bio_aof"},
    {"bio_lazy_free"},
    {"bio_rdb_save"}};
static const bio_worker_data *const bio_worker_end = bio_workers + (sizeof bio_workers / sizeof *bio_workers);
#define BIO_WORKER_NUM(X) ((X) - bio_workers)

static unsigned long bio_jobs_counter[BIO_NUM_OPS] = {0};
static _Thread_local size_t bio_worker_num = 0;

/* This structure represents a background Job. It is only used locally to this
 * file as the API does not expose the internals at all. */
typedef union bio_job {
    struct {
        int type; /* Job-type tag. This needs to appear as the first element in all union members. */
    } header;

    /* Job specific arguments.*/
    struct {
        int type;
        int fd;                          /* Fd for file based background jobs */
        long long offset;                /* A job-specific offset, if applicable */
        unsigned need_fsync : 1;         /* A flag to indicate that a fsync is required before
                                          * the file is closed. */
        unsigned need_reclaim_cache : 1; /* A flag to indicate that reclaim cache is required before
                                          * the file is closed. */
    } fd_args;

    struct {
        int type;
        lazy_free_fn *free_fn; /* Function that will free the provided arguments */

Review comment (suggestion):
In general I do not find it bad to wrap this as a macro. However I would suggest to make this a static function.
why?
1. macro's expansion is int (IIRC) and in some cases you might want to get an unsigned value out of it and it will force the user to case. a function can return unsigned
2. it might cause compiler warnings if the passes worker is const... probably not something that will happen (unless we pass it the bio_worker_end ) but a function can convert the input pointer correctly IMO.
3. a function is a good place to place asserts and/or return some invalid value in cases the user provides some bad pointer which is not a bio_workers


Code after:
#include "bio.h"
#include <stdatomic.h>

static unsigned int bio_job_to_worker[] = {
    [BIO_CLOSE_FILE] = 0,
    [BIO_AOF_FSYNC] = 1,
    [BIO_CLOSE_AOF] = 1,
    [BIO_LAZY_FREE] = 2,
    [BIO_RDB_SAVE] = 3,
};

typedef struct {
    const char *const bio_worker_title;
    pthread_t bio_thread_id;
    pthread_mutex_t bio_mutex;
    pthread_cond_t bio_newjob_cond;
    list *bio_jobs;
} bio_worker_data;

static bio_worker_data bio_workers[] = {
    {"bio_close_file"},
    {"bio_aof"},
    {"bio_lazy_free"},
    {"bio_rdb_save"},
};
static const bio_worker_data *const bio_worker_end = bio_workers + (sizeof bio_workers / sizeof *bio_workers);

static size_t bioWorkerNum(const bio_worker_data *const bwd) {
    /* Ensure the pointer is valid - casting to uintptr_t to not make the comparison itself UB in case
     * the pointer is outside the valid range. It's a best effort thing anyway. */
    serverAssert((uintptr_t)bwd >= (uintptr_t)bio_workers && (uintptr_t)bwd < (uintptr_t)bio_worker_end);
    return (size_t)(bwd - bio_workers);
}

static unsigned long bio_jobs_counter[BIO_NUM_OPS] = {0};
static _Thread_local size_t bio_worker_num = 0;

/* This structure represents a background Job. It is only used locally to this
 * file as the API does not expose the internals at all. */
typedef union bio_job {
    struct {
        int type; /* Job-type tag. This needs to appear as the first element in all union members. */
    } header;

    /* Job specific arguments.*/
    struct {
        int type;
        int fd;                          /* Fd for file based background jobs */
        long long offset;                /* A job-specific offset, if applicable */
        unsigned need_fsync : 1;         /* A flag to indicate that a fsync is required before
                                          * the file is closed. */