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

Correct lock ASSERTs in vdev_label_read/write #5983

Merged
merged 1 commit into from
Apr 21, 2017
Merged

Correct lock ASSERTs in vdev_label_read/write #5983

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions module/zfs/vdev_label.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,9 @@ static void
vdev_label_read(zio_t *zio, vdev_t *vd, int l, abd_t *buf, uint64_t offset,
uint64_t size, zio_done_func_t *done, void *private, int flags)
{
ASSERT(spa_config_held(zio->io_spa, SCL_STATE_ALL, RW_WRITER) ==
SCL_STATE_ALL);
ASSERT(
spa_config_held(zio->io_spa, SCL_STATE, RW_READER) == SCL_STATE ||
spa_config_held(zio->io_spa, SCL_STATE, RW_WRITER) == SCL_STATE);
ASSERT(flags & ZIO_FLAG_CONFIG_WRITER);

zio_nowait(zio_read_phys(zio, vd,
Expand All @@ -196,10 +197,9 @@ static void
vdev_label_write(zio_t *zio, vdev_t *vd, int l, abd_t *buf, uint64_t offset,
uint64_t size, zio_done_func_t *done, void *private, int flags)
{
ASSERT(spa_config_held(zio->io_spa, SCL_ALL, RW_WRITER) == SCL_ALL ||
(spa_config_held(zio->io_spa, SCL_CONFIG | SCL_STATE, RW_READER) ==
(SCL_CONFIG | SCL_STATE) &&
dsl_pool_sync_context(spa_get_dsl(zio->io_spa))));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the dsl_pool_sync_context intentionally removed?

Copy link
Contributor Author

@ofaaland ofaaland Apr 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

dsl_pool_sync_context() == B_TRUE in two cases; one is that the caller is the sync thread or a thread performing a sync task. Based on all vdev state and vdev tree changes occurring under RW_WRITER, doing I/O is safe under only RW_READER regardless of which thread is doing it.

The other case, is that spa.spa_is_initializing == B_TRUE. That field is only true during calls to dsl_pool_create() and dsl_pool_open(), within spa_create() and spa_load_impl() respectively, both of which are setting up facilities that aren't necessary for reading from or writing to the labels on leaf vdevs.

I believe this was simply copied from the code protecting the dirty state or config lists, where the list is altered by the sync thread with only the read lock held. In the context of protecting those lists, the restriction makes sense - you can guarantee there is only one writer, since there is only one sync thread at the point where those lists are modified.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tuxoko ,
Do you approve of the patch now? Or do you have concerns I haven't addressed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't disapprove just have some questions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thank you

ASSERT(
spa_config_held(zio->io_spa, SCL_STATE, RW_READER) == SCL_STATE ||
spa_config_held(zio->io_spa, SCL_STATE, RW_WRITER) == SCL_STATE);
ASSERT(flags & ZIO_FLAG_CONFIG_WRITER);

zio_nowait(zio_write_phys(zio, vd,
Expand Down