From a37870a16944603e046a00510d8f49c460535535 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Marczewski?= Date: Tue, 6 Jul 2021 15:16:05 +0200 Subject: [PATCH] [LibOS] Make POSIX locks interruptible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Paweł Marczewski --- LibOS/shim/include/shim_fs_lock.h | 18 +- LibOS/shim/include/shim_ipc.h | 38 +++- LibOS/shim/src/fs/shim_fs_lock.c | 238 ++++++++++++++++-------- LibOS/shim/src/ipc/shim_ipc.c | 22 ++- LibOS/shim/src/ipc/shim_ipc_fs_lock.c | 38 +++- LibOS/shim/src/ipc/shim_ipc_worker.c | 1 + LibOS/shim/src/shim_parser.c | 2 +- LibOS/shim/test/ltp/ltp.cfg | 7 - LibOS/shim/test/regression/fcntl_lock.c | 179 ++++++++++++++---- 9 files changed, 412 insertions(+), 131 deletions(-) diff --git a/LibOS/shim/include/shim_fs_lock.h b/LibOS/shim/include/shim_fs_lock.h index 126a95fdc7..fbb8384718 100644 --- a/LibOS/shim/include/shim_fs_lock.h +++ b/LibOS/shim/include/shim_fs_lock.h @@ -111,7 +111,23 @@ int posix_lock_clear_pid(IDTYPE pid); * to add a lock (-EAGAIN, -ENOMEM etc.) will be sent in the response instead. */ int posix_lock_set_from_ipc(const char* path, struct posix_lock* pl, bool wait, IDTYPE vmid, - unsigned long seq); + uint64_t seq); + +/*! + * \brief Cancel a lock request (IPC handler) + * + * \param path absolute path for a file + * \param vmid target process for IPC response + * \param seq sequence number for IPC response + * + * Cancels the request previously added by `posix_lock_set_from_ipc`: if the request with given + * `vmid` and `seq` parameters is still unresolved, it is removed and a response with -EINTR is + * sent. If the request has been resolved in the meantime, nothing happens. + * + * This function should be called if the wait for IPC response has been interrupted. It ensures that + * a response will be sent immediately. + */ +int posix_lock_cancel_from_ipc(const char* path, IDTYPE vmid, uint64_t seq); /*! * \brief Check for conflicting locks on a file (IPC handler) diff --git a/LibOS/shim/include/shim_ipc.h b/LibOS/shim/include/shim_ipc.h index 44954c0d37..991b8fcd3b 100644 --- a/LibOS/shim/include/shim_ipc.h +++ b/LibOS/shim/include/shim_ipc.h @@ -33,6 +33,7 @@ enum { IPC_MSG_SYNC_CONFIRM_DOWNGRADE, IPC_MSG_SYNC_CONFIRM_CLOSE, IPC_MSG_POSIX_LOCK_SET, + IPC_MSG_POSIX_LOCK_CANCEL, IPC_MSG_POSIX_LOCK_GET, IPC_MSG_POSIX_LOCK_CLEAR_PID, IPC_MSG_CODE_BOUND, @@ -117,6 +118,28 @@ int ipc_send_message(IDTYPE dest, struct shim_ipc_msg* msg); * discarded, but still awaited for. */ int ipc_send_msg_and_get_response(IDTYPE dest, struct shim_ipc_msg* msg, void** resp); +/*! + * \brief Send an IPC message and wait for a response (cancel on interrupt) + * + * \param dest vmid of the destination process + * \param msg message to send + * \param on_cancel callback to invoke when the wait is interrupted + * \param arg argument for the callback + * \param[out] resp upon successful return contains a pointer to the response + * + * A version of `ipc_send_msg_and_get_response` that allows canceling a request after the wait is + * interrupted. Intended for implementation of interruptible function calls. + * + * Behaves as `ipc_send_msg_and_get_response`, but after the wait for response is interrupted, + * invokes `on_cancel(dest, arg)`. If it succeeds, the wait is resumed. The callback is invoked only + * once: if the wait is interrupted again afterwards, the function keeps waiting. + * + * The \p on_cancel callback should return 0 on success, or a negative error code on failure. If + * the callback fails, the function does not resume the wait, but returns the error immediately. + */ +int ipc_send_msg_and_get_response_with_cancel(IDTYPE dest, struct shim_ipc_msg* msg, + int (*on_cancel)(IDTYPE, void*), void* arg, + void** resp); /*! * \brief Broadcast an IPC message * @@ -258,6 +281,7 @@ int ipc_sync_confirm_close_callback(IDTYPE src, void* data, unsigned long seq); /* * POSIX_LOCK_SET: `struct shim_ipc_posix_lock` -> `int` + * POSIX_LOCK_CANCEL: `shim_ipc_posix_lock_cancel` (no response) * POSIX_LOCK_GET: `struct shim_ipc_posix_lock` -> `struct shim_ipc_posix_lock_resp` * POSIX_LOCK_CLEAR_PID: `IDTYPE` -> `int` */ @@ -273,6 +297,11 @@ struct shim_ipc_posix_lock { char path[]; /* null-terminated */ }; +struct shim_ipc_posix_lock_cancel { + uint64_t cancel_seq; + char path[]; /* null-terminated */ +}; + struct shim_ipc_posix_lock_resp { int result; @@ -286,11 +315,12 @@ struct shim_ipc_posix_lock_resp { struct posix_lock; int ipc_posix_lock_set(const char* path, struct posix_lock* pl, bool wait); -int ipc_posix_lock_set_send_response(IDTYPE vmid, unsigned long seq, int result); +int ipc_posix_lock_set_send_response(IDTYPE vmid, uint64_t seq, int result); int ipc_posix_lock_get(const char* path, struct posix_lock* pl, struct posix_lock* out_pl); int ipc_posix_lock_clear_pid(IDTYPE pid); -int ipc_posix_lock_set_callback(IDTYPE src, void* data, unsigned long seq); -int ipc_posix_lock_get_callback(IDTYPE src, void* data, unsigned long seq); -int ipc_posix_lock_clear_pid_callback(IDTYPE src, void* data, unsigned long seq); +int ipc_posix_lock_set_callback(IDTYPE src, void* data, uint64_t seq); +int ipc_posix_lock_cancel_callback(IDTYPE src, void* data, uint64_t seq); +int ipc_posix_lock_get_callback(IDTYPE src, void* data, uint64_t seq); +int ipc_posix_lock_clear_pid_callback(IDTYPE src, void* data, uint64_t seq); #endif /* SHIM_IPC_H_ */ diff --git a/LibOS/shim/src/fs/shim_fs_lock.c b/LibOS/shim/src/fs/shim_fs_lock.c index 817315ed6b..f39f4d1aeb 100644 --- a/LibOS/shim/src/fs/shim_fs_lock.c +++ b/LibOS/shim/src/fs/shim_fs_lock.c @@ -11,29 +11,30 @@ #include "shim_lock.h" /* - * Describes a pending request for a POSIX lock. After processing the request, the object is - * removed, and a possible waiter is notified (see below). + * Describes a pending or resolved request for a POSIX lock. * * If the request is initiated by another process over IPC, `notify.vmid` and `notify.seq` should be - * set to parameters of IPC message. After processing the request, IPC response will be sent. + * set to parameters of IPC message. After resolving the request, IPC response will be sent. The + * request will then be immediately deleted. * * If the request is initiated by process leader, `notify.vmid` should be set to 0, and - * `notify.event` should be set to an event handle. After processing the request, the event will be - * triggered, and `*notify.result` will be set to the result. + * `notify.event` should be set to an event handle. After resolving the request, the event will be + * triggered, `resolved` will be set to true, and `result` will be set to the result. The request + * will NOT be deleted; it is the responsibility of the waiting thread to delete it (after reading + * `result`). */ DEFINE_LISTP(posix_lock_request); DEFINE_LIST(posix_lock_request); struct posix_lock_request { struct posix_lock pl; + bool resolved; + int result; struct { IDTYPE vmid; - unsigned int seq; + uint64_t seq; - /* Note that `event` and `result` are owned by the side making the request, and outlive this - * object (we delete it as soon as the request is processed). */ PAL_HANDLE event; - int* result; } notify; LIST_TYPE(posix_lock_request) list; @@ -128,7 +129,7 @@ static void posix_lock_dump(struct fs_lock* fs_lock) { buf_flush(&buf); } -/* Removes `fs_lock` if it's not necessary (i.e. no locks are held or requested for a file). */ +/* Removes `fs_lock` if it's no longer necessary (i.e. there are no locks or lock requests). */ static void fs_lock_gc(struct fs_lock* fs_lock) { assert(locked(&fs_lock->dent->lock)); if (g_log_level >= LOG_LEVEL_TRACE) @@ -176,6 +177,7 @@ static int posix_lock_add_request(struct fs_lock* fs_lock, struct posix_lock* pl if (!req) return -ENOMEM; req->pl = *pl; + req->resolved = false; LISTP_ADD(req, &fs_lock->posix_lock_requests, list); *out_req = req; return 0; @@ -348,6 +350,34 @@ static int _posix_lock_set(struct fs_lock* fs_lock, struct posix_lock* pl) { return 0; } +/* Resolve a request with given result. This involves notifying a waiter, and deleting the request + * if it's a remote one. */ +static void posix_lock_resolve(struct fs_lock* fs_lock, struct posix_lock_request* req, + int result) { + assert(locked(&fs_lock->dent->lock)); + assert(!req->resolved); + req->result = result; + req->resolved = true; + + if (req->notify.vmid == 0) { + /* This is a local request: we set `req->notify.event`, and let the thread waiting for it + * handle the cleanup. */ + assert(req->notify.event); + DkEventSet(req->notify.event); + } else { + /* This is a remote request: we send a response over IPC, and then delete the request. */ + assert(!req->notify.event); + int ret = ipc_posix_lock_set_send_response(req->notify.vmid, req->notify.seq, + req->result); + if (ret < 0) { + log_warning("posix lock: error sending result over IPC: %d", ret); + } + + LISTP_DEL(req, &fs_lock->posix_lock_requests, list); + free(req); + } +} + /* * Process pending requests. This function should be called after any modification to the list of * locks, since we might have unblocked a request. @@ -364,78 +394,48 @@ static void posix_lock_process_requests(struct fs_lock* fs_lock) { struct posix_lock_request* req; struct posix_lock_request* tmp; LISTP_FOR_EACH_ENTRY_SAFE(req, tmp, &fs_lock->posix_lock_requests, list) { + if (req->resolved) + continue; + struct posix_lock* conflict = posix_lock_find_conflict(fs_lock, &req->pl); - if (!conflict) { - int result = _posix_lock_set(fs_lock, &req->pl); - LISTP_DEL(req, &fs_lock->posix_lock_requests, list); - - /* Notify the waiter that we processed their request. Note that the result might - * still be a failure (-ENOMEM). */ - if (req->notify.vmid == 0) { - assert(req->notify.event); - assert(req->notify.result); - *req->notify.result = result; - DkEventSet(req->notify.event); - } else { - assert(!req->notify.event); - assert(!req->notify.result); - - int ret = ipc_posix_lock_set_send_response(req->notify.vmid, req->notify.seq, - result); - if (ret < 0) { - log_warning("posix lock: error sending result over IPC: %d", ret); - } - } - free(req); - changed = true; - } + if (conflict) + continue; + + int result = _posix_lock_set(fs_lock, &req->pl); + posix_lock_resolve(fs_lock, req, result); + changed = true; } } while (changed); } /* Add/remove a lock if possible. On conflict, returns -EAGAIN (if `wait` is false) or adds a new * request (if `wait` is true). */ -static int posix_lock_set_or_add_request(struct shim_dentry* dent, struct posix_lock* pl, bool wait, +static int posix_lock_set_or_add_request(struct fs_lock* fs_lock, struct posix_lock* pl, bool wait, struct posix_lock_request** out_req) { - assert(locked(&dent->lock)); - - struct fs_lock* fs_lock = NULL; - int ret = find_fs_lock(dent, /*create=*/pl->type != F_UNLCK, &fs_lock); - if (ret < 0) - goto out; - if (!fs_lock) { - assert(pl->type == F_UNLCK); - /* Nothing to unlock. */ - return 0; - } + assert(locked(&fs_lock->dent->lock)); + int ret; struct posix_lock* conflict = NULL; if (pl->type != F_UNLCK) conflict = posix_lock_find_conflict(fs_lock, pl); if (conflict) { - if (!wait) { - ret = -EAGAIN; - goto out; - } + if (!wait) + return -EAGAIN; struct posix_lock_request* req; ret = posix_lock_add_request(fs_lock, pl, &req); if (ret < 0) - goto out; + return ret; *out_req = req; } else { ret = _posix_lock_set(fs_lock, pl); if (ret < 0) - goto out; + return ret; posix_lock_process_requests(fs_lock); *out_req = NULL; } - ret = 0; -out: - if (fs_lock) - fs_lock_gc(fs_lock); - return ret; + return 0; } int posix_lock_set(struct shim_dentry* dent, struct posix_lock* pl, bool wait) { @@ -465,38 +465,66 @@ int posix_lock_set(struct shim_dentry* dent, struct posix_lock* pl, bool wait) { lock(&dent->lock); - PAL_HANDLE event = NULL; + struct fs_lock* fs_lock = NULL; struct posix_lock_request* req = NULL; - ret = posix_lock_set_or_add_request(dent, pl, wait, &req); + + ret = find_fs_lock(dent, /*create=*/pl->type != F_UNLCK, &fs_lock); + if (ret < 0) + goto out; + if (!fs_lock) { + assert(pl->type == F_UNLCK); + /* Nothing to unlock. */ + ret = 0; + goto out; + } + + ret = posix_lock_set_or_add_request(fs_lock, pl, wait, &req); if (ret < 0) goto out; if (req) { /* `posix_lock_set_or_add_request` is allowed to add a request only if `wait` is true */ assert(wait); - int result; + PAL_HANDLE event; ret = DkEventCreate(&event, /*init_signaled=*/false, /*auto_clear=*/false); - if (ret < 0) + if (ret < 0) { + ret = pal_to_unix_errno(ret); goto out; + } + req->notify.event = event; req->notify.vmid = 0; req->notify.seq = 0; - req->notify.event = event; - req->notify.result = &result; unlock(&dent->lock); - ret = object_wait_with_retry(event); + ret = DkEventWait(event, /*timeout=*/NULL); lock(&dent->lock); - if (ret < 0) - goto out; - ret = result; + /* + * After waiting and retaking the lock, `fs_lock` and `req` should still exist: `req` is + * only deleted by us, and `fs_lock` is deleted only if the list of requests is empty. + */ + + if (ret == 0) { + assert(req->resolved); + } else if (ret < 0 && ret != -PAL_ERROR_INTERRUPTED) { + log_warning("posix_lock_set: unexpected error from DkEventWait: %d\n", ret); + } + + /* + * At this point, the request might not be resolved yet, in which case we need to return + * -EINTR. Either way, we delete the request. + */ + ret = req->resolved ? req->result : -EINTR; + LISTP_DEL(req, &fs_lock->posix_lock_requests, list); + DkObjectClose(event); + free(req); } else { ret = 0; } out: + if (fs_lock) + fs_lock_gc(fs_lock); unlock(&dent->lock); - if (event) - DkObjectClose(event); return ret; } @@ -504,18 +532,29 @@ int posix_lock_set_from_ipc(const char* path, struct posix_lock* pl, bool wait, unsigned long seq) { assert(!g_process_ipc_ids.leader_vmid); - struct shim_dentry* dent = NULL; - struct posix_lock_request* req = NULL; + struct shim_dentry* dent; int ret = path_lookupat(g_dentry_root, path, LOOKUP_NO_FOLLOW, &dent); if (ret < 0) { log_warning("posix_lock_set_from_ipc: error on dentry lookup for %s: %d", path, ret); - goto out; + return ret; } lock(&dent->lock); - ret = posix_lock_set_or_add_request(dent, pl, wait, &req); - unlock(&dent->lock); + + struct posix_lock_request* req = NULL; + struct fs_lock* fs_lock = NULL; + ret = find_fs_lock(dent, /*create=*/pl->type != F_UNLCK, &fs_lock); + if (ret < 0) + goto out; + if (!fs_lock) { + assert(pl->type == F_UNLCK); + /* Nothing to unlock. */ + ret = 0; + goto out; + } + + ret = posix_lock_set_or_add_request(fs_lock, pl, wait, &req); if (ret < 0) goto out; @@ -526,19 +565,64 @@ int posix_lock_set_from_ipc(const char* path, struct posix_lock* pl, bool wait, req->notify.vmid = vmid; req->notify.seq = seq; req->notify.event = NULL; - req->notify.result = NULL; } ret = 0; out: - if (dent) - put_dentry(dent); + if (fs_lock) + fs_lock_gc(fs_lock); + unlock(&dent->lock); + put_dentry(dent); if (req) { /* We added a request, so response will be sent later. */ + assert(ret == 0); return 0; } return ipc_posix_lock_set_send_response(vmid, seq, ret); } +int posix_lock_cancel_from_ipc(const char* path, IDTYPE vmid, uint64_t seq) { + assert(!g_process_ipc_ids.leader_vmid); + + struct shim_dentry* dent; + int ret = path_lookupat(g_dentry_root, path, LOOKUP_NO_FOLLOW, &dent); + if (ret < 0) { + log_warning("posix_lock_cancel_from_ipc: error on dentry lookup for %s: %d", path, ret); + return ret; + } + + lock(&dent->lock); + + struct fs_lock* fs_lock = NULL; + ret = find_fs_lock(dent, /*create=*/false, &fs_lock); + if (ret < 0) + goto out; + if (!fs_lock) { + /* No lock information, it looks like the request has already been resolved and deleted. */ + ret = 0; + goto out; + } + + struct posix_lock_request* req; + LISTP_FOR_EACH_ENTRY(req, &fs_lock->posix_lock_requests, list) { + if (req->notify.vmid == vmid && req->notify.seq == seq) { + /* This is a remote request, so it must still be unresolved, otherwise we would have + * deleted it already. */ + assert(!req->resolved); + + posix_lock_resolve(fs_lock, req, -EINTR); + break; + } + } + + ret = 0; +out: + if (fs_lock) + fs_lock_gc(fs_lock); + unlock(&dent->lock); + put_dentry(dent); + return ret; +} + int posix_lock_get(struct shim_dentry* dent, struct posix_lock* pl, struct posix_lock* out_pl) { assert(pl->type != F_UNLCK); diff --git a/LibOS/shim/src/ipc/shim_ipc.c b/LibOS/shim/src/ipc/shim_ipc.c index cf8b9e7ea7..6071ac7888 100644 --- a/LibOS/shim/src/ipc/shim_ipc.c +++ b/LibOS/shim/src/ipc/shim_ipc.c @@ -240,19 +240,30 @@ int ipc_send_message(IDTYPE dest, struct shim_ipc_msg* msg) { return ret; } -static int wait_for_response(struct ipc_msg_waiter* waiter) { +static int wait_for_response(struct ipc_msg_waiter* waiter, int (*on_cancel)(IDTYPE, void*), + void* arg) { log_debug("Waiting for a response to %lu", waiter->seq); int ret = 0; + bool canceled = false; do { ret = pal_to_unix_errno(DkEventWait(waiter->event, /*timeout=*/NULL)); + if (ret == -EINTR && on_cancel && !canceled) { + log_debug("Wait for response to %lu interrupted, canceling", waiter->seq); + int cancel_ret = on_cancel(waiter->dest, arg); + if (cancel_ret < 0) + return cancel_ret; + canceled = true; + } } while (ret == -EINTR); log_debug("Waiting finished: %d", ret); return ret; } -int ipc_send_msg_and_get_response(IDTYPE dest, struct shim_ipc_msg* msg, void** resp) { +int ipc_send_msg_and_get_response_with_cancel(IDTYPE dest, struct shim_ipc_msg* msg, + int (*on_cancel)(IDTYPE, void*), void* arg, + void** resp) { static uint64_t ipc_seq_counter = 1; uint64_t seq = __atomic_fetch_add(&ipc_seq_counter, 1, __ATOMIC_RELAXED); SET_UNALIGNED(msg->header.seq, seq); @@ -276,7 +287,7 @@ int ipc_send_msg_and_get_response(IDTYPE dest, struct shim_ipc_msg* msg, void** goto out; } - ret = wait_for_response(&waiter); + ret = wait_for_response(&waiter, on_cancel, arg); if (ret < 0) { goto out; } @@ -301,6 +312,11 @@ int ipc_send_msg_and_get_response(IDTYPE dest, struct shim_ipc_msg* msg, void** return ret; } +int ipc_send_msg_and_get_response(IDTYPE dest, struct shim_ipc_msg* msg, void** resp) { + return ipc_send_msg_and_get_response_with_cancel(dest, msg, /*on_cancel=*/NULL, /*arg=*/NULL, + resp); +} + int ipc_response_callback(IDTYPE src, void* data, uint64_t seq) { int ret = 0; if (!seq) { diff --git a/LibOS/shim/src/ipc/shim_ipc_fs_lock.c b/LibOS/shim/src/ipc/shim_ipc_fs_lock.c index 8804b2cff8..64297edd2e 100644 --- a/LibOS/shim/src/ipc/shim_ipc_fs_lock.c +++ b/LibOS/shim/src/ipc/shim_ipc_fs_lock.c @@ -10,6 +10,26 @@ #include "shim_fs_lock.h" #include "shim_ipc.h" +static int cancel_posix_lock(IDTYPE dest, void* arg) { + struct shim_ipc_msg* lock_msg = arg; + char* path = ((struct shim_ipc_posix_lock*)lock_msg->data)->path; + size_t path_len = strlen(path); + + struct shim_ipc_posix_lock_cancel msgin = { + /* Sequence number of the original message (set by + * `ipc_send_msg_and_get_response_with_cancel` before sending) */ + .cancel_seq = lock_msg->header.seq, + }; + + size_t total_msg_size = get_ipc_msg_size(sizeof(msgin) + path_len + 1); + struct shim_ipc_msg* msg = __alloca(total_msg_size); + init_ipc_msg(msg, IPC_MSG_POSIX_LOCK_CANCEL, total_msg_size); + memcpy(msg->data, &msgin, sizeof(msgin)); + memcpy(((struct shim_ipc_posix_lock_cancel*)&msg->data)->path, path, path_len + 1); + + return ipc_send_message(dest, msg); +} + int ipc_posix_lock_set(const char* path, struct posix_lock* pl, bool wait) { assert(g_process_ipc_ids.leader_vmid); @@ -30,7 +50,8 @@ int ipc_posix_lock_set(const char* path, struct posix_lock* pl, bool wait) { memcpy(((struct shim_ipc_posix_lock*)&msg->data)->path, path, path_len + 1); void* data; - int ret = ipc_send_msg_and_get_response(g_process_ipc_ids.leader_vmid, msg, &data); + int ret = ipc_send_msg_and_get_response_with_cancel( + g_process_ipc_ids.leader_vmid, msg, &cancel_posix_lock, msg, &data); if (ret < 0) return ret; int result = *(int*)data; @@ -38,7 +59,7 @@ int ipc_posix_lock_set(const char* path, struct posix_lock* pl, bool wait) { return result; } -int ipc_posix_lock_set_send_response(IDTYPE vmid, unsigned long seq, int result) { +int ipc_posix_lock_set_send_response(IDTYPE vmid, uint64_t seq, int result) { assert(!g_process_ipc_ids.leader_vmid); size_t total_msg_size = get_ipc_msg_size(sizeof(result)); @@ -99,7 +120,7 @@ int ipc_posix_lock_clear_pid(IDTYPE pid) { return result; } -int ipc_posix_lock_set_callback(IDTYPE src, void* data, unsigned long seq) { +int ipc_posix_lock_set_callback(IDTYPE src, void* data, uint64_t seq) { struct shim_ipc_posix_lock* msgin = data; struct posix_lock pl = { .type = msgin->type, @@ -111,7 +132,14 @@ int ipc_posix_lock_set_callback(IDTYPE src, void* data, unsigned long seq) { return posix_lock_set_from_ipc(msgin->path, &pl, msgin->wait, src, seq); } -int ipc_posix_lock_get_callback(IDTYPE src, void* data, unsigned long seq) { +int ipc_posix_lock_cancel_callback(IDTYPE src, void* data, uint64_t seq) { + __UNUSED(seq); + + struct shim_ipc_posix_lock_cancel* msgin = data; + return posix_lock_cancel_from_ipc(msgin->path, src, msgin->cancel_seq); +} + +int ipc_posix_lock_get_callback(IDTYPE src, void* data, uint64_t seq) { struct shim_ipc_posix_lock* msgin = data; struct posix_lock pl = { .type = msgin->type, @@ -137,7 +165,7 @@ int ipc_posix_lock_get_callback(IDTYPE src, void* data, unsigned long seq) { return ipc_send_message(src, msg); } -int ipc_posix_lock_clear_pid_callback(IDTYPE src, void* data, unsigned long seq) { +int ipc_posix_lock_clear_pid_callback(IDTYPE src, void* data, uint64_t seq) { IDTYPE* pid = data; int result = posix_lock_clear_pid(*pid); diff --git a/LibOS/shim/src/ipc/shim_ipc_worker.c b/LibOS/shim/src/ipc/shim_ipc_worker.c index 16a9d1a849..1029b26427 100644 --- a/LibOS/shim/src/ipc/shim_ipc_worker.c +++ b/LibOS/shim/src/ipc/shim_ipc_worker.c @@ -62,6 +62,7 @@ static ipc_callback ipc_callbacks[] = { [IPC_MSG_SYNC_CONFIRM_CLOSE] = ipc_sync_confirm_close_callback, [IPC_MSG_POSIX_LOCK_SET] = ipc_posix_lock_set_callback, + [IPC_MSG_POSIX_LOCK_CANCEL] = ipc_posix_lock_cancel_callback, [IPC_MSG_POSIX_LOCK_GET] = ipc_posix_lock_get_callback, [IPC_MSG_POSIX_LOCK_CLEAR_PID] = ipc_posix_lock_clear_pid_callback, }; diff --git a/LibOS/shim/src/shim_parser.c b/LibOS/shim/src/shim_parser.c index 750ad1b88b..16ab428259 100644 --- a/LibOS/shim/src/shim_parser.c +++ b/LibOS/shim/src/shim_parser.c @@ -203,7 +203,7 @@ struct parser_table { parse_pointer_arg, parse_pointer_arg, parse_long_arg, parse_integer_arg}}, [__NR_msgctl] = {.slow = true, .name = "msgctl", .parser = {parse_long_arg, parse_integer_arg, parse_integer_arg, parse_pointer_arg}}, - [__NR_fcntl] = {.slow = false, .name = "fcntl", .parser = {parse_long_arg, parse_integer_arg, + [__NR_fcntl] = {.slow = true, .name = "fcntl", .parser = {parse_long_arg, parse_integer_arg, parse_fcntlop, parse_pointer_arg}}, [__NR_flock] = {.slow = false, .name = "flock", .parser = {NULL}}, [__NR_fsync] = {.slow = false, .name = "fsync", .parser = {parse_long_arg, parse_integer_arg}}, diff --git a/LibOS/shim/test/ltp/ltp.cfg b/LibOS/shim/test/ltp/ltp.cfg index 0b29248ab9..45ad93bf4f 100644 --- a/LibOS/shim/test/ltp/ltp.cfg +++ b/LibOS/shim/test/ltp/ltp.cfg @@ -369,13 +369,6 @@ timeout = 60 [fcntl14_64] timeout = 60 -# depends on POSIX locks returning EINTR after signal, which we don't support -[fcntl16] -skip = yes - -[fcntl16_64] -skip = yes - # no deadlock detection for POSIX locks [fcntl17] skip = yes diff --git a/LibOS/shim/test/regression/fcntl_lock.c b/LibOS/shim/test/regression/fcntl_lock.c index 21861a0495..85870096b7 100644 --- a/LibOS/shim/test/regression/fcntl_lock.c +++ b/LibOS/shim/test/regression/fcntl_lock.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -56,6 +57,7 @@ static const char* str_err(int err) { switch (err) { case EACCES: return "EACCES"; case EAGAIN: return "EAGAIN"; + case EINTR: return "EINTR"; default: return "???"; } } @@ -92,35 +94,16 @@ static int try_fcntl(int cmd, struct flock* fl) { if (ret != -1 && ret != 0) errx(1, "fcntl returned unexpected value"); if (ret == -1) { - /* We permit -1 only for F_SETLK, and only with EACCES or EAGAIN errors (which means the - * lock could not be placed immediately). */ - if (!(cmd == F_SETLK && (errno == EACCES || errno == EAGAIN))) { - err(1, "fcntl"); + if (errno == EINTR) + return ret; + if (cmd == F_SETLK && (errno == EACCES || errno == EAGAIN)) { + return ret; } + err(1, "fcntl"); } return ret; } -/* Wrapper for `try_fcntl`. Returns true if it succeeds (F_SETLK returns success, F_GETLK returns no - * conflicting lock). */ -static bool try_lock(int cmd, int type, int whence, long int start, long int len) { - struct flock fl = { - .l_type = type, - .l_whence = whence, - .l_start = start, - .l_len = len, - }; - int ret = try_fcntl(cmd, &fl); - - if (cmd == F_GETLK) { - assert(ret == 0); - - return fl.l_type == F_UNLCK; - } else { - return ret == 0; - } -} - /* Check whether F_GETLK returns the right conflicting lock. */ static void lock_check(int type, long int start, long int len, int conflict_type, long int conflict_start, long int conflict_len) { @@ -144,27 +127,81 @@ static void lock_check(int type, long int start, long int len, int conflict_type } static void unlock(long int start, long int len) { - if (!try_lock(F_SETLK, F_UNLCK, SEEK_SET, start, len)) - errx(1, "unlock failed"); + struct flock fl = { + .l_type = F_UNLCK, + .l_whence = SEEK_SET, + .l_start = start, + .l_len = len, + }; + int ret; + do { + ret = try_fcntl(F_SETLK, &fl); + } while (ret == -1 && errno == -EINTR); + if (ret == -1) + errx(1, "unlocking failed"); } static void lock(int type, long int start, long int len) { assert(type == F_RDLCK || type == F_WRLCK); - if (!try_lock(F_GETLK, type, SEEK_SET, start, len) - || !try_lock(F_SETLK, type, SEEK_SET, start, len)) + struct flock fl = { + .l_type = type, + .l_whence = SEEK_SET, + .l_start = start, + .l_len = len, + }; + int ret; + do { + ret = try_fcntl(F_SETLK, &fl); + } while (ret == -1 && errno == EINTR); + if (ret == -1) errx(1, "setting %s failed", str_type(type)); } static void lock_wait_ok(int type, long int start, long int len) { - if (!try_lock(F_SETLKW, type, SEEK_SET, start, len)) - errx(1, "waiting for %s failed", str_type(type)); + assert(type == F_RDLCK || type == F_WRLCK); + + struct flock fl = { + .l_type = type, + .l_whence = SEEK_SET, + .l_start = start, + .l_len = len, + }; + int ret; + do { + ret = try_fcntl(F_SETLKW, &fl); + } while (ret == -1 && errno == EINTR); + if (ret == -1) + errx(1, "setting %s failed", str_type(type)); } static void lock_fail(int type, long int start, long int len) { - if (try_lock(F_GETLK, type, SEEK_SET, start, len) - || try_lock(F_SETLK, type, SEEK_SET, start, len)) - errx(1, "setting %s succeeded unexpectedly", str_type(type)); + assert(type == F_RDLCK || type == F_WRLCK); + + struct flock fl = { + .l_type = type, + .l_whence = SEEK_SET, + .l_start = start, + .l_len = len, + }; + int ret; + do { + ret = try_fcntl(F_SETLK, &fl); + } while (ret == -1 && errno == EINTR); + if (ret == 0) + errx(1, "expected setting %s to fail", str_type(type)); +} + +static void lock_wait_eintr(int type, long int start, long int len) { + struct flock fl = { + .l_type = type, + .l_whence = SEEK_SET, + .l_start = start, + .l_len = len, + }; + int ret = try_fcntl(F_SETLKW, &fl); + if (ret == 0 || errno != EINTR) + errx(1, "expected setting %s to fail with EINTR", str_type(type)); } /* @@ -371,6 +408,74 @@ static void test_parent_wait() { close_pipes(pipes); } +/* Test: child waits for a lock, and the wait is interrupted by EINTR due to alarm. */ +static void test_child_eintr() { + printf("testing child EINTR...\n"); + unlock(0, 0); + + int pipes[2][2]; + open_pipes(pipes); + + lock(F_WRLCK, 0, 100); + + pid_t pid = fork(); + if (pid < 0) + err(1, "fork"); + + if (pid == 0) { + alarm(1); + + lock_wait_eintr(F_WRLCK, 0, 100); + write_pipe(pipes[0]); + read_pipe(pipes[1]); + lock(F_WRLCK, 0, 100); + exit(0); + } + + /* parent process: */ + + read_pipe(pipes[0]); + unlock(0, 100); + write_pipe(pipes[1]); + + wait_for_child(); + close_pipes(pipes); +} + +/* Test: parent waits for a lock, and the wait is interrupted by EINTR due to alarm. */ +static void test_parent_eintr() { + printf("testing parent EINTR...\n"); + unlock(0, 0); + + int pipes[2][2]; + open_pipes(pipes); + + pid_t pid = fork(); + if (pid < 0) + err(1, "fork"); + + if (pid == 0) { + lock(F_RDLCK, 0, 100); + write_pipe(pipes[0]); + read_pipe(pipes[1]); + unlock(0, 100); + write_pipe(pipes[0]); + exit(0); + } + + /* parent process: */ + + read_pipe(pipes[0]); + alarm(1); + lock_wait_eintr(F_WRLCK, 0, 100); + write_pipe(pipes[1]); + read_pipe(pipes[0]); + lock(F_WRLCK, 0, 100); + + wait_for_child(); + close_pipes(pipes); +} + /* Test: check that a range until EOF (len == 0) is handled correctly. */ static void test_range_with_eof() { printf("testing range with EOF...\n"); @@ -400,9 +505,15 @@ static void test_range_with_eof() { close_pipes(pipes); } +static void alarm_handler(int signum) { + fprintf(stderr, "got signal %d\n", signum); + fflush(stderr); +} int main(void) { setbuf(stdout, NULL); + if (signal(SIGALRM, &alarm_handler) < 0) + err(1, "signal"); g_fd = open(TEST_FILE, O_RDWR | O_CREAT | O_TRUNC, 0600); if (g_fd < 0) @@ -413,6 +524,8 @@ int main(void) { test_file_close(); test_child_wait(); test_parent_wait(); + test_child_eintr(); + test_parent_eintr(); test_range_with_eof(); if (close(g_fd) < 0)