From 6bfb4fc09e5b6f1f54c784dd1f7e2c6700020bb3 Mon Sep 17 00:00:00 2001 From: Peter Amstutz Date: Fri, 22 Sep 2017 11:21:07 -0400 Subject: [PATCH] 12276: Reduce number of spurious invalidations sent to kernel. * New policy to only send invalidations on objects that have a nonzero kernel reference count. * When clearing the contents of a CollectionDirectory, only invalidate the topmost directory entry, this should take care of invalidating all paths beneath it as well. * Don't send invalidate_inode() when deleting an inode (by definition, the inode is unreferenced by the kernel). * Remove allow_dirent_cache for now (this only relates to caching negative lookups, which we don't support). * Set attribute attr_timeout based on the polling period of the underlying object. * FuncToJSONFile sets allow_attr_cache = False instead of allow_dirent_cache = False (which is useless in this case). Arvados-DCO-1.1-Signed-off-by: Peter Amstutz --- services/fuse/arvados_fuse/__init__.py | 20 +++++++----- services/fuse/arvados_fuse/fresh.py | 13 +++++++- services/fuse/arvados_fuse/fusedir.py | 42 +++++++++++++++----------- services/fuse/arvados_fuse/fusefile.py | 9 +++--- 4 files changed, 53 insertions(+), 31 deletions(-) diff --git a/services/fuse/arvados_fuse/__init__.py b/services/fuse/arvados_fuse/__init__.py index 30770fc015..de0338ea79 100644 --- a/services/fuse/arvados_fuse/__init__.py +++ b/services/fuse/arvados_fuse/__init__.py @@ -159,8 +159,8 @@ class InodeCache(object): if obj.in_use(): _logger.debug("InodeCache cannot clear inode %i, in use", obj.inode) return + obj.kernel_invalidate() if obj.has_ref(True): - obj.kernel_invalidate() _logger.debug("InodeCache sent kernel invalidate inode %i", obj.inode) return obj.clear() @@ -266,17 +266,22 @@ class Inodes(object): del self._entries[entry.inode] with llfuse.lock_released: entry.finalize() - self.invalidate_inode(entry.inode) entry.inode = None else: entry.dead = True _logger.debug("del_entry on inode %i with refcount %i", entry.inode, entry.ref_count) - def invalidate_inode(self, inode): - llfuse.invalidate_inode(inode) + def invalidate_inode(self, entry): + if entry.has_ref(False): + # Only necessary if the kernel has previously done a lookup on this + # inode and hasn't yet forgotten about it. + llfuse.invalidate_inode(entry.inode) - def invalidate_entry(self, inode, name): - llfuse.invalidate_entry(inode, name.encode(self.encoding)) + def invalidate_entry(self, entry, name): + if entry.has_ref(False): + # Only necessary if the kernel has previously done a lookup on this + # inode and hasn't yet forgotten about it. + llfuse.invalidate_entry(entry.inode, name.encode(self.encoding)) def clear(self): self.inode_cache.clear() @@ -432,8 +437,7 @@ class Operations(llfuse.Operations): entry = llfuse.EntryAttributes() entry.st_ino = inode entry.generation = 0 - entry.entry_timeout = 60 if e.allow_dirent_cache else 0 - entry.attr_timeout = 60 if e.allow_attr_cache else 0 + entry.attr_timeout = e.time_to_next_poll() if e.allow_attr_cache else 0 entry.st_mode = stat.S_IRUSR | stat.S_IRGRP | stat.S_IROTH if isinstance(e, Directory): diff --git a/services/fuse/arvados_fuse/fresh.py b/services/fuse/arvados_fuse/fresh.py index a51dd909b6..8b680f0663 100644 --- a/services/fuse/arvados_fuse/fresh.py +++ b/services/fuse/arvados_fuse/fresh.py @@ -70,8 +70,9 @@ class FreshBase(object): self.dead = False self.cache_size = 0 self.cache_uuid = None + + # Can the kernel cache attributes? self.allow_attr_cache = True - self.allow_dirent_cache = True def invalidate(self): """Indicate that object contents should be refreshed from source.""" @@ -142,3 +143,13 @@ class FreshBase(object): def child_event(self, ev): pass + + def time_to_next_poll(self): + if self._poll: + t = (self._last_update + self._poll_time) - self._atime + if t < 0: + return 0 + else: + return t + else: + return self._poll_time diff --git a/services/fuse/arvados_fuse/fusedir.py b/services/fuse/arvados_fuse/fusedir.py index 0178fe5544..12c02e2c5d 100644 --- a/services/fuse/arvados_fuse/fusedir.py +++ b/services/fuse/arvados_fuse/fusedir.py @@ -150,12 +150,12 @@ class Directory(FreshBase): # delete any other directory entries that were not in found in 'items' for i in oldentries: _logger.debug("Forgetting about entry '%s' on inode %i", i, self.inode) - self.inodes.invalidate_entry(self.inode, i.encode(self.inodes.encoding)) + self.inodes.invalidate_entry(self, i) self.inodes.del_entry(oldentries[i]) changed = True if changed: - self.inodes.invalidate_inode(self.inode) + self.inodes.invalidate_inode(self) self._mtime = time.time() self.fresh() @@ -182,16 +182,22 @@ class Directory(FreshBase): self._entries = {} for n in oldentries: oldentries[n].clear() - self.inodes.invalidate_entry(self.inode, n.encode(self.inodes.encoding)) self.inodes.del_entry(oldentries[n]) - self.inodes.invalidate_inode(self.inode) self.invalidate() def kernel_invalidate(self): - for n, e in self._entries.iteritems(): - self.inodes.invalidate_entry(self.inode, n.encode(self.inodes.encoding)) - e.kernel_invalidate() - self.inodes.invalidate_inode(self.inode) + # Invalidating the dentry on the parent implies invalidating all paths + # below it as well. + parent = self.inodes[self.parent_inode] + + # Find self on the parent in order to invalidate this path. + # Calling the public items() method might trigger a refresh, + # which we definitely don't want, so read the internal dict directly. + for k,v in parent._entries.items(): + _logger.debug("looking at %s %s, self is %s", k, v, self) + if v is self: + self.inodes.invalidate_entry(parent, k) + break def mtime(self): return self._mtime @@ -266,13 +272,13 @@ class CollectionDirectoryBase(Directory): elif event == arvados.collection.DEL: ent = self._entries[name] del self._entries[name] - self.inodes.invalidate_entry(self.inode, name.encode(self.inodes.encoding)) + self.inodes.invalidate_entry(self, name) self.inodes.del_entry(ent) elif event == arvados.collection.MOD: if hasattr(item, "fuse_entry") and item.fuse_entry is not None: - self.inodes.invalidate_inode(item.fuse_entry.inode) + self.inodes.invalidate_inode(item.fuse_entry) elif name in self._entries: - self.inodes.invalidate_inode(self._entries[name].inode) + self.inodes.invalidate_inode(self._entries[name]) def populate(self, mtime): self._mtime = mtime @@ -547,7 +553,7 @@ class TmpCollectionDirectory(CollectionDirectoryBase): if self.collection_record_file: with llfuse.lock: self.collection_record_file.invalidate() - self.inodes.invalidate_inode(self.collection_record_file.inode) + self.inodes.invalidate_inode(self.collection_record_file) _logger.debug("%s invalidated collection record", self) def collection_record(self): @@ -639,6 +645,7 @@ will appear if it exists. return False try: + e = None e = self.inodes.add_entry(CollectionDirectory( self.inode, self.inodes, self.api, self.num_retries, k)) @@ -649,12 +656,13 @@ will appear if it exists. self.inodes.del_entry(e) return True else: - self.inodes.invalidate_entry(self.inode, k) + self.inodes.invalidate_entry(self, k) self.inodes.del_entry(e) return False except Exception as ex: - _logger.debug('arv-mount exception keep %s', ex) - self.inodes.del_entry(e) + _logger.exception("arv-mount lookup '%s':", k) + if e is not None: + self.inodes.del_entry(e) return False def __getitem__(self, item): @@ -963,7 +971,7 @@ class ProjectDirectory(Directory): # Acually move the entry from source directory to this directory. del src._entries[name_old] self._entries[name_new] = ent - self.inodes.invalidate_entry(src.inode, name_old.encode(self.inodes.encoding)) + self.inodes.invalidate_entry(src, name_old) @use_counter def child_event(self, ev): @@ -1000,7 +1008,7 @@ class ProjectDirectory(Directory): if old_name in self._entries: ent = self._entries[old_name] del self._entries[old_name] - self.inodes.invalidate_entry(self.inode, old_name.encode(self.inodes.encoding)) + self.inodes.invalidate_entry(self, old_name) if new_name: if ent is not None: diff --git a/services/fuse/arvados_fuse/fusefile.py b/services/fuse/arvados_fuse/fusefile.py index 8189a19742..5855361760 100644 --- a/services/fuse/arvados_fuse/fusefile.py +++ b/services/fuse/arvados_fuse/fusefile.py @@ -122,12 +122,11 @@ class FuncToJSONFile(StringFile): super(FuncToJSONFile, self).__init__(parent_inode, "", 0) self.func = func - # invalidate_inode() and invalidate_entry() are asynchronous - # with no callback to wait for. In order to guarantee - # userspace programs don't get stale data that was generated - # before the last invalidate(), we must disallow dirent + # invalidate_inode() is asynchronous with no callback to wait for. In + # order to guarantee userspace programs don't get stale data that was + # generated before the last invalidate(), we must disallow inode # caching entirely. - self.allow_dirent_cache = False + self.allow_attr_cache = False def size(self): self._update() -- 2.30.2