[guardian-dev] debugging libsqlfs
Hans of Guardian
hans at guardianproject.info
Thu Jan 10 10:12:35 EST 2013
On Jan 10, 2013, at 6:49 AM, Abel Luck wrote:
> Hans-Christoph Steiner:
>> On 01/08/2013 09:44 AM, Hans of Guardian wrote:
>>> On Jan 8, 2013, at 4:05 AM, Abel Luck wrote:
>>>> Hans-Christoph Steiner:
>>>>> It seems that sqlfs_t gets re-issued a lot, I added this to get_sqlfs() and
>>>>> got it many many times:
>>>>> if (p)
>>>>> return p;
>>>>> show_msg(stderr, "sqlfs was 0");
>>>>> This lead me to examine the threading stuff, since it seems that when sqlfs ==
>>>>> 0, its getting a new sqlfs issued from:
>>>>> sqlfs = (sqlfs_t *) (pthread_getspecific(sql_key));
>>>>> Since the libsqlfs code only has pthread code relating to the pthread_key_t
>>>>> creation and use, my guess is that FUSE itself is handling the rest of the
>>>>> threading. So the good news is that libsqlfs was made with threading in mind,
>>>>> the bad news is that a lot of it is normally handled in libfuse, so we'll have
>>>>> to figure that part out and implement it in IOCipher in order to support threads.
>>>> Yes! This was a conclusion I had several months ago when looking at
>>>> Aaron's threading bug. There are definitely discrepancies between the
>>>> way IOCipher runs and the we test things with libsqlfs and fuse.
>>> Hey Abel,
>>> I think that first thing's first: let's getting fully passing long fsx tests, then we can check whether we need to handle threads different in IOCipher.
>> Ok, today was a good day, much better than before.
>> * we isolated the super mysterious crash/corruption issues in libsqlfs itself.
>> It only happens with mmap'ed writes in multi-threaded mode. So we can easily
>> avoid doing that in IOCipher.
>> * we now have a pretty solid grasp of the multi-threading issues in IOCipher's
>> JNI C code. We have some working test cases, so by the end of tomorrow we hope
>> to have something for you to test in your app. We need to restructure things
>> a little bit to make sure that we're using the pthread_key_t to keep track of
>> the instances of sqlfs_t state struct.
>> I pushed my libsqlfs changes to implement the sqlfs threaded mode with a key,
>> i.e. using the key in get_sqlfs():
>> Abel, feel free to merge this to the official repo if you think they're good.
> Alright, so things aren't all rosy quite yet. Your changes are more or
> less good on the sqlfs side of things, but some dramatic changes are
> necessary on the IOCipher side of things, at least I think so.
> First: the way things are currently in IOCipher:
> VirtualFileSystem is responsible for opening and holding onto the
> sqlfs*. .mount() calls the _open(db_name, key) method to open the
> database and get a handle on the sqlfs*. isMounted() and unmount() rely
> on this pointer to sqlfs.
> This central global sqlfs* we suspect is the cause of our
> multithreading issues.
> Yesterday, we changed Posix.cpp and File.cpp to not use the global
> sqlfs* but to grab it from the pthread specific context. Of course this
> context doesn't yet exist.
> Second: your changes attempt to create this context, but breaks our VFS
> model (I'll explain).
> We're moving from a central sqlfs* to a created-on-the-fly per thread
> access to sqlfs*. This destroys our notion of a "mounted" VFS, or at
> least it breaks semantics of "a valid sqlfs* means we're mounted".
> You've seen how this works with fuse, sqlfs gets created and freed quite
> a bit, that is, per thread. The only state that really sticks around is
> (1) the database file path and (2) the sqlcipher key.
> My conclusion is that we will simple not handle sqlfs* structs in our
> IOCipher code. That will be internal to sqlfs only. Therefore, the
> notion of a VirtualFileSystem being "mounted" means (1) we have a valid
> path and (2) a valid key.
> Implementation wise, I propose:
> (1) VirtualFileSystem.init() validates databaseFileName is writable
> (dispensing with the sqlfs_init call)
> (2) VirtualFileSystem.mount() does two things:
> a. if a key is provided AND the database file exists, check if
> the key is valid by attempting an open on the database
> b. call the new sqlfs_init_key() function to store the db name
> and key for thread access.
> c. set a boolean isMounted = true
> (3) VirtualFileSystem.isMounted() returns the boolean set by .mount()
> (4) VirtualFileSystem.unmount() unsets the boolean and overwrites the
> databaseFileName and key values to null.
> Issues: 1,2 are fine I think, but 3,4 are quite hacky. unmount() doesn't
> mean anything know.
> For example, if you have 2 threads using an IOCipher database, and one
> thread unmounts(), the other can continue to write, because "mounted"
> merely means a valid sqlite db and password.
> Should we get rid of the notion of mount() altogether?
> Gonna get some lunch and ponder.
Yeah I think we can get rid of the whole idea of mounting for now. Just make mount/unmount do nothing and isMounted always return true. We might need those methods later on to handle multiple VFSs in a single process.
More information about the Guardian-dev