Re: [patch] concurrency fixes for the fuse daemon

[David Zeuthen <david fubar dk>]

On Sat, 2008-04-19 at 23:18 -0500, Hans Petter Jansson wrote:
> On Fri, 2008-04-18 at 02:22 -0400, David Zeuthen wrote:
> 
> > The attached patch adds some reference counting to FileHandle; seems to
> > fix the problems for me; at least no more valgrind warnings and no
> > SEGV's. As I'm not 100% familiar with the fuse daemon code base maybe
> > there's an easier and/or more correct fix. The locking seems a bit
> > fishy/racy in general but, as I said, I'm not 100% familiar with the
> > code base nor the operating constraints of libfuse. 
> > 
> > Please review carefully. Thanks!
> 
> The patch looks reasonable to me. Go for it.
> 

After some more testing I realized we're leaking the reference. Which
means the same stream will get reused and this fails if the backend
doesn't support seeking (for example the archive backend; still worked
great with backends that support seeking e.g. cdda, sftp, gphoto2). With
the old patch this would happen on a fresh mount

$ file .gvfs/f9-20080319.iso/README 
.gvfs/f9-20080319.iso/README: UTF-8 Unicode English text

$ file .gvfs/f9-20080319.iso/README 
.gvfs/f9-20080319.iso/README: ERROR: cannot read `.gvfs/f9-20080319.iso/README' (Operation not supported)

The attached patch fixes that by unreffing the FileHandle in
vfs_release() before freeing it. Hmm.. I wonder if the correct fix
involves not sharing FileHandle's at all? I mean, we only use it for
stream operations anyway; doesn't seem we gain a lot by sharing these.

Thoughts?

    David

Index: client/gvfsfusedaemon.c
===================================================================
--- client/gvfsfusedaemon.c	(revision 1746)
+++ client/gvfsfusedaemon.c	(working copy)
@@ -71,6 +71,8 @@
 } FileOp;
 
 typedef struct {
+  gint      refcount;
+
   GMutex   *mutex;
   FileOp    op;
   gpointer  stream;
@@ -188,12 +190,26 @@
   FileHandle *file_handle;
 
   file_handle = g_new0 (FileHandle, 1);
+  file_handle->refcount = 1;
   file_handle->mutex = g_mutex_new ();
   file_handle->op = FILE_OP_NONE;
 
   return file_handle;
 }
 
+static FileHandle *
+file_handle_ref (FileHandle *file_handle)
+{
+  g_atomic_int_inc (&file_handle->refcount);
+  return file_handle;
+}
+
+static gboolean
+file_handle_unref (FileHandle *file_handle)
+{
+  return g_atomic_int_dec_and_test (&file_handle->refcount);
+}
+
 static void
 file_handle_close_stream (FileHandle *file_handle)
 {
@@ -278,21 +294,19 @@
   g_static_mutex_unlock (&global_mutex);
 }
 
-static gboolean
+static void
 free_file_handle_for_path (const gchar *path)
 {
   FileHandle *fh;
 
-  fh = get_file_handle_for_path (path);
+  g_static_mutex_lock (&global_mutex);
+  fh = g_hash_table_lookup (global_fh_table, path);
   if (fh)
     {
-      g_static_mutex_lock (&global_mutex);
-      g_hash_table_remove (global_fh_table, path);
-      g_static_mutex_unlock (&global_mutex);
-      return TRUE;
+      if (file_handle_unref (fh))
+        g_hash_table_remove (global_fh_table, path);
     }
-
-  return FALSE;
+  g_static_mutex_unlock (&global_mutex);
 }
 
 static MountRecord *
@@ -923,6 +937,7 @@
               
               /* File exists */
 
+              file_handle_ref (fh);
               SET_FILE_HANDLE (fi, fh);
 
               debug_print ("vfs_open: flags=%o\n", fi->flags);
@@ -1013,6 +1028,7 @@
 
               /* Success */
 
+              file_handle_ref (fh);
               SET_FILE_HANDLE (fi, fh);
 
               g_assert (fh->stream == NULL);
@@ -1047,7 +1063,10 @@
   debug_print ("vfs_release: %s\n", path);
 
   if (fh)
-    free_file_handle_for_path (path);
+    {
+      if (!file_handle_unref (fh))
+        free_file_handle_for_path (path);
+    }
 
   return 0;
 }