Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 21 Apr 2017 00:17:54 +0000 (UTC)
From:      Josh Paetzel <jpaetzel@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r317237 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
Message-ID:  <201704210017.v3L0HsdH077760@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: jpaetzel
Date: Fri Apr 21 00:17:54 2017
New Revision: 317237
URL: https://svnweb.freebsd.org/changeset/base/317237

Log:
  MFV 316870
  
  7448 ZFS doesn't notice when disk vdevs have no write cache
  
  illumos/illumos-gate@295438ba3230419314faaa889a2616f561658bd5
  https://github.com/illumos/illumos-gate/commit/295438ba3230419314faaa889a2616f561658bd5
  
  https://www.illumos.org/issues/7448
         I built a SmartOS image with all the NVMe commits including 7372
         (support NVMe volatile write cache) and repeated my dd testing:
         > #!/bin/bash
         > for i in `seq 1 1000`; do
         > dd if=/dev/zero of=file00 bs=1M count=102400 oflag=sync &
         > dd if=/dev/zero of=file01 bs=1M count=102400 oflag=sync &
         > wait
         > rm file00 file01
         > done
         >
         Previously each dd command took ~145 seconds to finish, now it takes
         ~400 seconds.
         Eventually I figured out it is 7372 that causes unnecessary
         nvme_bd_sync() executions which wasted CPU cycles.
    If a NVMe device doesn't support a write cache, the nvme_bd_sync function will
    return ENOTSUP to indicate this to upper layers.
    It seems this returned value is ignored by ZFS, and as such this bug is not
    really specific to NVMe. In vdev_disk_io_start() ZFS sends the flush to the
    disk driver (blkdev) with a callback to vdev_disk_ioctl_done(). As nvme filled
    in the bd_sync_cache function pointer, blkdev will not return ENOTSUP, as the
    nvme driver in general does support cache flush. Instead it will issue an
    asynchronous flush to nvme and immediately return 0, and hence ZFS will not set
    vdev_nowritecache here. The nvme driver will at some point process the cache
    flush command, and if there is no write cache on the device it will return
    ENOTSUP, which will be delivered to the vdev_disk_ioctl_done() callback. This
    function will not check the error code and not set nowritecache.
    The right place to check the error code from the cache flush is in
    zio_vdev_io_assess(). This would catch both cases, synchronous and asynchronous
    cache flushes. This would also be independent of the implementation detail that
    some drivers can return ENOTSUP immediately.
  
  Reviewed by: Dan Fields <dan.fields@nexenta.com>
  Reviewed by: Alek Pinchuk <alek.pinchuk@nexenta.com>
  Reviewed by: George Wilson <george.wilson@delphix.com>
  Approved by: Dan McDonald <danmcd@omniti.com>
  Author: Hans Rosenfeld <hans.rosenfeld@nexenta.com>
  Obtained from:	Illumos

Modified:
  head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_disk.c
  head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio.c
Directory Properties:
  head/sys/cddl/contrib/opensolaris/   (props changed)

Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_disk.c
==============================================================================
--- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_disk.c	Fri Apr 21 00:17:47 2017	(r317236)
+++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_disk.c	Fri Apr 21 00:17:54 2017	(r317237)
@@ -21,7 +21,7 @@
 /*
  * Copyright (c) 2005, 2010, Oracle and/or its affiliates. All rights reserved.
  * Copyright (c) 2012, 2015 by Delphix. All rights reserved.
- * Copyright 2013 Nexenta Systems, Inc.  All rights reserved.
+ * Copyright 2016 Nexenta Systems, Inc.  All rights reserved.
  * Copyright (c) 2013 Joyent, Inc.  All rights reserved.
  */
 
@@ -743,16 +743,6 @@ vdev_disk_io_start(zio_t *zio)
 				return;
 			}
 
-			if (error == ENOTSUP || error == ENOTTY) {
-				/*
-				 * If we get ENOTSUP or ENOTTY, we know that
-				 * no future attempts will ever succeed.
-				 * In this case we set a persistent bit so
-				 * that we don't bother with the ioctl in the
-				 * future.
-				 */
-				vd->vdev_nowritecache = B_TRUE;
-			}
 			zio->io_error = error;
 
 			break;

Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio.c
==============================================================================
--- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio.c	Fri Apr 21 00:17:47 2017	(r317236)
+++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zio.c	Fri Apr 21 00:17:54 2017	(r317237)
@@ -3302,6 +3302,16 @@ zio_vdev_io_assess(zio_t *zio)
 		vd->vdev_cant_write = B_TRUE;
 	}
 
+	/*
+	 * If a cache flush returns ENOTSUP or ENOTTY, we know that no future
+	 * attempts will ever succeed. In this case we set a persistent bit so
+	 * that we don't bother with it in the future.
+	 */
+	if ((zio->io_error == ENOTSUP || zio->io_error == ENOTTY) &&
+	    zio->io_type == ZIO_TYPE_IOCTL &&
+	    zio->io_cmd == DKIOCFLUSHWRITECACHE && vd != NULL)
+		vd->vdev_nowritecache = B_TRUE;
+
 	if (zio->io_error)
 		zio->io_pipeline = ZIO_INTERLOCK_PIPELINE;
 



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201704210017.v3L0HsdH077760>