[src/trunk]: src/sys/ufs/lfs Break deadlock in PR kern/52301.

[riastradh <riastradh%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/807ec8abd9c2
branches:  trunk
changeset: 1007592:807ec8abd9c2
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Sun Feb 23 08:40:37 2020 +0000

description:
Break deadlock in PR kern/52301.

The lock order is lfs_writer -> lfs_seglock.  The problem in 52301 is
that lfs_segwrite violates this lock order by sometimes doing
lfs_seglock -> lfs_writer, either (a) when doing a checkpoint or (b),
opportunistically, when there are no dirops pending.  Both cases can
deadlock, because dirops sometimes take the seglock (lfs_truncate,
lfs_valloc, lfs_vfree):

(a) There may be dirops pending, and they may be waiting for the
seglock, so we can't wait for them to complete while holding the
seglock.

(b) The test for fs->lfs_dirops == 0 happens unlocked, and the state
may change by the time lfs_writer_enter acquires lfs_lock.

To resolve this in each case:

(a) Do lfs_writer_enter before lfs_seglock, since we will need it
unconditionally anyway.  The worst performance impact of this should
be that some dirops get delayed a little bit.

(b) Create a new lfs_writer_tryenter to use at this point so that the
test for fs->lfs_dirops == 0 and the acquisition of lfs_writer happen
atomically under lfs_lock.

diffstat:

 sys/ufs/lfs/lfs_extern.h  |   3 ++-
 sys/ufs/lfs/lfs_segment.c |  22 +++++++++++++++-------
 sys/ufs/lfs/lfs_subr.c    |  21 ++++++++++++++++++---
 3 files changed, 35 insertions(+), 11 deletions(-)

diffs (116 lines):

diff -r aec35e60fb4c -r 807ec8abd9c2 sys/ufs/lfs/lfs_extern.h
--- a/sys/ufs/lfs/lfs_extern.h  Sun Feb 23 08:40:27 2020 +0000
+++ b/sys/ufs/lfs/lfs_extern.h  Sun Feb 23 08:40:37 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: lfs_extern.h,v 1.115 2020/02/18 20:23:17 chs Exp $     */
+/*     $NetBSD: lfs_extern.h,v 1.116 2020/02/23 08:40:37 riastradh Exp $       */
 
 /*-
  * Copyright (c) 1999, 2000, 2001, 2002, 2003 The NetBSD Foundation, Inc.
@@ -211,6 +211,7 @@
 void lfs_segunlock(struct lfs *);
 void lfs_segunlock_relock(struct lfs *);
 int lfs_writer_enter(struct lfs *, const char *);
+int lfs_writer_tryenter(struct lfs *);
 void lfs_writer_leave(struct lfs *);
 void lfs_wakeup_cleaner(struct lfs *);
 
diff -r aec35e60fb4c -r 807ec8abd9c2 sys/ufs/lfs/lfs_segment.c
--- a/sys/ufs/lfs/lfs_segment.c Sun Feb 23 08:40:27 2020 +0000
+++ b/sys/ufs/lfs/lfs_segment.c Sun Feb 23 08:40:37 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: lfs_segment.c,v 1.284 2020/02/23 08:40:08 riastradh Exp $      */
+/*     $NetBSD: lfs_segment.c,v 1.285 2020/02/23 08:40:37 riastradh Exp $      */
 
 /*-
  * Copyright (c) 1999, 2000, 2001, 2002, 2003 The NetBSD Foundation, Inc.
@@ -60,7 +60,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: lfs_segment.c,v 1.284 2020/02/23 08:40:08 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: lfs_segment.c,v 1.285 2020/02/23 08:40:37 riastradh Exp $");
 
 #ifdef DEBUG
 # define vndebug(vp, str) do {                                         \
@@ -621,6 +621,15 @@
         */
        do_ckp = LFS_SHOULD_CHECKPOINT(fs, flags);
 
+       /*
+        * If we know we're gonna need the writer lock, take it now to
+        * preserve the lock order lfs_writer -> lfs_seglock.
+        */
+       if (do_ckp) {
+               lfs_writer_enter(fs, "ckpwriter");
+               writer_set = 1;
+       }
+
        /* We can't do a partial write and checkpoint at the same time. */
        if (do_ckp)
                flags &= ~SEGM_SINGLE;
@@ -650,11 +659,10 @@
                                break;
                        }
 
-                       if (do_ckp || fs->lfs_dirops == 0) {
-                               if (!writer_set) {
-                                       lfs_writer_enter(fs, "lfs writer");
-                                       writer_set = 1;
-                               }
+                       if (do_ckp ||
+                           (writer_set = lfs_writer_tryenter(fs)) != 0) {
+                               KASSERT(writer_set);
+                               KASSERT(fs->lfs_writer);
                                error = lfs_writevnodes(fs, mp, sp, VN_DIROP);
                                if (um_error == 0)
                                        um_error = error;
diff -r aec35e60fb4c -r 807ec8abd9c2 sys/ufs/lfs/lfs_subr.c
--- a/sys/ufs/lfs/lfs_subr.c    Sun Feb 23 08:40:27 2020 +0000
+++ b/sys/ufs/lfs/lfs_subr.c    Sun Feb 23 08:40:37 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: lfs_subr.c,v 1.98 2020/02/23 08:38:58 riastradh Exp $  */
+/*     $NetBSD: lfs_subr.c,v 1.99 2020/02/23 08:40:37 riastradh Exp $  */
 
 /*-
  * Copyright (c) 1999, 2000, 2001, 2002, 2003 The NetBSD Foundation, Inc.
@@ -60,7 +60,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: lfs_subr.c,v 1.98 2020/02/23 08:38:58 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: lfs_subr.c,v 1.99 2020/02/23 08:40:37 riastradh Exp $");
 
 #include <sys/param.h>
 #include <sys/systm.h>
@@ -575,7 +575,7 @@
 {
        int error = 0;
 
-       ASSERT_MAYBE_SEGLOCK(fs);
+       ASSERT_NO_SEGLOCK(fs);
        mutex_enter(&lfs_lock);
 
        /* disallow dirops during flush */
@@ -596,6 +596,21 @@
        return error;
 }
 
+int
+lfs_writer_tryenter(struct lfs *fs)
+{
+       int writer_set;
+
+       ASSERT_MAYBE_SEGLOCK(fs);
+       mutex_enter(&lfs_lock);
+       writer_set = (fs->lfs_dirops == 0);
+       if (writer_set)
+               fs->lfs_writer++;
+       mutex_exit(&lfs_lock);
+
+       return writer_set;
+}
+
 void
 lfs_writer_leave(struct lfs *fs)
 {