From ad5662b892f933fe736659f53aad6e15c4076d17 Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Tue, 24 Aug 2021 15:52:30 -0700 Subject: [PATCH] Handle dupe invalidation requests during recovery Client lock invalidation handling was very strict about not receiving duplicate invalidation requests from the server because it could only track one pending request. The promise to only send one invalidate at a time is made by one server, it can't be enforced across server failover. Particularly because invalidation processing can have to do quite a lot of work with the server as it tears down state associated with the lock. We fix this by recording and processing each individual incoming invalidation request on the lock. The code that handled reordering of incoming grant responses and invalidation requests waited for the lock's mode to match the old mode in the invalidation request before proceeding. That would have prevented duplicate invalidation requests from making forward progress. To fix this we make lock client recieve processing synchronous instead of going through async work which can reorder. Now grant responses are processed as they're received and will always be resolved before all the invalidation requests are queued and processed in order. Signed-off-by: Zach Brown --- kmod/src/counters.h | 1 - kmod/src/lock.c | 222 +++++++++++++++++++------------------------- kmod/src/lock.h | 7 +- kmod/src/net.c | 11 ++- 4 files changed, 108 insertions(+), 133 deletions(-) diff --git a/kmod/src/counters.h b/kmod/src/counters.h index 7504515a..0e7db927 100644 --- a/kmod/src/counters.h +++ b/kmod/src/counters.h @@ -122,7 +122,6 @@ EXPAND_COUNTER(lock_free) \ EXPAND_COUNTER(lock_grant_request) \ EXPAND_COUNTER(lock_grant_response) \ - EXPAND_COUNTER(lock_grant_work) \ EXPAND_COUNTER(lock_invalidate_coverage) \ EXPAND_COUNTER(lock_invalidate_inode) \ EXPAND_COUNTER(lock_invalidate_request) \ diff --git a/kmod/src/lock.c b/kmod/src/lock.c index e85fa7ff..41479ded 100644 --- a/kmod/src/lock.c +++ b/kmod/src/lock.c @@ -80,8 +80,6 @@ struct lock_info { struct list_head lru_list; unsigned long long lru_nr; struct workqueue_struct *workq; - struct work_struct grant_work; - struct list_head grant_list; struct work_struct inv_work; struct list_head inv_list; struct work_struct shrink_work; @@ -253,7 +251,6 @@ static void lock_free(struct lock_info *linfo, struct scoutfs_lock *lock) BUG_ON(!RB_EMPTY_NODE(&lock->node)); BUG_ON(!RB_EMPTY_NODE(&lock->range_node)); BUG_ON(!list_empty(&lock->lru_head)); - BUG_ON(!list_empty(&lock->grant_head)); BUG_ON(!list_empty(&lock->inv_head)); BUG_ON(!list_empty(&lock->shrink_head)); BUG_ON(!list_empty(&lock->cov_list)); @@ -281,8 +278,8 @@ static struct scoutfs_lock *lock_alloc(struct super_block *sb, RB_CLEAR_NODE(&lock->node); RB_CLEAR_NODE(&lock->range_node); INIT_LIST_HEAD(&lock->lru_head); - INIT_LIST_HEAD(&lock->grant_head); INIT_LIST_HEAD(&lock->inv_head); + INIT_LIST_HEAD(&lock->inv_list); INIT_LIST_HEAD(&lock->shrink_head); spin_lock_init(&lock->cov_list_lock); INIT_LIST_HEAD(&lock->cov_list); @@ -545,14 +542,6 @@ static void put_lock(struct lock_info *linfo,struct scoutfs_lock *lock) } } -static void queue_grant_work(struct lock_info *linfo) -{ - assert_spin_locked(&linfo->lock); - - if (!list_empty(&linfo->grant_list)) - queue_work(linfo->workq, &linfo->grant_work); -} - /* * The caller has made a change (set a lock mode) which can let one of the * invalidating locks make forward progress. @@ -610,60 +599,13 @@ static void bug_on_inconsistent_grant_cache(struct super_block *sb, } /* - * Each lock has received a grant response message from the server. + * The client is receiving a grant response message from the server. + * This is being called synchronously in the networking receive path so + * our work should be quick and reasonably non-blocking. * - * Grant responses can be reordered with incoming invalidation requests - * from the server so we have to be careful to only set the new mode - * once the old mode matches. - */ -static void lock_grant_worker(struct work_struct *work) -{ - struct lock_info *linfo = container_of(work, struct lock_info, - grant_work); - struct super_block *sb = linfo->sb; - struct scoutfs_net_lock *nl; - struct scoutfs_lock *lock; - struct scoutfs_lock *tmp; - - scoutfs_inc_counter(sb, lock_grant_work); - - spin_lock(&linfo->lock); - - list_for_each_entry_safe(lock, tmp, &linfo->grant_list, grant_head) { - nl = &lock->grant_nl; - - /* wait for reordered invalidation to finish */ - if (lock->mode != nl->old_mode) - continue; - - bug_on_inconsistent_grant_cache(sb, lock, nl->old_mode, - nl->new_mode); - - if (!lock_mode_can_read(nl->old_mode) && - lock_mode_can_read(nl->new_mode)) { - lock->refresh_gen = - atomic64_inc_return(&linfo->next_refresh_gen); - } - - lock->request_pending = 0; - lock->mode = nl->new_mode; - lock->write_seq = le64_to_cpu(nl->write_seq); - - trace_scoutfs_lock_granted(sb, lock); - list_del_init(&lock->grant_head); - wake_up(&lock->waitq); - put_lock(linfo, lock); - } - - /* invalidations might be waiting for our reordered grant */ - queue_inv_work(linfo); - spin_unlock(&linfo->lock); -} - -/* - * The client is receiving a grant response message from the server. We - * find the lock, record the response, and add it to the list for grant - * work to process. + * The server's state machine can immediately send an invalidate request + * after sending this grant response. We won't process the incoming + * invalidate request until after processing this grant response. */ int scoutfs_lock_grant_response(struct super_block *sb, struct scoutfs_net_lock *nl) @@ -681,45 +623,51 @@ int scoutfs_lock_grant_response(struct super_block *sb, trace_scoutfs_lock_grant_response(sb, lock); BUG_ON(!lock->request_pending); - lock->grant_nl = *nl; - list_add_tail(&lock->grant_head, &linfo->grant_list); - queue_grant_work(linfo); + bug_on_inconsistent_grant_cache(sb, lock, nl->old_mode, nl->new_mode); + + if (!lock_mode_can_read(nl->old_mode) && lock_mode_can_read(nl->new_mode)) + lock->refresh_gen = atomic64_inc_return(&linfo->next_refresh_gen); + + lock->request_pending = 0; + lock->mode = nl->new_mode; + lock->write_seq = le64_to_cpu(nl->write_seq); + + trace_scoutfs_lock_granted(sb, lock); + wake_up(&lock->waitq); + put_lock(linfo, lock); spin_unlock(&linfo->lock); return 0; } +struct inv_req { + struct list_head head; + struct scoutfs_lock *lock; + u64 net_id; + struct scoutfs_net_lock nl; +}; + /* * Each lock has received a lock invalidation request from the server - * which specifies a new mode for the lock. The server will only send - * one invalidation request at a time for each lock. The server can - * send another invalidate request after we send the response but before - * we reacquire the lock and finish invalidation. + * which specifies a new mode for the lock. Our processing state + * machine and server failover and lock recovery can both conspire to + * give us triplicate invalidation requests. The incoming requests for + * a given lock need to be processed in order, but we can process locks + * in any order. * * This is an unsolicited request from the server so it can arrive at - * any time after we make the server aware of the lock by initially - * requesting it. We wait for users of the current mode to unlock - * before invalidating. + * any time after we make the server aware of the lock. We wait for + * users of the current mode to unlock before invalidating. * * This can arrive on behalf of our request for a mode that conflicts * with our current mode. We have to proceed while we have a request * pending. We can also be racing with shrink requests being sent while * we're invalidating. * - * This can be processed concurrently and experience reordering with a - * grant response sent back-to-back from the server. We carefully only - * invalidate once the lock mode matches what the server told us to - * invalidate. - * * Before we start invalidating the lock we set the lock to the new * mode, preventing further incompatible users of the old mode from * using the lock while we're invalidating. - * - * This does a lot of serialized inode invalidation in one context and - * performs a lot of repeated calls to sync. It would be nice to get - * some concurrent inode invalidation and to more carefully only call - * sync when needed. */ static void lock_invalidate_worker(struct work_struct *work) { @@ -728,8 +676,8 @@ static void lock_invalidate_worker(struct work_struct *work) struct scoutfs_net_lock *nl; struct scoutfs_lock *lock; struct scoutfs_lock *tmp; + struct inv_req *ireq; LIST_HEAD(ready); - u64 net_id; int ret; scoutfs_inc_counter(sb, lock_invalidate_work); @@ -737,11 +685,8 @@ static void lock_invalidate_worker(struct work_struct *work) spin_lock(&linfo->lock); list_for_each_entry_safe(lock, tmp, &linfo->inv_list, inv_head) { - nl = &lock->inv_nl; - - /* wait for reordered grant to finish */ - if (lock->mode != nl->old_mode) - continue; + ireq = list_first_entry(&lock->inv_list, struct inv_req, head); + nl = &ireq->nl; /* wait until incompatible holders unlock */ if (!lock_counts_match(nl->new_mode, lock->users)) @@ -761,8 +706,8 @@ static void lock_invalidate_worker(struct work_struct *work) /* invalidate once the lock is read */ list_for_each_entry(lock, &ready, inv_head) { - nl = &lock->inv_nl; - net_id = lock->inv_net_id; + ireq = list_first_entry(&lock->inv_list, struct inv_req, head); + nl = &ireq->nl; /* only lock protocol, inv can't call subsystems after shutdown */ if (!linfo->shutdown) { @@ -770,11 +715,10 @@ static void lock_invalidate_worker(struct work_struct *work) BUG_ON(ret); } - /* allow another request after we respond but before we finish */ - lock->inv_net_id = 0; - - /* respond with the key and modes from the request */ - ret = scoutfs_client_lock_response(sb, net_id, nl); + /* respond with the key and modes from the request, server might have died */ + ret = scoutfs_client_lock_response(sb, ireq->net_id, nl); + if (ret == -ENOTCONN) + ret = 0; BUG_ON(ret); scoutfs_inc_counter(sb, lock_invalidate_response); @@ -784,66 +728,87 @@ static void lock_invalidate_worker(struct work_struct *work) spin_lock(&linfo->lock); list_for_each_entry_safe(lock, tmp, &ready, inv_head) { + ireq = list_first_entry(&lock->inv_list, struct inv_req, head); + trace_scoutfs_lock_invalidated(sb, lock); - if (lock->inv_net_id == 0) { + + list_del(&ireq->head); + kfree(ireq); + + if (list_empty(&lock->inv_list)) { /* finish if another request didn't arrive */ list_del_init(&lock->inv_head); lock->invalidate_pending = 0; wake_up(&lock->waitq); } else { - /* another request filled nl/net_id, back on the list and requeue */ + /* another request arrived, back on the list and requeue */ list_move_tail(&lock->inv_head, &linfo->inv_list); queue_inv_work(linfo); } + put_lock(linfo, lock); } - /* grant might have been waiting for invalidate request */ - queue_grant_work(linfo); spin_unlock(&linfo->lock); } /* - * Record an incoming invalidate request from the server and add its - * lock to the list for processing. This request can be from a new - * server and racing with invalidation that frees from an old server. - * It's fine to not find the requested lock and send an immediate - * response. + * Add an incoming invalidation request to the end of the list on the + * lock and queue it for blocking invalidation work. This is being + * called synchronously in the net recv path to avoid reordering with + * grants that were sent immediately before the server sent this + * invalidation. * - * The invalidation process drops the linfo lock to send responses. The - * moment it does so we can receive another invalidation request (the - * server can ask us to go from write->read then read->null). We allow - * for one chain like this but it's a bug if we receive more concurrent - * invalidation requests than that. The server should be only sending - * one at a time. + * Incoming invalidation requests are a function of the remote lock + * server's state machine and are slightly decoupled from our lock + * state. We can receive duplicate requests if the server is quick + * enough to send the next request after we send a previous reply, or if + * pending invalidation spans server failover and lock recovery. + * + * Similarly, we can get a request to invalidate a lock we don't have if + * invalidation finished just after lock recovery to a new server. + * Happily we can just reply because we satisfy the invalidation + * response promise to not be using the old lock's mode if the lock + * doesn't exist. */ int scoutfs_lock_invalidate_request(struct super_block *sb, u64 net_id, struct scoutfs_net_lock *nl) { DECLARE_LOCK_INFO(sb, linfo); - struct scoutfs_lock *lock; + struct scoutfs_lock *lock = NULL; + struct inv_req *ireq; int ret = 0; scoutfs_inc_counter(sb, lock_invalidate_request); + ireq = kmalloc(sizeof(struct inv_req), GFP_NOFS); + BUG_ON(!ireq); /* lock server doesn't handle response errors */ + if (ireq == NULL) { + ret = -ENOMEM; + goto out; + } + spin_lock(&linfo->lock); lock = get_lock(sb, &nl->key); if (lock) { - BUG_ON(lock->inv_net_id != 0); - lock->inv_net_id = net_id; - lock->inv_nl = *nl; - if (list_empty(&lock->inv_head)) { + trace_scoutfs_lock_invalidate_request(sb, lock); + ireq->lock = lock; + ireq->net_id = net_id; + ireq->nl = *nl; + if (list_empty(&lock->inv_list)) { list_add_tail(&lock->inv_head, &linfo->inv_list); lock->invalidate_pending = 1; queue_inv_work(linfo); - /* otherwise inv work queues itself when it sees inv_net_id */ } - trace_scoutfs_lock_invalidate_request(sb, lock); + list_add_tail(&ireq->head, &lock->inv_list); } spin_unlock(&linfo->lock); - if (!lock) +out: + if (!lock) { ret = scoutfs_client_lock_response(sb, net_id, nl); + BUG_ON(ret); /* lock server doesn't fence timed out client requests */ + } return ret; } @@ -1598,6 +1563,8 @@ void scoutfs_lock_destroy(struct super_block *sb) struct scoutfs_sb_info *sbi = SCOUTFS_SB(sb); DECLARE_LOCK_INFO(sb, linfo); struct scoutfs_lock *lock; + struct inv_req *ireq_tmp; + struct inv_req *ireq; struct rb_node *node; enum scoutfs_lock_mode mode; @@ -1640,15 +1607,21 @@ void scoutfs_lock_destroy(struct super_block *sb) * of free). */ spin_lock(&linfo->lock); + node = rb_first(&linfo->lock_tree); while (node) { lock = rb_entry(node, struct scoutfs_lock, node); node = rb_next(node); + + list_for_each_entry_safe(ireq, ireq_tmp, &lock->inv_list, head) { + list_del_init(&ireq->head); + put_lock(linfo, ireq->lock); + kfree(ireq); + } + lock->request_pending = 0; if (!list_empty(&lock->lru_head)) __lock_del_lru(linfo, lock); - if (!list_empty(&lock->grant_head)) - list_del_init(&lock->grant_head); if (!list_empty(&lock->inv_head)) { list_del_init(&lock->inv_head); lock->invalidate_pending = 0; @@ -1658,6 +1631,7 @@ void scoutfs_lock_destroy(struct super_block *sb) lock_remove(linfo, lock); lock_free(linfo, lock); } + spin_unlock(&linfo->lock); kfree(linfo); @@ -1682,8 +1656,6 @@ int scoutfs_lock_setup(struct super_block *sb) linfo->shrinker.seeks = DEFAULT_SEEKS; register_shrinker(&linfo->shrinker); INIT_LIST_HEAD(&linfo->lru_list); - INIT_WORK(&linfo->grant_work, lock_grant_worker); - INIT_LIST_HEAD(&linfo->grant_list); INIT_WORK(&linfo->inv_work, lock_invalidate_worker); INIT_LIST_HEAD(&linfo->inv_list); INIT_WORK(&linfo->shrink_work, lock_shrink_worker); diff --git a/kmod/src/lock.h b/kmod/src/lock.h index c1848cf9..71b65464 100644 --- a/kmod/src/lock.h +++ b/kmod/src/lock.h @@ -31,11 +31,8 @@ struct scoutfs_lock { unsigned long request_pending:1, invalidate_pending:1; - struct list_head grant_head; - struct scoutfs_net_lock grant_nl; - struct list_head inv_head; - struct scoutfs_net_lock inv_nl; - u64 inv_net_id; + struct list_head inv_head; /* entry in linfo's list of locks with invalidations */ + struct list_head inv_list; /* list of lock's invalidation requests */ struct list_head shrink_head; spinlock_t cov_list_lock; diff --git a/kmod/src/net.c b/kmod/src/net.c index f7f4aa9e..8368f49b 100644 --- a/kmod/src/net.c +++ b/kmod/src/net.c @@ -677,8 +677,15 @@ static void scoutfs_net_recv_worker(struct work_struct *work) scoutfs_tseq_add(&ninf->msg_tseq_tree, &mrecv->tseq_entry); - /* synchronously process greeting before next recvmsg */ - if (nh.cmd == SCOUTFS_NET_CMD_GREETING) + /* + * Initial received greetings are processed + * synchronously before any other incoming messages. + * + * Incoming requests or responses to the lock client are + * called synchronously to avoid reordering. + */ + if (nh.cmd == SCOUTFS_NET_CMD_GREETING || + (nh.cmd == SCOUTFS_NET_CMD_LOCK && !conn->listening_conn)) scoutfs_net_proc_worker(&mrecv->proc_work); else queue_work(conn->workq, &mrecv->proc_work);