unable to mount ceph root at boot due to stripping of trailing slashes

Applying this patch and rebuilding mountall fixed the problem for me also. What needs to be done to get this patch merged and a fixed package released for precise?

Debdiff of patch by Damien Churchill <email address hidden> attached.

Damien,

I'm a bit confused by this part of the patch, which seems a bit odd to me:

+ int devlen;
[...]
+ devlen = strlen(mnt->device);
+ if ( devlen < 2 || strcmp(mnt->device + (devlen - 2), ":/")) {
+ strip_slashes (mnt->device);
+ }

Neither part of that conditional makes a whole lot of sense to me (though, admittedly, I know little of ceph). If ceph doesn't need the trailing slashes removed, why do it "sometimes"?

(1) In the first case, "if ( devlen < 2) strip_slashes()" seems to assume that the device would be "/" and we're reducing it to an empty string. Is that a sane result?
(2) The second part of the conditional won't strip slashes from "127.0.0.1:/" (which is correct, and addresses the bug report), but would then needlessly strip slashes from "127.0.0.1:/foo/", which I'm told is pointless, since both :/foo/ and :/foo are valid device names.

Can you comment on the above notes? If this conditional is pointless, I'd happily remove it and sponsor the upload and SRUs to older releases. If either of these is actually necessary, some rationale explaining the code (maybe even some comments to throw in the code so later auditors don't ask WTF, as I've just done) would be lovely.

If :/foo and :/foo/ have the same meaning, then we would want to still normalize them for mountall's purposes. I think ceph is not the only network filesystem where stripping the only / may give unexpected results; I get strange results for NFSv4 root mounts too, though with NFS it doesn't actually fail to mount, it just looks strange.

I haven't reviewed the patch, but the above general principle makes sense to me.

mountall calls strip_slashes for the device and the mount path. For mount path, it makes sense. For mounting /dev/sda on something, having /dev mounted first makes sense, and I guess for that you need strip_slashes. Perhaps the strip slashes and that part of the dependency logic should only trigger for bind mounts or things in /dev. A simpler check for that might be "not a remote filesystem" -- you already have is_remote, and for remote filesystems the device just isn't a local path ever.

The Ceph device string can be (EBNF off the top of my head)

address = ip | hostname
mon = address [ ":" port ]
mons = mon [ "," mons ]
dev = mons ":" path

For example:

1.2.3.4:/
1.2.3.4:/foo
1.2.3.4:6789:/foo
1.2.3.4,2.3.4.5:/foo
1.2.3.4:6789,2.3.4.5:/foo
1.2.3.4:6789,2.3.4.5:6789:/foo

path always begins with a slash.

And there's IPv6 support in there too.

But really, mountall should treat dev as an opaque string until proven otherwise (is_remote gives false, etc).

Seems to me that the filesystem-specific mount and/or the filesystem module should get the 'device' path unmodified, and deal with it how it will. (For instance, as noted, nfs mounts can also contain a trailing path, and nfsserv:/ should work unsurprisingly).

nfs currently works because it doesn't try to validate that '/' exists; mount.nfs requires a :, and passes on a
potentially-empty path to the kernel mount function. Ceph's kernel module could accept that as well, but the
fact remains that mountall really should not be removing the trailing /.

Thanks, I've reviewed the patch here and pushed a fix to the mountall branch with some modifications. We don't actually want to special-case the 'ceph' filesystem type, the same parsing should be used for all network filesystems.

This fix will be included in the next upload.

This bug was fixed in the package mountall - 2.40

---------------
mountall (2.40) unstable; urgency=low

  * Add /run/user as a default mountpoint, in support of $XDG_RUNTIME_DIR.
    LP: #894391.

mountall (2.39) unstable; urgency=low

  * Adjust parsing of device names so that network mounts pointing to the
    server's root don't have the path mis-normalized to the empty string.
    Thanks to Damien Churchill <email address hidden> for the initial
    implementation. LP: #809221.
  * Add compatibility jobs with names to match the historic sysvinit init
    scripts, so that startpar can DTRT.
 -- Steve Langasek <email address hidden> Fri, 31 Aug 2012 19:55:01 -0700