Fix FIXME's introduced by PR#1072

[mulkieran]

#1072 introduced two very similar fixme. We should fix that before some other code does the same thing.

[agrover]

// FIXME: To avoid this expect, modify create_filesystem
// so that it returns a mutable reference to the
// filesystem created.

// FIXME: To avoid this expect, modify add_blockdevs
// so that it returns a mutable reference to each
// blockdev created.

[mulkieran]

I took a stab at the filesystem's one and that turns out to require some thought. It's impossible to implement it in StratPool::create_filesystems unless ThinPool::create_filesystems is changed so that its signature matches that of StratPool::create_filesystems. Otherwise, the thinpool is mutably borrowed multiple times, which we all know is bad. The mutable borrow would be safe in this case, because all we're getting is one filesystem after the other. But the compiler doesn't know that, so we need to do something to help it out.

Another way might be to get all the component filesystems mutably and look them up by name, but the first idea is probably the better.

[mulkieran]

This has drifted too far down my stack, I don't expect I'll get back to it before January.

[tasleson]

The filesystem is ultimately inserted into a hashmap buried inside the Table . When you get a mutable reference to the filesystem from the hash there will always be an expect if we know that the item exists. Therefore the expect won't really go away in the dbus call, it's just moved somewhere else right?

[mulkieran]

@jbaublitz This is a puzzle problem, and not really urgent. If it conflicts w/ #1614, please mark it as blocked and leave it alone for now.

[jbaublitz]

Blocked by stratis-storage/project#51 or more specifically the PR #1614.

[jbaublitz]

Therefore the expect won't really go away in the dbus call, it's just moved somewhere else right?

I have looked into this a bit and I think I agree with this claim. Modifying the API for blockdevs to return a mutable reference has the same problem as filesystems apparently has. We can get a mutable reference from the newly created blockdevs, but once we have to transfer ownership to self.cache_devs or self.block_devs in the .add_blockdevs() method, we will run into using moved values errors from the borrow checker. We could do the same thing and reacquire mutable references to each of the blockdevs we've inserted, but as stated above, we will end up calling expect on the get operations on the HashMaps as we know for certain that these already exist. We have a few options here:

1. Move the expect into the engine.
2. Return an error if the UUID does not correspond to a blockdev struct, a code path which will never be reached.
3. Filter out any UUID that does not correspond to a blockdev which feels error prone and could potentially cause some nasty bugs down the line if something changes (I would not recommend this).

@mulkieran What are your thoughts?

[mulkieran]

Going to struggle with this one more time in December. Maybe I was thinking of using Entry?

[jbaublitz]

Maybe there is a way to do it with Entry after all! I looked into this API and it looks very promising. I'm happy to remain the owner of this issue and try it again with that additional information. Thank you for the pointer.

Three things to consider:

• Once we introduce threading down the road, if we lock at the pool level only, this should continue to work without any intervention. If we do more granular locking, the Entry approach will cease to work and we'll have to replace it with an Arc. This is not necessarily a good thing or a bad thing but something worth consideration. We may even want to address this issue at the same time as multithreading if we go with more granular locking.
• I would like to evaluate the code in a little more depth to ensure that we are never going to bump into an issue where we call create_filesystems with a filesystem name that already exists. If we ever reach create_filesystems with a duplicate name, insert_or() will just return a mutable reference to the existing filesystem and we would never have any indication. If we are able to pass duplicates, we will need to do checks beforehand to make sure that this never occurs in create_filesystems. If it cannot occur, I'd like to add a comment specifying the precondition.
• I think it would make sense that if we go with the Entry approach, we should modify Table, the structure causing the problem with ownership, to support a new method that will insert and give us back a mutable reference.

How does this all sound?

[jbaublitz]

@mulkieran Transferring to you. I think I was a little overeager about this potentially working. I've done some digging and the Entry API gives different errors from the previous approach but still lifetime errors. Ultimately it seems to come down to this:

• .entry() creates an Entry struct
• Entry::or_insert() returns a mutable reference with a lifetime bound to the lifetime of the Entry struct the method was called on
• If we return the mutable reference up the stack, the Entry struct goes out of scope which violates the rules of the borrow checker

I may be missing something but I don't think there's any more I can do on this issue. Let me know if you find a way to get it working.