Date: Fri, 27 Aug 2021 21:40:01 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Alexander Lochmann <alexander.lochmann@tu-dortmund.de> Cc: freebsd-fs <freebsd-fs@freebsd.org>, Horst Schirmeier <horst@schirmeier.de> Subject: Re: Various unprotected accesses to buf and vnode Message-ID: <YSkxgXyXZfNvrXA/@kib.kiev.ua> In-Reply-To: <55f3661e-2173-793e-4834-bbcd79d3d99e@tu-dortmund.de> References: <55f3661e-2173-793e-4834-bbcd79d3d99e@tu-dortmund.de>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Aug 27, 2021 at 03:48:38PM +0200, Alexander Lochmann wrote: > Hi folks, > > I'm still analyzing our LockDoc (lock analysis) data for FreeBSD. I came > across accesses that do not adhere to the locking documentation. I cannot > tell whether these accesses are made deliberately without locks or not. > I listed them below. > > Can you please shed some light on those cases? > > Thx and regards, > Alex > > - Write to buf.b_error without buf.b_lock: > https://github.com/freebsd/freebsd-src/blob/main/sys/kern/vfs_vnops.c#L2846 I believe this is fine. If the buffer is still on the delayed write queue, then nobody can start the write, and buffer cannot leave the queue because we locked the bo lock. It might be slightly formally better to move clearing of b_error after we obtained the buffer lock, but I do not want to change this code just for formality. It might be that the clearing can be avoided at all, in fact. > > - Read of buf.b_blkno in cluster_write(): > According to the documentation b_lock is needed. > Is b_blkno maybe a read-only element of struct buf? No, b_blkno is not read-only, it is the disk block number for the block, as opposed to b_lbklno which is logical file block number. The buffer is instantiated with b_blkno == b_lblkno, and when the buffer is mapped to the real disk block, b_blkno is updated. Could you show me the backtrace of the situation where cluster_write() is called with unlocked buffer? > > - Read of buf.b_flags, buf.b_xflags and buf.b_vp: > https://github.com/freebsd/freebsd-src/blob/main/sys/kern/vfs_subr.c#L2351 > Are those reads innocent races? > According to our data, buf.b_lock is not acquired. These are fine. We check that nbp buffer is still on the clean/dirty queue of our vnode, and we own the buffer object lock. Since buffers are moved from the queues under the bo lock, the operation is safe. You may think about it in the following way: - the update of the flags word require owning corresponding lock to not loose other updates - but changes of the flags that are tested in the referenced lines are also protected by the buffer object lock > > - Write to vnode.v_bufobj.bo_object: > https://github.com/freebsd/freebsd-src/blob/main/sys/vm/vnode_pager.c#L291 > According to the documentation, '[...] the vnode lock which embeds the > bufobj' is needed. However, interlock is taken in line 276. > Is the interlock equivalent to the vnode lock? > (I assume 'the vnode lock' refers to vnode.v_lock.) vnode_pager_alloc() must be called with the vnode exclusively locked. This is asserted at the beginning of the function. > > - Is buf.b_bufobj a read-only element? You should scope the question. While buffer is owned by a vnode, b_bufobj is constant. Since buffers are type-stable, they migrate between vnodes as cache finds it required to reclaim and reuse. There, b_bufobj is changed.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?YSkxgXyXZfNvrXA/>