Windows does not update mtime of directory when file is added or removed from directory on FAT filesystem, so fontconfig does not rebuild cache as it checks only mtime and size of directory. This leads to stale caches. Fix is discussed in ML thread: http://lists.freedesktop.org/archives/fontconfig/2009-December/003315.html
Created attachment 32209 [details] [review] [patch] Fix cache aging for fonts on FAT
*ping*
This changes the cache format on FAT filesystems. Not a huge deal, but I'd rather you just emulate correct mtime instead of using a hash. That would require stating all the files in the directory and taking the max mtime I guess?
*ping* See http://lists.freedesktop.org/archives/fontconfig/2010-March/003396.html
Using alphasort() is wrong since it uses strcoll and hence is locale-dependent and not stable. Use a strcmp()-based sort routine.
Also, as you can see there's already some win32-specific mess in FcStat(). Lets just move FcStat() to fcstat.c and add your magic to the same function.
Ack, going to work on updated patch.
Thanks.
Created attachment 43671 [details] [review] Fix cache aging for fonts on FAT [v2] Attached is a new version of patch with all issues mentioned above resolved and fixed all bugs found in meantime.
Created attachment 43672 [details] [review] Add new entries to .gitignore A few new .gitignore entries found out during developing the previous patch.
I did not merge FcStatChecksum into FcStat as there are several places in code which call FcStat and don't care about mtime, so no overhead is incurred.
I'm wondering: isn't using the stat structure to convey the checksum a bad idea? Not sure. The code sure looks fine since it always calls stat, then adjusts the checksum. Not sure. I've changed FcStat a bit in the mean time, so if you can update to master while, it would be great.
Does this still persist?
Sure it does. The problem still exists, and patch is not yet applied.
(In reply to comment #15) > Sure it does. The problem still exists, and patch is not yet applied. in 2.9.0? and would be nice if you can have a comment to comment#13.
Checked git master. Carrying checksum in stat struct is a small but very convenient hack: otherwise it would require creating another structure, carrying stat and checksum, which is only needed if fonts directory is on FAT filesystem. Will rebase patch against git master ASAP.
Created attachment 60763 [details] [review] Move-FcStat-to-separate-compilation-unit
Created attachment 60764 [details] [review] Fix-cache-aging-for-fonts-on-FAT-filesystem-under-Linux
Updated patchset is attached.
Comment on attachment 60764 [details] [review] Fix-cache-aging-for-fonts-on-FAT-filesystem-under-Linux Review of attachment 60764 [details] [review]: ----------------------------------------------------------------- I was presuming that this bug is targeted Windows platform but your patch looks like just targeting FAT filesystem regardless of the platforms. so current code of FcStat() works on Windows but not on other platforms that support FAT filesystem? ::: src/fcstat.c @@ +33,1 @@ > I don't think requiring the linux kernel header files are a good idea. we could use the hardcode magic number instead, so far. @@ +132,5 @@ > + > + if (FcDebug () & FC_DBG_CACHE) > + printf ("FcFsMtimeBroken %s\n", file); > + > + ret = statfs ((const char *)file, &fs_stat); FcFsMtimeBroken() can be revised according to the recent changes in master. that supports multiple platforms for statv?fs() but not only for Linux. we may want to have FcStatFS() to wrap up.
(In reply to comment #21) > I was presuming that this bug is targeted Windows platform but your patch looks > like just targeting FAT filesystem regardless of the platforms. That's right. > so current code > of FcStat() works on Windows but not on other platforms that support FAT > filesystem? Not sure how whether works on Windows at all, given it special-cased (and I don't have Windows to test against at the moment). It might be broken on there as well. > I don't think requiring the linux kernel header files are a good idea. we could > use the hardcode magic number instead, so far. Okay. > FcFsMtimeBroken() can be revised according to the recent changes in master. > that supports multiple platforms for statv?fs() but not only for Linux. we may > want to have FcStatFS() to wrap up. Will do.
(In reply to comment #22) > Not sure how whether works on Windows at all, given it special-cased (and I > don't have Windows to test against at the moment). It might be broken on there > as well. Well, the question is, then how better it is than current specialized FcStat() for Windows. it do more than stat() because it's broken. but if your patch works enough on Windows as well, it may be duplicate to accomplish this purpose.
(In reply to comment #23) > Well, the question is, then how better it is than current specialized FcStat() > for Windows. stat() on Windows is broken according to the comment, so Windows has to use FindFirstFile. stan() on FAT on non-Windows is broken due to mtime handling on FAT, so it has to be worked around. Hence both codepaths have to stay.
Okay. got it.
Created attachment 61532 [details] [review] 0001-Move-statfs-statvfs-wrapper-to-fcstat.c-and-add-a-te.patch
Created attachment 61533 [details] [review] 0002-Fix-cache-aging-for-fonts-on-FAT-filesystem-under-Li.patch
Updated patch is attached. fcstat is so convoluted to avoid combinatorial explosion of #ifdefs for new checks.
Created attachment 61534 [details] [review] 0001-Move-statfs-statvfs-wrapper-to-fcstat.c-and-add-a-te.patch
Uhm. Due to some reason Bugzilla shows first patch (Move-FcStat-...) instead of freshly attached (0001-Move-statfs-statvfs-...). Weird. Bad Bugzilla! Here's the git repository with patches applied: http://git.dottedmag.net/fontconfig.git/
Thanks for the patch. some comments on them: 1. sys/param.h is required to use fstatfs() on BSD. 2. changing the order of ifdef affects its usage. struct statfs/statvfs is so incompatible among platforms. 3. fstatvfs() is preferred rather than fstatfs() on Solaris. so this patch may not works properly on Solaris. 4. there are typos. HAVE_STATFS/HAVE_STATVFS should be HAVE_FSTATFS/HAVE_FSTATVFS.
BTW I'm planning to have new release next month and want to contain this fix if possible. please update the patch.
This becomes quite hairy: Linux has statvfs(3), but struct statvfs does not have a field with FS type, so under Linux statfs(3) is preferred, opposite to Solaris. I think I'll resort to long list of #if defined(<ONE_OS>) ... #elif defineD(<ANOTHER OS>) .... What do you think about it?
(In reply to comment #33) > This becomes quite hairy: Linux has statvfs(3), but struct statvfs does not > have a field with FS type, so under Linux statfs(3) is preferred, opposite to > Solaris. Right. that's why I did make a conditional with both of HAVE_FSTATVFS and HAVE_STRUCT_STATVFS_F_BASETYPE say to get it working properly. > I think I'll resort to long list of #if defined(<ONE_OS>) ... #elif > defineD(<ANOTHER OS>) .... What do you think about it? I'd get rid of the platform dependent ifdefs as far as possible, because it causes a trouble when adding additional platform support. also that may has a duplicate code. though we have __linux__ in the code. it's the kind of exception. the magic code of the filesystem is the platform dependent and there are no way to determine the filesystem type from the string in Linux AFAIK. Anyway, I'll think about it more.
Okay, reworked: http://cgit.freedesktop.org/~tagoh/fontconfig/log/?h=bz25535 the main changes is: http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=bz25535&id=6d699869ab9649f51e9240add6e4573c7394df85 BTW have you tried to create the cache files on FAT filesystem? just found and fixed in master though, there seems a bug in FcAtomicLock() and it always fails if link() is available and run under FAT filesystem. this prevented to create a cache file anyway.
(In reply to comment #35) > Okay, reworked: > http://cgit.freedesktop.org/~tagoh/fontconfig/log/?h=bz25535 > > the main changes is: > http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=bz25535&id=6d699869ab9649f51e9240add6e4573c7394df85 Looks good. Feel free to add Signed-off-by: Mikhail Gusarov <dottedmag@dottedmag.net> if you like to squash commits together. > BTW have you tried to create the cache files on FAT filesystem? Didn't try it: in my situation fonts were on FAT, but cache files were on jffs2 or ubifs.
(In reply to comment #36) > Looks good. Feel free to add Signed-off-by: Mikhail Gusarov > <dottedmag@dottedmag.net> if you like to squash commits together. Oh, they are already, misread the patch.
Thanks. merged into master.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.