> On April 15, 2017, 8:30 a.m., David Faure wrote:
> > But doesn't this make copying much slower in the normal case? (copying onto
> > a non-removable harddisk partition).
> >
> > 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
>
> Oswald Buddenhagen wrote:
> 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)
>
> KJ Tsanaktsidis wrote:
> I'm using kernel `Linux kj-hedt-arch 4.10.8-1-ARCH #1 SMP PREEMPT Fri Mar
> 31 16:50:19 CEST 2017 x86_64 GNU/Linux`. 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.
>
> 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.
>
> 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 `kio_file`? On Linux it looks like I
> can get this from sysfs `/sys/dev/block/maj:min/removeable`, 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?
Solid has portable API for this.
Something like this (completely untested, I'm no Solid expert, these are just
old notes from a TODO)
const QString query = QLatin1String("[StorageAccess.accessible == true]");
const QList<Solid::Device> lst = Solid::Device::listFromQuery(query);
iterating and then using Solid::StorageDrive::isRemovable() ||
Solid::StorageDrive::isHotpluggable() (these checks can probably be integrated
into the query string)
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130084/#review103048
-----------------------------------------------------------
On April 15, 2017, 8:28 a.m., KJ Tsanaktsidis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130084/
> -----------------------------------------------------------
>
> (Updated April 15, 2017, 8:28 a.m.)
>
>
> Review request for KDE Frameworks, Oswald Buddenhagen and Thiago Macieira.
>
>
> Repository: kio
>
>
> Description
> -------
>
> 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.
>
> 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.
>
> 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.
>
> 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.
>
>
> Diffs
> -----
>
> src/core/copyjob.cpp 7c02cb50d7e9c11bbcd9264832357f3fd6dc8c16
> src/core/filecopyjob.cpp 301b7039158b7dc537b9004c14845b3d1d60f8eb
> src/core/job_base.h 0be9629f42277afc5f72d00d0cae5c9c1cd2b8bc
> src/core/slavebase.cpp 3778df813b8568657a2cbd9412c1244f94696a0c
> src/ioslaves/file/file_unix.cpp 3c1b9927e3dd2d0134f77caec6e6b24a0356d26f
>
> Diff: https://git.reviewboard.kde.org/r/130084/diff/
>
>
> Testing
> -------
>
> 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.
>
> 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.
>
>
> Thanks,
>
> KJ Tsanaktsidis
>
>
