[PATCH 1/2] aio-dio-eof-race: handle aio pwrite errors and short reads

[PATCH 1/2] aio-dio-eof-race: handle aio pwrite errors and short reads

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

generic/427 fails on Btrfs with a cryptic "pread: Success" message. This
is because an aio pwrite fails with ENOSPC, so the file isn't as long as
we expect it to be. Make sure we check the result of the aio writes and
also print a more explicit message for short reads (which are
technically valid but in practice shouldn't happen for this test case).
Now the test fails with a much more informative "pwrite: No space left
on device".

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 src/aio-dio-regress/aio-dio-eof-race.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/src/aio-dio-regress/aio-dio-eof-race.c b/src/aio-dio-regress/aio-dio-eof-race.c
index bb1890b5..04479035 100644
--- a/src/aio-dio-regress/aio-dio-eof-race.c
+++ b/src/aio-dio-regress/aio-dio-eof-race.c
@@ -154,6 +154,9 @@ int main(int argc, char *argv[])
 
 	/* Keep extending until size_MB */
 	while (eof < size_MB * 1024 * 1024) {
+		ssize_t sret;
+		int i;
+
 		memset(buf, IO_PATTERN, buf_size);
 		fstat(fd, &statbuf);
 		eof = statbuf.st_size;
@@ -186,15 +189,32 @@ int main(int argc, char *argv[])
 			return 1;
 		}
 
+		for (i = 0; i < err; i++) {
+			/*
+			 * res is unsigned for some reason, so this is the best
+			 * way to detect that it contains a negative errno.
+			 */
+			if (evs[i].res > buf_size / 4) {
+				fprintf(stderr, "pwrite: %s\n",
+					strerror(-evs[i].res));
+				return 1;
+			}
+		}
+
 		/*
 		 * And then read it back.
 		 *
 		 * Using pread to keep it simple, but AIO has the same effect.
 		 * eof is the prior eof; we just wrote buf_size more.
 		 */
-		if (pread(fd, buf, buf_size, eof) != buf_size) {
+		sret = pread(fd, buf, buf_size, eof);
+		if (sret == -1) {
 			perror("pread");
 			return 1;
+		} else if (sret != buf_size) {
+			fprintf(stderr, "short read %zd was less than %zu\n",
+				sret, buf_size);
+			return 1;
 		}
 
 		/*
-- 
2.17.0

[PATCH 2/2] generic/427: used mixed mode for Btrfs

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

This test creates a 256 MB filesystem and then writes a 200 MB file.
With separate data and metadata, Btrfs will run out of data space since
it needs to allocate some metadata space. Use mixed mode, which is the
recommendation for smaller filesystems.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 tests/generic/427 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/generic/427 b/tests/generic/427
index 9cde5f50..b2cb4526 100755
--- a/tests/generic/427
+++ b/tests/generic/427
@@ -55,6 +55,8 @@ _require_test_program "feature"
 _require_aiodio aio-dio-eof-race
 
 # limit the filesystem size, to save the time of filling filesystem
+# Btrfs needs to use mixed mode for such a small filesystem
+[ "$FSTYP" = "btrfs" ] && MKFS_OPTIONS="$MKFS_OPTIONS -M"
 _scratch_mkfs_sized $((256 * 1024 * 1024)) >>$seqres.full 2>&1
 _scratch_mount
 
-- 
2.17.0

Re: [PATCH 2/2] generic/427: used mixed mode for Btrfs

[Dave Chinner]

On Wed, Apr 11, 2018 at 04:47:30PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> This test creates a 256 MB filesystem and then writes a 200 MB file.
> With separate data and metadata, Btrfs will run out of data space since
> it needs to allocate some metadata space. Use mixed mode, which is the
> recommendation for smaller filesystems.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  tests/generic/427 | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tests/generic/427 b/tests/generic/427
> index 9cde5f50..b2cb4526 100755
> --- a/tests/generic/427
> +++ b/tests/generic/427
> @@ -55,6 +55,8 @@ _require_test_program "feature"
>  _require_aiodio aio-dio-eof-race
>  
>  # limit the filesystem size, to save the time of filling filesystem
> +# Btrfs needs to use mixed mode for such a small filesystem
> +[ "$FSTYP" = "btrfs" ] && MKFS_OPTIONS="$MKFS_OPTIONS -M"


_scratch_mkfs_sized() should already be adding this for small btrfs
filesystems. Yup, it does:

    btrfs)                                                                       
        local mixed_opt=                                                         
        (( fssize <= 100 * 1024 * 1024 )) && mixed_opt='--mixed'                 
        $MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV        
        ;;                                                                       

>  _scratch_mkfs_sized $((256 * 1024 * 1024)) >>$seqres.full 2>&1

But this uses a filesystem larger than the mixed mode threshold in
_scratch_mkfs_sized(). Please update the generic threshold rather
than special case this test.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] generic/427: used mixed mode for Btrfs

[Omar Sandoval]

On Thu, Apr 12, 2018 at 10:56:30AM +1000, Dave Chinner wrote:
> On Wed, Apr 11, 2018 at 04:47:30PM -0700, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > This test creates a 256 MB filesystem and then writes a 200 MB file.
> > With separate data and metadata, Btrfs will run out of data space since
> > it needs to allocate some metadata space. Use mixed mode, which is the
> > recommendation for smaller filesystems.
> > 
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > ---
> >  tests/generic/427 | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/tests/generic/427 b/tests/generic/427
> > index 9cde5f50..b2cb4526 100755
> > --- a/tests/generic/427
> > +++ b/tests/generic/427
> > @@ -55,6 +55,8 @@ _require_test_program "feature"
> >  _require_aiodio aio-dio-eof-race
> >  
> >  # limit the filesystem size, to save the time of filling filesystem
> > +# Btrfs needs to use mixed mode for such a small filesystem
> > +[ "$FSTYP" = "btrfs" ] && MKFS_OPTIONS="$MKFS_OPTIONS -M"
> 
> 
> _scratch_mkfs_sized() should already be adding this for small btrfs
> filesystems. Yup, it does:
> 
>     btrfs)                                                                       
>         local mixed_opt=                                                         
>         (( fssize <= 100 * 1024 * 1024 )) && mixed_opt='--mixed'                 
>         $MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV        
>         ;;                                                                       
> 
> >  _scratch_mkfs_sized $((256 * 1024 * 1024)) >>$seqres.full 2>&1
> 
> But this uses a filesystem larger than the mixed mode threshold in
> _scratch_mkfs_sized(). Please update the generic threshold rather
> than special case this test.

I missed this, thanks, I'll send a v2.