From 63f339623382383d0bb1b387e950723a503780a3 Mon Sep 17 00:00:00 2001 From: Brian Behlendorf Date: Wed, 28 Mar 2018 10:19:22 -0700 Subject: [PATCH] Fix mmap / libaio deadlock Calling uiomove() in mappedread() under the page lock can result in a deadlock if the user space page needs to be faulted in. Resolve the issue by dropping the page lock before the uiomove(). The inode range lock protects against concurrent updates via zfs_read() and zfs_write(). Reviewed-by: Albert Lee Reviewed-by: Chunwei Chen Signed-off-by: Brian Behlendorf Closes #7335 Closes #7339 --- config/user-libaio.m4 | 14 +++ config/user.m4 | 1 + config/zfs-build.m4 | 4 +- configure.ac | 1 + module/zfs/zfs_vnops.c | 2 +- rpm/generic/zfs.spec.in | 1 + tests/runfiles/linux.run | 2 +- tests/zfs-tests/cmd/Makefile.am | 1 + tests/zfs-tests/cmd/mmap_libaio/.gitignore | 1 + tests/zfs-tests/cmd/mmap_libaio/Makefile.am | 9 ++ tests/zfs-tests/cmd/mmap_libaio/mmap_libaio.c | 88 +++++++++++++++++++ tests/zfs-tests/include/commands.cfg | 1 + .../tests/functional/mmap/Makefile.am | 3 +- .../functional/mmap/mmap_libaio_001_pos.ksh | 61 +++++++++++++ 14 files changed, 184 insertions(+), 5 deletions(-) create mode 100644 config/user-libaio.m4 create mode 100644 tests/zfs-tests/cmd/mmap_libaio/.gitignore create mode 100644 tests/zfs-tests/cmd/mmap_libaio/Makefile.am create mode 100644 tests/zfs-tests/cmd/mmap_libaio/mmap_libaio.c create mode 100755 tests/zfs-tests/tests/functional/mmap/mmap_libaio_001_pos.ksh diff --git a/config/user-libaio.m4 b/config/user-libaio.m4 new file mode 100644 index 000000000..d7a7cb508 --- /dev/null +++ b/config/user-libaio.m4 @@ -0,0 +1,14 @@ +dnl # +dnl # Check for libaio - only used for libaiot test cases. +dnl # +AC_DEFUN([ZFS_AC_CONFIG_USER_LIBAIO], [ + LIBAIO= + + AC_CHECK_HEADER([libaio.h], [ + user_libaio=yes + AC_SUBST([LIBAIO], ["-laio"]) + AC_DEFINE([HAVE_LIBAIO], 1, [Define if you have libaio]) + ], [ + user_libaio=no + ]) +]) diff --git a/config/user.m4 b/config/user.m4 index 3d725195d..3f52c3e5c 100644 --- a/config/user.m4 +++ b/config/user.m4 @@ -13,6 +13,7 @@ AC_DEFUN([ZFS_AC_CONFIG_USER], [ ZFS_AC_CONFIG_USER_LIBBLKID ZFS_AC_CONFIG_USER_LIBUDEV ZFS_AC_CONFIG_USER_FRAME_LARGER_THAN + ZFS_AC_CONFIG_USER_LIBAIO ZFS_AC_CONFIG_USER_RUNSTATEDIR ZFS_AC_CONFIG_USER_MAKEDEV_IN_SYSMACROS ZFS_AC_CONFIG_USER_MAKEDEV_IN_MKDEV diff --git a/config/zfs-build.m4 b/config/zfs-build.m4 index e6c9c21e0..85571a8ad 100644 --- a/config/zfs-build.m4 +++ b/config/zfs-build.m4 @@ -118,11 +118,11 @@ AC_DEFUN([ZFS_AC_CONFIG], [ AM_CONDITIONAL([CONFIG_KERNEL], [test "$ZFS_CONFIG" = kernel -o "$ZFS_CONFIG" = all] && [test "x$enable_linux_builtin" != xyes ]) - AM_CONDITIONAL([WANT_DEVNAME2DEVID], - [test "x$user_libudev" = xyes ]) AM_CONDITIONAL([CONFIG_QAT], [test "$ZFS_CONFIG" = kernel -o "$ZFS_CONFIG" = all] && [test "x$qatsrc" != x ]) + AM_CONDITIONAL([WANT_DEVNAME2DEVID], [test "x$user_libudev" = xyes ]) + AM_CONDITIONAL([WANT_MMAP_LIBAIO], [test "x$user_libaio" = xyes ]) ]) dnl # diff --git a/configure.ac b/configure.ac index 77e5764fc..d9441a0f5 100644 --- a/configure.ac +++ b/configure.ac @@ -165,6 +165,7 @@ AC_CONFIG_FILES([ tests/zfs-tests/cmd/mkfiles/Makefile tests/zfs-tests/cmd/mktree/Makefile tests/zfs-tests/cmd/mmap_exec/Makefile + tests/zfs-tests/cmd/mmap_libaio/Makefile tests/zfs-tests/cmd/mmapwrite/Makefile tests/zfs-tests/cmd/randfree_file/Makefile tests/zfs-tests/cmd/readmmap/Makefile diff --git a/module/zfs/zfs_vnops.c b/module/zfs/zfs_vnops.c index 6f6ce79db..9646a1fa9 100644 --- a/module/zfs/zfs_vnops.c +++ b/module/zfs/zfs_vnops.c @@ -398,6 +398,7 @@ mappedread(struct inode *ip, int nbytes, uio_t *uio) pp = find_lock_page(mp, start >> PAGE_SHIFT); if (pp) { ASSERT(PageUptodate(pp)); + unlock_page(pp); pb = kmap(pp); error = uiomove(pb + off, bytes, UIO_READ, uio); @@ -407,7 +408,6 @@ mappedread(struct inode *ip, int nbytes, uio_t *uio) flush_dcache_page(pp); mark_page_accessed(pp); - unlock_page(pp); put_page(pp); } else { error = dmu_read_uio_dbuf(sa_get_db(zp->z_sa_hdl), diff --git a/rpm/generic/zfs.spec.in b/rpm/generic/zfs.spec.in index 769bdab10..0a5d1c543 100644 --- a/rpm/generic/zfs.spec.in +++ b/rpm/generic/zfs.spec.in @@ -183,6 +183,7 @@ Requires: acl Requires: sudo Requires: sysstat Requires: rng-tools +Requires: libaio AutoReqProv: no %description test diff --git a/tests/runfiles/linux.run b/tests/runfiles/linux.run index 516c1c3b8..a26f3b89b 100644 --- a/tests/runfiles/linux.run +++ b/tests/runfiles/linux.run @@ -485,7 +485,7 @@ tests = ['migration_001_pos', 'migration_002_pos', 'migration_003_pos', tags = ['functional', 'migration'] [tests/functional/mmap] -tests = ['mmap_write_001_pos', 'mmap_read_001_pos'] +tests = ['mmap_write_001_pos', 'mmap_read_001_pos', 'mmap_libaio_001_pos'] tags = ['functional', 'mmap'] [tests/functional/mmp] diff --git a/tests/zfs-tests/cmd/Makefile.am b/tests/zfs-tests/cmd/Makefile.am index 1cce6947b..7c3a96539 100644 --- a/tests/zfs-tests/cmd/Makefile.am +++ b/tests/zfs-tests/cmd/Makefile.am @@ -14,6 +14,7 @@ SUBDIRS = \ mkfiles \ mktree \ mmap_exec \ + mmap_libaio \ mmapwrite \ randfree_file \ readmmap \ diff --git a/tests/zfs-tests/cmd/mmap_libaio/.gitignore b/tests/zfs-tests/cmd/mmap_libaio/.gitignore new file mode 100644 index 000000000..792c8d340 --- /dev/null +++ b/tests/zfs-tests/cmd/mmap_libaio/.gitignore @@ -0,0 +1 @@ +/mmap_libaio diff --git a/tests/zfs-tests/cmd/mmap_libaio/Makefile.am b/tests/zfs-tests/cmd/mmap_libaio/Makefile.am new file mode 100644 index 000000000..67d0f0ece --- /dev/null +++ b/tests/zfs-tests/cmd/mmap_libaio/Makefile.am @@ -0,0 +1,9 @@ +include $(top_srcdir)/config/Rules.am + +pkgexecdir = $(datadir)/@PACKAGE@/zfs-tests/bin + +if WANT_MMAP_LIBAIO +pkgexec_PROGRAMS = mmap_libaio +mmap_libaio_SOURCES = mmap_libaio.c +mmap_libaio_LDADD = $(LIBAIO) +endif diff --git a/tests/zfs-tests/cmd/mmap_libaio/mmap_libaio.c b/tests/zfs-tests/cmd/mmap_libaio/mmap_libaio.c new file mode 100644 index 000000000..21119ebca --- /dev/null +++ b/tests/zfs-tests/cmd/mmap_libaio/mmap_libaio.c @@ -0,0 +1,88 @@ +/* + * CDDL HEADER START + * + * The contents of this file are subject to the terms of the + * Common Development and Distribution License (the "License"). + * You may not use this file except in compliance with the License. + * + * You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE + * or http://www.opensolaris.org/os/licensing. + * See the License for the specific language governing permissions + * and limitations under the License. + * + * When distributing Covered Code, include this CDDL HEADER in each + * file and include the License file at usr/src/OPENSOLARIS.LICENSE. + * If applicable, add the following below this CDDL HEADER, with the + * fields enclosed by brackets "[]" replaced with your own identifying + * information: Portions Copyright [yyyy] [name of copyright owner] + * + * CDDL HEADER END + */ + +/* + * Copyright 2018 Canonical. All rights reserved. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +io_context_t io_ctx; + +static void +do_sync_io(struct iocb *iocb) +{ + struct io_event event; + struct iocb *iocbs[] = { iocb }; + struct timespec ts = { 30, 0 }; + + if (io_submit(io_ctx, 1, iocbs) != 1) + err(1, "io_submit failed"); + + if (io_getevents(io_ctx, 0, 1, &event, &ts) != 1) + err(1, "io_getevents failed"); +} + +int +main(int argc, char **argv) +{ + char *buf; + int page_size = getpagesize(); + int buf_size = strtol(argv[2], NULL, 0); + int rwfd; + struct iocb iocb; + + if (io_queue_init(1024, &io_ctx)) + err(1, "io_queue_init failed"); + + rwfd = open(argv[1], O_RDWR | O_CREAT, S_IRUSR | S_IWUSR); + if (rwfd < 0) + err(1, "open failed"); + + if (ftruncate(rwfd, buf_size) < 0) + err(1, "ftruncate failed"); + + buf = mmap(0, page_size, PROT_READ | PROT_WRITE, MAP_SHARED, rwfd, 0); + if (buf == MAP_FAILED) + err(1, "mmap failed"); + + (void) io_prep_pwrite(&iocb, rwfd, buf, buf_size, 0); + do_sync_io(&iocb); + + (void) io_prep_pread(&iocb, rwfd, buf, buf_size, 0); + do_sync_io(&iocb); + + if (close(rwfd)) + err(1, "close failed"); + + if (io_queue_release(io_ctx) != 0) + err(1, "io_queue_release failed"); + + return (0); +} diff --git a/tests/zfs-tests/include/commands.cfg b/tests/zfs-tests/include/commands.cfg index 0d768a3cf..1399ad55f 100644 --- a/tests/zfs-tests/include/commands.cfg +++ b/tests/zfs-tests/include/commands.cfg @@ -158,6 +158,7 @@ export ZFSTEST_FILES='chg_usr_exec mkfiles mktree mmap_exec + mmap_libaio mmapwrite randfree_file readmmap diff --git a/tests/zfs-tests/tests/functional/mmap/Makefile.am b/tests/zfs-tests/tests/functional/mmap/Makefile.am index cb444ec37..4e1db6041 100644 --- a/tests/zfs-tests/tests/functional/mmap/Makefile.am +++ b/tests/zfs-tests/tests/functional/mmap/Makefile.am @@ -4,4 +4,5 @@ dist_pkgdata_SCRIPTS = \ cleanup.ksh \ mmap.cfg \ mmap_read_001_pos.ksh \ - mmap_write_001_pos.ksh + mmap_write_001_pos.ksh \ + mmap_libaio_001_pos.ksh diff --git a/tests/zfs-tests/tests/functional/mmap/mmap_libaio_001_pos.ksh b/tests/zfs-tests/tests/functional/mmap/mmap_libaio_001_pos.ksh new file mode 100755 index 000000000..36a7e76f9 --- /dev/null +++ b/tests/zfs-tests/tests/functional/mmap/mmap_libaio_001_pos.ksh @@ -0,0 +1,61 @@ +#!/bin/ksh -p +# +# CDDL HEADER START +# +# The contents of this file are subject to the terms of the +# Common Development and Distribution License (the "License"). +# You may not use this file except in compliance with the License. +# +# You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE +# or http://www.opensolaris.org/os/licensing. +# See the License for the specific language governing permissions +# and limitations under the License. +# +# When distributing Covered Code, include this CDDL HEADER in each +# file and include the License file at usr/src/OPENSOLARIS.LICENSE. +# If applicable, add the following below this CDDL HEADER, with the +# fields enclosed by brackets "[]" replaced with your own identifying +# information: Portions Copyright [yyyy] [name of copyright owner] +# +# CDDL HEADER END +# + +# +# Copyright 2018 Canonical. All rights reserved. +# + +. $STF_SUITE/include/libtest.shlib +. $STF_SUITE/tests/functional/mmap/mmap.cfg + +# +# DESCRIPTION: +# Verify libaio functions correctly with mmap()'d files. +# +# STRATEGY: +# 1. Call mmap_libaio binary +# 2. Verify the file exists and is the expected size +# 3. Verify the filesystem is intact and not hung in any way +# + +verify_runnable "global" + +log_assert "verify mmap'd pages work with libaio" + +# mmap_libaio is built when the libaio-devel package is installed. +if ! which mmap_libaio; then + log_unsupported "This test requires mmap_libaio." +fi + +log_must chmod 777 $TESTDIR + +for size in 512 4096 8192; do + log_mustnot stat $TESTDIR/test-libaio-file + log_must mmap_libaio $TESTDIR/test-libaio-file $size + log_must verify_eq $(stat --format=%s $TESTDIR/test-libaio-file) $size + log_must rm $TESTDIR/test-libaio-file +done + +typeset dir=$(get_device_dir $DISKS) +verify_filesys "$TESTPOOL" "$TESTPOOL/$TESTFS" "$dir" + +log_pass "mmap'd pages work with libaio"