On 15/10/15 12:46, Andreas Kinzler wrote: > On 14.10.2015 18:48, Pádraig Brady wrote: >>> some disks can be very slow with direct I/O and shred. I developed a >>> small patch that adds a new >>> option -i, --nodirectio to disable direct I/O. >> Interesting. direct I/O was added in 2004 to improve performance >> by avoid page cache thrashing, as suggested in: >> https://bugs.debian.org/207035 >> Can you give us some details on where direct I/O is slower? >> Are you working with files or device nodes? >> Which type of disk device is this? > > I use shred for block devices only. My measurements show that > direct I/O is indeed optimal for block devices where the device has > its write cache enabled. If for some reason the write cache is > disabled and/or cannot be enabled, then shred with direct I/O is > very slow because the device must complete a physical transaction > for each 64 KB write request. In this case, the option to disable > direct I/O is very helpful.
Thanks for the extra info. > In my case the cache could not be enabled on some older SATA (via AHCI) > drives. That is an edge case in fairness. I'd like to avoid an option for this performance only edge case. Attached is a patch to periodically request to discard written pages, rather than use O_DIRECT. I need to do a bit more testing to ensure it introduces no performance regressions, though it's worth noting that we ran without direct I/O between 2006-2013 without any requests to address that. thanks, Pádraig.
From dc77b61ba02d90692fd953b74e196478ac6ff5a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]> Date: Thu, 15 Oct 2015 14:30:53 +0100 Subject: [PATCH] shred: prefer POSIX_FADV_DONTNEED over direct I/O * src/shred.c (dopass): Avoid direct I/O as it's only used to avoid thrashing the page cache. It can be slower when used with devices with no write cache, as then each 64KiB is written sychronously. Instead periodically discard the cache with posix_fadvise(), when it's available and runs without error. --- src/shred.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/shred.c b/src/shred.c index 2857835..0d9e6fd 100644 --- a/src/shred.c +++ b/src/shred.c @@ -464,6 +464,12 @@ dopass (int fd, struct stat const *st, char const *qname, off_t *sizep, as it's just a performance rather then security consideration, and direct I/O can often be unsupported for small non aligned sizes. */ bool try_without_directio = 0 < size && size < output_size; +#if HAVE_POSIX_FADVISE + /* Avoid direct I/O if we can indicate to the system + to discard the cach for the file. */ + if (posix_fadvise (fd, 0, 0, POSIX_FADV_DONTNEED) == 0) + try_without_directio = true; +#endif if (! try_without_directio) direct_mode (fd, true); @@ -578,6 +584,12 @@ dopass (int fd, struct stat const *st, char const *qname, off_t *sizep, goto free_pattern_mem; } +#if HAVE_POSIX_FADVISE + /* Periodically release already written pages. */ + if ((offset % (output_size*16)) == 0) + posix_fadvise (fd, 0, 0, POSIX_FADV_DONTNEED); +#endif + offset += soff; bool done = offset == size; @@ -622,13 +634,7 @@ dopass (int fd, struct stat const *st, char const *qname, off_t *sizep, previous_human_offset = previous_offset_buf; thresh = now + VERBOSE_UPDATE; - /* - * Force periodic syncs to keep displayed progress accurate - * FIXME: Should these be present even if -v is not enabled, - * to keep the buffer cache from filling with dirty pages? - * It's a common problem with programs that do lots of writes, - * like mkfs. - */ + /* Force periodic syncs to keep displayed progress accurate. */ if (dosync (fd, qname) != 0) { if (errno != EIO) -- 2.5.0
