Locking in NodeEnvironment is completely broken

[rmuir]

after the lucene 5.3 upgrade, i looked at how ES uses lucene's filesystem locking. most places are ok, obtaining a lock and doing stuff in a try/finally. However NodeEnvironment is a totally different story. Can we fix the use of locking here?

1. deleteShardDirectorySafe is anything but safe. it calls deleteShardDirectoryUnderLock which doesn't actually delete under a lock either!!!! It calls this bogus method: acquireFSLockForPaths which acquires then releases locks. Why? Why? Why?
2. assertEnvIsLocked is only called under assert. why? Look at findAllIndices, its about to do something really expensive, why can't the call to ensureValid be a real check?
3. assertEnvIsLocked has a bunch of leniency, why in the hell would it return true when closed or when there are no locks at all, thats broken.

After this stuff is fixed, any places here doing heavy operations (e.g. N filesystem operations) should seriously consider calling ensureValid on any locks that are supposed to be held. It means you do N+1 operations or whatever but man, if what we are doing is not important, then why are we using fs locks?

[s1monw]

deleteShardDirectorySafe is anything but safe. it calls deleteShardDirectoryUnderLock which doesn't actually delete under a lock either!!!! It calls this bogus method: acquireFSLockForPaths which acquires then releases locks. Why? Why? Why?

so we need to clarify the naming here, we actually delete under lock but it's not the IW lock it's a shard lock that we maintain internally per Node.

public void deleteShardDirectorySafe(ShardId shardId, @IndexSettings Settings indexSettings) throws IOException {
        // This is to ensure someone doesn't use Settings.EMPTY
        assert indexSettings != Settings.EMPTY;
        final Path[] paths = availableShardPaths(shardId);
        logger.trace("deleting shard {} directory, paths: [{}]", shardId, paths);
        try (ShardLock lock = shardLock(shardId)) { // <==== here we lock and keep the lock - it's JVM internal
            deleteShardDirectoryUnderLock(lock, indexSettings);
        }
    }

the Why? Why? Why? is especially interesting and I think you should talk to the guy who reviewed the change: https://github.com/elastic/elasticsearch/pull/11127 He said:

yes, I am +1 for this approach. Maybe we can add a line to the javadocs just mentioning this is the case. Especially if you think about locking on shared filesystems, we should not rush to do something complex.

I am not sure if the Javadoc happened but it need clarification.

assertEnvIsLocked is only called under assert. why? Look at findAllIndices, its about to do something really expensive, why can't the call to ensureValid be a real check?

+1 to make it a real check where it makes sense...

assertEnvIsLocked has a bunch of leniency, why in the hell would it return true when closed or when there are no locks at all, thats broken.

+1 to beef it up.

[rmuir]

Yes, I remember this, but now is our chance to fix it, so locking is as good as we can make it. It seems we are broken because of the placement of the lock files being underneath what is deleted, but that is something fixable.

Its 2.0, there is no constraint about back compat here, so I think its time to fix it correctly.

Additionally we spent lots of time, and added lots of paranoia in lucene to actually help with shitty behavior from shared filesystems, so it would be nice if it stands a chance.

As far as the shard lock, i have no idea what that is. How is it better than a filesystem lock? Its definitely got a shitload of abstractions, but i can't tell if its anything more than a in-process RWL.

[s1monw]

I think we should make this straight forward and add a /locks directory to each root path we are using. This directory can then be full of locks and will never be deleted. The crucial part is that it has to be on the same mount as the actual data it protects otherwise it will likely not help at all in the shared FS case.

[rmuir]

yeah, i think something along those lines: though is 'never deleted' a problem with ppl that have tons and tons of shards cycling through? accumulating a bunch of 0-byte files sounds dangerous and eventually the directory is gonna crap its pants.

Deleting an NIOFS lock file is especially tricky and we just don't do it ever in lucene (we leave the lock file around). I dont know how to fix that without adding a "master" lock file that always stays around and is acquired around individual lock acquire/release+delete.

[s1monw]

Deleting an NIOFS lock file is especially tricky and we just don't do it ever in lucene (we leave the lock file around). I dont know how to fix that without adding a "master" lock file that always stays around and is acquired around individual lock acquire/release+delete.

yeah we won't have a way around that I guess. I think what we can do is to have an $index_name.lock that you need to own to make changes to the $index_name_$shardId.lock which also allows to delete it. That reduces the set to the number of indices. We can safely delete the $index_name.lock once the last shard of the index is deleted?

[rmuir]

Why do that? just have global.lock. Its only needed around the actaul acquire and release+delete. Its not gonna cause a concurrency issue.

Doing this in a more fine grained way makes zero sense.

[s1monw]

Doing this in a more fine grained way makes zero sense.

having an index level lock make sense to have here anyway since we also have index metadata we want to protect from concurrent modifications. All I was saying here is that we might be able to get away with not locking the global lock as long as we are in the context of an index.