Page MenuHomeFreeBSD

ffs: lock buffers after snaplk with LK_NOWITNESS
ClosedPublic

Authored by kib on Jan 28 2022, 4:46 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 7, 6:36 PM
Unknown Object (File)
Tue, Nov 5, 4:54 PM
Unknown Object (File)
Thu, Oct 31, 7:40 PM
Unknown Object (File)
Sep 24 2024, 9:06 AM
Unknown Object (File)
Sep 22 2024, 1:24 PM
Unknown Object (File)
Sep 22 2024, 10:00 AM
Unknown Object (File)
Sep 22 2024, 5:35 AM
Unknown Object (File)
Sep 18 2024, 7:24 PM
Subscribers

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kib retitled this revision from ffs: lock buffers after snalk with LK_NOWITNESS to ffs: lock buffers after snaplk with LK_NOWITNESS.Jan 28 2022, 5:35 AM
sys/kern/vfs_bio.c
4010 ↗(On Diff #102029)

IMO it would look nicer written this way:

lockflags = LK_EXCLUSIVE | LK_INTERLOCK |
    ((flags & GB_LOCK_NOWAIT) != 0 ? LK_NOWAIT : LK_SLEEPFAIL);
#ifdef WITNESS
lockflags |= (flags & GB_NOWITNESS) != 0 ? LK_NOWITNESS : 0;
#endif
sys/ufs/ffs/ffs_alloc.c
89

Why did these get reordered? It's not alphabetical.

sys/ufs/ffs/ffs_balloc.c
72 ↗(On Diff #102029)

Why not after mount.h?

633 ↗(On Diff #102029)

I think some comment is deserved. I also do not quite understand this given Kirk's comment in D33921:

Turning to buffer deadlocks, the only way that the snapshot can be updated is when it internally decides to make a copy. Here we need another buffer in which to grab the old contents. As you noted, this will always be a new buffer so cannot conflict with any existing buffer, so that is not a problem.

If we're always locking a new buffer, then isn't it sufficient to use LK_NOWAIT in buf_alloc() as is done in D34072? I think witness will not complain when LK_NOWAIT is used. If not, then why is this safe?

kib marked 4 inline comments as done.Jan 29 2022, 12:50 AM
kib added inline comments.
sys/ufs/ffs/ffs_alloc.c
89

There were even more. I think I fixed everything now.

sys/ufs/ffs/ffs_balloc.c
633 ↗(On Diff #102029)

I suspect LK_NOWAIT would not work. It is perfectly fine to have ufs_balloc_ufs2() to wait for some buffer lock, which is after the snap lock. For instance, the indirect block buffer might be LK_KERNPROC still after other CoW action allocated some indirect block and wrote the indirect buffer asynchronously.

kib marked 2 inline comments as done.

Update patch with the Mark' notes changes. This is the diff on top of D34072.

Once passing Peter Holm's testing, this looks good to go.

This revision is now accepted and ready to land.Jan 30 2022, 10:57 PM

Peter confirms that this patch hides snaplk->bufwait order from witness.