Remove "compatibility" zvol symlinks (the /dev/dataset_name ones, as opposed to the /dev/zvol/dataset_name ones)

[jgottula]

I don't have a super good sense for whether this is a good idea or a terrible idea. But I figured I'd put it forward anyway, and let people better-equipped than me figure that out.

(This PR/branch has a version of the commit that applies cleanly on master. See comment below for another branch with a different version of the commit that applies cleanly on top of PR #12302.)

Motivation and Context

Getting rid of old junk that's ancient and that nobody should be using. (At least in theory.)

Description

This modifies the 60-zvol.rules.in file such that, while it still creates the symlink tree /dev/zvol/dataset_name, it no longer creates the "compatibility" symlink tree /dev/dataset_name.

How Has This Been Tested?

Testing isn't particularly relevant here. The relevance mainly pertains to whether or not this is a good idea, which in turn probably mostly depends on to what extent we think people are relying on the old device node symlink layout.

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)
  • To a very small extent, less symlinks means udev will chug a bit less, particularly on systems with many zvols and/or with heavily-snapshotted zvols that have snapdev=visible.
• Code cleanup (non-breaking change which makes code smaller or more readable)
  • Arguably. I mean, it cleans up my /dev directory, for what that's worth.
• Breaking change (fix or feature that would cause existing functionality to change)
  • Depending on what users are expecting. This is kinda the crux of the matter.
• 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.
  • Hmmmmm, this might be a good thing to look into, come to think of it. Not the easiest thing to grep ever, though.
• 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.

This is a really simple technical change, with a big fat question mark hanging over it that I can't answer, which is "does the old zvol tree still need to exist?" It seems like it ought to be fine to get rid of it, but then there may well be reasons I haven't thought of or don't have good insight into.

[behlendorf]

I'm sure we do have people using these compatibility links, particularly since we never had a great way to make it clear which links should be used. That said, I think we probably should go ahead and remove them in the master branch. Anything depending on them should be able to migrate to the new paths fairly easily, and if we make the change now it'll be a while before it shows up in a tagged release. I'd love to see this long overdue bit of cleanup wrapped up.

[jgottula]

(All of the stuff in this comment is now moot.)

---

What I've done just now, is rebase the branch for this PR (remove-compat-zvol-symlinks) onto the current git master. So now there's less weirdness, in that the PR only contains the one commit of actual relevance, and it applies cleanly, as-is, on a "right now" basis.

I did preserve the version of the commit that's based on the branch from PR #12302 in another branch (remove-compat-zvol-symlinks--based-atop-PR12302-commits).

• There's a paragraph in the commit summary of each version of the commit noting which variation it is. This is a reminder to remove that later.

[behlendorf]

@jgottula would you mind rebasing this now that #12302 has been merged.

[jgottula]

@jgottula would you mind rebasing this now that #12302 has been merged.

Yep; was already in the process of working on that, actually.

[jgottula]

@behlendorf Rebased to master.

I also added a new comment to the top of the udev rules file itself, explaining that we used to create the compat symlink tree but that we no longer do. Seemed like a good idea, in case someone starts scratching their head about why the symlinks they're used to being there have disappeared, and they do the logical thing, which would be to check the zfs udev files.

[bghira]

pretty unfortunate how this stuff just blazes forward with no reason. what's the cost of leaving it in?

[Conan-Kudo]

pretty unfortunate how this stuff just blazes forward with no reason. what's the cost of leaving it in?

It's a potential namespace collision. /dev/zvol/* is way saner because it's namespaced to prevent collisions with other arbitrary devfs stuff. For example, someone can easily name a dataset sda or something, and the symlink would make it collide with udev's rules for generating SCSI block devices (such as the actual sda).

[bghira]

oh, interesting. can you link to more information about that problem? i can't reproduce it and i had other namespace conflicts fixed in the past, and this wasn't a problem then - don't see why it would be now.

[bghira]

it's fun that you point out namespace collisions because all pool names by default go into /. so if we're fixing one, fix both? it should be easy enough to migrate.

[Conan-Kudo]

We should, for sure, in my opinion...

[jgottula]

It might be interesting (and relatively easy) to add a setting somewhere (module parameter? zfs or zpool dataset property? var in one of the /etc/zfs/... config files?) that makes it a one-step process to alter the "default mount prefix". (Or whatever you want to call it.) Maybe on a system-wide level, maybe on a per-pool level.

Obviously it's possible to override the mountpoint property on every dataset, but that's clearly dumb. I don't know of any setting right now that lets you just completely change the prefix that zfs mount -a prepends to filesystem dataset names when determining what their mount point should be. (Caveat: I'm on my phone right now and haven't actually checked the code; maybe such a thing does exist already in some form.)

So right now it's as if the "default mount prefix" (if you will) is fixed to /; but in principle it seems like it'd be relatively easy, and not really break much if anything, to allow that to be user-settable to /zfs/, or /mount/zfs/, or whatever, for those who prefer that.

[Conan-Kudo]

We should consider moving the default to /mnt or /mnt/zfs, though. It doesn't make sense for datasets to be dumped into / by default for the same reasons we fixed this issue with collisions in /dev.

[bghira]

well there's another way to look at it, that both of these things have been done for more than 10 years and it's not been a problem til someone wanted to simply clean up the appearance of /dev on their own system - there's no performance gain to be had here.

[jgottula]

I should probably mention, for the future benefit of anyone who happens to come across this particular change and wants the old zvol symlink behavior back, it's very straightforward to do.

Create a new file, /etc/udev/rules.d/60-zvol.rules. The name matters: it's the same as the file in /usr/lib/udev/rules.d/, so it overrides that file. Then put this line in the file:

KERNEL=="zd*", SUBSYSTEM=="block", ACTION=="add|change", PROGRAM=="/usr/lib/udev/zvol_id $devnode", SYMLINK+="%c zvol/%c"

To make the change apply immediately without rebooting or anything, you can run sudo udevadm control -R; sudo udevadm trigger --action=change --settle /dev/zd*.

The nice thing about this kind of tweak, is that udev (similar to systemd) intentionally makes it so that rules in its /etc directory override same-named ones in its /usr/lib directory. Your distro's packages will install their files to the /usr/lib location; which means any overrides like this one in /etc won't get overwritten by package updates and will continue to act as an override (unless the package e.g. changes the name of its file or something like that).

(Oh yeah, and if your system is older and still has a filesystem configuration where /lib is an actually separate directory from /usr/lib, and not just a symlink to the latter: then replace all the cases where I put /usr/lib above with /lib instead.)

[jgottula]

well there's another way to look at it, that both of these things have been done for more than 10 years and it's not been a problem til someone wanted to simply clean up the appearance of /dev on their own system - there's no performance gain to be had here.

Well, strictly speaking, my understanding (which I readily admit may be wrong) is that originally, with ZFS on Solaris, filesystem datasets were mounted at root level, and zvol dev nodes were in /dev/zvol/.

And as far as I know, the initial ZFSonLinux port's behavior, years ago, of putting the zvol nodes directly in /dev/ was an aberration from that, and kind of a temporary kludge thing. That's my understanding, anyway.

So at least from the perspective of "how have things canonically been done WRT zvol nodes and fs dataset mountpoints", I believe zvol nodes in /dev/zvol/ and mounts in / is the most "correct" (if that's even the right word).

(Which isn't to say that it's necessarily the most consistent possible setup; or the one that avoids namespace collisions the best; or the one that every user must use; or anything like that. But there is at least a historical and cross-platform justification for it—again, as far as I know.)

[bghira]

so you're advocating for a simple meaningless change that only hurts people who aren't you? making them go out of their way to discover the reason that their stuff is no longer going to work, and then implement a workaround? its likely an equally small number of people who have a problem with the rule being implemented. can't you just please stop changing for the sake of changing? this has zero maintenance cost.

[jgottula]

so you're advocating

No. In my comment above, I just stated an arguable justification for things being this way. To be clear: I'm not particularly strongly advocating for anything here.

This PR only came into existence because I was already submitting patches to this exact udev rules file for unrelated reasons (see #12302 for context, if you care). And since I was already scrutinizing this file, I of course saw the compat symlink part over and over, wondered "does that really need to be there anymore?", checked the history of issues/commits behind it, etc.

It seemed arguably reasonable that those symlinks shouldn't still be there, so I submitted a PR with the change, with language in both the commit message and the PR body that I think pretty clearly indicates that I was explicitly not taking a strong position on whether this was necessarily the "Right" (with a capital 'R') change to make wrt compatibility concerns. But I figured it wouldn't hurt to at least propose it, and let the people in charge of the project decide the larger issue of "is this a good idea or not".

I don't have a super-strong personal opinion in either direction.

---

so you're advocating for a simple meaningless change that only hurts people who aren't you? making them go out of their way to discover the reason that their stuff is no longer going to work, and then implement a workaround? its likely an equally small number of people who have a problem with the rule being implemented. can't you just please stop changing for the sake of changing? this has zero maintenance cost.

If you think that merging this PR was a bad decision, that maybe not enough discussion went on before merging it, that on balance this change does have too much of a negative effect (which, again, is a perspective that I can certainly understand), then the thing to do is to raise the issue with @behlendorf et al; i.e. the people who actually make the decisions around here.

I'm just a lowly contributor who's submitted a handful of issues and PR's from time to time.

If merging this PR was a bad idea, then make your case, maybe we can have more discussion about which way things should be, and I'm sure the commit could be reverted with ease if that's how the project maintainers feel. I wouldn't be, like, personally offended.

So just please do me a favor and make this a little less of a personal flame war. I've tried to be pretty even-handed in this PR + comment thread about e.g. the potential for the change to break stuff, or about the arguable merits different ideas about where dev nodes or mount points "should" be. And I feel like the characterizations you've made aren't entirely fair.

[Conan-Kudo]

so you're advocating for a simple meaningless change that only hurts people who aren't you? making them go out of their way to discover the reason that their stuff is no longer going to work, and then implement a workaround? its likely an equally small number of people who have a problem with the rule being implemented. can't you just please stop changing for the sake of changing? this has zero maintenance cost.

There is no such thing as "zero maintenance cost". Speaking from experience with working with ZFS for the better part of the last decade, everything we do by default has an impact, both positive and negative. This particular behavior has caused me more than a few issues over the years, I just didn't know where it was coming from until this PR came up and I saw it fix my issues where I had device nodes failing to be created because they collided with dataset names on machines. This is particularly problematic because ZFS automatically discovers and populates everything.

So from my perspective, this PR actually fixed things and has the benefit of rationalizing the behavior to what is actually expected for zvols. It also mirrors what DM and other volume management stacks do by only putting their node aliases in a namespace subdirectory that they control.

Also, we're making this change for the next release series, where we change default behaviors anyway. Nothing has changed in the current stable release series.