Date: Thu, 02 Sep 2021 04:16:21 UTC
On Wed, Sep 01, 2021 at 03:10:53PM +0200, Alexander Lochmann wrote: > On 31.08.21 16:44, Konstantin Belousov wrote: > >> So in all of those call sequences the buffer lock is not acquired. > >> However, I'd not rule out that our tooling could be broken as well. > > Buffer is locked inside UFS_BALLOC(), which returns it to the ffs_write() > > use. > I took a deep dive into our data, and had a closer look at two samples. > In both cases the b_lock is not acquired. > > Since the debug information seems to be damaged, I had to use 'objdump > -S' to translate the pc of the unguarded memory access to a source code > position. > It seems to be vp->v_lasta = bp->b_blkno; in > https://thasos.cs.tu-dortmund.de/freebsd-lockdoc/lockdoc-v13.0-0.6/source/sys/kern/vfs_cluster.c#L802. > > It was release in binsfree() and bq_insert(): > https://thasos.cs.tu-dortmund.de/freebsd-lockdoc/latest/source/sys/kern/vfs_bio.c#L1537 > https://thasos.cs.tu-dortmund.de/freebsd-lockdoc/latest/source/sys/kern/vfs_bio.c#L1977 Ah, it is bp->b_blkno access after the b*write() functions were called to write out and release the buffer, right. I put the patch to fix this into https://reviews.freebsd.org/D31780 Please remind me what attributions to use for 'Reported by:' tagline. > > Read e.g. sys/ufs/ufs/inode.h gerald comment above struct inode definition. > > It provides more detailed exposure. > Aaah. Thx. This is about the struct inode. So I assume it also applies > for a vnode belonging to an inode. Am I right?> Vnode lock is a lock > obtained with vn_lock(). It is up to filesystem When needed, yes, it is a reasonable locking strategy. But I am not sure that we actually use for any of the struct vnode fields proper, Something closer to it is for v_writecount, but formally it is under the vnode interlock. Although I do not think we ever modify it without holding vnode lock, in some mode. > > to implement VOP_LOCK() which locks the vnode. > > > > Default VOP_LOCK() locks vp->v_vnlock, which again by default points > > to &vp->v_lock. > > > > There are several special cases. For instance, for FFS and snapshot vnodes, > > v_vnlock points to the snapdata->sn_lock (snapdata is unique per FFS mount). > > For nullfs non-reclaimed vnodes, v_vnlock points to the lower vnode lock. > > > Thx! Is this written down somewhere? No, but it is rather clear from the code.