vfs: add VOP_GETATTR_LITE

[mjg]

Summary

Provides a cheaper alternative for VOP_GETATTR for when only a fraction of the fields are needed (most notably for consumers which only want the size). The new struct is *not* yet fit for mmap due to missing va_flags, but provisions are made to allow fro growing the structure and catching filesytems which did not get updated.

The VOP_UGETATTR routine can be called unlocked which can be exploited by SMR-aware filesystems to provide scalable operation.

New type is declared to make things less error prone.

Support for UFS and TMPFS is included, along with sample conversion of lseek and execve. All parts will be committed separately.

[mjg]

sys/fs/tmpfs/tmpfs_vnops.c

466

smr enter/exit is conditional because it contains an atomic

[kib]

1. I suggest not to add vattr_lite, pass normal vattr.
2. Instead of two vops, pass locked arg.
3. pre hook should assert that the vnode is held
4. vnode_if.src chunks are missing
5. default (fallback) implementation is missing

1 and 2 should reduce the interface blow for fs to implement.

[kib]

sys/kern/vfs_vnops.c

2273

EAGAIN should be handled, by fallback to locked variant.

[mjg]

• upload the full diff. previous was generated against the wrong top commit
• EAGAIN -> ENOENT in ufs to properly denote failure

[mjg]

The previous diff was missing the initial commit, mistake when generating.

In D25983#576335, @kib wrote:

1. I suggest not to add vattr_lite, pass normal vattr.

This is very error prone. For example when converting an existing getattr user accesses to unsupported fields are automatically detected by the compiler. Also in the future someone may want to access a field not provided by the lite version and the compiler wont tell them.

2. Instead of two vops, pass locked arg.

I did this in my original patch, I find the U-variant nicer to use and less cumbersome to implement. vfs_smr_enter has an atomic inside, so routines have to special case for it. Nonetheless, if you insist, I will revert it back to that stage.

3. pre hook should assert that the vnode is held

The previous patch already did.

[mjg]

sys/ufs/ufs/ufs_vnops.c

580

So in comparison, with passing the flag this would be +/-:

tatic int
ufs_getattr_lite(struct vop_getattr_lite_args *ap)
{
        struct vnode *vp;
        struct inode *ip;
        struct vattr_lite *lvap;
        int locked;

        vp = ap->a_vp;
        locked = ap->a_locked;

        ip = VTOI(vp);
        if (locked == LA_UNLOCKED) {
                vfs_smr_enter();
                ip = VTOI_SMR(vp);
                if (__predict_false(ip == NULL)) {
                        vfs_smr_exit();
                        return (ENOENT);
                }
        }

        lvap = ap->a_lvap;
        if (I_IS_UFS1(ip)) {
                lvap->va_size = ip->i_din1->di_size;
        } else {
                lvap->va_size = ip->i_din2->di_size;
        }
        lvap->va_nlink = ip->i_effnlink;
        lvap->va_type = IFTOVT(ip->i_mode);
        lvap->va_mode = ip->i_mode & ~IFMT;
        lvap->va_uid = ip->i_uid;
        lvap->va_gid = ip->i_gid;

        if (locked == LA_UNLOCKED) {
                vfs_smr_exit();
        }
        return (0);
}

which i find way uglier

[kib]

sys/ufs/ufs/ufs_vnops.c

559

After the vnode is completely deconstructed, deadfs returns EBADF for VOP_GETATTR(). I think this is the right error code, not ENOENT.

Or better, lseek on error shold fall back to VOP_GETATTR().

580

You can write it as

if (locked) {
 ufs_getattr_lite_fill(ip, vattr);
 return (0);
}

vfs_smr_enter();
...
ufs_getattr_lite_fill(ip, vattr);
vfs_smr_exit();
return (0);

I consider increasing the surface that is needed to be grasped by random fs implementor much more important than added conditional.

[kib]

sys/kern/vfs_default.c

767

I think that new VOP might be (much) more useful if we define it as usable in non-sleepable context. In other words, if we have another vnode locked, or vnode interlock taken.

I think that default implementation for unlocked version should return a designated error like EAGAIN to indicate that caller should take some other code path.

[mjg]

• add flags argument to ugetattr to request non-blocking operation
• add manpage

[mjg]

sys/kern/vfs_default.c

767

So I added a flag for it so that consumers which want unlocked up but are fine with locked in the worst case don't have to check for it.

sys/ufs/ufs/ufs_vnops.c

559

Ops, missed this comment. I'll change to EBADF later. I'm returning ENOENT because _vn_lock is doing it.

580

If filling out is moved to a dedicated routine then I don't see what's the harm with separate entry point from vfs.

[kib]

share/man/man9/VOP_ATTRIB.9

92

I think you should be precise there, while it is completely unclear what kind of blocking is.

LK_NOWAIT requests that implementation does not take any locks.

130

s/expects/works with the unlocked vnode/

Would it break if the vnode is locked ?

168

s/blocking/locking

sys/kern/vnode_if.src

211

It is very clear, from these two definitions, that it is confusing. I still suggest to wrap two VOPs into one, selecting the locking regime by flags.

Think about readers of the code, not about number of if()s.

[mjg]

The comments are quite related so I'm going to address them here.

The fallback implementation of ugetattr has to take a lock to work and it trylocks to do it and as such describing it as not taking locks is imo misleading.

This is how things look like for various cases with my proposal

• the vnode is locked

error = VOP_GETATTR_LITE(vp, &attr, cred);
if (error)
    goto out;

• the vnode is unlocked and the callsite does not if the routine locks it (for example lseek)

error = VOP_UGETATTR_LITE(vp, &attr, cred, 0);
if (error)
    goto out; /* that's it, no need to retry with anything in contrast to LK_NOWAIT */

• the vnode is unlocked and the callsite does not want to block

error = VOP_UGETATTR_LITE(vp, &attr, cred, LK_NOWAIT);
if (error)
    goto fallback;
...
fallback:
...
error = VOP_GETATTR_LITE(vp, &attr, cred);
...

I think this is easy to follow and handy. The one-routine variant I see is cumbersome with separate indicator as to whether the vnode locked and whether blocking is allowed.