Bug 217261 - sys/aio/aio_test:aio_md_test fails semi-frequently with Jenkins with "aio_write failed: Operation not supported"
Summary: sys/aio/aio_test:aio_md_test fails semi-frequently with Jenkins with "aio_wri...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: tests (show other bugs)
Version: 11.0-STABLE
Hardware: Any Any
: --- Affects Some People
Assignee: Enji Cooper
URL:
Keywords:
: 217100 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-02-21 01:53 UTC by Enji Cooper
Modified: 2018-03-19 19:10 UTC (History)
6 users (show)

See Also:
ngie: mfc-stable11+
ngie: mfc-stable10-
ngie: mfc-stable9-


Attachments
Test program (3.45 KB, text/x-csrc)
2017-08-25 22:57 UTC, John Baldwin
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Enji Cooper freebsd_committer freebsd_triage 2017-02-21 01:53:17 UTC
See https://ci.freebsd.org/job/FreeBSD-stable-11-amd64-test/618/testReport/junit/sys.aio/aio_test/aio_md_test/ for one of many examples (this seems to be one of a handful of semi-consistent issues with this Jenkins job).
Comment 1 Alan Somers freebsd_committer freebsd_triage 2017-02-21 03:11:39 UTC
I noticed that problem, but I can't reproduce it locally.  This is what jhb told me:

Hmmm, qphysio to a character device is always considered "safe".  The
failure must be due to another reason (e.g. there is a system-wide
quota on the number of AIO character device requests in flight).

You might want to see if vfs.aio.num_buf_aio == vfs.aio.max_buf_aio when
it fails.
Comment 2 Li-Wen Hsu freebsd_committer freebsd_triage 2017-02-21 03:23:52 UTC
(In reply to Alan Somers from comment #1)
Is it possible to reproduce this through the VM images from here https://artifact.ci.freebsd.org/snapshot/ ?

And, that's the setting of the system-wide quota of the number of AIO character device requests?
Comment 3 John Baldwin freebsd_committer freebsd_triage 2017-02-21 20:01:44 UTC
Yes, that is a system-wide setting.  If you are able to force all the AIO tests (including the separate kqueue AIO tests not in aio_test.c) to run sequentially rather than in parallel, that might be sufficient to avoid resource limit issues.
Comment 4 Enji Cooper freebsd_committer freebsd_triage 2017-05-09 20:58:39 UTC
I'm pretty sure that this is happening due to r303154:

-       if (!enable_aio_unsafe)
+       safe = false;
+       if (fp->f_type == DTYPE_VNODE) {
+               vp = fp->f_vnode;
+               if (vp->v_type == VREG || vp->v_type == VDIR) {
+                       mp = fp->f_vnode->v_mount;
+                       if (mp == NULL || (mp->mnt_flag & MNT_LOCAL) != 0)
+                               safe = true;
+               }
+       }
+       if (!(safe || enable_aio_unsafe))

I'm not entirely sure that that commit accounted for these cases noted in aio(4) (in particular, the second paragraph):

     Asynchronous I/O operations on some file descriptor types may block an
     AIO daemon indefinitely resulting in process and/or system hangs.
     Operations on these file descriptor types are considered "unsafe" and
     disabled by default.  They can be enabled by setting the
     vfs.aio.enable_unsafe sysctl node to a non-zero value.

     Asynchronous I/O operations on sockets, raw disk devices, and regular
     files on local filesystems do not block indefinitely and are always
     enabled.

VBLK and VSOCK doesn't seem to be handled appropriately. VCHR seems trickier to handle, depending on the input device (pty's for instance could block).

The second paragraph is ambiguous too -- are block devices ok, but not character devices, or vice versa, or are all forms ok?
Comment 5 Enji Cooper freebsd_committer freebsd_triage 2017-05-09 21:00:07 UTC
(In reply to Ngie Cooper from comment #4)

Also: why are directory types permitted in vfs_aio.c, but not noted in aio(4)?
Comment 6 John Baldwin freebsd_committer freebsd_triage 2017-05-09 21:21:15 UTC
(In reply to Ngie Cooper from comment #4)
Block devices don't exist in FreeBSD.  VSOCK would not show up here either as those would use a socket fileops which has a custom fo_aio_queue method.

Character devices that are disks (like /dev/mdX) are handled in aio_qphysio().  If you hit a resource limit on the number of "physio" requests queued (the check against 'kaio_ballowed_count' e.g.) then request will fall through and trigger the unsafe check.  Arguably we should be failing with EAGAIN from aio_qphysio() and then flipping the '#if 0' in aio_queue_file() so that error gets returned directly rather than trying to re-queue the request via the helper kprocs.

All other VCHR are considered unsafe.  We could at some point provide a way for character devices to claim AIO requests via something like fo_aio_queue.  Note that ptys already have their own 'struct fileops' and could easily provide a custom f_aio_queue() along with custom AIO handling to make them "safe".

I'm not sure why kib@ added the VDIR check as I'm not sure any of the current AIO operations (fsync, read, write) make any sense on directories.  However, this probably just means that they are allowed to fail with EISDIR rather than EOPNOTSUPP.
Comment 7 Konstantin Belousov freebsd_committer freebsd_triage 2017-05-09 21:48:27 UTC
(In reply to Ngie Cooper from comment #4)
I do not understand you claims at all.  How VSOCK vnode can appear under DTYPE_VNODE file (other than due to a bug in the filesystem code) ?  And even if it appear there, how is it 'unhandled', when the operation is declared to be unsafe by the snipped you cited, as intended ?

That said, reading directories is the allowed operation.  You can even mmap(2) them, try "vi /".
Comment 8 commit-hook freebsd_committer freebsd_triage 2017-05-11 08:07:22 UTC
A commit references this bug:

Author: ngie
Date: Thu May 11 08:06:47 UTC 2017
New revision: 318180
URL: https://svnweb.freebsd.org/changeset/base/318180

Log:
  Mark all md tests as requiring unsafe AIO in order to function

  These tests have been flapping (failing<->passing) on Jenkins for months.
  It passes reliably for me if unsafe AIO is permitted, but it doesn't
  pass on Jenkins reliably if unsafe AIO is disabled (the current default).

  Mark the tests as requiring unsafe AIO to mitigate the intermittent
  failures when unsafe AIO isn't permitted. If the kernel code is changed
  to reliably function with md(4) devices using unsafe AIO, this commit can
  be reverted.

  MFC after:	2 months
  PR:		217261
  Reported by:	Jenkins
  Sponsored by:	Dell EMC Isilon

Changes:
  head/tests/sys/aio/aio_test.c
Comment 9 Enji Cooper freebsd_committer freebsd_triage 2017-05-11 08:16:48 UTC
(In reply to Konstantin Belousov from comment #7)

VSOCK might have been the wrong qualifier. aio(4) merely states "sockets" -- it doesn't state the address family or protocol for the sockets.

jhb's comment seems like the salient point here. If there are insufficient resources, the call should return EAGAIN and the caller should try and requeue the request.
Comment 10 Jung-uk Kim freebsd_committer freebsd_triage 2017-06-16 19:48:51 UTC
*** Bug 217100 has been marked as a duplicate of this bug. ***
Comment 11 commit-hook freebsd_committer freebsd_triage 2017-07-17 21:17:23 UTC
A commit references this bug:

Author: ngie
Date: Mon Jul 17 21:17:04 UTC 2017
New revision: 321095
URL: https://svnweb.freebsd.org/changeset/base/321095

Log:
  MFC r318180:

  Mark all md tests as requiring unsafe AIO in order to function

  These tests have been flapping (failing<->passing) on Jenkins for months.
  It passes reliably for me if unsafe AIO is permitted, but it doesn't
  pass on Jenkins reliably if unsafe AIO is disabled (the current default).

  Mark the tests as requiring unsafe AIO to mitigate the intermittent
  failures when unsafe AIO isn't permitted. If the kernel code is changed
  to reliably function with md(4) devices using unsafe AIO, this commit can
  be reverted.

  PR:		217261

Changes:
_U  stable/11/
  stable/11/tests/sys/aio/aio_test.c
Comment 12 John Baldwin freebsd_committer freebsd_triage 2017-08-25 22:56:46 UTC
So I've wanted to test a fix for this, but I've been utterly unable to reproduce.  The limit I thought of earlier (max_buf_aio) only applies to disk devices that do not support unmapped IO via SI_UNMAPPED.  However, /dev/mdX does support this for the malloc disks used by the tests, so requests don't use a 'pbuf' and are thus never limited.  If the process were to hit the other applicable limits (vfs.aio.max_aio_queue or vfs.aio.max_queue_per_proc), then it would always fail with EAGAIN well before the "safe" check.  (I have verified that behavior via a dedicated test program.)  It seems likely that the request may be failing for some other reason and so adding the "unsafe"  requirement has really just had the effect of disabling the test and masking whatever error might be occurring.

I've attached the test program I'm using, compile with 'cc -std=c11 -lthr'.  You can increase the number of parallel things queued by increasing the thread count (defaults to 16).  With the default per-process limit of 256 requests queued, any thread count over 256 will report retries, however, the program will exit if it gets EOPNOTSUPP which I've yet to provoke on an existing kernel.

The candidate fixes are in an 'aio_physio_again' branch at https://github.com/freebsd/freebsd/compare/master...bsdjhb:aio_physio_again
Comment 13 John Baldwin freebsd_committer freebsd_triage 2017-08-25 22:57:24 UTC
Created attachment 185771 [details]
Test program
Comment 14 Alan Somers freebsd_committer freebsd_triage 2017-08-28 16:57:54 UTC
Using your test program, I can't reproduce this on my machine, either.
Comment 15 commit-hook freebsd_committer freebsd_triage 2018-01-10 00:19:09 UTC
A commit references this bug:

Author: jhb
Date: Wed Jan 10 00:18:48 UTC 2018
New revision: 327755
URL: https://svnweb.freebsd.org/changeset/base/327755

Log:
  Allow the fast-path for disk AIO requests to fail requests.

  - If aio_qphysio() returns a non-zero error code, fail the request rather
    than queueing it to the AIO kproc pool to be retried via the slow path.
    Currently this means that if vm_fault_quick_hold_pages() reports an
    error, EFAULT is returned from the fast-path rather than retrying the
    request in the slow path where it will still fail with EFAULT.
  - If aio_qphysio() wishes to use the fast path for a device that doesn't
    support unmapped I/O but there are already the maximum number of
    such requests in flight, fail with EAGAIN as we do for other AIO
    resource limits rather than queueing the request to the AIO kproc pool.
  - Move the opcode check for aio_qphysio() out of the caller and into
    aio_qphysio() to simplify some logic and remove two goto's while here.
    It also uses a whitelist (only supported for LIO_READ / LIO_WRITE)
    rather than a blacklist (skipped for LIO_SYNC).

  PR:		217261
  Submitted by:	jkim (an earlier version)
  MFC after:	2 weeks
  Sponsored by:	Chelsio Communications

Changes:
  head/sys/kern/vfs_aio.c
Comment 16 John Baldwin freebsd_committer freebsd_triage 2018-01-10 00:20:19 UTC
I've committed all of the changes in the aio_physio_again branch I referenced earlier, and I'd like to re-enable this test in HEAD by removing the incorrect unsafe aio requirement.  Any objections?
Comment 17 commit-hook freebsd_committer freebsd_triage 2018-01-30 00:42:24 UTC
A commit references this bug:

Author: jhb
Date: Tue Jan 30 00:41:54 UTC 2018
New revision: 328581
URL: https://svnweb.freebsd.org/changeset/base/328581

Log:
  MFC 327755: Allow the fast-path for disk AIO requests to fail requests.

  - If aio_qphysio() returns a non-zero error code, fail the request rather
    than queueing it to the AIO kproc pool to be retried via the slow path.
    Currently this means that if vm_fault_quick_hold_pages() reports an
    error, EFAULT is returned from the fast-path rather than retrying the
    request in the slow path where it will still fail with EFAULT.
  - If aio_qphysio() wishes to use the fast path for a device that doesn't
    support unmapped I/O but there are already the maximum number of
    such requests in flight, fail with EAGAIN as we do for other AIO
    resource limits rather than queueing the request to the AIO kproc pool.
  - Move the opcode check for aio_qphysio() out of the caller and into
    aio_qphysio() to simplify some logic and remove two goto's while here.
    It also uses a whitelist (only supported for LIO_READ / LIO_WRITE)
    rather than a blacklist (skipped for LIO_SYNC).

  PR:		217261
  Sponsored by:	Chelsio Communications

Changes:
_U  stable/11/
  stable/11/sys/kern/vfs_aio.c
Comment 18 commit-hook freebsd_committer freebsd_triage 2018-03-19 19:10:12 UTC
A commit references this bug:

Author: jhb
Date: Mon Mar 19 19:09:15 UTC 2018
New revision: 331221
URL: https://svnweb.freebsd.org/changeset/base/331221

Log:
  Revert r318180 and re-enable AIO tests on md(4) by default.

  The 'physio' fast-path used by AIO requests on md(4) devices, is not
  gated on the unsafe_aio knob.  Prior to r327755, some AIO requests could
  fail the fast-path and fall back to the slow-path (requests for devices
  not supporting unmapped I/O and requests which failed with EFAULT during
  the fast-path).  However, those cases now return a suitable error rather
  than using the slow-path.

  PR:		217261
  Reviewed by:	asomers
  Sponsored by:	Chelsio Communications
  Differential Revision:	https://reviews.freebsd.org/D14742

Changes:
  head/tests/sys/aio/aio_test.c