unionfs: fix suspendability check bugs

[markj]

Summary

• MNTK_SUSPENDABLE is set in mnt_kern_flag, not mnt_flag.
• The lower layer of a unionfs mount is read-only, so the mount should be suspendable iff the upper layer is suspendable.
• While here, remove a couple of superfluous comments.

Test Plan

The fact that we weren't setting MNTK_SUSPENDABLE properly was triggering a KASSERT failure in tmpfs: tmpfs_alloc_node() verifies that mnt_writeopcount > 0, but vn_start_write() was a no-op for the non-suspendable unionfs mount. This change fixes the assertion failure.

[mjg]

Oops. The change looks right, thanks.

Note that MNTK_SUSPENDABLE business as implemented right now is a hack. vn_start_write performs the check on passed struct mount, but the actual operation is performed on a structure returned by VOP_GETWRITEMOUNT. I don't recall why the hack is in place, maybe there is no good reason after all. This would also get rid of room for crap mistakes like the one fixed here.

[kib]

In D2714#51749, @mjg wrote:

Oops. The change looks right, thanks.

Note that MNTK_SUSPENDABLE business as implemented right now is a hack. vn_start_write performs the check on passed struct mount, but the actual operation is performed on a structure returned by VOP_GETWRITEMOUNT. I don't recall why the hack is in place, maybe there is no good reason after all. This would also get rid of room for crap mistakes like the one fixed here.

VOP_GETWRITEMOUNT() provides the stacking required for filesystems like nullfs or unionfs, to work.

[markj]

In D2714#51824, @kib wrote:

In D2714#51749, @mjg wrote:

Oops. The change looks right, thanks.

Note that MNTK_SUSPENDABLE business as implemented right now is a hack. vn_start_write performs the check on passed struct mount, but the actual operation is performed on a structure returned by VOP_GETWRITEMOUNT. I don't recall why the hack is in place, maybe there is no good reason after all. This would also get rid of room for crap mistakes like the one fixed here.

VOP_GETWRITEMOUNT() provides the stacking required for filesystems like nullfs or unionfs, to work.

As I understand it, Mateusz's suggestion is to perform the suspendability check on the mount point returned by VOP_GETWRITEMOUNT(vp) rather than the vnode's mount. This would obviate the need to propagate the MNTK_SUSPENDABLE check through passthrough filesystems.

This (untested) patch implements that change: https://people.freebsd.org/~markj/patches/20150606-correct_suspend_mount.diff

With this, the checks in nullfs and unionfs could be removed entirely.

[kib]

In D2714#52249, @markj wrote:

In D2714#51824, @kib wrote:

In D2714#51749, @mjg wrote:

Oops. The change looks right, thanks.

Note that MNTK_SUSPENDABLE business as implemented right now is a hack. vn_start_write performs the check on passed struct mount, but the actual operation is performed on a structure returned by VOP_GETWRITEMOUNT. I don't recall why the hack is in place, maybe there is no good reason after all. This would also get rid of room for crap mistakes like the one fixed here.

VOP_GETWRITEMOUNT() provides the stacking required for filesystems like nullfs or unionfs, to work.

As I understand it, Mateusz's suggestion is to perform the suspendability check on the mount point returned by VOP_GETWRITEMOUNT(vp) rather than the vnode's mount. This would obviate the need to propagate the MNTK_SUSPENDABLE check through passthrough filesystems.

This (untested) patch implements that change: https://people.freebsd.org/~markj/patches/20150606-correct_suspend_mount.diff

With this, the checks in nullfs and unionfs could be removed entirely.

The change probably works, modulo my note above about mnt_ref.

But this brings back the issue of r273271/r273336, since you could just test the mnt->vfs_susp_clean without the problem of r273271. In other words, MNTK_SUSPENDABLE can go away in any case.

Yes, tmpfs would need to provide dummy .vfs_susp_clean method again.

[markj]

In D2714#52250, @kib wrote:

In D2714#52249, @markj wrote:

In D2714#51824, @kib wrote:

In D2714#51749, @mjg wrote:

Oops. The change looks right, thanks.

Note that MNTK_SUSPENDABLE business as implemented right now is a hack. vn_start_write performs the check on passed struct mount, but the actual operation is performed on a structure returned by VOP_GETWRITEMOUNT. I don't recall why the hack is in place, maybe there is no good reason after all. This would also get rid of room for crap mistakes like the one fixed here.

VOP_GETWRITEMOUNT() provides the stacking required for filesystems like nullfs or unionfs, to work.

As I understand it, Mateusz's suggestion is to perform the suspendability check on the mount point returned by VOP_GETWRITEMOUNT(vp) rather than the vnode's mount. This would obviate the need to propagate the MNTK_SUSPENDABLE check through passthrough filesystems.

This (untested) patch implements that change: https://people.freebsd.org/~markj/patches/20150606-correct_suspend_mount.diff

With this, the checks in nullfs and unionfs could be removed entirely.

The change probably works, modulo my note above about mnt_ref.

Hm, I don't see the note you're referring to.

But this brings back the issue of r273271/r273336, since you could just test the mnt->vfs_susp_clean without the problem of r273271. In other words, MNTK_SUSPENDABLE can go away in any case.

Ok, that makes sense to me. I'll test and submit a change to that effect.

[kib]

In D2714#52492, @markj wrote:

Hm, I don't see the note you're referring to.

I mean that before the vn_suspendable(), the struct mount was always referenced after vn_start_write().

[markj]

In D2714#52525, @kib wrote:

In D2714#52492, @markj wrote:

Hm, I don't see the note you're referring to.

I mean that before the vn_suspendable(), the struct mount was always referenced after vn_start_write().

Sorry, I don't quite follow. Are you pointing out that with the change, the reference acquired in VOP_GETWRITEMOUNT() may be leaked?

[kib]

In D2714#53124, @markj wrote:

In D2714#52525, @kib wrote:

In D2714#52492, @markj wrote:

Hm, I don't see the note you're referring to.

I mean that before the vn_suspendable(), the struct mount was always referenced after vn_start_write().

Sorry, I don't quite follow. Are you pointing out that with the change, the reference acquired in VOP_GETWRITEMOUNT() may be leaked?

My comment is about vn_suspendable() existence, not about your changes.