devfs: Move most ioctl logic down to vnode layer

[cem]

Summary

File layer is now a thin shim around vnode layer.

Test Plan

Please CC anyone you think should review this.

[jhb]

FWIW, phk@ did this on purpose (not having a VOP_IOCTL). What is the reason for changing it? Does it enable something else?

[cem]

In D7286#151600, @jhb wrote:

FWIW, phk@ did this on purpose (not having a VOP_IOCTL).

Do you recall what the reason was?

What is the reason for changing it? Does it enable something else?

Yeah, it allows ioctls on devfs devices from other vnode layer kernel components. In my ddb nextboot(8) series, I use this to issue an ioctl to the devvp backing a filesystem.

The sketch of the idea is here: https://github.com/cemeyer/freebsd/commits/ddb_nextboot

Maybe there is a better way of accomplishing that. But no good reason occurs to me to handle devfs ioctls only at the file layer.

[jhb]

In D7286#151601, @cem wrote:

In D7286#151600, @jhb wrote:

FWIW, phk@ did this on purpose (not having a VOP_IOCTL).

Do you recall what the reason was?

Just an optimization to avoid the extra layers of indirection.

What is the reason for changing it? Does it enable something else?

Yeah, it allows ioctls on devfs devices from other vnode layer kernel components. In my ddb nextboot(8) series, I use this to issue an ioctl to the devvp backing a filesystem.

The sketch of the idea is here: https://github.com/cemeyer/freebsd/commits/ddb_nextboot

Maybe there is a better way of accomplishing that. But no good reason occurs to me to handle devfs ioctls only at the file layer.

I think this is a sufficient reason to change it.

[kib]

sys/fs/devfs/devfs_vnops.c

804 ↗

(On Diff #18675)

This is half-done. Either you should call vnops.fo_ioctl() instead of VOP_IOCTL() (see e.g. devfs_close_f() how it is done), or you do not need VOP_IOCTL() call there at all, instead directly dispatching the calls to appropriate routines from this file.

My preference is to redirect to vnops.fo_ioctl, it would collapse error !=0 and error == 0 cases.

Also, I do not see why do you need devfs_fp_check() in the file-level method.

[cem]

Thanks for taking a look.

sys/fs/devfs/devfs_vnops.c

804 ↗

(On Diff #18675)

Why vnops.fo_ioctl over VOP_IOCTL()? The only cases it handles today are VDIR and VREG, while these are character devices.

Also, I do not see why do you need devfs_fp_check() in the file-level method.

At the vnode layer, we may not have a file to check. So the check is done at the appropriate layer.

[kib]

sys/fs/devfs/devfs_vnops.c

804 ↗

(On Diff #18675)

Calling VOP_XXX() in the filesystem code for its own vnode is silly. This and the fact that vnops.fo_ioctl() does some additional handling, which is relevant now for VDIR, and might be relevant for other cases in future.

My point is that you do not need devfs_fp_check(), the file method only should handle cdevpriv.

[cem]

I'm not exactly sure I understand, but is this closer to what you are looking
for?

[cem]

Fix error handling in devfs_fp_check.

[kib]

sys/fs/devfs/devfs_vnops.c

141 ↗

(On Diff #18702)

It would be cleaner to check for errors earlier, returning to the previous nesting level type.

error = devfs_vn_check();
if (error != 0)
  return (error);
if (*devvp != fp->f_data)
   return (ENXIO);
curthread->td_fpop = fp;
return (0);

And no need to check for *dswp != NULL, since devfs_vn_check() does not return success and NULL *dswp.

[kib]

sys/fs/devfs/devfs_vnops.c

141 ↗

(On Diff #18702)

In fact, after this simplification,ee a reason for devfs_vn_check. Just use devvn_refthread() in devfs_ioctl() directly and things become much simpler.

[cem]

sys/fs/devfs/devfs_vnops.c

141 ↗

(On Diff #18702)

Ok.

[cem]

Eliminate devfs_vn_check and inline refthread into devfs_ioctl.

[cem]

Fix a few minor typos from the shuffling of vn_check.

[kib]

sys/fs/devfs/devfs_vnops.c

121 ↗

(On Diff #18718)

Did you ever boot the system with these changes ? All VCHR io should abort with ENXIO.

Just keep devfs_fp_check() intact.

[cem]

Fix one last typo in fp_check.

[kib]

sys/fs/devfs/devfs_vnops.c

121 ↗

(On Diff #18718)

I do not see why this rearrangement of devfs_fp_check is needed or useful. Drop it.

Otherwise the patch looks fine.

[cem]

Remove fp_check shuffling.