Re: device vnodes, and structural confusion

["J. Hannken-Illjes" <hannken%eis.cs.tu-bs.de@localhost>]

On Jun 17, 2013, at 5:02 AM, David Holland <dholland-tech%netbsd.org@localhost> 
wrote:

> As has come up many times in the past, we have a long-standing
> problem, or family of problems, where in the course of operating on a
> particular file system we end up not calling the file system's own
> vnode or fs operations, but the root file system's operations. This
> causes varying degrees of lossage depending on exactly what happens.
> 
> One of the ways this happens came up today while working on PR 47937.
> The vnode structure has both a v_mount field and a v_specmountpoint
> field(*); both are pointers to a struct mount (that is, a filesystem)
> and it's very easy for people to use the wrong one, or not be sure
> which is which.
> 
> (*) v_specmountpoint is not actually a field, it's a fake field
> provided by a macro.
> 
> In the short term, to reduce the confusion I would like to make the
> following changes:
> 
>   1. Rename v_mount, which is the filesystem the vnode is on (almost
>   always / for device vnodes), to "v_myfs".

What is wrong with this name?  `v_mount' is the `struct mount' this
vnode resides on and I don't see any confusion.

>   2. Rather than renaming v_specmountpoint, which is the filesystem
>   mounted on a device vnode, kill it off entirely, and replace it
>   with an inline (or noninline) function "specvn_getmountedfs()".
>   Make this function assert if called on a non-VBLK vnode instead of
>   silently producing trash.

Yes, please.  While here, you could account for spec node aliases like:

int
spec_node_mount_by_node(vnode_t devvp, struct mount **mpp)
{       
        vnode_t *vp;

        KASSERT(vp->v_type == VBLK);
        *mpp = NULL;
        mutex_enter(&device_lock);
        for (vp = SPECHASH(devvp->v_rdev); vp; vp = vp->v_specnext) {
                if (vp->v_specmountpoint) {
                        KASSERT(*mpp == NULL);
                        *mpp = vp->v_specmount;
#ifndef DIAGNOSTIC
                        break;
#endif
                }
        }
        mutex_exit(&device_lock);

        return 0;
}

> 
> This is going to be a fairly large massedit, but I think it's
> worthwhile for clarity even if it doesn't turn up any mistakes.
> And I expect it to turn up at least a couple howlers. :-/
> 
> Also, while doing the edit, where feasible stop retrieving the fs
> structure from the vnode structure (or other structures) by going
> through the device-mounted-on. There should in general be no reason to
> do this at all; a filesystem's own structures should all have adequate
> pointers to one another without going through the device.
> 
> In the long run, I think we should rearrange the way device vnodes are
> handled, so that the device vnodes seen and handled belong entirely to
> specfs and the vnodes for the on-disk inodes are stubs hidden behind
> the specfs vnodes. This would make it much more obvious when the wrong
> fs's ops get called; also it would have the generally beneficial
> effect of allowing a lot of rote cut and paste filesystem stuff for
> supporting device nodes to be flushed.
> 
> More on that when my ideas solidify a bit.
> 
> -- 
> David A. Holland
> dholland%netbsd.org@localhost

--
J. Hannken-Illjes - hannken%eis.cs.tu-bs.de@localhost - TU Braunschweig 
(Germany)