[hurd] 14/30: Fix race condition in ext2fs when remounting

[Samuel Thibault]

This is an automated email from the git hooks/post-receive script.

sthibault pushed a commit to branch upstream
in repository hurd.

commit 7a2c17fcbb789997421725d726340301ef35d84c
Author: James Clarke <address@hidden>
Date:   Thu Aug 27 17:22:11 2015 +0100

    Fix race condition in ext2fs when remounting
    
    On some systems, ext2fs.static would regularly hang at startup, as a
    race condition meant it would process paging requests while remounting.
    To fix this, libpager has been altered to allow inhibiting and resuming
    its worker threads, and ext2fs uses this to inhibit paging while
    remounting.
    
    * console/pager.c (pager_requests): New variable.
    (user_pager_init): Updated call to pager_start_workers to use new
    pager_requests variable.
    * daemons/runsystem.sh: Removed artificial delay working around the race
    condition.
    * ext2fs/ext2fs.c (diskfs_reload_global_state): Call new
    inhibit_ext2_pager and resume_ext2_pager functions, and leave sblock as
    non-NULL so it will be munmapped.
    * ext2fs/ext2fs.h (inhibit_ext2_pager,resume_ext2_pager): New functions.
    * ext2fs/pager.c (file_pager_requests): New variable.
    (create_disk_pager): Updated call to pager_start_workers to use new
    file_pager_requests variable.
    (inhibit_ext2_pager,resume_ext2_pager): New functions.
    * fatfs/fatfs.h (inhibit_fat_pager,resume_fat_pager): New functions.
    * fatfs/pager.c (file_pager_requests): New variable.
    (create_fat_pager): Updated call to pager_start_workers to use new
    file_pager_requests variable.
    (inhibit_fat_pager,resume_fat_pager): New functions.
    * libdiskfs/disk-pager.c (diskfs_disk_pager_requests): New variable.
    (diskfs_start_disk_pager): Updated call to pager_start_workers to use
    new diskfs_disk_pager_requests variable.
    * libdiskfs/diskfs-pager.h (diskfs_disk_pager_requests): New variable.
    * libpager/demuxer.c (struct pager_requests): Renamed struct requests to
    struct pager_requests. Replaced queue with queue_in and queue_out
    pointers. Added inhibit_wakeup field.
    (pager_demuxer): Updated to use new queue_in/queue_out pointers. Only
    wake up workers if not inhibited.
    (worker_func): Updated to use new queue_in/queue_out pointers. Final
    worker thread to sleep notifies the inhibit_wakeup condition variable.
    (pager_start_workers): Added out parameter for the requests instance.
    Allocate heap space shared by both queues. Initialise new inhibit_wakeup
    condition.
    (pager_inhibit_workers,pager_resume_workers): New functions.
    * libpager/pager.h (struct pager_requests): Public forward definition.
    (pager_start_workers): Added out parameter for the requests instance.
    (pager_inhibit_workers,pager_resume_workers): New functions.
    * libpager/queue.h (queue_empty): New function.
    * storeio/pager.c (pager_requests): New variable.
    (init_dev_paging): Updated call to pager_start_workers to use new
    pager_requests variable.
---
 console/pager.c          |   3 +-
 daemons/runsystem.sh     |   3 --
 ext2fs/ext2fs.c          |  12 ++++-
 ext2fs/ext2fs.h          |   6 +++
 ext2fs/pager.c           |  33 ++++++++++++-
 fatfs/fatfs.h            |   2 +
 fatfs/pager.c            |  33 ++++++++++++-
 libdiskfs/disk-pager.c   |   3 +-
 libdiskfs/diskfs-pager.h |   1 +
 libpager/demuxer.c       | 119 ++++++++++++++++++++++++++++++++++++++++-------
 libpager/pager.h         |  28 ++++++++++-
 libpager/queue.h         |   8 ++++
 storeio/pager.c          |   3 +-
 13 files changed, 227 insertions(+), 27 deletions(-)

diff --git a/console/pager.c b/console/pager.c
index 5e13ba4..818e49d 100644
--- a/console/pager.c
+++ b/console/pager.c
@@ -42,6 +42,7 @@ struct user_pager_info
 
 /* We need a separate bucket for the pager ports.  */
 static struct port_bucket *pager_bucket;
+static struct pager_requests *pager_requests;
 
 
 /* Implement the pager_clear_user_data callback from the pager library. */
@@ -133,7 +134,7 @@ user_pager_init (void)
     error (5, errno, "Cannot create pager bucket");
 
   /* Start libpagers worker threads.  */
-  err = pager_start_workers (pager_bucket);
+  err = pager_start_workers (pager_bucket, &pager_requests);
   if (err)
     error (5, err, "Cannot start pager worker threads");
 }
diff --git a/daemons/runsystem.sh b/daemons/runsystem.sh
index ae25a7d..5d0ad01 100644
--- a/daemons/runsystem.sh
+++ b/daemons/runsystem.sh
@@ -118,9 +118,6 @@ esac
 /hurd/mach-defpager
 
 # This is necessary to make stat / return the correct device ids.
-# Work around a race condition (probably in the root translator).
-for i in `seq 1 100000` ; do : ; done # XXX
-
 fsysopts / --update --readonly
 
 # Finally, start the actual init.
diff --git a/ext2fs/ext2fs.c b/ext2fs/ext2fs.c
index d0fdfe7..03c9eed 100644
--- a/ext2fs/ext2fs.c
+++ b/ext2fs/ext2fs.c
@@ -207,10 +207,20 @@ main (int argc, char **argv)
 error_t
 diskfs_reload_global_state ()
 {
+  error_t err;
+
   pokel_flush (&global_pokel);
   pager_flush (diskfs_disk_pager, 1);
-  sblock = NULL;
+
+  /* libdiskfs is not responsible for inhibiting paging.  */
+  err = inhibit_ext2_pager ();
+  if (err)
+    return err;
+
   get_hypermetadata ();
   map_hypermetadata ();
+
+  resume_ext2_pager ();
+
   return 0;
 }
diff --git a/ext2fs/ext2fs.h b/ext2fs/ext2fs.h
index 72b7b09..42717c5 100644
--- a/ext2fs/ext2fs.h
+++ b/ext2fs/ext2fs.h
@@ -202,6 +202,12 @@ struct user_pager_info
 /* Set up the disk pager.  */
 void create_disk_pager (void);
 
+/* Inhibit the disk pager.  */
+error_t inhibit_ext2_pager (void);
+
+/* Resume the disk pager.  */
+void resume_ext2_pager (void);
+
 /* Call this when we should turn off caching so that unused memory object
    ports get freed.  */
 void drop_pager_softrefs (struct node *node);
diff --git a/ext2fs/pager.c b/ext2fs/pager.c
index b56c923..3e080f8 100644
--- a/ext2fs/pager.c
+++ b/ext2fs/pager.c
@@ -34,6 +34,10 @@ struct port_bucket *disk_pager_bucket;
 /* A ports bucket to hold file pager ports.  */
 struct port_bucket *file_pager_bucket;
 
+/* Stores a reference to the requests instance used by the file pager so its
+   worker threads can be inhibited and resumed.  */
+struct pager_requests *file_pager_requests;
+
 pthread_spinlock_t node_to_page_lock = PTHREAD_SPINLOCK_INITIALIZER;
 
 
@@ -1217,11 +1221,38 @@ create_disk_pager (void)
   file_pager_bucket = ports_create_bucket ();
 
   /* Start libpagers worker threads.  */
-  err = pager_start_workers (file_pager_bucket);
+  err = pager_start_workers (file_pager_bucket, &file_pager_requests);
   if (err)
     ext2_panic ("can't create libpager worker threads: %s", strerror (err));
 }
 
+error_t
+inhibit_ext2_pager (void)
+{
+  error_t err;
+
+  /* The file pager can rely on the disk pager, so inhibit the file
+     pager first.  */
+
+  err = pager_inhibit_workers (file_pager_requests);
+  if (err)
+    return err;
+
+  err = pager_inhibit_workers (diskfs_disk_pager_requests);
+  /* We don't want only one pager disabled.  */
+  if (err)
+    pager_resume_workers (file_pager_requests);
+
+  return err;
+}
+
+void
+resume_ext2_pager (void)
+{
+  pager_resume_workers (diskfs_disk_pager_requests);
+  pager_resume_workers (file_pager_requests);
+}
+
 /* Call this to create a FILE_DATA pager and return a send right.
    NODE must be locked.  */
 mach_port_t
diff --git a/fatfs/fatfs.h b/fatfs/fatfs.h
index 3c3d836..54c3426 100644
--- a/fatfs/fatfs.h
+++ b/fatfs/fatfs.h
@@ -121,6 +121,8 @@ extern struct dirrect dr_root_node;
 void drop_pager_softrefs (struct node *);
 void allow_pager_softrefs (struct node *);
 void create_fat_pager (void);
+error_t inhibit_fat_pager (void);
+void resume_fat_pager (void);
 
 void flush_node_pager (struct node *node);
 
diff --git a/fatfs/pager.c b/fatfs/pager.c
index 10d1fc9..d255f29 100644
--- a/fatfs/pager.c
+++ b/fatfs/pager.c
@@ -29,6 +29,10 @@ struct port_bucket *disk_pager_bucket;
 /* A ports bucket to hold file pager ports.  */
 struct port_bucket *file_pager_bucket;
 
+/* Stores a reference to the requests instance used by the file pager so its
+   worker threads can be inhibited and resumed.  */
+struct pager_requests *file_pager_requests;
+
 /* Mapped image of the FAT.  */
 void *fat_image;
 
@@ -776,11 +780,38 @@ create_fat_pager (void)
   file_pager_bucket = ports_create_bucket ();
 
   /* Start libpagers worker threads.  */
-  err = pager_start_workers (file_pager_bucket);
+  err = pager_start_workers (file_pager_bucket, &file_pager_requests);
   if (err)
     error (2, err, "can't create libpager worker threads");
 }
 
+error_t
+inhibit_fat_pager (void)
+{
+  error_t err;
+
+  /* The file pager can rely on the disk pager, so inhibit the file
+     pager first.  */
+
+  err = pager_inhibit_workers (file_pager_requests);
+  if (err)
+    return err;
+
+  err = pager_inhibit_workers (diskfs_disk_pager_requests);
+  /* We don't want only one pager disabled.  */
+  if (err)
+    pager_resume_workers (file_pager_requests);
+
+  return err;
+}
+
+void
+resume_fat_pager (void)
+{
+  pager_resume_workers (diskfs_disk_pager_requests);
+  pager_resume_workers (file_pager_requests);
+}
+
 /* Call this to create a FILE_DATA pager and return a send right.
    NODE must be locked.  */
 mach_port_t
diff --git a/libdiskfs/disk-pager.c b/libdiskfs/disk-pager.c
index 008aa2d..434ceed 100644
--- a/libdiskfs/disk-pager.c
+++ b/libdiskfs/disk-pager.c
@@ -24,6 +24,7 @@
 __thread struct disk_image_user *diskfs_exception_diu;
 
 struct pager *diskfs_disk_pager;
+struct pager_requests *diskfs_disk_pager_requests;
 
 static void fault_handler (int sig, long int sigcode, struct sigcontext *scp);
 static struct hurd_signal_preemptor preemptor =
@@ -43,7 +44,7 @@ diskfs_start_disk_pager (struct user_pager_info *upi,
   mach_port_t disk_pager_port;
 
   /* Start libpagers worker threads.  */
-  err = pager_start_workers (pager_bucket);
+  err = pager_start_workers (pager_bucket, &diskfs_disk_pager_requests);
   if (err)
     error (2, err, "creating pager worker threads failed");
 
diff --git a/libdiskfs/diskfs-pager.h b/libdiskfs/diskfs-pager.h
index a253069..550ca64 100644
--- a/libdiskfs/diskfs-pager.h
+++ b/libdiskfs/diskfs-pager.h
@@ -40,6 +40,7 @@ extern void diskfs_start_disk_pager (struct user_pager_info 
*info,
                                     size_t size, void **image);
 
 extern struct pager *diskfs_disk_pager;
+extern struct pager_requests *diskfs_disk_pager_requests;
 
 struct disk_image_user
   {
diff --git a/libpager/demuxer.c b/libpager/demuxer.c
index 4dd3cd8..59dd1c5 100644
--- a/libpager/demuxer.c
+++ b/libpager/demuxer.c
@@ -60,7 +60,7 @@ request_inp (const struct request *r)
 /* A worker.  */
 struct worker
 {
-  struct requests *requests;   /* our pagers request queue */
+  struct pager_requests *requests;     /* our pagers request queue */
   struct queue queue;  /* other workers may delegate requests to us */
   unsigned long tag;   /* tag of the object we are working on */
 };
@@ -68,12 +68,18 @@ struct worker
 /* This is the queue for incoming requests.  A single thread receives
    messages from the port set, looks the service routine up, and
    enqueues the request here.  */
-struct requests
+struct pager_requests
 {
   struct port_bucket *bucket;
-  struct queue queue;
+  /* Normally, both queues are the same.  However, when the workers are
+     inhibited, a new queue_in is created, but queue_out is left as the
+     old value, so the workers drain queue_out but do not receive new
+     requests.  */
+  struct queue *queue_in;      /* the queue to add to */
+  struct queue *queue_out;     /* the queue to take from */
   int asleep;
   pthread_cond_t wakeup;
+  pthread_cond_t inhibit_wakeup;
   pthread_mutex_t lock;
   struct worker workers[WORKER_COUNT];
 };
@@ -81,7 +87,7 @@ struct requests
 /* Demultiplex a single message directed at a pager port; INP is the
    message received; fill OUTP with the reply.  */
 static int
-pager_demuxer (struct requests *requests,
+pager_demuxer (struct pager_requests *requests,
               mach_msg_header_t *inp,
               mach_msg_header_t *outp)
 {
@@ -108,10 +114,10 @@ pager_demuxer (struct requests *requests,
 
   pthread_mutex_lock (&requests->lock);
 
-  queue_enqueue (&requests->queue, &r->item);
+  queue_enqueue (requests->queue_in, &r->item);
 
-  /* Awake worker.  */
-  if (requests->asleep > 0)
+  /* Awake worker, but only if not inhibited.  */
+  if (requests->asleep > 0 && requests->queue_in == requests->queue_out)
       pthread_cond_signal (&requests->wakeup);
 
   pthread_mutex_unlock (&requests->lock);
@@ -160,7 +166,7 @@ static void *
 worker_func (void *arg)
 {
   struct worker *self = (struct worker *) arg;
-  struct requests *requests = self->requests;
+  struct pager_requests *requests = self->requests;
   struct request *r = NULL;
   mig_reply_header_t reply_msg;
 
@@ -186,9 +192,11 @@ worker_func (void *arg)
 
     get_request_locked:
       /* ... get a request from the global queue instead.  */
-      while ((r = queue_dequeue (&requests->queue)) == NULL)
+      while ((r = queue_dequeue (requests->queue_out)) == NULL)
        {
          requests->asleep += 1;
+         if (requests->asleep == WORKER_COUNT)
+           pthread_cond_broadcast (&requests->inhibit_wakeup);
          pthread_cond_wait (&requests->wakeup, &requests->lock);
          requests->asleep -= 1;
        }
@@ -281,7 +289,7 @@ worker_func (void *arg)
 static void *
 service_paging_requests (void *arg)
 {
-  struct requests *requests = arg;
+  struct pager_requests *requests = arg;
 
   int demuxer (mach_msg_header_t *inp,
               mach_msg_header_t *outp)
@@ -298,27 +306,44 @@ service_paging_requests (void *arg)
 
 /* Start the worker threads libpager uses to service requests.  */
 error_t
-pager_start_workers (struct port_bucket *pager_bucket)
+pager_start_workers (struct port_bucket *pager_bucket,
+                    struct pager_requests **out_requests)
 {
   error_t err;
   int i;
   pthread_t t;
-  struct requests *requests;
+  struct pager_requests *requests;
+
+  assert (out_requests != NULL);
 
   requests = malloc (sizeof *requests);
   if (requests == NULL)
-    return ENOMEM;
+    {
+      err = ENOMEM;
+      goto done;
+    }
 
   requests->bucket = pager_bucket;
   requests->asleep = 0;
-  queue_init (&requests->queue);
+
+  requests->queue_in = malloc (sizeof *requests->queue_in);
+  if (requests->queue_in == NULL)
+    {
+      err = ENOMEM;
+      goto done;
+    }
+  queue_init (requests->queue_in);
+  /* Until the workers are inhibited, both queues are the same.  */
+  requests->queue_out = requests->queue_in;
+
   pthread_cond_init (&requests->wakeup, NULL);
+  pthread_cond_init (&requests->inhibit_wakeup, NULL);
   pthread_mutex_init (&requests->lock, NULL);
 
   /* Make a thread to service paging requests.  */
   err = pthread_create (&t, NULL, service_paging_requests, requests);
   if (err)
-    return err;
+    goto done;
   pthread_detach (t);
 
   for (i = 0; i < WORKER_COUNT; i++)
@@ -329,9 +354,71 @@ pager_start_workers (struct port_bucket *pager_bucket)
 
       err = pthread_create (&t, NULL, &worker_func, &requests->workers[i]);
       if (err)
-       return err;
+       goto done;
       pthread_detach (t);
     }
 
+done:
+  if (err)
+    *out_requests = NULL;
+  else
+    *out_requests = requests;
+
   return err;
 }
+
+error_t
+pager_inhibit_workers (struct pager_requests *requests)
+{
+  error_t err = 0;
+
+  pthread_mutex_lock (&requests->lock);
+
+  /* Check the workers are not already inhibited.  */
+  assert (requests->queue_out == requests->queue_in);
+
+  /* Any new paging requests will go into a new queue.  */
+  struct queue *new_queue = malloc (sizeof *new_queue);
+  if (new_queue == NULL)
+    {
+      err = ENOMEM;
+      goto done_locked;
+    }
+  queue_init (new_queue);
+  requests->queue_in = new_queue;
+
+  /* Wait until all the workers are asleep and the queue has been
+     drained. All individual worker queues must have been drained, as
+     they are populated while the relevant worker is still running, and
+     it will always drain its personal queue before sleeping.
+     Check that the queue is empty, since it's possible that a request
+     came in, was queued and a worker was signalled but the lock was
+     acquired here before the worker woke up.  */
+  while (requests->asleep < WORKER_COUNT || !queue_empty(requests->queue_out))
+    pthread_cond_wait (&requests->inhibit_wakeup, &requests->lock);
+
+done_locked:
+  pthread_mutex_unlock (&requests->lock);
+  return err;
+}
+
+void
+pager_resume_workers (struct pager_requests *requests)
+{
+  pthread_mutex_lock (&requests->lock);
+
+  /* Check the workers are inhibited.  */
+  assert (requests->queue_out != requests->queue_in);
+  assert (requests->asleep == WORKER_COUNT);
+  assert (queue_empty(requests->queue_out));
+
+  /* The queue has been drained and will no longer be used.  */
+  free (requests->queue_out);
+  requests->queue_out = requests->queue_in;
+
+  /* We need to wake up all workers, as there could be multiple requests
+     in the new queue.  */
+  pthread_cond_broadcast (&requests->wakeup);
+
+  pthread_mutex_unlock (&requests->lock);
+}
diff --git a/libpager/pager.h b/libpager/pager.h
index fe34238..df4db68 100644
--- a/libpager/pager.h
+++ b/libpager/pager.h
@@ -25,8 +25,32 @@
    scope.  */
 struct user_pager_info;
 
-/* Start the worker threads libpager uses to service requests.  */
-error_t pager_start_workers (struct port_bucket *pager_bucket);
+struct pager_requests;
+
+/* Start the worker threads libpager uses to service requests. If no
+   error is returned, *requests will be a valid pointer, else it will be
+   set to NULL.  */
+error_t
+pager_start_workers (struct port_bucket *pager_bucket,
+                    struct pager_requests **requests);
+
+/* Inhibit the worker threads libpager uses to service requests,
+   blocking until all requests sent before this function is called have
+   finished.
+   Note that RPCs will not be inhibited, so new requests will
+   queue up, but will not be handled until the workers are resumed. If
+   RPCs should be inhibited as well, call ports_inhibit_bucket_rpcs with
+   the bucket used to create the workers before calling this. However,
+   inhibiting RPCs and not calling this is generally insufficient, as
+   libports is unaware of our internal worker pool, and will return once
+   all the RPCs have been queued, before they have been handled by a
+   worker thread.  */
+error_t
+pager_inhibit_workers (struct pager_requests *requests);
+
+/* Resume the worker threads libpager uses to service requests.  */
+void
+pager_resume_workers (struct pager_requests *requests);
 
 /* Create a new pager.  The pager will have a port created for it
    (using libports, in BUCKET) and will be immediately ready
diff --git a/libpager/queue.h b/libpager/queue.h
index d3cf738..abcd3b9 100644
--- a/libpager/queue.h
+++ b/libpager/queue.h
@@ -19,6 +19,8 @@
    You should have received a copy of the GNU General Public License
    along with the GNU Hurd.  If not, see <http://www.gnu.org/licenses/>.  */
 
+#include <stdbool.h>
+
 /* A FIFO queue with constant-time enqueue and dequeue operations.  */
 struct item {
   struct item *next;
@@ -59,3 +61,9 @@ queue_dequeue (struct queue *q)
   r->next = NULL;
   return r;
 }
+
+static inline bool
+queue_empty (struct queue *q)
+{
+  return q->head == NULL;
+}
diff --git a/storeio/pager.c b/storeio/pager.c
index c260d73..f8f59cd 100644
--- a/storeio/pager.c
+++ b/storeio/pager.c
@@ -142,6 +142,7 @@ pager_clear_user_data (struct user_pager_info *upi)
 }
 
 static struct port_bucket *pager_port_bucket = 0;
+static struct pager_requests *pager_requests;
 
 /* Initialize paging for this device.  */
 static void
@@ -160,7 +161,7 @@ init_dev_paging ()
          pager_port_bucket = ports_create_bucket ();
 
          /* Start libpagers worker threads.  */
-         err = pager_start_workers (pager_port_bucket);
+         err = pager_start_workers (pager_port_bucket, &pager_requests);
          if (err)
            {
              errno = err;

-- 
Alioth's /usr/local/bin/git-commit-notice on 
/srv/git.debian.org/git/pkg-hurd/hurd.git