From 3ca629ceb54fe79dc788d988f74b8e3a252f8c52 Mon Sep 17 00:00:00 2001 From: "Ryan C. Gordon" Date: Sun, 6 Oct 2024 02:00:45 -0400 Subject: [PATCH] asyncio: Allow file closes to request a flush/fsyncdata. --- include/SDL3/SDL_asyncio.h | 14 ++++++++- src/dynapi/SDL_dynapi_procs.h | 2 +- src/file/SDL_asyncio.c | 5 ++-- src/file/SDL_sysasyncio.h | 1 + src/file/generic/SDL_asyncio_generic.c | 7 ++++- src/file/io_uring/SDL_asyncio_liburing.c | 37 ++++++++++++++++++++---- test/testasyncio.c | 15 ++++++++++ 7 files changed, 70 insertions(+), 11 deletions(-) diff --git a/include/SDL3/SDL_asyncio.h b/include/SDL3/SDL_asyncio.h index 13afdce7c..4d90ddaa1 100644 --- a/include/SDL3/SDL_asyncio.h +++ b/include/SDL3/SDL_asyncio.h @@ -299,6 +299,17 @@ extern SDL_DECLSPEC SDL_AsyncIOTask * SDLCALL SDL_WriteAsyncIO(SDL_AsyncIO *asyn * were to happen during the closing process, for example, the * task results will report it as usual. * + * Closing a file that has been written to does not guarantee the data + * has made it to physical media; it may remain in the operating + * system's file cache, for later writing to disk. This means that + * a successfully-closed file can be lost if the system crashes or + * loses power in this small window. To prevent this, call this + * function with the `flush` parameter set to true. This will make + * the operation take longer, but a successful result guarantees that + * the data has made it to physical storage. Don't use this for + * temporary files, caches, and unimportant data, and definitely use + * it for crucial irreplaceable files, like game saves. + * * This function guarantees that the close will happen after any other * pending tasks to `asyncio`, so it's safe to open a file, start * several operations, close the file immediately, then check for all @@ -315,6 +326,7 @@ extern SDL_DECLSPEC SDL_AsyncIOTask * SDLCALL SDL_WriteAsyncIO(SDL_AsyncIO *asyn * it's safe to attempt to close again later. * * \param asyncio a pointer to an SDL_AsyncIO structure to close. + * \param flush true if data should sync to disk before the task completes. * \param queue a queue to add the new SDL_AsyncIO to. * \param userdata an app-defined pointer that will be provided with the task results. * \returns A new task handle if a task was started, NULL on complete failure; @@ -325,7 +337,7 @@ extern SDL_DECLSPEC SDL_AsyncIOTask * SDLCALL SDL_WriteAsyncIO(SDL_AsyncIO *asyn * * \since This function is available since SDL 3.0.0. */ -extern SDL_DECLSPEC SDL_AsyncIOTask * SDLCALL SDL_CloseAsyncIO(SDL_AsyncIO *asyncio, SDL_AsyncIOQueue *queue, void *userdata); +extern SDL_DECLSPEC SDL_AsyncIOTask * SDLCALL SDL_CloseAsyncIO(SDL_AsyncIO *asyncio, bool flush, SDL_AsyncIOQueue *queue, void *userdata); /** * Create a task queue for tracking multiple I/O operations. diff --git a/src/dynapi/SDL_dynapi_procs.h b/src/dynapi/SDL_dynapi_procs.h index 92a163371..ebf559f44 100644 --- a/src/dynapi/SDL_dynapi_procs.h +++ b/src/dynapi/SDL_dynapi_procs.h @@ -1224,7 +1224,7 @@ SDL_DYNAPI_PROC(SDL_AsyncIO*,SDL_AsyncIOFromFile,(const char *a, const char *b), SDL_DYNAPI_PROC(Sint64,SDL_GetAsyncIOSize,(SDL_AsyncIO *a),(a),return) SDL_DYNAPI_PROC(SDL_AsyncIOTask*,SDL_ReadAsyncIO,(SDL_AsyncIO *a, void *b, Uint64 c, Uint64 d, SDL_AsyncIOQueue *e, void *f),(a,b,c,d,e,f),return) SDL_DYNAPI_PROC(SDL_AsyncIOTask*,SDL_WriteAsyncIO,(SDL_AsyncIO *a, void *b, Uint64 c, Uint64 d, SDL_AsyncIOQueue *e, void *f),(a,b,c,d,e,f),return) -SDL_DYNAPI_PROC(SDL_AsyncIOTask*,SDL_CloseAsyncIO,(SDL_AsyncIO *a, SDL_AsyncIOQueue *b, void *c),(a,b,c),return) +SDL_DYNAPI_PROC(SDL_AsyncIOTask*,SDL_CloseAsyncIO,(SDL_AsyncIO *a, bool b, SDL_AsyncIOQueue *c, void *d),(a,b,c,d),return) SDL_DYNAPI_PROC(SDL_AsyncIOQueue*,SDL_CreateAsyncIOQueue,(void),(),return) SDL_DYNAPI_PROC(void,SDL_DestroyAsyncIOQueue,(SDL_AsyncIOQueue *a),(a),) SDL_DYNAPI_PROC(bool,SDL_GetAsyncIOResult,(SDL_AsyncIOQueue *a, SDL_AsyncIOOutcome *b),(a,b),return) diff --git a/src/file/SDL_asyncio.c b/src/file/SDL_asyncio.c index 5d4f32e4f..f25ffa832 100644 --- a/src/file/SDL_asyncio.c +++ b/src/file/SDL_asyncio.c @@ -143,7 +143,7 @@ SDL_AsyncIOTask *SDL_WriteAsyncIO(SDL_AsyncIO *asyncio, void *ptr, Uint64 offset return RequestAsyncIO(false, asyncio, ptr, offset, size, queue, userdata); } -SDL_AsyncIOTask *SDL_CloseAsyncIO(SDL_AsyncIO *asyncio, SDL_AsyncIOQueue *queue, void *userdata) +SDL_AsyncIOTask *SDL_CloseAsyncIO(SDL_AsyncIO *asyncio, bool flush, SDL_AsyncIOQueue *queue, void *userdata) { if (!asyncio) { SDL_InvalidParamError("asyncio"); @@ -166,6 +166,7 @@ SDL_AsyncIOTask *SDL_CloseAsyncIO(SDL_AsyncIO *asyncio, SDL_AsyncIOQueue *queue, task->type = SDL_ASYNCIO_TASK_CLOSE; task->app_userdata = userdata; task->queue = queue; + task->flush = flush; asyncio->closing = task; @@ -328,7 +329,7 @@ SDL_AsyncIOTask *SDL_LoadFileAsync(const char *file, SDL_AsyncIOQueue *queue, vo SDL_free(ptr); } - SDL_CloseAsyncIO(asyncio, queue, userdata); // if this fails, we'll have a resource leak, but this would already be a dramatic system failure. + SDL_CloseAsyncIO(asyncio, false, queue, userdata); // if this fails, we'll have a resource leak, but this would already be a dramatic system failure. } return task; diff --git a/src/file/SDL_sysasyncio.h b/src/file/SDL_sysasyncio.h index e7c89c3aa..70824fc3c 100644 --- a/src/file/SDL_sysasyncio.h +++ b/src/file/SDL_sysasyncio.h @@ -68,6 +68,7 @@ struct SDL_AsyncIOTask SDL_AsyncIOTaskType type; SDL_AsyncIOQueue *queue; Uint64 offset; + bool flush; void *buffer; char *error; SDL_AsyncIOResult result; diff --git a/src/file/generic/SDL_asyncio_generic.c b/src/file/generic/SDL_asyncio_generic.c index 85a83b70c..9c1e442dc 100644 --- a/src/file/generic/SDL_asyncio_generic.c +++ b/src/file/generic/SDL_asyncio_generic.c @@ -76,7 +76,12 @@ static void SynchronousIO(SDL_AsyncIOTask *task) // files will still run in parallel. An app can also open the same file twice to avoid this. SDL_LockMutex(data->lock); if (task->type == SDL_ASYNCIO_TASK_CLOSE) { - task->result = SDL_CloseIO(data->io) ? SDL_ASYNCIO_COMPLETE : SDL_ASYNCIO_FAILURE; + bool okay = true; + if (task->flush) { + okay = SDL_FlushIO(data->io); + } + okay = SDL_CloseIO(data->io) && okay; + task->result = okay ? SDL_ASYNCIO_COMPLETE : SDL_ASYNCIO_FAILURE; } else if (SDL_SeekIO(io, (Sint64) task->offset, SDL_IO_SEEK_SET) < 0) { task->result = SDL_ASYNCIO_FAILURE; } else { diff --git a/src/file/io_uring/SDL_asyncio_liburing.c b/src/file/io_uring/SDL_asyncio_liburing.c index 90b228895..eaae7d852 100644 --- a/src/file/io_uring/SDL_asyncio_liburing.c +++ b/src/file/io_uring/SDL_asyncio_liburing.c @@ -58,6 +58,7 @@ static void *liburing_handle = NULL; SDL_LIBURING_FUNC(void, io_uring_prep_fsync, (struct io_uring_sqe *sqe, int fd, unsigned fsync_flags)) \ SDL_LIBURING_FUNC(void, io_uring_prep_cancel, (struct io_uring_sqe *sqe, void *user_data, int flags)) \ SDL_LIBURING_FUNC(void, io_uring_prep_timeout, (struct io_uring_sqe *sqe, struct __kernel_timespec *ts, unsigned count, unsigned flags)) \ + SDL_LIBURING_FUNC(void, io_uring_prep_nop, (struct io_uring_sqe *sqe)) \ SDL_LIBURING_FUNC(void, io_uring_sqe_set_data, (struct io_uring_sqe *sqe, void *data)) \ SDL_LIBURING_FUNC(void, io_uring_sqe_set_flags, (struct io_uring_sqe *sqe, unsigned flags)) \ SDL_LIBURING_FUNC(int, io_uring_submit, (struct io_uring *ring)) \ @@ -211,7 +212,7 @@ static SDL_AsyncIOTask *ProcessCQE(LibUringAsyncIOQueueData *queuedata, struct i } SDL_AsyncIOTask *task = (SDL_AsyncIOTask *) io_uring_cqe_get_data(cqe); - if (task) { // can be NULL if this was just a wakeup message, etc. + if (task) { // can be NULL if this was just a wakeup message, a NOP, etc. if (!task->queue) { // We leave `queue` blank to signify this was a task cancellation. SDL_AsyncIOTask *cancel_task = task; task = (SDL_AsyncIOTask *) cancel_task->app_userdata; @@ -227,13 +228,19 @@ static SDL_AsyncIOTask *ProcessCQE(LibUringAsyncIOQueueData *queuedata, struct i } else { if ((task->type == SDL_ASYNCIO_TASK_WRITE) && (((Uint64) cqe->res) < task->requested_size)) { task->result = SDL_ASYNCIO_FAILURE; // it's always a failure on short writes. - } else { - task->result = SDL_ASYNCIO_COMPLETE; } + + // don't explicitly mark it as COMPLETE; that's the default value and a linked task might have failed in an earlier operation and this would overwrite it. + if ((task->type == SDL_ASYNCIO_TASK_READ) || (task->type == SDL_ASYNCIO_TASK_WRITE)) { task->result_size = (Uint64) cqe->res; } } + + if ((task->type == SDL_ASYNCIO_TASK_CLOSE) && task->flush) { + task->flush = false; + task = NULL; // don't return this one, it's a linked task, so it'll arrive in a later CQE. + } } return task; @@ -358,6 +365,7 @@ static bool SDL_SYS_CreateAsyncIOQueue_liburing(SDL_AsyncIOQueue *queue) static bool liburing_asyncio_read(void *userdata, SDL_AsyncIOTask *task) { LibUringAsyncIOQueueData *queuedata = (LibUringAsyncIOQueueData *) task->queue->userdata; + const int fd = (int) (size_t) userdata; // !!! FIXME: `unsigned` is likely smaller than requested_size's Uint64. If we overflow it, we could try submitting multiple SQEs // !!! FIXME: and make a note in the task that there are several in sequence. @@ -372,7 +380,7 @@ static bool liburing_asyncio_read(void *userdata, SDL_AsyncIOTask *task) return SDL_SetError("io_uring: submission queue is full"); } - liburing.io_uring_prep_read(sqe, (int) (size_t) userdata, task->buffer, (unsigned) task->requested_size, task->offset); + liburing.io_uring_prep_read(sqe, fd, task->buffer, (unsigned) task->requested_size, task->offset); liburing.io_uring_sqe_set_data(sqe, task); const bool retval = task->queue->iface.queue_task(task->queue->userdata, task); @@ -383,6 +391,7 @@ static bool liburing_asyncio_read(void *userdata, SDL_AsyncIOTask *task) static bool liburing_asyncio_write(void *userdata, SDL_AsyncIOTask *task) { LibUringAsyncIOQueueData *queuedata = (LibUringAsyncIOQueueData *) task->queue->userdata; + const int fd = (int) (size_t) userdata; // !!! FIXME: `unsigned` is likely smaller than requested_size's Uint64. If we overflow it, we could try submitting multiple SQEs // !!! FIXME: and make a note in the task that there are several in sequence. @@ -397,7 +406,7 @@ static bool liburing_asyncio_write(void *userdata, SDL_AsyncIOTask *task) return SDL_SetError("io_uring: submission queue is full"); } - liburing.io_uring_prep_write(sqe, (int) (size_t) userdata, task->buffer, (unsigned) task->requested_size, task->offset); + liburing.io_uring_prep_write(sqe, fd, task->buffer, (unsigned) task->requested_size, task->offset); liburing.io_uring_sqe_set_data(sqe, task); const bool retval = task->queue->iface.queue_task(task->queue->userdata, task); @@ -408,6 +417,7 @@ static bool liburing_asyncio_write(void *userdata, SDL_AsyncIOTask *task) static bool liburing_asyncio_close(void *userdata, SDL_AsyncIOTask *task) { LibUringAsyncIOQueueData *queuedata = (LibUringAsyncIOQueueData *) task->queue->userdata; + const int fd = (int) (size_t) userdata; // have to hold a lock because otherwise two threads could get_sqe and submit while one request isn't fully set up. SDL_LockMutex(queuedata->sqe_lock); @@ -416,7 +426,22 @@ static bool liburing_asyncio_close(void *userdata, SDL_AsyncIOTask *task) return SDL_SetError("io_uring: submission queue is full"); } - liburing.io_uring_prep_close(sqe, (int) (size_t) userdata); + if (task->flush) { + struct io_uring_sqe *flush_sqe = sqe; + sqe = liburing.io_uring_get_sqe(&queuedata->ring); // this will be our actual close task. + if (!sqe) { + liburing.io_uring_prep_nop(flush_sqe); // we already have the first sqe, just make it a NOP. + liburing.io_uring_sqe_set_data(flush_sqe, NULL); + task->queue->iface.queue_task(task->queue->userdata, task); + return SDL_SetError("io_uring: submission queue is full"); + } + + liburing.io_uring_prep_fsync(flush_sqe, fd, IORING_FSYNC_DATASYNC); + liburing.io_uring_sqe_set_data(flush_sqe, task); + liburing.io_uring_sqe_set_flags(flush_sqe, IOSQE_IO_HARDLINK); // must complete before next sqe starts, and next sqe should run even if this fails. + } + + liburing.io_uring_prep_close(sqe, fd); liburing.io_uring_sqe_set_data(sqe, task); const bool retval = task->queue->iface.queue_task(task->queue->userdata, task); diff --git a/test/testasyncio.c b/test/testasyncio.c index 4c7e16b46..b330b4a9a 100644 --- a/test/testasyncio.c +++ b/test/testasyncio.c @@ -23,6 +23,7 @@ static SDLTest_CommonState *state = NULL; SDL_AppResult SDL_AppInit(void **appstate, int argc, char *argv[]) { const char *base = NULL; + SDL_AsyncIO *asyncio = NULL; char **bmps = NULL; int bmpcount = 0; int i; @@ -104,6 +105,15 @@ SDL_AppResult SDL_AppInit(void **appstate, int argc, char *argv[]) SDL_free(bmps); + SDL_Log("Opening asyncio.tmp..."); + asyncio = SDL_AsyncIOFromFile("asyncio.tmp", "w"); + if (!asyncio) { + SDL_Log("Failed!"); + return SDL_APP_FAILURE; + } + SDL_WriteAsyncIO(asyncio, "hello", 0, 5, queue, "asyncio.tmp (write)"); + SDL_CloseAsyncIO(asyncio, true, queue, "asyncio.tmp (flush/close)"); + return SDL_APP_CONTINUE; } @@ -135,6 +145,10 @@ static void async_io_task_complete(const SDL_AsyncIOOutcome *outcome) SDL_Log("File '%s' async results: %s", fname, resultstr); + if (SDL_strncmp(fname, "asyncio.tmp", 11) == 0) { + return; + } + if (outcome->result == SDL_ASYNCIO_COMPLETE) { SDL_Surface *surface = SDL_LoadBMP_IO(SDL_IOFromConstMem(outcome->buffer, (size_t) outcome->bytes_transferred), true); if (surface) { @@ -171,6 +185,7 @@ void SDL_AppQuit(void *appstate, SDL_AppResult result) { SDL_DestroyAsyncIOQueue(queue); SDL_DestroyTexture(texture); + SDL_RemovePath("asyncio.tmp"); SDLTest_CommonQuit(state); }