Reviewing at 9479599:

I want to clarify that I understand the thread safety concerns here. Because llfuse ensures that request handlers do not run concurrently, it's not necessary to explicitly protect the code against thread contention -- however, because calls to the API server might cause all llfuse clients to block unnecessarily, each arvados_fuse client explicitly releases the lock when it's waiting on a response from the API server (which is also why each thread gets its own API client).

Is that an accurate summary? The locking protocol was very puzzling until I read the arvados_fuse.Operations doc comment, which seemed to indicate that something like the above is what's happening.

• The Unix ctime field of an inode isn't the file's creation time, it's the time the metadata (i.e. the inode) was last changed. I think this should probably just be equal to the collection modification time.
• CollectionDirectory
  • It looks as though a CollectionDirectory will never refresh itself once it's been instantiated: the update() method checks self.collection_object is not None and I don't see any place where this field gets reset to none. Is that intentional?
• TagsDirectory, SharedDirectory
  • There's code here to invalidate the directory upon any event involving a Link, commented out. Is this unfinished business or code that needs to be removed?
• FreshBase
  • I don't think it's necessary for FreshBase to provide ctime and mtime methods if they aren't used in the base class. In a way it would be better if they're not provided at all, so we have clear feedback if a derived class fails to implement the method and tries to call it anyway.

Tim Pierce wrote:

Reviewing at 9479599:

I want to clarify that I understand the thread safety concerns here. Because llfuse ensures that request handlers do not run concurrently, it's not necessary to explicitly protect the code against thread contention -- however, because calls to the API server might cause all llfuse clients to block unnecessarily, each arvados_fuse client explicitly releases the lock when it's waiting on a response from the API server (which is also why each thread gets its own API client).

Is that an accurate summary? The locking protocol was very puzzling until I read the arvados_fuse.Operations doc comment, which seemed to indicate that something like the above is what's happening.

That's exactly right.

services/fuse/arvados_fuse/__init__.py:

• The Unix ctime field of an inode isn't the file's creation time, it's the time the metadata (i.e. the inode) was last changed. I think this should probably just be equal to the collection modification time.

Man! That's annoying. Fixed so ctime == mtime. Also added atime().

• CollectionDirectory
  • It looks as though a CollectionDirectory will never refresh itself once it's been instantiated: the update() method checks self.collection_object is not None and I don't see any place where this field gets reset to none. Is that intentional?

You're missing the second half of that conditional:

if self.collection_object is not None and portable_data_hash_pattern.match(self.collection_locator):
    return True

If the collection_locator is a portable data hash, it's guaranteed to never change its contents. If the collection locator is a uuid, it is possible that the manifest associated with the collection was changed by the user, so then in proceeds to checks to see if the portable_data_hash associated with the collection record has changed.

• TagsDirectory, SharedDirectory
  • There's code here to invalidate the directory upon any event involving a Link, commented out. Is this unfinished business or code that needs to be removed?

I took this out. This was using websockets, but I was running into problems where it would just hang on connecting. I want to have a better change detection and invalidation scheme than "refresh if the object hasn't been refreshed in the last 60 seconds" but I think that's out of scope for this story.

• FreshBase
  • I don't think it's necessary for FreshBase to provide ctime and mtime methods if they aren't used in the base class. In a way it would be better if they're not provided at all, so we have clear feedback if a derived class fails to implement the method and tries to call it anyway.

Removed mtime() and ctime() and added atime() (which is used by the base class.)

Peter Amstutz wrote:

Tim Pierce wrote:

Reviewing at 9479599:

I want to clarify that I understand the thread safety concerns here. Because llfuse ensures that request handlers do not run concurrently, it's not necessary to explicitly protect the code against thread contention -- however, because calls to the API server might cause all llfuse clients to block unnecessarily, each arvados_fuse client explicitly releases the lock when it's waiting on a response from the API server (which is also why each thread gets its own API client).

Is that an accurate summary? The locking protocol was very puzzling until I read the arvados_fuse.Operations doc comment, which seemed to indicate that something like the above is what's happening.

That's exactly right.

OK. It's clever. I don't see any red flags in the thread locking, but I don't think I understand why it's necessary for RecursiveInvalidateDirectory.invalidate to explicitly acquire and release the llfuse lock?

services/fuse/arvados_fuse/__init__.py:

• The Unix ctime field of an inode isn't the file's creation time, it's the time the metadata (i.e. the inode) was last changed. I think this should probably just be equal to the collection modification time.

Man! That's annoying. Fixed so ctime == mtime. Also added atime().

Nice, thanks.

• CollectionDirectory
  • It looks as though a CollectionDirectory will never refresh itself once it's been instantiated: the update() method checks self.collection_object is not None and I don't see any place where this field gets reset to none. Is that intentional?

You're missing the second half of that conditional:

[...]

If the collection_locator is a portable data hash, it's guaranteed to never change its contents. If the collection locator is a uuid, it is possible that the manifest associated with the collection was changed by the user, so then in proceeds to checks to see if the portable_data_hash associated with the collection record has changed.

Ah, of course! Thanks for clarifying.

• TagsDirectory, SharedDirectory
  • There's code here to invalidate the directory upon any event involving a Link, commented out. Is this unfinished business or code that needs to be removed?

I took this out. This was using websockets, but I was running into problems where it would just hang on connecting. I want to have a better change detection and invalidation scheme than "refresh if the object hasn't been refreshed in the last 60 seconds" but I think that's out of scope for this story.

Makes sense. Do we have a story for improving arv-mount/arvados_fuse invalidation logic?

Tim Pierce wrote:

OK. It's clever. I don't see any red flags in the thread locking, but I don't think I understand why it's necessary for RecursiveInvalidateDirectory.invalidate to explicitly acquire and release the llfuse lock?

Because that is intended to be called from a thread separate from llfuse which is listening on websockets.

Makes sense. Do we have a story for improving arv-mount/arvados_fuse invalidation logic?

#3833

Fuse tests should pass now.