From 5946a6ec18976c0f52162fe0f47e9b5171af87ec Mon Sep 17 00:00:00 2001 From: Soumya Koduri Date: Mon, 6 Apr 2020 12:36:44 +0530 Subject: [PATCH 503/511] gfapi: Suspend synctasks instead of blocking them There are certain conditions which blocks the current execution thread (like waiting on mutex lock or condition variable or I/O response). In such cases, if it is a synctask thread, we should suspend the task instead of blocking it (like done in SYNCOP using synctask_yield) This is to avoid deadlock like the one mentioned below - 1) synctaskA sets fs->migration_in_progress to 1 and does I/O (LOOKUP) 2) Other synctask threads wait for fs->migration_in_progress to be reset to 0 by synctaskA and hence blocked 3) but synctaskA cannot resume as all synctask threads are blocked on (2). Note: this same approach is already used by few other components like syncbarrier etc. >Change-Id: If90f870d663bb242c702a5b86ac52eeda67c6f0d >Fixes: #1146 >Signed-off-by: Soumya Koduri Upstream patch: https://review.gluster.org/c/glusterfs/+/24276 BUG: 1779238 Change-Id: If90f870d663bb242c702a5b86ac52eeda67c6f0d Signed-off-by: nik-redhat Reviewed-on: https://code.engineering.redhat.com/gerrit/221081 Tested-by: RHGS Build Bot Reviewed-by: Soumya Koduri --- api/src/glfs-internal.h | 34 ++++++++++++++++++++++++++++++++-- api/src/glfs-resolve.c | 9 +++++++++ api/src/glfs.c | 9 +++++++++ 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/api/src/glfs-internal.h b/api/src/glfs-internal.h index 55401b2..15cf0ee 100644 --- a/api/src/glfs-internal.h +++ b/api/src/glfs-internal.h @@ -16,6 +16,7 @@ #include #include "glfs-handles.h" #include +#include #define GLFS_SYMLINK_MAX_FOLLOW 2048 @@ -207,6 +208,7 @@ struct glfs { glfs_upcall_cbk up_cbk; /* upcall cbk function to be registered */ void *up_data; /* Opaque data provided by application * during upcall registration */ + struct list_head waitq; /* waiting synctasks */ }; /* This enum is used to maintain the state of glfd. In case of async fops @@ -442,6 +444,34 @@ glfs_process_upcall_event(struct glfs *fs, void *data) THIS = glfd->fd->inode->table->xl->ctx->master; \ } while (0) +#define __GLFS_LOCK_WAIT(fs) \ + do { \ + struct synctask *task = NULL; \ + \ + task = synctask_get(); \ + \ + if (task) { \ + list_add_tail(&task->waitq, &fs->waitq); \ + pthread_mutex_unlock(&fs->mutex); \ + synctask_yield(task, NULL); \ + pthread_mutex_lock(&fs->mutex); \ + } else { \ + /* non-synctask */ \ + pthread_cond_wait(&fs->cond, &fs->mutex); \ + } \ + } while (0) + +#define __GLFS_SYNCTASK_WAKE(fs) \ + do { \ + struct synctask *waittask = NULL; \ + \ + while (!list_empty(&fs->waitq)) { \ + waittask = list_entry(fs->waitq.next, struct synctask, waitq); \ + list_del_init(&waittask->waitq); \ + synctask_wake(waittask); \ + } \ + } while (0) + /* By default all lock attempts from user context must use glfs_lock() and glfs_unlock(). This allows @@ -466,10 +496,10 @@ glfs_lock(struct glfs *fs, gf_boolean_t wait_for_migration) pthread_mutex_lock(&fs->mutex); while (!fs->init) - pthread_cond_wait(&fs->cond, &fs->mutex); + __GLFS_LOCK_WAIT(fs); while (wait_for_migration && fs->migration_in_progress) - pthread_cond_wait(&fs->cond, &fs->mutex); + __GLFS_LOCK_WAIT(fs); return 0; } diff --git a/api/src/glfs-resolve.c b/api/src/glfs-resolve.c index 062b7dc..58b6ace 100644 --- a/api/src/glfs-resolve.c +++ b/api/src/glfs-resolve.c @@ -65,6 +65,9 @@ __glfs_first_lookup(struct glfs *fs, xlator_t *subvol) fs->migration_in_progress = 0; pthread_cond_broadcast(&fs->cond); + /* wake up other waiting tasks */ + __GLFS_SYNCTASK_WAKE(fs); + return ret; } @@ -154,6 +157,9 @@ __glfs_refresh_inode(struct glfs *fs, xlator_t *subvol, inode_t *inode, fs->migration_in_progress = 0; pthread_cond_broadcast(&fs->cond); + /* wake up other waiting tasks */ + __GLFS_SYNCTASK_WAKE(fs); + return newinode; } @@ -841,6 +847,9 @@ __glfs_migrate_fd(struct glfs *fs, xlator_t *newsubvol, struct glfs_fd *glfd) fs->migration_in_progress = 0; pthread_cond_broadcast(&fs->cond); + /* wake up other waiting tasks */ + __GLFS_SYNCTASK_WAKE(fs); + return newfd; } diff --git a/api/src/glfs.c b/api/src/glfs.c index f36616d..ae994fa 100644 --- a/api/src/glfs.c +++ b/api/src/glfs.c @@ -740,6 +740,7 @@ glfs_new_fs(const char *volname) INIT_LIST_HEAD(&fs->openfds); INIT_LIST_HEAD(&fs->upcall_list); + INIT_LIST_HEAD(&fs->waitq); PTHREAD_MUTEX_INIT(&fs->mutex, NULL, fs->pthread_flags, GLFS_INIT_MUTEX, err); @@ -1228,6 +1229,7 @@ pub_glfs_fini(struct glfs *fs) call_pool_t *call_pool = NULL; int fs_init = 0; int err = -1; + struct synctask *waittask = NULL; DECLARE_OLD_THIS; @@ -1249,6 +1251,13 @@ pub_glfs_fini(struct glfs *fs) call_pool = fs->ctx->pool; + /* Wake up any suspended synctasks */ + while (!list_empty(&fs->waitq)) { + waittask = list_entry(fs->waitq.next, struct synctask, waitq); + list_del_init(&waittask->waitq); + synctask_wake(waittask); + } + while (countdown--) { /* give some time for background frames to finish */ pthread_mutex_lock(&fs->mutex); -- 1.8.3.1