vfs: add VOP_GETATTR_LITE
AbandonedPublic
Actions

Authored by mjg on Aug 7 2020, 8:11 AM.

Details

Reviewers

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.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

mjg created this revision.Aug 7 2020, 8:11 AM

Herald added a subscriber: imp. · View Herald TranscriptAug 7 2020, 8:11 AM

mjg requested review of this revision.Aug 7 2020, 8:11 AM

Harbormaster completed remote builds in B32828: Diff 75540.Aug 7 2020, 8:11 AM

mjg added inline comments.Aug 7 2020, 8:11 AM

sys/fs/tmpfs/tmpfs_vnops.c
466	smr enter/exit is conditional because it contains an atomic

mjg edited the summary of this revision. (Show Details)Aug 7 2020, 8:18 AM

mjg edited the summary of this revision. (Show Details)

mjg updated this revision to Diff 75547.Aug 7 2020, 10:11 AM

mjg edited the summary of this revision. (Show Details)

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

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

kib added inline comments.Aug 7 2020, 12:13 PM

sys/kern/vfs_vnops.c
2273	EAGAIN should be handled, by fallback to locked variant.

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

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

In D25983#576335, @kib wrote:

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.

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.

pre hook should assert that the vnode is held

The previous patch already did.

mjg added inline comments.Aug 8 2020, 2:08 AM

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 added inline comments.Aug 8 2020, 4:14 PM

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 added inline comments.Aug 8 2020, 5:55 PM

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.

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

mjg added inline comments.Aug 9 2020, 9:21 AM

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 added inline comments.Aug 9 2020, 4:04 PM

share/man/man9/VOP_ATTRIB.9
92 ↗	(On Diff #75622)	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 ↗	(On Diff #75622)	s/expects/works with the unlocked vnode/ Would it break if the vnode is locked ?
168 ↗	(On Diff #75622)	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.

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.

mjg abandoned this revision.Mar 12 2024, 10:32 PM