<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/130084/">https://git.reviewboard.kde.org/r/130084/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 15th, 2017, 8:30 a.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But doesn't this make copying much slower in the normal case? (copying onto a non-removable harddisk partition).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It sounds to me like this should be
1) done internally in kio_file (no Job API for this)
2) only when the destination is a removable device</p></pre>
</blockquote>
<p>On April 15th, 2017, 4:25 p.m. UTC, <b>Oswald Buddenhagen</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">i've read multiple times that fsync isn't a performance problem on modern file systems any more. whatever that may mean.
limiting this to cross-device isn't really sensible imo - a) one can have multiple internal disks and b) even if the disk stays in, at some point the flushing will commence and will be a major slowdown for subsequent operations.
in fact, this problem is bad enough that the linux kernel community realized it (which, in the area of disk i/o, never ceases to amaze) - https://lwn.net/Articles/682582/ (obvious followup question: what kernel do you use? this code seems to have landed in 4.10)</p></pre>
</blockquote>
<p>On April 16th, 2017, 2:01 a.m. UTC, <b>KJ Tsanaktsidis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm using kernel <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Linux kj-hedt-arch 4.10.8-1-ARCH #1 SMP PREEMPT Fri Mar 31 16:50:19 CEST 2017 x86_64 GNU/Linux</code>. My understanding is that the patchset you're talking about will not allow synchronous reads, such as faulting in application code, to get stuck behind a full writeback queue, by limiting how much work the MM subsystem can send to the disk - not by throttling how fast applications can dirty the device. I've not noticed any problems using the disk whilst writing with or without my patch to KIO here but I haven't really stressed it.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As to what the performance implications of fsync - I guess it depends how much you care about what you were planning to do with the file after you copy it. I implemented the "fsync if source/dest are on different filesystems" logic because in that case, one of the things you might be wanting to do is unmount the disk. If you wanted to interact with the file on the destination system instead, this patch would make it take (much) longer. The reason I implemented this with a job flag is that I was envisiging making it an option in Dolphin - like the "move/copy" menu when you drop, you could also get "copy with fsync" perhaps for this reason - we don't know what the user plans to do with the file afterwards.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm happy enough to use "is the device removeable?" as a heuristic for "the users next desired operation on this file is probably to unmount it" instead and delete the Job API - this would address my use case at least. How do you suggest I get this information in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kio_file</code>? On Linux it looks like I can get this from sysfs <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">/sys/dev/block/maj:min/removeable</code>, but I don't know how to do this for other platforms and don't have them available to test. Would the patch be OK if I just added this for linux?</p></pre>
</blockquote>
<p>On April 16th, 2017, 8 a.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Solid has portable API for this.
Something like this (completely untested, I'm no Solid expert, these are just old notes from a TODO)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span></span><span style="color: #008000; font-weight: bold">const</span> <span style="color: #008000; font-weight: bold">QString</span> <span style="color: #008000; font-weight: bold">query</span> <span style="color: #666666">=</span> <span style="color: #008000; font-weight: bold">QLatin1String</span><span style="color: #666666">(</span><span style="color: #BA2121">"</span><span style="color: #BC7A00">[</span>StorageAccess.accessible <span style="color: #666666">==</span> <span style="color: #008000; font-weight: bold">true</span><span style="color: #BC7A00">]</span><span style="color: #BA2121">"</span><span style="color: #666666">);</span>
<span style="color: #008000; font-weight: bold">const</span> <span style="color: #008000; font-weight: bold">QList</span><span style="color: #666666"><</span><span style="color: #008000; font-weight: bold">Solid</span><span style="color: #666666">:</span><span style="color: #AA22FF">:Device</span><span style="color: #666666">></span> <span style="color: #008000; font-weight: bold">lst</span> <span style="color: #666666">=</span> <span style="color: #008000; font-weight: bold">Solid</span><span style="color: #666666">:</span><span style="color: #AA22FF">:Device</span><span style="color: #666666">:</span><span style="color: #AA22FF">:listFromQuery</span><span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">query</span><span style="color: #666666">);</span>
<span style="color: #008000; font-weight: bold">iterating</span> <span style="color: #008000; font-weight: bold">and</span> <span style="color: #008000; font-weight: bold">then</span> <span style="color: #008000; font-weight: bold">using</span> <span style="color: #008000; font-weight: bold">Solid</span><span style="color: #666666">:</span><span style="color: #AA22FF">:StorageDrive</span><span style="color: #666666">:</span><span style="color: #AA22FF">:isRemovable</span><span style="color: #666666">()</span> <span style="color: #666666">||</span> <span style="color: #008000; font-weight: bold">Solid</span><span style="color: #666666">:</span><span style="color: #AA22FF">:StorageDrive</span><span style="color: #666666">:</span><span style="color: #AA22FF">:isHotpluggable</span><span style="color: #666666">()</span> <span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">these</span> <span style="color: #008000; font-weight: bold">checks</span> <span style
="color: #008000; font-weight: bold">can</span> <span style="color: #008000; font-weight: bold">probably</span> <span style="color: #008000; font-weight: bold">be</span> <span style="color: #008000; font-weight: bold">integrated</span> <span style="color: #008000; font-weight: bold">into</span> <span style="color: #008000; font-weight: bold">the</span> <span style="color: #008000; font-weight: bold">query</span> <span style="color: #008000; font-weight: bold">string</span><span style="color: #666666">)</span>
</pre></div>
</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">OK - that seems reasonable enough. I guess I should be able to search Solid's disks for the major/minor numbers from <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">fstat</code>'s <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">st_dev</code>. Whilst doing this I ran into a bug with Solid, which I've proposed a patch for here: https://git.reviewboard.kde.org/r/130090/. If that gets OK'd I'll come back to finishing this off :)</p></pre>
<br />
<p>- KJ</p>
<br />
<p>On April 15th, 2017, 8:28 a.m. UTC, KJ Tsanaktsidis wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks, Oswald Buddenhagen and Thiago Macieira.</div>
<div>By KJ Tsanaktsidis.</div>
<p style="color: grey;"><i>Updated April 15, 2017, 8:28 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kio
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">When copying a large-ish file (~1-2GB) from very fast storage to very slow storage (e.g. an NVME SSD to a cheap USB flash drive) on a machine with lots of RAM, Dolphin displays a progress bar which finishes in a fraction of a second (i.e. as fast as it takes to read the source file into the Linux page cache). Unmounting the drive then of course takes a long time, with only an indeterminate spinner.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch adds an option to force fsync during copy jobs, so that the copy progress bar measures how long it will take to actually copy the file to the destination.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've added two flags - Fsync and FsyncCrossFilesystem - to the JobEnum flag. The former will cause all copy operations to fsync during the copy loop, whilst the latter will only fsync copies that are across different filesystems.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If this patch gets OK'd, I have another patch which adds support for this into the appropriate places in Dolphin. I would think that at least FsyncCrossFilesystem should be the default, but Fsync always might be a little heavy handed. At the least fsync'ing cross-filesystem copies ensures that the unmount won't take forever.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Tested the patch with KDE/Dolphin on Arch Linux, which is version 5.32.0. The diff applies cleanly to master so I assume there shouldn't be any issues there, but I've not actually checked that. As advertised, copying a file to USB flash storage now displays an accurate progress bar.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I experimented with how often fsync should be called on my hardware, and I found calling it every ~1M copied caused no decrease in copy performance whilst still providing accurate progress info. That is the setting I've gone with in this patch. I'm open to suggestions on how this could be tuned better though.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/core/copyjob.cpp <span style="color: grey">(7c02cb50d7e9c11bbcd9264832357f3fd6dc8c16)</span></li>
<li>src/core/filecopyjob.cpp <span style="color: grey">(301b7039158b7dc537b9004c14845b3d1d60f8eb)</span></li>
<li>src/core/job_base.h <span style="color: grey">(0be9629f42277afc5f72d00d0cae5c9c1cd2b8bc)</span></li>
<li>src/core/slavebase.cpp <span style="color: grey">(3778df813b8568657a2cbd9412c1244f94696a0c)</span></li>
<li>src/ioslaves/file/file_unix.cpp <span style="color: grey">(3c1b9927e3dd2d0134f77caec6e6b24a0356d26f)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/130084/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>