mountpoint_cmp: flatten mountpoint= parameter before sorting

[robn]

[Sponsors: TrueNAS]

Motivation and Context

A user showed me a curious situation where after a reboot his datasets did not mount correctly, so some of his applications would start with apparently-empty data directories. He'd worked around it by stopping services, unmount everything, mounting datasets in two distinct sets, then starting up. "That's awful and I am so sorry" I said, and we stared for a long time into the screen before finally noticing that the root dataset of the affected tree had a trailing slash on the mountpoint= parameter. With the little bit of soul that didn't leave my body, I promised I would fix it because that behaviour is miserably unfair.

@portablejim thank you for your patience :)

Description

Datasets with nested mountpoints must be mounted in order from shallowest to deepest, otherwise the deeper ones will be obscured by the shallow ones.

mountpoint_cmp() has only traditionally been a character-based sort, with no awareness of "legal" patterns in pathnames, in particular, runs of '/' characters. These are (for better or worse) legal paths, but confuse a character-based comparison, causing mounts to be attempted in an incorrect order, with mixed results.

This commit adds a path "flattening" function, and applies it to the string returned from the mountpoint= paramater before using it as a path. This corrects the sort order and allows mounting to proceed correctly.

I did originally think about requiring mountpoint= to be stored in some sort of "flattened" or (lets say) "canonical" form, so it would have been down a layer in the property getter/setter (the getter would be necessary for existing pools). I eventually decided against this for a few reasons:

• the "true" value of the property would be hidden, making it difficult to debug
• a special-case inside the property code felt very nasty (really it wants to be a value type)
• there may be non-POSIX path structures in mountpoint=, and I don't want to mess with them

/cc @lundman on that last one. I don't know what you currently do on mountpoint= for Windows but I tried to structure this so it could be moved out of your way without too much fuss. It seems to me that zfs_flatten_path() could easily a platform-specific function, and be renamed as zfs_convert_mountpoint_os() or something like that. I'm not looking to do that here (yet), I'm mostly just curious to make sure I'm not getting in your way, and/or if you have a better approach to this kind of thing already.

How Has This Been Tested?

New test added, that fails on both Linux and FreeBSD before the change, and succeeds afterwards. The test method is finicky, but the test has a lot of commentary.

I've not had chance to do a full ZTS run, so I'll will let CI take a look at it. I would be surprised if it broke an existing test other than an outright bug.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Quality assurance (non-breaking change which makes the code more robust against bugs)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[amotin]

To me the main questions here are whether we should be more strict on allowed mountpoint values, and whether we should clean the value before storing it. mountpoint_namecheck() is really relaxed on it.

[amotin]

That's quite a comparison function for sorting.

[robn]

Yeah, fortunately its not particularly time-sensitive. I could maybe do something to cache the flattened form though, if you think its worth it?

[robn]

To me the main questions here are whether we should be more strict on allowed mountpoint values, and whether we should clean the value before storing it. mountpoint_namecheck() is really relaxed on it.

Yeah, I don't know either. If we define (and enforce) mountpoint= to be a UNIX-like path, then we require any non-UNIX-like platforms to need some kind of translation. I don't know if that's reasonable or not. It might just be Windows (which I would still like to see fully supported someday), but also platforms might bring additional restrictions outside just the format - eg, I believe macOS won't let things mount at the root. But if we don't lock it down, then in theory we need all platforms to be able to cope with everything - eg, what would it mean to regularly move a pool between a Windows and a FreeBSD host?

On the other other hand, after #18205 I'm already thinking about what it would mean for mountpoint= to have a list of paths, which of course other systems might not be able to deal with. But given that, maybe the mount point is really a per-platform value, that we shouldn't expect to translate to other platforms at all? I mean, are people really transplanting a pool to a different OS and expecting the literal same mountpoints to stay?

I don't think there's any hurry in deciding this though, certainly not if we have to commit to an on-disk meaning change.