panic: condition seqc_in_modify(_vp->v_seqc) not met at zfs_acl.c:1147 (zfs_acl_chown_setattr)

[Mateusz Guzik]

I think for future proofing it would be best if all vnodes going there
had seqc marked, thus I think this should do the trick:

diff --git a/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c
b/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c
index d5f0da9ecd4b..8172916c4329 100644
--- a/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c
+++ b/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c
@@ -2756,7 +2756,9 @@ zfs_setattr(znode_t *zp, vattr_t *vap, int
flags, cred_t *cr)
                err = zfs_acl_chown_setattr(zp);
                ASSERT(err == 0);
                if (attrzp) {
+                       vn_seqc_write_begin(ZTOV(attrzp));
                        err = zfs_acl_chown_setattr(attrzp);
+                       vn_seqc_write_end(ZTOV(attrzp));
                        ASSERT(err == 0);
                }
        }

I don't see other calls to the routine.

On 2/16/21, Andriy Gapon <avg at freebsd.org> wrote:
> On 15/02/2021 11:45, Andriy Gapon wrote:
>> On 15/02/2021 10:22, Andriy Gapon wrote:
>>>
>>> I've got this panic once when copying a couple of files.
>>> The system is stable/13 as of 1996360d7338d, a custom kernel
>>> configuration, but
>>> no local source code modifications.
>>>
>>> Unread portion of the kernel message buffer:
>>> VNASSERT failed: ({ seqc_t __seqc = (_vp->v_seqc);
>>> __builtin_expect((__seqc &
>>> 1), 0); }) not true at
>>> /usr/devel/git/trant/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_acl.c:1147
>>> (zfs_acl_chown_setattr)
>>> 0xfffff8013e4e85b8: type VDIR
>>>     usecount 1, writecount 0, refcount 1 seqc users 0 mountedhere 0
>>>     hold count flags ()
>>>     flags ()
>>>     lock type zfs: EXCL by thread 0xfffffe01dd1cd560 (pid 30747,
>>> kdeinit5, tid
>>> 159911)
>>> panic: condition seqc_in_modify(_vp->v_seqc) not met at
>>> /usr/devel/git/trant/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_acl.c:1147
>>> (zfs_acl_chown_setattr)
>>>
>>> Any ideas, suggestions, hints?
>>> Thanks!
>>>
>> ...
>>> #4  0xffffffff8036fd21 in zfs_acl_chown_setattr (zp=0xfffff801ccd203b0)
>>>     at
>>> /usr/devel/git/trant/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_acl.c:1147
>>> #5  0xffffffff8037e52d in zfs_setattr (zp=0xfffff8024b04f760,
>>>     vap=vap at entry=0xfffffe029a36c870, flags=flags at entry=0,
>>>     cr=<optimized out>, cr at entry=0xfffff8003ecedc00)
>>>     at
>>> /usr/devel/git/trant/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c:2758
>>
>> So, this is actually the second zfs_acl_chown_setattr call here:
>>                 err = zfs_acl_chown_setattr(zp);
>>                 ASSERT(err == 0);
>>                 if (attrzp) {
>>                         err = zfs_acl_chown_setattr(attrzp);
>>                         ASSERT(err == 0);
>>                 }
>>
>> I am not sure if the assertion is actually applicable to attrzp (extended
>> attributes "directory").
>> At least I do not see any seq calls for it.
>>
>
> So, I think that the problem should be reproducible by simply chown-ing a
> file
> with an extended attribute.  The kernel should be compiled with both
> DEBUG_VFS_LOCKS and INVARIANTS.
>
> --
> Andriy Gapon
>


-- 
Mateusz Guzik <mjguzik gmail.com>