Whamcloud - gitweb
LU-9468 llite: fix for stat under kthread and X86_X32 92/26992/14
authorFrank Zago <fzago@cray.com>
Fri, 5 May 2017 21:10:47 +0000 (17:10 -0400)
committerOleg Drokin <oleg.drokin@intel.com>
Tue, 13 Jun 2017 16:55:16 +0000 (16:55 +0000)
Under the following conditions, ll_getattr will flatten the inode
number when it shouldn't:

  - the X86_X32 architecture is defined CONFIG_X86_X32, and not even
    used,
  - ll_getattr is called from a kernel thread (though vfs_getattr for
    instance.)

This has the result that inode numbers are different whether the same
file is stat'ed from a kernel thread, or from a syscall. For instance,
4198401 vs. 144115205272502273.

ll_getattr calls ll_need_32bit_api to determine whether the task is 32
bits. When the combination is kthread+X86_X32, that function returns
that the task is 32 bits, which is incorrect, as the kernel is 64
bits.

The solution is to check whether the call is from a kernel thread
(which is 64 bits) and act consequently.

Added test_410 to test the condition. A failed run will have a message like this:

  lustre_kinode_54354: CONFIG_X86_X32 is set
  lustre_kinode_54354: inode is 144115205272502273
  lustre_kinode_54354: inode is 4198401
  lustre_kinode_54354: inode numbers are different: 144115205272502273 4198401

while a successfull one will be:

  lustre_kinode_10519: CONFIG_X86_X32 is set
  lustre_kinode_10519: inode is 144115205272502275
  lustre_kinode_10519: inode is 144115205272502275
  lustre_kinode_10519: inode numbers are identical: 144115205272502275

Signed-off-by: Frank Zago <fzago@cray.com>
Change-Id: Ib8f07de47eaa29046a61c488368d466f9096a994
Reviewed-on: https://review.whamcloud.com/26992
Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>
Reviewed-by: Dmitry Eremin <dmitry.eremin@intel.com>
Tested-by: Jenkins
Tested-by: Maloo <hpdd-maloo@intel.com>
lustre.spec.in
lustre/Makefile.in
lustre/autoMakefile.am
lustre/autoconf/lustre-core.m4
lustre/llite/llite_internal.h
lustre/tests/kernel/Makefile.in [new file with mode: 0644]
lustre/tests/kernel/autoMakefile.am [new file with mode: 0644]
lustre/tests/kernel/kinode.c [new file with mode: 0644]
lustre/tests/sanity.sh

index 6ed1996..59a3352 100644 (file)
@@ -331,6 +331,8 @@ mv $basemodpath/fs/osd_zfs.ko $basemodpath-osd-zfs/fs/osd_zfs.ko
 %if %{with lustre_tests}
 mkdir -p $basemodpath-tests/fs
 mv $basemodpath/fs/llog_test.ko $basemodpath-tests/fs/llog_test.ko
+mkdir -p $RPM_BUILD_ROOT%{_libdir}/lustre/tests/kernel/
+mv $basemodpath/fs/kinode.ko $RPM_BUILD_ROOT%{_libdir}/lustre/tests/kernel/
 %endif
 
 :> lustre.files
index 2794a62..319a405 100644 (file)
@@ -3,6 +3,7 @@ subdir-m += obdclass
 subdir-m += ptlrpc
 subdir-m += obdecho
 subdir-m += mgc
+subdir-m += tests/kernel
 
 @SERVER_TRUE@subdir-m += ost mgs mdt mdd ofd quota osp lod lfsck
 @CLIENT_TRUE@subdir-m += lov osc mdc lmv llite fld
index 3b8a55e..e94ce3d 100644 (file)
@@ -36,7 +36,7 @@ AUTOMAKE_OPTIONS = foreign
 
 # also update lustre/autoconf/lustre-core.m4 AC_CONFIG_FILES
 ALWAYS_SUBDIRS = include obdclass ldlm ptlrpc obdecho \
-       mgc fid fld doc utils tests scripts autoconf contrib conf
+       mgc fid fld doc utils tests tests/kernel scripts autoconf contrib conf
 
 SERVER_SUBDIRS = ost mgs mdt mdd ofd osd-zfs osd-ldiskfs \
        quota osp lod target lfsck
index 77d6a98..c6f2377 100644 (file)
@@ -3068,6 +3068,8 @@ lustre/scripts/Makefile
 lustre/scripts/systemd/Makefile
 lustre/tests/Makefile
 lustre/tests/mpi/Makefile
+lustre/tests/kernel/Makefile
+lustre/tests/kernel/autoMakefile
 lustre/utils/Makefile
 lustre/utils/gss/Makefile
 lustre/osp/Makefile
index eb9298d..03ab7e6 100644 (file)
@@ -661,7 +661,19 @@ static inline int ll_need_32bit_api(struct ll_sb_info *sbi)
 #if BITS_PER_LONG == 32
        return 1;
 #elif defined(CONFIG_COMPAT)
-       return unlikely(in_compat_syscall() || (sbi->ll_flags & LL_SBI_32BIT_API));
+       if (unlikely(sbi->ll_flags & LL_SBI_32BIT_API))
+               return true;
+
+# ifdef CONFIG_X86_X32
+       /* in_compat_syscall() returns true when called from a kthread
+        * and CONFIG_X86_X32 is enabled, which is wrong. So check
+        * whether the caller comes from a syscall (ie. not a kthread)
+        * before calling in_compat_syscall(). */
+       if (current->flags & PF_KTHREAD)
+               return false;
+# endif
+
+       return unlikely(in_compat_syscall());
 #else
        return unlikely(sbi->ll_flags & LL_SBI_32BIT_API);
 #endif
diff --git a/lustre/tests/kernel/Makefile.in b/lustre/tests/kernel/Makefile.in
new file mode 100644 (file)
index 0000000..9cba127
--- /dev/null
@@ -0,0 +1,5 @@
+MODULES := kinode
+
+EXTRA_DIST = kinode.c
+
+@INCLUDE_RULES@
diff --git a/lustre/tests/kernel/autoMakefile.am b/lustre/tests/kernel/autoMakefile.am
new file mode 100644 (file)
index 0000000..9309582
--- /dev/null
@@ -0,0 +1,34 @@
+#
+# GPL HEADER START
+#
+# DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 2 only,
+# as published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# General Public License version 2 for more details (a copy is included
+# in the LICENSE file that accompanied this code).
+#
+# You should have received a copy of the GNU General Public License
+# version 2 along with this program; If not, see
+# http://www.gnu.org/licenses/gpl-2.0.html
+#
+# GPL HEADER END
+#
+
+#
+# This file is part of Lustre, http://www.lustre.org/
+# Lustre is a trademark of Sun Microsystems, Inc.
+#
+
+if MODULES
+if TESTS
+modulefs_DATA = kinode$(KMODEXT)
+endif
+endif
+
+MOSTLYCLEANFILES := @MOSTLYCLEANFILES@
diff --git a/lustre/tests/kernel/kinode.c b/lustre/tests/kernel/kinode.c
new file mode 100644 (file)
index 0000000..0283c8e
--- /dev/null
@@ -0,0 +1,169 @@
+/*
+ * GPL HEADER START
+ *
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 only,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License version 2 for more details (a copy is included
+ * in the LICENSE file that accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License
+ * version 2 along with this program; If not, see
+ * http://www.gnu.org/licenses/gpl-2.0.html
+ *
+ * GPL HEADER END
+ */
+
+/*
+ * Copyright 2017 Cray Inc. All rights reserved.
+ * Author: Frank Zago.
+ */
+
+/* Check that the inode number is the same whether the call to
+ * vfs_getattr is coming from a system call or from a kthread. When
+ * CONFIG_X86_X32 was set, the result used to be different for
+ * Lustre. In addition, a user can also check that the same inode
+ * number is also seen from the kernel and userspace.  */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/kthread.h>
+#include <linux/fs.h>
+#include <linux/version.h>
+
+/* Random ID passed by userspace, and printed in messages, used to
+ * separate different runs of that module. */
+static int run_id;
+module_param(run_id, int, 0644);
+MODULE_PARM_DESC(run_id, "run ID");
+
+/* Name of the file to stat. */
+static char fname[4096];
+module_param_string(fname, fname, sizeof(fname), 0644);
+MODULE_PARM_DESC(fname, "name of file to stat");
+
+struct completion thr_start;
+
+#define PREFIX "lustre_kinode_%u:"
+
+static int stat_file(struct kstat *stbuf)
+{
+       struct file *fd;
+       int rc;
+
+       fd = filp_open(fname, O_RDONLY, 0);
+       if (IS_ERR(fd)) {
+               pr_err(PREFIX " can't open file %s\n", run_id, fname);
+               return -EIO;
+       }
+
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 9, 0)
+       rc = vfs_getattr(&fd->f_path, stbuf);
+#else
+       rc = vfs_getattr(fd->f_path.mnt, fd->f_path.dentry, stbuf);
+#endif
+       if (rc != 0) {
+               pr_err(PREFIX " vfs_getattr failed: %d\n", run_id, rc);
+               goto out;
+       }
+
+       pr_err(PREFIX " inode is %llu\n", run_id, stbuf->ino);
+       rc = 0;
+
+out:
+       filp_close(fd, NULL);
+
+       return rc;
+}
+
+static int stat_thread(void *data)
+{
+       struct kstat *stbuf = data;
+       int rc;
+
+       /* Signal caller that thread has started. */
+       complete(&thr_start);
+
+       rc = stat_file(stbuf);
+
+       /* Wait for call to kthread_stop. */
+       set_current_state(TASK_INTERRUPTIBLE);
+       while (!kthread_should_stop()) {
+               schedule();
+               set_current_state(TASK_INTERRUPTIBLE);
+       }
+       set_current_state(TASK_RUNNING);
+
+       return rc;
+}
+
+static int __init kinode_init(void)
+{
+       struct task_struct *thr;
+       struct kstat stbuf1;
+       struct kstat stbuf2;
+       int rc;
+
+#ifdef CONFIG_X86_X32
+       pr_err(PREFIX " CONFIG_X86_X32 is set\n", run_id);
+#else
+       pr_err(PREFIX " CONFIG_X86_X32 is not set\n", run_id);
+#endif
+
+       if (strlen(fname) < 1) {
+               pr_err(PREFIX " invalid file name '%s'\n", run_id, fname);
+               goto out;
+       }
+
+       rc = stat_file(&stbuf1);
+       if (rc) {
+               pr_err(PREFIX " direct stat failed: %d\n", run_id, rc);
+               goto out;
+       }
+
+       /* Run the same from a kthread. */
+       init_completion(&thr_start);
+       thr = kthread_run(stat_thread, &stbuf2, "kinode_%u", run_id);
+       if (IS_ERR(thr)) {
+               pr_err(PREFIX " Cannot create kthread\n", run_id);
+               goto out;
+       }
+
+       /* Wait for the thread to start, then wait for it to
+        * terminate. */
+       wait_for_completion(&thr_start);
+       rc = kthread_stop(thr);
+       if (rc) {
+               pr_err(PREFIX " indirect stat failed: %d\n", run_id, rc);
+               goto out;
+       }
+
+       if (stbuf1.ino != stbuf2.ino)
+               pr_err(PREFIX " inode numbers are different: %llu %llu\n",
+                      run_id, stbuf1.ino, stbuf2.ino);
+       else
+               pr_err(PREFIX " inode numbers are identical: %llu\n",
+                      run_id, stbuf1.ino);
+
+out:
+       /* Don't load. */
+       return -EINVAL;
+}
+
+static void __exit kinode_exit(void)
+{
+}
+
+MODULE_AUTHOR("OpenSFS, Inc. <http://www.lustre.org/>");
+MODULE_DESCRIPTION("Lustre inode stat test module");
+MODULE_VERSION(LUSTRE_VERSION_STRING);
+MODULE_LICENSE("GPL");
+
+module_init(kinode_init);
+module_exit(kinode_exit);
index c120a2e..49114e9 100755 (executable)
@@ -16419,6 +16419,27 @@ test_409()
 }
 run_test 409 "Large amount of cross-MDTs hard links on the same file"
 
+test_410()
+{
+       # Create a file, and stat it from the kernel
+       local testfile=$DIR/$tfile
+       touch $testfile
+
+       local run_id=$RANDOM
+       local my_ino=$(stat --format "%i" $testfile)
+
+       # Try to insert the module. This will always fail as the
+       # module is designed to not be inserted.
+       insmod $LUSTRE/tests/kernel/kinode.ko run_id=$run_id fname=$testfile \
+           &> /dev/null
+
+       # Anything but success is a test failure
+       dmesg | grep -q \
+           "lustre_kinode_$run_id: inode numbers are identical: $my_ino" ||
+           error "no inode match"
+}
+run_test 410 "Test inode number returned from kernel thread"
+
 prep_801() {
        [[ $(lustre_version_code mds1) -lt $(version_code 2.9.55) ]] ||
        [[ $(lustre_version_code ost1) -lt $(version_code 2.9.55) ]] &&