On Sun, Mar 27, 2022 at 03:36:20PM +0200, Sebastien Marie wrote:
> Hi,
>
> v_numoutput is a struct member of vnode which is used to keep track the
> number
> of writes in progress.
>
> in several function comments, it is marked as "Manipulates v_numoutput. Must
> be
> called at splbio()".
>
> So I added a "[B]" mark in the comment to properly document the need of
> IPL_BIO
> protection.
>
> Next, I audited the tree for usage. I found 2 occurrences of v_numoutput
> (modification) without the required protection, inside dev/softraid.c. I
> added
> them.
>
> Comments or OK ?
anyone ?
the purpose of splbio() is to protect v_numoutput manipulation from interrupts.
for example, bread_cluster_callback() is called from interrupt context (per
code
documentation), and could modify v_numoutput (code path: bread_cluster_callback
-> biodone -> vwakeup).
to ensure that it effectively required (code not already called at IPL_BIO
level), I added some splassert() (the machine is using softraid with encrypting
discipline):
- for sr_rw:
splassert_check(6,ffffffff81fde589,30859b2dea1d26e8,400,8000,8000) at
splassert_check+0x4d
sr_rw(ffff80000041f000,400,ffff800000472000,8000,10,0) at sr_rw+0x1fe
sr_meta_save(ffff800000422000,1,628df3ebb476eac1,ffff800000422000,ffffffff82712a68,0)
at sr_meta_save+0x23d
sr_ioctl_createraid(ffff80000041f000,ffffffff82712a68,0,ffffffff822fe290,cae617dbeff84e1f,ffffffff82712d50)
at sr_ioctl_createraid+0xe03
sr_boot_assembly(ffff80000041f000,ffff80000041f000,e2eb5961e3789080,ffff80000041f328,ffff80000041f000,ffffffff8229fd40)
at sr_boot_assembly+0x936
sr_attach(0,ffff80000041f000,0,0,ab529b23aa62cd2b,0) at sr_attach+0x157
config_attach(0,ffffffff822bff60,0,0,33d5fbe4fc2cb990,c000000) at
config_attach+0x1f4
main(0,0,c000000,1001000,1000000,ffff8000fffffa40) at main+0x4fb
splassert: sr_rw: want 6 have 0
Starting stack trace...
splassert_check(6,ffffffff81fde589) at splassert_check+0x4d
sr_rw(ffff80000041f000,413,ffff800001a52000,8000,10,0) at sr_rw+0x1fe
sr_meta_save(ffff800001a0b000,1) at sr_meta_save+0x23d
sr_ioctl_createraid(ffff80000041f000,ffff80000199b800,1,0) at
sr_ioctl_createraid+0xe03
sr_bio_handler(ffff80000041f000,0,c2e84226,ffff80000199b800) at
sr_bio_handler+0x223
VOP_IOCTL(ffff8000a7ae9730,c2e84226,ffff80000199b800,3,fffffd879e7e4f00,ffff8000a76acd28)
at VOP_IOCTL+0x5c
vn_ioctl(fffffd8746f39080,c2e84226,ffff80000199b800,ffff8000a76acd28) at
vn_ioctl+0x75
sys_ioctl(ffff8000a76acd28,ffff8000a7d277e0,ffff8000a7d27830) at sys_ioctl+0x2c4
syscall(ffff8000a7d278a0) at syscall+0x374
Xsyscall() at Xsyscall+0x128
end of kernel
- for sr_ccb_rw, I had some problem to get proper/readable stack trace as the
console is just spammed too much and stack trace are interleaved.
splassert: sr_ccb_rw: want 6 have 0
Starting stack trace...
splassert_check(6,ffffffff81f0b74d) at splassert_check+0x4d
sr_ccb_rw(ffff800000422000,0,836b80,3800,ffff800021bff000,1001,f0cbd67d55f39eb0)
at sr_ccb_rw+0x194
sr_crypto_dev_rw(ffff800000423300,ffff800000423300) at sr_crypto_dev_rw+0x4a
sr_crypto_rw(ffff800000423300) at sr_crypto_rw+0xb6
sr_scsi_cmd(fffffd86af9f11c0) at sr_scsi_cmd+0x30c
scsi_xs_exec(fffffd86af9f11c0) at scsi_xs_exec+0x3f
sdstart(fffffd86af9f11c0) at sdstart+0x2d1
scsi_iopool_run(ffff800000422aa8) at scsi_iopool_run+0x159
scsi_xsh_runqueueffffd86af9f11c0) at scsi_xs_exec+0x3f
sdstart(fffffd86af9f11c0) 00000419d80) at scsi_xsh_add+0x91
sdstrategy(fffffd86b037fec8) at sdstrategy+0x112
spec_strategy(ffff8000a7689c88) at spec_strategy+0x56
VOP_STRATEGY(ffff8000fffda770,fffffd86b037fec8) at VOP_STRATEGY+0x4c
ufs_strategy(ffff8000a7689d18) at ufs_strategy+0xe5
VOP_STRATEGY(ffff8000a78bab98,fffffd86b037fec8) at VOP_STRATEGY+0x4c
bwrite(fffffd86b037fec8) at bwrite+0x138
ufs_dirremove(ffff8000a78bab98,fffffd874cda31e0,800c,0) at ufs_dirremove+0x19f
ufs_remove(ffff8000a7689e50) at ufs_remove+0xbe
VOP_REMOVE(ffff80s_truncate+0x7d0
ufs_inactive(ffff8000a7689d98) at ufs_inactive+
dounlinkat(ffff8000a766e540,ffffff9c,146b649b778,0) at dounlinkat+0xbd
syscall(ffff8000a768a070) at syscall+0x374
Xsyscall() at Xsyscall+0x128
end of kernel
end trace frame: 0x7f7ffffbffa0, count: 235
End of stack trace.
> --
> Sebastien Marie
>
> Index: dev/softraid.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/softraid.c,v
> retrieving revision 1.422
> diff -u -p -r1.422 softraid.c
> --- dev/softraid.c 20 Mar 2022 13:14:02 -0000 1.422
> +++ dev/softraid.c 27 Mar 2022 13:28:55 -0000
> @@ -437,8 +437,12 @@ sr_rw(struct sr_softc *sc, dev_t dev, ch
> b.b_resid = bufsize;
> b.b_vp = vp;
>
> - if ((b.b_flags & B_READ) == 0)
> + if ((b.b_flags & B_READ) == 0) {
> + int s;
> + s = splbio();
> vp->v_numoutput++;
> + splx(s);
> + }
>
> LIST_INIT(&b.b_dep);
> VOP_STRATEGY(vp, &b);
> @@ -2006,8 +2010,12 @@ sr_ccb_rw(struct sr_discipline *sd, int
> ccb->ccb_buf.b_vp = sc->src_vn;
> ccb->ccb_buf.b_bq = NULL;
>
> - if (!ISSET(ccb->ccb_buf.b_flags, B_READ))
> + if (!ISSET(ccb->ccb_buf.b_flags, B_READ)) {
> + int s;
> + s = splbio();
> ccb->ccb_buf.b_vp->v_numoutput++;
> + splx(s);
> + }
>
> LIST_INIT(&ccb->ccb_buf.b_dep);
>
> Index: sys/vnode.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/vnode.h,v
> retrieving revision 1.163
> diff -u -p -r1.163 vnode.h
> --- sys/vnode.h 12 Dec 2021 09:14:59 -0000 1.163
> +++ sys/vnode.h 27 Mar 2022 13:28:56 -0000
> @@ -89,6 +89,7 @@ RBT_HEAD(namecache_rb_cache, namecache);
> * Locks used to protect struct members in struct vnode:
> * a atomic
> * V vnode_mtx
> + * B IPL_BIO
> */
> struct uvm_vnode;
> struct vnode {
> @@ -113,7 +114,7 @@ struct vnode {
> struct buf_rb_bufs v_bufs_tree; /* lookup of all bufs */
> struct buflists v_cleanblkhd; /* clean blocklist head */
> struct buflists v_dirtyblkhd; /* dirty blocklist head */
> - u_int v_numoutput; /* num of writes in progress */
> + u_int v_numoutput; /* [B] num of writes in progress */
> LIST_ENTRY(vnode) v_synclist; /* vnode with dirty buffers */
> union {
> struct mount *vu_mountedhere;/* ptr to mounted vfs (VDIR) */
--
Sebastien Marie
