Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
Closed #187. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#event-1305249129___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
Functionality included in slightly different for as of commit 4087530f0fcbb14167be8296957e44e6ffc97579, thanks for your work on this, not to mention patience. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-338605013___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
Updated this PR to use a static variable. I'm still fine with either solution, but @pmatilai and @megaumi @ffesti - can someone weigh in, please? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-305674583___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
Yes, fdatasync+fadvise_syncfs removes pages from system cache: ` --> Fincore(0x61408e40) fdno 16 path /opt/local/tmp/lib/modules/4.11.0-0.rc6.git2.1.fc27.x86_64/System.map;58f7761a <-- Fincore(0x61408e40) fdno 16 rc 0 <-- fdClose(0x61408e40) rc 0| UFD -1 fp 0x61204e40 FDIO: 30write(s), 3763601 total bytes in 0.003806 secs FDIO: 32 digest(s), 3763601 total bytes in 0.037065 secs FDIO: 1alloc(s), 3763601 total bytes in 0.40 secs FDIO: 1 datasync(s),0 total bytes in 0.072015 secs FDIO: 1 syncfs(s),0 total bytes in 0.063909 secs FDIO: 2 incore(s), 3764224 total bytes in 0.000263 secs ` I'm not sure whether per-partition rather than per-file durability is usefully needed for RPM. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-295296195___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
@cgwalters: easy enough to wrap syncfs(2) into a measurement harness: ` ==> Fclose(0x61408e40) | LIBIO 0x61204fc0(-1) fdno -1 | UFD 16 fp 0x61204fc0 --> Fincore(0x61408e40) fdno 16 path /opt/local/tmp/lib/modules/4.11.0-0.rc6.git2.1.fc27.x86_64/System.map;58f6dc66 <-- Fincore(0x61408e40) fdno 16 rc 919 --> Syncfs(0x61408e40) fdno 16 path /opt/local/tmp/lib/modules/4.11.0-0.rc6.git2.1.fc27.x86_64/System.map;58f6dc66 <-- Syncfs(0x61408e40) fdno 16 rc 0 --> Fincore(0x61408e40) fdno 16 path /opt/local/tmp/lib/modules/4.11.0-0.rc6.git2.1.fc27.x86_64/System.map;58f6dc66 <-- Fincore(0x61408e40) fdno 16 rc 919 <-- fdClose(0x61408e40) rc 0| UFD -1 fp 0x61204fc0 FDIO: 30write(s), 3763601 total bytes in 0.003511 secs FDIO: 32 digest(s), 3763601 total bytes in 0.034357 secs FDIO: 1alloc(s), 3763601 total bytes in 0.40 secs FDIO: 1 syncfs(s),0 total bytes in 0.166156 secs FDIO: 2 incore(s), 7528448 total bytes in 0.000425 secs <-- fdClose(0x61408e40) rc fffe | LIBIO 0x61204fc0(-1) fdno -1 | UFD -1 fp (nil) D: fini 100600 1 ( 0, 0) 3763601 /opt/local/tmp/lib/modules/4.11.0-0.rc6.git2.1.fc27.x86_64/System.map;58f6dc66 ` For starters, Fincore (which sums the pages reported by mincore(2) on the mmap'd fdno) indicates that no pages are removed from the system cache by syncfs(2) whatsoever. Likely fdatasync+fadvise are still needed with syncfs(2), just like with fsync(2). Then syncfs(2) is clocking in at ~165 millisecs, which is ~3x worse than fdatasync+fadvise+fsync ~50 millisecs. Which is hardly surprising: the ext4 partition being sync'd is ~500Gb, much larger than the ~4Mb file I am measuring. Hard to compare coconuts with bananas ... (aside) And sure os-tree can create/use smaller partitions to limit the amount of I/O necessary to sync to disk. RPM hasn't that luxury, and adding an artificial partition around an I/O benchmark is ... well ... rather a cheat. YMMV. Finally the wall-clock time for kernel-core RPM install clocks in at `2.84user 1.04system 3:44.12elapsed 1%CPU (0avgtext+0avgdata 221228maxresident)k 200inputs+75808outputs (3major+92612minor)pagefaults 0swaps` So syncfs(2) is merely 45x slower than the BASE measurements I reported before. I'll stick with fdatasync+fadvise+fsync (~30x slower) until I hear something better. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-295073080___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
@n3npq Regarding full-system durability - I'm the maintainer of https://github.com/projectatomic/rpm-ostree/ which uses https://github.com/ostreedev/ostree/ to implement full-system transactional updates. Every change (package install, update) is atomic. libostree currently [stages objects per boot ID, then uses syncfs()](https://mail.gnome.org/archives/ostree-list/2017-January/msg0.html). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-294845668___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
@pmatilai - I'd like to get something merged sooner rather than later. There are two options I see - (1) my patch (plus moving the parsing of the file into some static variable) or (2) @n3npq patch, but turning it off by default. Those are the two safe, clean, easy methods going forward. Can you weigh in here, so we can move forward? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-294570200___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
@pmatila: if you *do* attempt to configure +/- fsync(2) somehow, then durability is likely the better approach. By "durability", I mean "this file is essential to booting" (and hence is worth paying the cost of fsync(2) to ensure that the file is correctly written to disk). Some simple patterns could be applied to paths to choose fsync(2) (or not). One could also add a %sync flag in spec files to set a flag for per-file-durability under packager control. OTOH, per-file != per-package or per-transaction durability. But at least per-package atomicity can be achieved by delaying the FILE.TID -> FILE rename into place. Unclear whether its worth the effort *shrug*. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-294410365___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
Wall clock time measurements for an RPM command (this is RPM5, but the I/O trace is identical to rpm.org): `sudo /usr/bin/time /opt/local/bin/rpm -Uvh --nodeps --noscripts --relocate /=/opt/local/tmp /var/tmp/kernel-core-4.11.0-0.rc6.git2.1.fc27.x86_64.rpm` BASE 2.26user 0.43system 0:03.90elapsed 69%CPU (0avgtext+0avgdata 196708maxresident)k 0inputs+75744outputs (1major+82590minor)pagefaults 0swaps 2.36user 0.44system 0:07.54elapsed 37%CPU (0avgtext+0avgdata 195668maxresident)k 45280inputs+75784outputs (10major+82618minor)pagefaults 0swaps USING fallocate+fdatasync+fadvise+fsync --- 2.60user 0.89system 2:01.76elapsed 2%CPU (0avgtext+0avgdata 196632maxresident)k 0inputs+76488outputs (1major+82685minor)pagefaults 0swaps 2.59user 0.85system 2:01.93elapsed 2%CPU (0avgtext+0avgdata 196456maxresident)k 0inputs+75752outputs (1major+82684minor)pagefaults 0swaps 2.80user 0.89system 2:30.68elapsed 2%CPU (0avgtext+0avgdata 196004maxresident)k 44952inputs+76520outputs (19major+82666minor)pagefaults 0swaps 2.77user 0.95system 3:00.88elapsed 2%CPU (0avgtext+0avgdata 196216maxresident)k 50008inputs+75792outputs (27major+82668minor)pagefuls 0swaps (aside) These measurements were taken on a build server -- the slower measurements were the result of active builds in docker. So fallocate+fdatasync+fadvise+fsync is (nominally) ~30x slower (using a wall clock measured install on rotating media). @jayzmh: (to summarize, I have said most of this before indirectly. JMHO, YMMV) I'd like to hear simple measurements of the effect of RPM+patch on both SSD and rotating media. The measurements (or at least a warning) should be included with any "opt-in" macro that you choose for configuration so that other users might be forewarned of the significant delay to expect from using fsync(2). (aside) I'll be happy to run these measurements for you on whatever patch you choose to merge into RPM, as well as the tests below. I'd also like to hear that RPM+whatever_patch_you_use "works" because the installed files are not in cache, not because RPM is running ~30x (or whatever delay on SSD) slower and hence not competing for cache effectively. There's nothing in the fsync(2) man page that specifies the effect on kernel cache, only that fsync(2) returns when file info has been written to disk. Basically I'd like to see what mincore(2) has to say about installed-file-in-cache after RPM exits if you only wish to use fsync(2), or (perhaps) a comparison when fsync(2) is replaced with a ~50ms delay per file (what I am seeing as the approx fdatasync(2) delay for every file installed) if only `usleep(5);` is used. One simple way to check the kernel cache would be to use fincore(1) from linux ftools here [https://code.google.com/p/linux-ftools/](url) immediately after an RPM install (or use the C routine fmincore() in my patch above). @cgwalters: Jens Axboe's "buffer bloat" patch deals only with autoscaling to prevent large writes from degrading overall system performance. The other part of the problem is that file durability using fsync(2) is costly, and there isn't an obvious solution (at least that I can find) to how to mitigate that cost. Perhaps spinning off a helper thread on file close will "work": I'm not sure I trust the widely cited conclusions here [http://oldblog.antirez.com/post/fsync-different-thread-useless.html](url), particularly with the "buffer bloat" patch. RPM I/O is also many files, not one very large file. @pmatilai: worrying about naming a macro "_periodic_fsync" and later generalizing to call f*sync(2) to be, say, every nBytes/nSecs is likely unnecessary. For starters, RPM installations are many files, not a single large file, and fsync(2) takes a single fdno argument. Doing "w+.fdio" (i.e. open RW and truncating), and pre-sizing using fallocate(2) (so that st->st_size metadata does not need to be repeatedly sync'd as writes progress) is likely a trivial implementation in the right direction on all media. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-294409265___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
@jaymzh But are your tests with a kernel with the writeback changes or not? It looks like this commit? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e34cbd307477ae07c5d8a8d0bd15e65a9ddaba5c Which offhand appears to be in v4.10. Ah yes, [kernelnewbies has a link](https://kernelnewbies.org/LinuxChanges#head-aa8362f44837f5f05342ca34683ccd63642466b0). I'm not saying it's a bad idea to patch RPM - there will be people using older kernels for a while. But it is an argument against extensively polishing the patch and testing alternative changes, and possibly dropping the functionality after some time. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-294288704___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
@cgwalters - Jens works here... yes, the problem description is related, but while a system-wide solution can make this better, being able to administratively choose things that can never ever full the buffer is still important. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-294250985___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
Yes, with about 20 other similar articles and implementations, some even accurate. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-294249350___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
Isn't this https://lwn.net/Articles/682582/ ? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-294231945___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
The buffer is 32 * BUFSIZ )= 128Kb) not 32Kb. Sorry for the confusion. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-294225532___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
In regards to your questions an optimal implementation can come whenever, I'd like to get something merged nowish that I can opt-in to on hosts that t need it. So changing your patch to default to off, and then leaving it at the sync rate its at is fine. 1MB is probably nicer to the disk but 32k is totally fine. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-294217509___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
One last note: doing fdatasync once, before fadvise+fsync, not after every write is ~10x faster: ` FDIO: 30 writes, 3763601 total bytes in 0.003558 secs FDIO: 1 allocs, 3763601 total bytes in 0.24 secs FDIO: 1 dsyncs,0 total bytes in 0.074081 secs FDIO: 1 syncs,0 total bytes in 0.005785 secs ` That makes the best case ~17x slower than using the kernel caches. A cost of 17x is almost tolerable in RPM for the benefits of having files written to disk and not blowing out the kernel caches. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-294217202___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
If you are arguing for "opt-in" rather than "opt-out" by default when a macro
is not explicitly set, then change the "1" in this line:
`
_periodic_fsync =
rpmExpandNumeric("%{?_periodic_fsync}%{!?_periodic_fsync:1}");
`
Otherwise set %_periodic_fsync as you wish.
There wasn't any significant speedup using a 1Mb rather than a 32Kb buffer when
implemented.
If you wish fdatasync to be called every ~1Mb, then add logic to call every
other trip through the loop by, say, interpreting a small value of
%_periodic_fsync as a count, and a large value as a desired fdatasync interval,
and proceed accordingly.
If you are saying that there is no interest in ensuring either that RPM always
syncs to disk, or that there aren't other systems where package management
interferes with other running processes, you are incorrect.
The critical factor choosing to do fsync in RPM is going to be knowing the cost
of using, and only measurements can tell the cost.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/187#issuecomment-294151888___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
In our case ~ 1MB is what we want. This will never be a good idea for rotating disks... and also most people will never need this. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-294064508___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
A measurement of the impact is needed first, enabler/disabler configuration can always be added. In general, RPM does best with "opt-out", not "opt-in" policies. There may be other changes, such as doing the fdatasync() less often, may be useful too. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-293899781___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
Here is an updated patch, including the bare minimum changes with a %_periodic_fsync "opt-out" macro to the "production" rpmfiArchiveReadToFilePsm() to use fdatasync+posix_fadvise+fsync. I've left all the debugging gook in the patch to document the --rpmiodebug trace and to illustrate the mincore(2) etc verification testing in the previous comment. The important & necessary parts of the patch should be obvious. The patch passes "make check" and isn't going to kill your kittens or steal your girl friend. ;-) Experience with the cost of always (including rotating DASD) installing with fsync is needed before undertaking further steps. [rpm+fsync.patch.gz](https://github.com/rpm-software-management/rpm/files/908578/rpm.fsync.patch.gz) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-292816086___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
OK, here is one way to install a file without leaving the file in the cache.
The simplest explanation is to show the --rpmiodebug log (interspersed with
some debugging):
*** Fopen ufdio path /usr/share/doc/hello-2.0/README;58ea9807 fmode w+.ufdio
*** ufdOpen(/usr/share/doc/hello-2.0/README;58ea9807,0x242,0666)
==> Fileno(0x1c2c260) rc 31 | ufdio 31 fp (nil)
*** Fdopen(0x1c2c260,w+.ufdio) | ufdio 31 fp (nil)
==> Fdopen(0x1c2c260,"w+.ufdio") returns fd 0x1c2c260 | ufdio 31 fp (nil)
==> Fopen("/usr/share/doc/hello-2.0/README;58ea9807",242,0666) | ufdio
31 fp (nil)
==> Ferror(0x1c2c260) rc 0 | ufdio 31 fp (nil)
==> Fileno(0x1c2c260) rc 31 | ufdio 31 fp (nil)
==> Fread(0x1c2fff0,0x7ffd4dedc7d0,39) rc 39| gzdio 0x1c29f20 fp 30
| fdio -1 fp (nil)
==> Fwrite(0x1c2c260,0x7ffd4dedc7d0,39) rc 39 | ufdio 31 fp (nil)
==> Ferror(0x1c2c260) rc 0 | ufdio 31 fp (nil)
*** fdatasync(31)
*** fbefore: 1
*** posix_fadvise(31, 0, 0, 0x4)
*** fsync(31)
*** fafter: 0
ufdio: 1 writes, 39 total bytes in 0.13 secs
`
Basically there are 3 pieces to an RPM install implementation that does not
blow out the system cache.
1) After Fwrite(), a Flush() followed by fdatasync(2) is needed.
After writing all the bytes, one page is cached (see mincore(2), the "fbefore:
1" line).
2) posix_fadvise(POSIX_MADV_DONTNEED) is called to hint that the file need not
remain in cache.
3) fsync is called to write the file metadata to disk.
After syncing all the bytes, no pages are cached (the "fafter: 0" line).
This is basically what rsync-fadvise is doing here:
[https://github.com/MageSlayer/rsync-fadvise](url)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/187#issuecomment-292812018___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
I knew if I wrote a patch, better answers would instantly appear. There is fadvise(FADV_DONTNEED) wrto fsync(2) analogous to madvise(MADV_DONTNEED) wrto msync(2). I will add to my patch in the next few days for measurement, todo++. The easy answer for your "Chef installs packages and my databases stall!" is likely to run use nocache from [https://github.com/Feh/nocache](url). There's also a means to use a cgroup there to put a resource cap on RPM (or any other application) memory usage. Perhaps the group scripting can be use with rpm lib: todo++. Meanwhile rsync backups have a similar effect to your RPM issue. Both rpm/rsync have huge I/O needs that easily cause cache blowout. Recent rsync has not only a solution, but rsync can also use mincore(2) to explicitly verify that no blocks/pages from a file remain in the cache. Ditto, todo++ to add to patch ... -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-292762306___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
As promised, here is a proof-of-concept patch to permit using either fwrite+fsync or mmap+madvise+msync while writing files onto the file system from a *.rpm package. The patch passes "make check" (with the current known failures) on tip. There are various switches to enable/disable the added functionality whose usage should be fairly obvious. I have not bothered with either build/runtime configuration with macros etc because its not yet clear whether msync(MS_ASYNC) actually solves any problem yet. If the proof-of-concept patch using msync(MS_ASYNC) does work, then we can proceed from there. [rpm+mmap.patch.gz](https://github.com/rpm-software-management/rpm/files/907882/rpm.mmap.patch.gz) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-292753264___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
Thanks for measurements! Could you also attempt to measure the effect on RPM too? Wall clock time is sufficient, as the time necessary to perform an upgrade is what most RPM users are sensitive to. Your graphs (presumably on SSD?) show smaller/periodic writes w fsync enabled, and larger/delayed writes w/o fsync, as expected. The delay (most obvious w/o sync, but occasionally w fsync(2)) is part of the cache flush algorithm to prevent active writes from starving reads (i.e. your database stalls). The issue (for RPM) is that fwrite(2)+fsync(2) ends with a blocking system call, while mmap(2)+madvise(MADV_DONTNEED)+msync(MS_ASYNC) permits RPM to continue running while the cache is flushed and resources are free'd. By continuing to run, RPM can provide more information on RPM near term write needs to the kernel cache flushing scheduler. The end result in both cases is that resources used by RPM write's are free'd as soon as possible. Does that explanation make sense to you? I'll try to get a patch to (again) use mmap+madvise+msync for rpm.org together this weekend. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-292720982___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
OK, so here's a few different takes on numbers. First graphs. Graph one is with the macro on: https://g.defau.lt/#gluSzlb39ScnZU4FS2AtrYUF31IYct Graph2 is with the macro off: https://g.defau.lt/#6We3VgDfcXi7zkhMMzdesQLKD5iMUd The colors are reads, read bytes, writes, write bytes. You can see the load spreads out as expected We also managed to do a synthetic test which was interesting and showed that for our dominant use-case of RPMs with lots of small files this is a big win. This is the mount of time the storage system was not doing work: # fb-dba-core: ## With macro - many smaller files, one huge file 2017-04-06 16:26:57.722013873 -0700 PDT 5.73155ms 104.968µs 6 2017-04-06 16:26:58.722147493 -0700 PDT 5.252973ms 117.358µs 5 2017-04-06 16:26:59.722343955 -0700 PDT 7.422587ms 139.914µs 8 2017-04-06 16:27:00.72254866 -0700 PDT 6.109201ms 122.32µs 6 2017-04-06 16:27:01.722734509 -0700 PDT 7.40091ms 155.112µs 8 2017-04-06 16:27:02.722875662 -0700 PDT 7.794604ms 148.676µs 8 2017-04-06 16:27:12.723782443 -0700 PDT 208.33143ms 25.153745ms 202 2017-04-06 16:27:13.723915361 -0700 PDT 5.998775ms 133.564µs 7 2017-04-06 16:27:14.724062693 -0700 PDT 5.89304ms 132.267µs 7 2017-04-06 16:27:15.724212253 -0700 PDT 6.98323ms 117.4µs 8 2017-04-06 16:27:16.724324089 -0700 PDT 5.402875ms 132.129µs 5 2017-04-06 16:27:17.724437579 -0700 PDT 5.926757ms 127.274µs 7 2017-04-06 16:27:18.724621554 -0700 PDT 5.837904ms 121.583µs 6 2017-04-06 16:27:19.724740404 -0700 PDT 5.080387ms 115.304µs 5 2017-04-06 16:27:20.724855913 -0700 PDT 5.573811ms 131.026µs 6 2017-04-06 16:27:21.725034955 -0700 PDT 3.856319ms 102.18µs 4 2017-04-06 16:27:22.725188062 -0700 PDT 4.405144ms 111.026µs 5 ## Without macro 2017-04-06 16:27:55.728507697 -0700 PDT 27.318392ms 885.188µs 26 2017-04-06 16:28:02.729271342 -0700 PDT 7.49606ms 116.632µs 8 2017-04-06 16:28:15.730604087 -0700 PDT 84.956988ms 10.598699ms 83 2017-04-06 16:28:20.73111897 -0700 PDT 48.069056ms 457.543µs 53 2017-04-06 16:28:21.731312738 -0700 PDT 65.498909ms 2.925142ms 63 # fb-gcc-5-glibc-2.23-runtime-6-1.x86_64.rpm, some 100MB files ## With macro 2017-04-06 16:33:41.765007508 -0700 PDT 1.320882ms 88.294µs 3 2017-04-06 16:33:42.765164056 -0700 PDT 13.984165ms 279.486µs 13 2017-04-06 16:33:43.765326558 -0700 PDT 1.890576ms 80.318µs 2 2017-04-06 16:33:46.765670291 -0700 PDT 39.313771ms 1.022946ms 38 2017-04-06 16:33:47.765840591 -0700 PDT 3.801104ms 94.404µs 5 2017-04-06 16:33:51.766274676 -0700 PDT 67.966698ms 2.87646ms 67 2017-04-06 16:33:52.766424373 -0700 PDT 1.996679ms 90.686µs 2 2017-04-06 16:33:56.766825593 -0700 PDT 101.579654ms 5.967763ms 98 2017-04-06 16:34:02.767494189 -0700 PDT 19.626819ms 332.579µs 20 2017-04-06 16:34:03.767628848 -0700 PDT 4.940615ms 116.663µs 5 2017-04-06 16:34:04.76287 -0700 PDT 2.480112ms 97.978µs 4 2017-04-06 16:34:05.7679422 -0700 PDT 1.551916ms 62.483µs 2 2017-04-06 16:34:06.768067462 -0700 PDT 1.756435ms 68.526µs 2 2017-04-06 16:34:07.768176248 -0700 PDT 1.319394ms 87.179µs 2 2017-04-06 16:33:02.761068223 -0700 PDT 6.764001ms 99.465µs 7 ## Without macro 2017-04-06 16:34:59.773285651 -0700 PDT 1.695824ms 87.892µs 2 2017-04-06 16:35:02.773564104 -0700 PDT 5.68678ms 105.131µs 6 2017-04-06 16:35:12.774532746 -0700 PDT 65.836492ms 1.853472ms 70 2017-04-06 16:35:13.774667005 -0700 PDT 82.757401ms 8.41281ms 80 2017-04-06 16:35:18.77511384 -0700 PDT 73.289284ms 3.436747ms 71 2017-04-06 16:35:24.775796768 -0700 PDT 1.199151ms 85.286µs 2 2017-04-06 16:35:26.77602982 -0700 PDT 1.203794ms 105.355µs 2 2017-04-06 16:35:43.777684277 -0700 PDT 51.357162ms 2.053203ms 50 So large enough files can still cause problems, but it's not perfect. Madvise isn't quite the same here... though certainly keeping track and always writing after a certain number of bytes would be even better, but I think this shows this diff is a good start. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-292681920___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
@jayzmh: major invasive change to ... RPM? or something else? I'll happily write the code for rpm.org RPM if you wish. I fail to see how reverting to what RPM used to do (i.e. use mmap with MADV_DONTNEED and most importantly for you using msync(MS_ASYNC) which schedules flush to disk like fsync freeing resources but returns immediately permitting RPM to continue its software updating. There's another approach using O_SYNC/O_DSYNC which I've not looked at because (correctly) added to POSIX and linux in 2008, which is long after I looked/measured the original mmap implementation. But lets not go to O_SYNC/O_DSYNC until MADV_DONTNEED is measured please. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-292327785___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
To your other question we're still trying to come up with numbers, the system has many things going on and showing something 100% attributable to this is hard - I'm trying to see if we can get a more exact A/B test. madvise() doesn't help ensure there's less dirty data in the system, which is different from what you are describing. If the write-queue becomes too long, then anything else that needs to write now has to wait... which doesn't work well with a database load. Now mysql sync's and has to wait for a half-gig of data to be flushed. That's a problem. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-292278640___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
@n3npq as I called out a few times... the examples I found were all in the build/ directory... maybe I'm just not seeing another good example. Moving to mmap() and madvise() is a major invasive change... if someone wants to take that on, awesome, but this is a simple solution that buys us a lot in the interim. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-292251567___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
@jayzmh - add a static variable to expand the macro as a "one shot" ... there are many places in rpm that do that, hurts little (until rpm can capture global configuration object). Meanwhile I recommend the *_DONTNEED mmap implementation instead of fsync. RPM used that for years, and *_DONTNEED was originally suggested by Ben LaHaise (a linux kernel developer) as the right implementation (particularly for what you want): let the kernel manage its resources, don't try to devise some configurable opt-in policy permitting an application using fsync to second guess (or permit sysadmins the pretense of second guessing) kernel resource management algorithms. Meanwhile: do you have measurements that you can report? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-292091677___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
@pmatilai - I'm still looking for a good suggestion on where to read that macro and store it in a static variable. @pmatilai - I'm ok with fsync_periodically or some such, it just means that if we ever want to change it, we have no way to do it nicely... we can't make a new `%_fsync_on_mb 50` with some sort of nice transition where people can move from one to the other. That's why I liked fsync_on_close, it's clear what it does, and when we want to implement something else we can. But I don't feel strongly, if you want it to be _fsync_aggressively or _fsync_periodically or something, let me know. However, it *should* have something about fsync in it so it's clear people don't want to do this on rotational media. @n3npq - Yes, the slowdown is intolerable on rotational media. This is why its default off and an admin needs to make a smart choice. Like with mount-options, there's destructive options that are generally terrible, but we provide them for admins who need them. Like turning off ordered mode in ext4. :) @n3npq - As you correctly characterized we're just slowing down RPM... giving more even access to IO... but to be clear we're not *just* slowing it down. There's other implications as well. For example we have some machines with 18 or 24 disks attached and installing a big RPM blows own the disk cache and causes *massive* IO load as the kernel sucks it all back in as the app continues doing it's work. By limiting the amount in the buffercache we don't evict diskcache. We can always afford the buffercache of a *single* file, but when Chef comes along and installed 25 RPMs with varying sizes of and numbers of files, we shoot ourselves in the foot. This is a different case than filling the IO buffers on our database systems where when MySQL comes along and and there's a huge queue of stuff to be flushed, and we cause stalls (the original incentive for this work). We actually played a lot with sync-on-write, sync-on-transaction, sync-on-close, and it turns out sync-on-close works incredibly well for a variety of different cases ... provided you're not on rotational media. I'm aware we could do more complex stuff... but it's a pretty hairy code-base and a simple solution that has shown to provide significant benefit seems like an easy win. Which is why I sat down with @pmatilai and others at DevConf.cz to discuss this work and that the approach would be acceptable before writing the patch. @n3npq - yes, @pmatilai also pointed out I shouldn't read the macros repeatedly, and said I should put it in a static variable, just haven't found a good place to do that, per above, still looking for a suggestion. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-291959681___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
jaymzh commented on this pull request.
> +int rc = 0, ec = 0;
+
+if (fd == NULL)
+ return -1;
+
+fd = fdLink(fd);
+for (FDSTACK_t fps = fd->fps; fps != NULL; fps = fps->prev) {
+ if (fps->fdno >= 0) {
+rc = fsync(fps->fdno);
+ if (ec == 0 && rc)
+ ec = rc;
+ }
+
+ /* Leave freeing the last one after stats */
+ if (fps->prev == NULL)
+ break;
Ah. Woops, cleaned both of those up. Thanks.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/187#discussion_r109998465___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
There is a modestly obscure means to attach *_DONTNEED (to achieve kernel delayed writes and removing pages when I/O is complete) using the existing rpm.org rpmfiArchiveReadToFilePsm() Fwrite->write(3) loop calling setvbuf(3) immediately after Fopen on the fopencookie(3) FILE *. (aside) Personally, I think mmap(2) and madvise(2) are easier to implement/maintain than rpmio fopencookie(3) wrapped stdio(3) abstractions like setvbuf(3): YMMV. The idea behind *_DONTNEED -- to minimize competition for scarce resources -- is to hint to the kernel that RPM write buffers (actually pages) can be reused by other processes as soon as written to disk, rather than cached. RPM package installation is always fire-and-forget for package files, not true in general for file writes. Letting the kernel manage its own resources as necessary may be sufficient to prevent database stalling/starvation as an alternative to attempting explicit fsync(3) invalidation of cached I/O buffers which has the unfortunate side effect of blocking RPM until the fsync(3) system call returns. (aside) This *IS* what used to be in RPM, and was a significant win at the time (but also included directly uncompressing into the mmap(2) buffer avoiding a copy, and increasing the zlib buffer size from 32K -> 256K, which has long since been upstreamed). Someone else will have to speak to why mmap(2) was ripped out at rpm.org. (hth, off to bed) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-291795090___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
Hmmm ... glibc (which uses mmap(2) for stdio if requested) would benefit from being able to mark the stdio buffer-in-use with *_DONTNEED using madvise/posix_fadvise. At a minimum, cvtfmode() in rpmio/rpmio.c would need to be extended to pass the glibc-peculier 'm' flag through to fopen(3) ah "info libc" indicates mmap(2) is permitted only for input files, never mind. (aside) Note that rpmio/rpmio.c cvtfmode() likely should be modified to pass all the glibc-peculier flags through to fopen(3): 'c', 'e', 'm', 'x' (see "info libc" and search for "open"). todo++ @rpm5.org -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-291782667___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
For completeness (and humor ;-), there is also O_DIRECT. See [https://lkml.org/lkml/2007/1/10/233](url) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-291777802___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
Another issue is that certain file systems (NFS? DFS? I forget ...) also delay I/O operations until close(2). What I recall is that an application MUST check the return code of close(2) in order to ensure that the I/O was actually performed: it isn't sufficient to check the return code of write(2). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-291775574___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
There is an alternative (but more complicated) lazy implementation that might be attempted by doing a fsync in Fwrite (or fdWrite) when the accumulated I/O passes a certain mark. H ... A general solution to add a resource cap on all rpmio I/O could also be attempted (but isn't easy). Berkeley DB does something like this with mpool file handles, and it would not be impossibly hard to use aio_write(3) instead of write(2) as a new type of rpmio. But that would require a common buffer allocation pool (or buffer-in-flight mapping) underneath rpmio (to provide the resource cap), as well as a reliable means to determine when I/O has actually been written to DASD (to free/unmap the buffer-in-flight). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-291774478___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
FYI: several notes (after looking at the patch) 1) The measurements I cited above are using mmap(2) to decompress directly into pages (avoiding a buffer copy) marked with MADV_DONTNEED (which permits delayed/larger writes) to avoid a buffer copy. In addition, there is a check on file size of <= 128Mb to avoid consuming too many pages on extremely large files. However, I have no reason to believe that mmap(2) instead of a loop over buffered writes will change the basic observation(s) that package management is a huge I/O load, and that fsync(2) will schedule large amounts of I/O that can starve/stall other running processes.. I suggest testing on a moderately large package with 3000+ files (like kernel source) to see the affect on package management. Slowing down package management (by perhaps a factor of 50x) will most certainly use fewer resources, and not compete with other running processes, as a side effect of calling fsync(2), but is perhaps not the best implementation. YMMV. 2) Instead of adding Fsync to walk the FD_t stack, fsync'ing every fdno, there is already Fileno() to return the topmost fdno of a FD_t, which is almost certainly all that is needed. Using fsync(Fileno(fd)) (or equivalent: Fileno() has negative error returns) either at the end of the loop in lib/rpmfi.c rpmfiArchiveReadToFilePsm(), or (equivalently) in the parent caller, achieves the same fsync without pretending that Fsync() "works" with all possible fdno's. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-291768318___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
pmatilai commented on this pull request.
> +int rc = 0, ec = 0;
+
+if (fd == NULL)
+ return -1;
+
+fd = fdLink(fd);
+for (FDSTACK_t fps = fd->fps; fps != NULL; fps = fps->prev) {
+ if (fps->fdno >= 0) {
+rc = fsync(fps->fdno);
+ if (ec == 0 && rc)
+ ec = rc;
+ }
+
+ /* Leave freeing the last one after stats */
+ if (fps->prev == NULL)
+ break;
This condition is quite clearly a leftover from copy-pasting Fclose(). It's
probably not harmful to the cause here but it's also unnecessary and the
comment is just plain wrong in this context.
The also copy-pasted fdLink() earlier is more harmful, it'll cause a file
descriptor (and memory) leak since it's incrementing the reference count
without ever decrementing it.
These were kinda implied between the lines in the "just walk fps->prev"
(instead of using copy-pasted Fclose code), sorry for not being more clear
about it.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/187#pullrequestreview-30958967___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
Good if doing in fsm.h ( I haven't looked). Meanwhile a 50x slower installation is intolerable for a package manager even if opt-in. Nor can an application like RPM be expected to detect whether the underlying filesystem is on SSD or USB or rotational disk and adapt accordingly: there simply aren't the necessary API/s. I believe that your databases are stalling, package management is a huge I/O load, unlike any other typical application. Meanwhile -- depending on what statistics you report (if any) -- the issue of 1) "hardening" install's in spite of exceptional outages 2) preventing "write starvation" of other running applications by introducing a resource cap on RPM I/O, will be clear. What measurements are you using to justify your fsync patch? That is not yet clear ... -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-291698648___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
@n3npq Uh, I am doing it in lib/fsm.h not in Fclose(), I dunno what you are talking about. Yes, the fsync() is super expensive on rotation media, as mentioned, don't use it in those cases. As I mentioned, I don't have good numbers on the positive effects, only that our databases stall far less often with this on. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-291636330___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
Do you have measurements of the effect of fsync()? The last time sync/fdatasync were suggested for RPM that I am aware of was ~2011 in Mandriva. The issue comes up every few years this century ... Here was the effect of adding fsync/fdatasync to RPM, measure on otherwise identical system with a package with lots of files: ` /* Measurements from installing kernel-source package: * +fsync * total: 1 0.00 MB640.854524 secs * +fdatasync * total: 1 0.00 MB419.983200 secs * w/o fsync/fdsatasync: * total: 1 0.00 MB 12.492918 secs */ xx = fsync(Fileno(fsm->wfd)); ` Yes this was rotating DSADI, not SSD. BTW, the right place to patch isn't Fclose(), which is one of the more subtle (or hacked, YMMV) pieces of code in RPM. Instead, do the sync before closing the FD_t in lib/fsm.c (or whatever replaces). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-291632032___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
* added docs * stopped destroying the FDs on sync * changed the name of the macro to be prefixed with an `_` -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-291592668___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
jaymzh commented on this pull request.
> @@ -1116,6 +1116,28 @@ int Fseek(FD_t fd, off_t offset, int whence)
return rc;
}
+int Fsync(FD_t fd)
+{
+int rc = 0, ec = 0;
+
+if (fd == NULL)
+ return -1;
+
+fd = fdLink(fd);
+for (FDSTACK_t fps = fd->fps; fps != NULL; fps = fdPop(fd)) {
Oh, good call. Copy/pasta from Fclose() :)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/187#discussion_r109741866___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
As to the name, the implementation has significant implications, and so I think the name should be kept as-is... -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-291590349___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
jaymzh commented on this pull request.
> @@ -231,6 +232,9 @@ static int expandRegular(rpmfi fi, const char *dest,
> rpmpsm psm, int nodigest, i
exit:
if (wfd) {
int myerrno = errno;
+if (rpmExpandNumeric("%{force_fsync_on_close}")) {
`oneshot` is processed in `processSourceFiles` which won't apply here... as I
imagine everythign in build/ is ... where would be an appropriate place to
check for this and set it once?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/187#discussion_r109741819___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
pmatilai commented on this pull request.
> @@ -1116,6 +1116,28 @@ int Fseek(FD_t fd, off_t offset, int whence)
return rc;
}
+int Fsync(FD_t fd)
+{
+int rc = 0, ec = 0;
+
+if (fd == NULL)
+ return -1;
+
+fd = fdLink(fd);
+for (FDSTACK_t fps = fd->fps; fps != NULL; fps = fdPop(fd)) {
Mm, AFAICS this ends up destroying the fd. Which doesn't perhaps matter for
this particular use, but for a general purpose public API that's not sensical
behavior. Just walk the fps->prev chain instead.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/187#pullrequestreview-30718939___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
pmatilai commented on this pull request.
> @@ -231,6 +232,9 @@ static int expandRegular(rpmfi fi, const char *dest,
> rpmpsm psm, int nodigest, i
exit:
if (wfd) {
int myerrno = errno;
+if (rpmExpandNumeric("%{force_fsync_on_close}")) {
You don't really want to invoke the macro parser once per every single file
within a transaction, more like once per transaction. But passing transaction
config down to this level gets cumbersome, so for this I'd suggest using a
static variable which is initialized once (grep for "oneshot" in build/
directory for an example)
The macro name should start with an underscore, as it's really rpm internal
thing.
Also I wonder if we'd better name this macro for what it tries to achieve
rather than the implementation detail of how exactly it does it - think of
something along the lines of spreading or leveling the IO load, but as a native
speaker you'll probably find some more fitting term for it. That'd leave the
implementation open in case you come up with something even more effective. And
who knows, somebody else in some datacenter might be interested too, but
they'll never find an option called "fsync on close" :)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/187#pullrequestreview-30718097___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
Actually I'm more on the side of questioning the need for a command line switch in the first place. This is a highly special tweak that you'd permanently enable and forget on those systems benefitting from it AIUI, and not something you enable for every third transaction on the system. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-291429491___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
Ah yes, good call @Conan-Kudo - will add that tomorrow. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-291410528___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add macro to force fsync() on close() (#187)
@jaymzh Could you add information to `doc/rpm.8` about the command line option? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/187#issuecomment-291382337___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
