Skip to content

Commit

Permalink
Fix lead of file descriptors for PhysicalInode:s
Browse files Browse the repository at this point in the history
This fixes flatpak#689

The mechanism we're using to flush the dcache and thus avoiding
long-term caching of physical inodes which keep a file descriptor open
doesn't seem to work quite right.

A trivial test of cat /run/user/1000/doc/$docid/$filename properly
leads to the PhysicalFile being freed. However, an application that
keeps the file open for a longer time (longer than the delay we have
before the current dcache flush workaround is run), ends up with the
dcache entry for the inode to be kept in cache. This means that we
when the last other kernel reference (like an open fd) to the inode is
gone we don't get a FORGET event from fuse, and thus we never free the
PhysicalInode.

This is clearly visible, because if you end up with the file
descriptor leak like in the issue, then flushing the dcache (`sudo sh
-c 'echo 2 > /proc/sys/vm/drop_caches'`) fixes the leak.

It seems the current workaround of just invalidating the toplevel
directory entry doesn't work, so we replace it with tracking each
individual lookup that ends up with a physical inode and flush all of
them. This seems to fix the issue for me.
  • Loading branch information
alexlarsson committed Nov 7, 2023
1 parent 36dc6e9 commit 64cc950
Showing 1 changed file with 44 additions and 22 deletions.
66 changes: 44 additions & 22 deletions document-portal/document-portal-fuse.c
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,6 @@ struct _XdpDomain {
guint64 doc_dir_inode;
guint32 doc_flags;

int doc_queued_invalidate; /* Access atomically, 1 if queued invalidate */

/* Below is mutable, protected by mutex */
GMutex tempfile_mutex;
GHashTable *tempfiles; /* Name -> physical */
Expand Down Expand Up @@ -223,6 +221,14 @@ typedef struct {
XdpInode *root_inode;
XdpInode *by_app_inode;

typedef struct {
ino_t parent_ino;
char *name;
} XdpInvalidateData;

static GList *invalidate_list;
G_LOCK_DEFINE (invalidate_list);

static XdpInode *xdp_inode_ref (XdpInode *inode);
static void xdp_inode_unref (XdpInode *inode);

Expand Down Expand Up @@ -1707,37 +1713,53 @@ ensure_doc_inode (XdpInode *parent,
}

static gboolean
invalidate_doc_domain (gpointer user_data)
invalidate_dentry_cb (gpointer user_data)
{
g_autoptr(XdpDomain) doc_domain = user_data;
ino_t parent_ino;

g_atomic_int_set (&doc_domain->doc_queued_invalidate, 0);

parent_ino = xdp_inode_to_ino (doc_domain->parent_inode);
GList *to_invalidate = NULL;
{
XDP_AUTOLOCK (invalidate_list);
to_invalidate = g_steal_pointer (&invalidate_list);
}
to_invalidate = g_list_reverse (to_invalidate);

XDP_AUTOLOCK (session);
if (session && g_atomic_int_get (&doc_domain->parent_inode->kernel_ref_count) > 0)
fuse_lowlevel_notify_inval_entry (session, parent_ino, doc_domain->doc_id,
strlen (doc_domain->doc_id));
for (GList *l = to_invalidate; l != NULL; l = l->next)
{
XdpInvalidateData *data = l->data;
if (session)
fuse_lowlevel_notify_inval_entry (session, data->parent_ino, data->name,
strlen (data->name));
g_free (data);
}

g_list_free (to_invalidate);

return FALSE;
}

/* Queue an inval_entry call on this domain, thereby freeing all unused inodes
* in the dcache which will free up a bunch of O_PATH fds in the fuse implementation
/* Queue an inval_dentry, thereby freeing unused inodes in the dcache
* which will free up a bunch of O_PATH fds in the fuse implementation.
*/
static void
doc_domain_queue_entry_invalidate (XdpDomain *doc_domain)
queue_invalidate_dentry (ino_t parent_ino, const char *name)
{
int old = g_atomic_int_get (&doc_domain->doc_queued_invalidate);
if (old != 0)
return;
XDP_AUTOLOCK (invalidate_list);

for (GList *l = invalidate_list; l != NULL; l = l->next)
{
XdpInvalidateData *data = l->data;
if (data->parent_ino == parent_ino && strcmp (name, data->name) == 0)
return;
}

XdpInvalidateData *data = g_new0(XdpInvalidateData, 1);
data->parent_ino = parent_ino;
data->name = g_strdup(name);

if (!g_atomic_int_compare_and_exchange (&doc_domain->doc_queued_invalidate, old, 1))
return; // Someone else set it to 1, return
if (invalidate_list == NULL)
g_timeout_add (1000, invalidate_dentry_cb, NULL);

g_timeout_add (1000, invalidate_doc_domain, xdp_domain_ref (doc_domain));
invalidate_list = g_list_append(invalidate_list, data);
}

static void
Expand Down Expand Up @@ -1800,7 +1822,7 @@ xdp_fuse_lookup (fuse_req_t req,
if (res != 0)
return xdp_reply_err (op, req, -res);

doc_domain_queue_entry_invalidate (parent_domain);
queue_invalidate_dentry (parent_ino, name);
}

g_debug ("LOOKUP %lx:%s => %lx", parent_ino, name, e.ino);
Expand Down

0 comments on commit 64cc950

Please sign in to comment.