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
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:
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:
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
@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
<--
@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
@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
@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
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
@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
@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
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:
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___
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:
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
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
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
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:
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
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
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
***
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
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
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
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
@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
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
@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
@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
@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`
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)
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)
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)
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:
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
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).
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
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 &&
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
@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
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
* 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:
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
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:
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
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;
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
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
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:
@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:
50 matches
Mail list logo
