On Thu, Jan 4, 2018 at 7:20 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > Until this change, we decoded upper file handles by instantiating an > overlay dentry from the real upper dentry. This is sufficient to handle > pure upper files, but insufficient to handle merge/impure dirs. > > To that end, if decoded real upper dir is connected and hashed, we > lookup an overlay dentry with the same path as the real upper dir. > If decoded real upper is non-dir, we instantiate a disconnected overlay > dentry as before this change. > > Because ovl_fh_to_dentry() returns connected overlay dir dentries, > exportfs never need to call get_parent() and get_name() to reconnect an > upper overlay dir. Because connectable non-dir file handles are not > supported, exportfs will not be able to use fh_to_parent() and get_name() > methods to reconnect a disconnected non-dir to its parent. Therefore, the > methods get_parent() and get_name() are implemented just to print out a > sanity warning and the method fh_to_parent() is implemented to warn the > user that using the 'subtree_check' exportfs option is not supported. > Reviewers who will get this far, should have their eyebrows slightly raised after reading this commit message and should be asking themselves: "Why not return a disconnected overlay dentry like any other fs and implement ovl_get_parent()/ovl_get_name() by looking at parent/name of upper dir?" I have had this debate with myself for a while and experimented a bit with both approaches and in the end, I liked the "return connected dentry" result better. I did not want to write this entire story in commit message, because in the end, there is nothing incorrect about the choice of either implementation there are only pros and cons to each choice. At the moment, the only argument I can think of to counter the chosen approach is that it adds ~100 lines on code in ovl_lookup_real() and ovl_lookup_real_one() helpers that could have been avoided by using the common reconnect_path() code in fs/exportfs/expfs.c. The arguments to counter the disconnected dir approach are: - Obtaining a disconnected overlay dir dentry would requires a delicate re-factoring of ovl_lookup() to get a dentry with overlay parent info. I personally preferred to avoid doing that re-factoring unless it was proven worthy. - Going down the path of disconnected dir would mean that the (non trivial) code path of d_splice_alias() could be traveled and that meant writing more tests and introduces race cases that are very hard to hit on purpose. Taking the path of connecting overlay dentry by forward lookup is therefore the safe and boring way to avoid surprises. - In the current implementation, there is an anomaly in the multi lower layer setup. In that case, indexed upper dir inodes are hashed by the lower inode, but their file handles are encoded from the upper inode. Obtaining a disconnected dir from this type of upper file handle would have been a special case that would add more code and more complexity. With the forward lookup connect approach, the anomaly does not require changing the code - connecting the dentry is just less efficient in case there is an ancestor in inode cache (we won't find it in cache because we will be looking with the wrong inode) and that can be fixed later if we find that use case important enough. There. Now you see why I did not want this story in commit message? Amir. -- To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
