Re: [PATCH] generic: test concurrent non-overlapping direct I/O on the same extents

2016-11-17 Thread Eryu Guan 
On Thu, Nov 17, 2016 at 10:14:12AM -0800, Omar Sandoval wrote:
> On Thu, Nov 17, 2016 at 01:26:11PM +0800, Eryu Guan wrote:
> > On Wed, Nov 16, 2016 at 04:29:34PM -0800, Omar Sandoval wrote:
> > > From: Omar Sandoval 
> > > 
> > > There have been a couple of logic bugs in `btrfs_get_extent()` which
> > > could lead to spurious -EEXIST errors from read or write. This test
> > > exercises those conditions by having two threads race to add an extent
> > > to the extent map.
> > > 
> > > This is fixed by Linux commit 8dff9c853410 ("Btrfs: deal with duplciates
> > > during extent_map insertion in btrfs_get_extent") and the patch "Btrfs:
> > > deal with existing encompassing extent map in btrfs_get_extent()"
> > > (http://marc.info/?l=linux-btrfs=147873402311143=2).
> > > 
> > > Although the bug is Btrfs-specific, nothing about the test is.
> > > 
> > > Signed-off-by: Omar Sandoval 
> > > ---
> > [snip]
> > > +# real QA test starts here
> > > +
> > > +_supported_fs generic
> > > +_supported_os Linux
> > > +_require_test
> > > +_require_xfs_io_command "falloc"
> > > +_require_test_program "dio-interleaved"
> > > +
> > > +extent_size="$(($(stat -f -c '%S' "$TEST_DIR") * 2))"
> > 
> > There's a helper to get fs block size: "get_block_size".
> > 
> > > +num_extents=1024
> > > +testfile="$TEST_DIR/$$-testfile"
> > > +
> > > +truncate -s 0 "$testfile"
> > 
> > I prefer using xfs_io to do the truncate, 
> > 
> > $XFS_IO_PROG -fc "truncate 0" "$testfile"
> > 
> > Because in rare cases truncate(1) may be unavailable, e.g. RHEL5,
> > usually it's not a big issue, but xfs_io works all the time, we have a
> > better way, so why not :)
> > 
> > > +for ((off = 0; off < num_extents * extent_size; off += extent_size)); do
> > > + xfs_io -c "falloc $off $extent_size" "$testfile"
> > 
> > Use $XFS_IO_PROG not bare xfs_io.
> > 
> > I can fix all the tiny issues at commit time.
> > 
> > Thanks,
> > Eryu
> 
> Thank you, Eryu, that's all okay with me. I also had a typo here:
> 
> diff --git a/src/dio-interleaved.c b/src/dio-interleaved.c
> index 831a191..6b04c99 100644
> --- a/src/dio-interleaved.c
> +++ b/src/dio-interleaved.c
> @@ -58,7 +58,7 @@ int main(int argc, char **argv)
>   int fd;
>  
>   if (argc != 4) {
> - fprintf(stderr, "usage: %s SECTORSIZE NUM_EXTENTS PATH\n",
> + fprintf(stderr, "usage: %s EXTENT_SIZE NUM_EXTENTS PATH\n",
>   argv[0]);
>   return EXIT_FAILURE;
>   }
> 
> 
> Feel free to fix this at commit time, too, or I can send a v2 if you
> prefer.

Thanks! I'll fix them all.

Eryu
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] generic: test concurrent non-overlapping direct I/O on the same extents

2016-11-17 Thread Omar Sandoval 
On Thu, Nov 17, 2016 at 01:26:11PM +0800, Eryu Guan wrote:
> On Wed, Nov 16, 2016 at 04:29:34PM -0800, Omar Sandoval wrote:
> > From: Omar Sandoval 
> > 
> > There have been a couple of logic bugs in `btrfs_get_extent()` which
> > could lead to spurious -EEXIST errors from read or write. This test
> > exercises those conditions by having two threads race to add an extent
> > to the extent map.
> > 
> > This is fixed by Linux commit 8dff9c853410 ("Btrfs: deal with duplciates
> > during extent_map insertion in btrfs_get_extent") and the patch "Btrfs:
> > deal with existing encompassing extent map in btrfs_get_extent()"
> > (http://marc.info/?l=linux-btrfs=147873402311143=2).
> > 
> > Although the bug is Btrfs-specific, nothing about the test is.
> > 
> > Signed-off-by: Omar Sandoval 
> > ---
> [snip]
> > +# real QA test starts here
> > +
> > +_supported_fs generic
> > +_supported_os Linux
> > +_require_test
> > +_require_xfs_io_command "falloc"
> > +_require_test_program "dio-interleaved"
> > +
> > +extent_size="$(($(stat -f -c '%S' "$TEST_DIR") * 2))"
> 
> There's a helper to get fs block size: "get_block_size".
> 
> > +num_extents=1024
> > +testfile="$TEST_DIR/$$-testfile"
> > +
> > +truncate -s 0 "$testfile"
> 
> I prefer using xfs_io to do the truncate, 
> 
> $XFS_IO_PROG -fc "truncate 0" "$testfile"
> 
> Because in rare cases truncate(1) may be unavailable, e.g. RHEL5,
> usually it's not a big issue, but xfs_io works all the time, we have a
> better way, so why not :)
> 
> > +for ((off = 0; off < num_extents * extent_size; off += extent_size)); do
> > +   xfs_io -c "falloc $off $extent_size" "$testfile"
> 
> Use $XFS_IO_PROG not bare xfs_io.
> 
> I can fix all the tiny issues at commit time.
> 
> Thanks,
> Eryu

Thank you, Eryu, that's all okay with me. I also had a typo here:

diff --git a/src/dio-interleaved.c b/src/dio-interleaved.c
index 831a191..6b04c99 100644
--- a/src/dio-interleaved.c
+++ b/src/dio-interleaved.c
@@ -58,7 +58,7 @@ int main(int argc, char **argv)
int fd;
 
if (argc != 4) {
-   fprintf(stderr, "usage: %s SECTORSIZE NUM_EXTENTS PATH\n",
+   fprintf(stderr, "usage: %s EXTENT_SIZE NUM_EXTENTS PATH\n",
argv[0]);
return EXIT_FAILURE;
}


Feel free to fix this at commit time, too, or I can send a v2 if you
prefer.

-- 
Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] generic: test concurrent non-overlapping direct I/O on the same extents

2016-11-16 Thread Eryu Guan 
On Wed, Nov 16, 2016 at 04:29:34PM -0800, Omar Sandoval wrote:
> From: Omar Sandoval 
> 
> There have been a couple of logic bugs in `btrfs_get_extent()` which
> could lead to spurious -EEXIST errors from read or write. This test
> exercises those conditions by having two threads race to add an extent
> to the extent map.
> 
> This is fixed by Linux commit 8dff9c853410 ("Btrfs: deal with duplciates
> during extent_map insertion in btrfs_get_extent") and the patch "Btrfs:
> deal with existing encompassing extent map in btrfs_get_extent()"
> (http://marc.info/?l=linux-btrfs=147873402311143=2).
> 
> Although the bug is Btrfs-specific, nothing about the test is.
> 
> Signed-off-by: Omar Sandoval 
> ---
[snip]
> +# real QA test starts here
> +
> +_supported_fs generic
> +_supported_os Linux
> +_require_test
> +_require_xfs_io_command "falloc"
> +_require_test_program "dio-interleaved"
> +
> +extent_size="$(($(stat -f -c '%S' "$TEST_DIR") * 2))"

There's a helper to get fs block size: "get_block_size".

> +num_extents=1024
> +testfile="$TEST_DIR/$$-testfile"
> +
> +truncate -s 0 "$testfile"

I prefer using xfs_io to do the truncate, 

$XFS_IO_PROG -fc "truncate 0" "$testfile"

Because in rare cases truncate(1) may be unavailable, e.g. RHEL5,
usually it's not a big issue, but xfs_io works all the time, we have a
better way, so why not :)

> +for ((off = 0; off < num_extents * extent_size; off += extent_size)); do
> + xfs_io -c "falloc $off $extent_size" "$testfile"

Use $XFS_IO_PROG not bare xfs_io.

I can fix all the tiny issues at commit time.

Thanks,
Eryu

> +done
> +
> +# To reproduce the Btrfs bug, the extent map must not be cached in memory.
> +sync
> +echo 3 > /proc/sys/vm/drop_caches
> +
> +"$here/src/dio-interleaved" "$extent_size" "$num_extents" "$testfile"
> +
> +echo "Silence is golden"
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/390.out b/tests/generic/390.out
> new file mode 100644
> index 000..3c7b405
> --- /dev/null
> +++ b/tests/generic/390.out
> @@ -0,0 +1,2 @@
> +QA output created by 390
> +Silence is golden
> diff --git a/tests/generic/group b/tests/generic/group
> index 08007d7..d137d01 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -392,3 +392,4 @@
>  387 auto clone
>  388 auto log metadata
>  389 auto quick acl
> +390 auto quick rw
> -- 
> 2.10.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] generic: test concurrent non-overlapping direct I/O on the same extents

2016-11-16 Thread Omar Sandoval 
From: Omar Sandoval 

There have been a couple of logic bugs in `btrfs_get_extent()` which
could lead to spurious -EEXIST errors from read or write. This test
exercises those conditions by having two threads race to add an extent
to the extent map.

This is fixed by Linux commit 8dff9c853410 ("Btrfs: deal with duplciates
during extent_map insertion in btrfs_get_extent") and the patch "Btrfs:
deal with existing encompassing extent map in btrfs_get_extent()"
(http://marc.info/?l=linux-btrfs=147873402311143=2).

Although the bug is Btrfs-specific, nothing about the test is.

Signed-off-by: Omar Sandoval 
---
 .gitignore|  1 +
 src/Makefile  |  2 +-
 src/dio-interleaved.c | 98 +++
 tests/generic/390 | 76 +++
 tests/generic/390.out |  2 ++
 tests/generic/group   |  1 +
 6 files changed, 179 insertions(+), 1 deletion(-)
 create mode 100644 src/dio-interleaved.c
 create mode 100755 tests/generic/390
 create mode 100644 tests/generic/390.out

diff --git a/.gitignore b/.gitignore
index 915d2d8..b8d13a0 100644
--- a/.gitignore
+++ b/.gitignore
@@ -44,6 +44,7 @@
 /src/bulkstat_unlink_test_modified
 /src/dbtest
 /src/devzero
+/src/dio-interleaved
 /src/dirperf
 /src/dirstress
 /src/dmiperf
diff --git a/src/Makefile b/src/Makefile
index dd51216..4056496 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -21,7 +21,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize 
preallo_rw_pattern_reader \
stale_handle pwrite_mmap_blocked t_dir_offset2 seek_sanity_test \
seek_copy_test t_readdir_1 t_readdir_2 fsync-tester nsexec cloner \
renameat2 t_getcwd e4compact test-nextquota punch-alternating \
-   attr-list-by-handle-cursor-test listxattr
+   attr-list-by-handle-cursor-test listxattr dio-interleaved
 
 SUBDIRS =
 
diff --git a/src/dio-interleaved.c b/src/dio-interleaved.c
new file mode 100644
index 000..831a191
--- /dev/null
+++ b/src/dio-interleaved.c
@@ -0,0 +1,98 @@
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static pthread_barrier_t barrier;
+
+static unsigned long extent_size;
+static unsigned long num_extents;
+
+struct dio_thread_data {
+   int fd;
+   int thread_id;
+};
+
+static void *dio_thread(void *arg)
+{
+   struct dio_thread_data *data = arg;
+   off_t off;
+   ssize_t ret;
+   void *buf;
+
+   if ((errno = posix_memalign(, extent_size / 2, extent_size / 2))) {
+   perror("malloc");
+   return NULL;
+   }
+   memset(buf, 0, extent_size / 2);
+
+   off = (num_extents - 1) * extent_size;
+   if (data->thread_id)
+   off += extent_size / 2;
+   while (off >= 0) {
+   pthread_barrier_wait();
+
+   ret = pread(data->fd, buf, extent_size / 2, off);
+   if (ret == -1)
+   perror("pread");
+
+   off -= extent_size;
+   }
+
+   free(buf);
+   return NULL;
+}
+
+int main(int argc, char **argv)
+{
+   struct dio_thread_data data[2];
+   pthread_t thread;
+   int fd;
+
+   if (argc != 4) {
+   fprintf(stderr, "usage: %s SECTORSIZE NUM_EXTENTS PATH\n",
+   argv[0]);
+   return EXIT_FAILURE;
+   }
+
+   extent_size = strtoul(argv[1], NULL, 0);
+   num_extents = strtoul(argv[2], NULL, 0);
+
+   errno = pthread_barrier_init(, NULL, 2);
+   if (errno) {
+   perror("pthread_barrier_init");
+   return EXIT_FAILURE;
+   }
+
+   fd = open(argv[3], O_RDONLY | O_DIRECT);
+   if (fd == -1) {
+   perror("open");
+   return EXIT_FAILURE;
+   }
+
+   data[0].fd = fd;
+   data[0].thread_id = 0;
+   errno = pthread_create(, NULL, dio_thread, [0]);
+   if (errno) {
+   perror("pthread_create");
+   close(fd);
+   return EXIT_FAILURE;
+   }
+
+   data[1].fd = fd;
+   data[1].thread_id = 1;
+   dio_thread([1]);
+
+   pthread_join(thread, NULL);
+
+   close(fd);
+   return EXIT_SUCCESS;
+}
diff --git a/tests/generic/390 b/tests/generic/390
new file mode 100755
index 000..0ef6537
--- /dev/null
+++ b/tests/generic/390
@@ -0,0 +1,76 @@
+#! /bin/bash
+# FS QA Test 390
+#
+# Test two threads doing non-overlapping direct I/O in the same extents.
+# Motivated by a bug in Btrfs' direct I/O get_block function which would lead
+# to spurious -EEXIST failures from direct I/O reads.
+#
+#---
+# Copyright (c) 2016 Facebook.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#