Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix overly broad locking in spa_vdev_config_exit() #11585

Merged
merged 1 commit into from
Feb 24, 2021
Merged

Fix overly broad locking in spa_vdev_config_exit() #11585

merged 1 commit into from
Feb 24, 2021

Conversation

behlendorf
Copy link
Contributor

@behlendorf behlendorf commented Feb 9, 2021 •
edited
Loading

Motivation and Context

Resolve a deadlock which can occur when the ZED or zpool
command attaches a new device.

Description

Calling vdev_free() only requires the we acquire the spa config
SCL_STATE_ALL locks, not the SCL_ALL locks. In particular, we need
need to avoid taking the SCL_CONFIG lock (included in SCL_ALL) as a
writer since this can lead to a deadlock. The txg_sync_thread() may
block in spa_txg_history_init_io() when taking the SCL_CONFIG lock
as a reader when it detects there's a pending writer.

# `spa_vdev_exit->spa_vdev_config_exit->spa_config_enter` unnecessarily
# taking `SCL_ALL` instead of `SCL_STATE_ALL` as a writer.  Unfortunately,
# I wasn't able to spot the thread holding the `SCL_CONFIG` lock as a reader
# which PID 18970 is waiting on.

PID: 18970  TASK: ffff9a8f3abba080  CPU: 14  COMMAND: "zed"
 #0 [ffff9a8f9c7a7b28] __schedule at ffffffffb216ab17
    /usr/src/debug/kernel-3.10.0-957.1.3.x4.3.20/linux-3.10.0-957.1.3957.1.3.x4.3.20.x86_64/kernel/sched/core.c: 2574
 #1 [ffff9a8f9c7a7bb8] schedule at ffffffffb216b019
    /usr/src/debug/kernel-3.10.0-957.1.3.x4.3.20/linux-3.10.0-957.1.3957.1.3.x4.3.20.x86_64/kernel/sched/core.c: 3642
 #2 [ffff9a8f9c7a7bc8] cv_wait_common at ffffffffc073f325 [spl]
 #3 [ffff9a8f9c7a7c30] __cv_wait at ffffffffc073f365 [spl]
 #4 [ffff9a8f9c7a7c40] spa_config_enter at ffffffffc0c38e92 [zfs]
    /usr/src/debug/zfs-2.0.0/module/zfs/spa_misc.c: 512
 #5 [ffff9a8f9c7a7c90] spa_vdev_config_exit at ffffffffc0c3b694 [zfs]
    /usr/src/debug/zfs-2.0.0/module/zfs/spa_misc.c: 1279
 #6 [ffff9a8f9c7a7cd0] spa_vdev_exit at ffffffffc0c3b811 [zfs]
    /usr/src/kernels/3.10.0-957.1.3957.1.3.x4.3.20.x86_64/include/linux/spinlock.h: 337
 #7 [ffff9a8f9c7a7d00] spa_vdev_attach at ffffffffc0c31f61 [zfs]
    /usr/src/debug/zfs-2.0.0/module/zfs/spa.c: 6864
 #8 [ffff9a8f9c7a7d80] zfs_ioc_vdev_attach at ffffffffc0c86df3 [zfs]
    /usr/src/debug/zfs-2.0.0/module/zfs/zfs_ioctl.c: 1946
 #9 [ffff9a8f9c7a7dc8] zfsdev_ioctl_common at ffffffffc0c90137 [zfs]
    /usr/src/debug/zfs-2.0.0/include/os/linux/spl/sys/kmem.h: 126
#10 [ffff9a8f9c7a7e58] zfsdev_ioctl at ffffffffc0cbbb06 [zfs]
    /usr/src/debug/zfs-2.0.0/module/os/linux/zfs/zfs_ioctl_os.c: 196
#11 [ffff9a8f9c7a7e80] do_vfs_ioctl at ffffffffb1c567e0
    /usr/src/debug/kernel-3.10.0-957.1.3.x4.3.20/linux-3.10.0-957.1.3957.1.3.x4.3.20.x86_64/fs/ioctl.c: 44
#12 [ffff9a8f9c7a7f00] sys_ioctl at ffffffffb1c56a81
    /usr/src/debug/kernel-3.10.0-957.1.3.x4.3.20/linux-3.10.0-957.1.3957.1.3.x4.3.20.x86_64/fs/ioctl.c: 646
#13 [ffff9a8f9c7a7f50] system_call_fastpath at ffffffffb2177ddb
    /usr/src/debug/kernel-3.10.0-957.1.3.x4.3.20/linux-3.10.0-957.1.3957.1.3.x4.3.20.x86_64/arch/x86/kernel/entry_64.S: 503
    RIP: 00007fcfb372d8d7  RSP: 00007fcfb1d6fdd0  RFLAGS: 00010246
    RAX: 0000000000000010  RBX: 00007fcfa400d750  RCX: 00007fcfb4455b28
    RDX: 00007fcfb1d70280  RSI: 0000000000005a0e  RDI: 000000000000000d
    RBP: 00007fcfb1d73c70   R8: 0000000000000080   R9: 0000000800000001
    R10: 6f6c735f76656476  R11: 0000000000000246  R12: 00007fcfb1d70280
    R13: 0000000000000000  R14: 0000000000000001  R15: 00007fcfb1d73830
    ORIG_RAX: 0000000000000010  CS: 0033  SS: 002b

# txg_sync thread blocked due to queued SCL_CONFIG writer (PID 18970)

PID: 124102  TASK: ffff9a8eab791040  CPU: 10  COMMAND: "txg_sync"
 #0 [ffff9a8eab79fc38] __schedule at ffffffffb216ab17
    /usr/src/debug/kernel-3.10.0-957.1.3.x4.3.20/linux-3.10.0-957.1.3957.1.3.x4.3.20.x86_64/kernel/sched/core.c: 2574
 #1 [ffff9a8eab79fcc8] schedule at ffffffffb216b019
    /usr/src/debug/kernel-3.10.0-957.1.3.x4.3.20/linux-3.10.0-957.1.3957.1.3.x4.3.20.x86_64/kernel/sched/core.c: 3642
 #2 [ffff9a8eab79fcd8] cv_wait_common at ffffffffc073f325 [spl]
 #3 [ffff9a8eab79fd40] __cv_wait at ffffffffc073f365 [spl]
 #4 [ffff9a8eab79fd50] spa_config_enter at ffffffffc0c38ef1 [zfs]
    /usr/src/debug/zfs-2.0.0/module/zfs/spa_misc.c: 504
 #5 [ffff9a8eab79fda0] spa_txg_history_init_io at ffffffffc0c3e4b2 [zfs]
    /usr/src/debug/zfs-2.0.0/module/zfs/spa_stats.c: 410
 #6 [ffff9a8eab79fde8] txg_sync_thread at ffffffffc0c42c46 [zfs]
    /usr/src/debug/zfs-2.0.0/module/zfs/txg.c: 570
 #7 [ffff9a8eab79fe98] thread_generic_wrapper at ffffffffc0746bb3 [spl]
 #8 [ffff9a8eab79fec8] kthread at ffffffffb1ac1f81
    /usr/src/debug/kernel-3.10.0-957.1.3.x4.3.20/linux-3.10.0-957.1.3957.1.3.x4.3.20.x86_64/kernel/kthread.c: 202
 #9 [ffff9a8eab79ff50] ret_from_fork_nospec_begin at ffffffffb2177c1d
    /usr/src/debug/kernel-3.10.0-957.1.3.x4.3.20/linux-3.10.0-957.1.3957.1.3.x4.3.20.x86_64/arch/x86/kernel/entry_64.S: 410

How Has This Been Tested?

Locally by running ztest for multiple hours. Additional, manual
testing has so far been able to reproduce the issue with the
patch applied. Previously similar testing would recreate the
issue.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@behlendorf
Fix overly broad locking in spa_vdev_config_exit()
565da9f 
Calling vdev_free() only requires the we acquire the spa config
SCL_STATE_ALL locks, not the SCL_ALL locks.  In particular, we need
need to avoid taking the SCL_CONFIG lock (included in SCL_ALL) as a
writer since this can lead to a deadlock.  The txg_sync_thread() may
block in spa_txg_history_init_io() when taking the SCL_CONFIG lock
as a reading when it detects there's a pending writer.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Feb 9, 2021
@behlendorf behlendorf requested review from ahrens and mmaybee February 10, 2021 18:44
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) Status: Code Review Needed Ready for review and testing and removed Status: Code Review Needed Ready for review and testing Status: Accepted Ready to integrate (reviewed, tested) labels Feb 11, 2021
@behlendorf behlendorf mentioned this pull request Feb 24, 2021
Merged
13 tasks
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Feb 24, 2021
@behlendorf behlendorf merged commit 75a089e into openzfs:master Feb 24, 2021
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
@behlendorf @jsai20
Fix overly broad locking in spa_vdev_config_exit()
a34d6dc 
Calling vdev_free() only requires the we acquire the spa config
SCL_STATE_ALL locks, not the SCL_ALL locks.  In particular, we need
need to avoid taking the SCL_CONFIG lock (included in SCL_ALL) as a
writer since this can lead to a deadlock.  The txg_sync_thread() may
block in spa_txg_history_init_io() when taking the SCL_CONFIG lock
as a reading when it detects there's a pending writer.

Reviewed-by: Igor Kozhukhov <igor@dilos.org>
Reviewed-by: Mark Maybee <mark.maybee@delphix.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#11585
@behlendorf behlendorf deleted the spa_vdev_config_exit-locking branch April 19, 2021 19:20
ghost pushed a commit to truenas/zfs that referenced this pull request May 6, 2021
@behlendorf
Fix overly broad locking in spa_vdev_config_exit()
9374c8a 
Calling vdev_free() only requires the we acquire the spa config
SCL_STATE_ALL locks, not the SCL_ALL locks.  In particular, we need
need to avoid taking the SCL_CONFIG lock (included in SCL_ALL) as a
writer since this can lead to a deadlock.  The txg_sync_thread() may
block in spa_txg_history_init_io() when taking the SCL_CONFIG lock
as a reading when it detects there's a pending writer.

Reviewed-by: Igor Kozhukhov <igor@dilos.org>
Reviewed-by: Mark Maybee <mark.maybee@delphix.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#11585
ghost pushed a commit to truenas/zfs that referenced this pull request May 10, 2021
@behlendorf
Fix overly broad locking in spa_vdev_config_exit()
20368ff 
Calling vdev_free() only requires the we acquire the spa config
SCL_STATE_ALL locks, not the SCL_ALL locks.  In particular, we need
need to avoid taking the SCL_CONFIG lock (included in SCL_ALL) as a
writer since this can lead to a deadlock.  The txg_sync_thread() may
block in spa_txg_history_init_io() when taking the SCL_CONFIG lock
as a reading when it detects there's a pending writer.

Reviewed-by: Igor Kozhukhov <igor@dilos.org>
Reviewed-by: Mark Maybee <mark.maybee@delphix.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#11585
behlendorf added a commit that referenced this pull request May 20, 2021
@behlendorf
Fix overly broad locking in spa_vdev_config_exit()
ad2638b 
Calling vdev_free() only requires the we acquire the spa config
SCL_STATE_ALL locks, not the SCL_ALL locks.  In particular, we need
need to avoid taking the SCL_CONFIG lock (included in SCL_ALL) as a
writer since this can lead to a deadlock.  The txg_sync_thread() may
block in spa_txg_history_init_io() when taking the SCL_CONFIG lock
as a reading when it detects there's a pending writer.

Reviewed-by: Igor Kozhukhov <igor@dilos.org>
Reviewed-by: Mark Maybee <mark.maybee@delphix.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes #11585
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
@behlendorf
Fix overly broad locking in spa_vdev_config_exit()
b27f093 
Calling vdev_free() only requires the we acquire the spa config
SCL_STATE_ALL locks, not the SCL_ALL locks.  In particular, we need
need to avoid taking the SCL_CONFIG lock (included in SCL_ALL) as a
writer since this can lead to a deadlock.  The txg_sync_thread() may
block in spa_txg_history_init_io() when taking the SCL_CONFIG lock
as a reading when it detects there's a pending writer.

Reviewed-by: Igor Kozhukhov <igor@dilos.org>
Reviewed-by: Mark Maybee <mark.maybee@delphix.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#11585
tonyhutter pushed a commit that referenced this pull request Jun 23, 2021
@behlendorf @tonyhutter
Fix overly broad locking in spa_vdev_config_exit()
395583e 
Calling vdev_free() only requires the we acquire the spa config
SCL_STATE_ALL locks, not the SCL_ALL locks.  In particular, we need
need to avoid taking the SCL_CONFIG lock (included in SCL_ALL) as a
writer since this can lead to a deadlock.  The txg_sync_thread() may
block in spa_txg_history_init_io() when taking the SCL_CONFIG lock
as a reading when it detects there's a pending writer.

Reviewed-by: Igor Kozhukhov <igor@dilos.org>
Reviewed-by: Mark Maybee <mark.maybee@delphix.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes #11585
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmaybee mmaybee mmaybee approved these changes

@ikozhukhov ikozhukhov ikozhukhov approved these changes

@ahrens ahrens Awaiting requested review from ahrens

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@behlendorf @ikozhukhov @mmaybee