Bug 25535 - Cache file does not get updated when files are added or removed to directory on FAT
Summary: Cache file does not get updated when files are added or removed to directory ...
Status: RESOLVED FIXED
Alias: None
Product: fontconfig
Classification: Unclassified
Component: library (show other bugs)
Version: 2.7
Hardware: Other All
: medium normal
Assignee: Akira TAGOH
QA Contact: Behdad Esfahbod
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-09 01:55 UTC by Mikhail Gusarov
Modified: 2012-05-28 00:51 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[patch] Fix cache aging for fonts on FAT (6.74 KB, patch)
2009-12-20 10:11 UTC, Mikhail Gusarov
Details | Splinter Review
Fix cache aging for fonts on FAT [v2] (14.57 KB, patch)
2011-02-22 10:18 UTC, Mikhail Gusarov
Details | Splinter Review
Add new entries to .gitignore (1.07 KB, patch)
2011-02-22 10:19 UTC, Mikhail Gusarov
Details | Splinter Review
Move-FcStat-to-separate-compilation-unit (8.56 KB, patch)
2012-04-29 05:14 UTC, Mikhail Gusarov
Details | Splinter Review
Fix-cache-aging-for-fonts-on-FAT-filesystem-under-Linux (6.95 KB, patch)
2012-04-29 05:15 UTC, Mikhail Gusarov
Details | Splinter Review
0001-Move-statfs-statvfs-wrapper-to-fcstat.c-and-add-a-te.patch (8.56 KB, patch)
2012-05-12 16:24 UTC, Mikhail Gusarov
Details | Splinter Review
0002-Fix-cache-aging-for-fonts-on-FAT-filesystem-under-Li.patch (6.95 KB, patch)
2012-05-12 16:24 UTC, Mikhail Gusarov
Details | Splinter Review
0001-Move-statfs-statvfs-wrapper-to-fcstat.c-and-add-a-te.patch (8.56 KB, patch)
2012-05-12 16:27 UTC, Mikhail Gusarov
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Gusarov 2009-12-09 01:55:39 UTC
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
Comment 1 Mikhail Gusarov 2009-12-20 10:11:21 UTC
Created attachment 32209 [details] [review]
[patch] Fix cache aging for fonts on FAT
Comment 2 Mikhail Gusarov 2010-02-18 05:54:33 UTC
*ping*
Comment 3 Behdad Esfahbod 2010-02-23 13:08:55 UTC
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?
Comment 4 Mikhail Gusarov 2010-12-31 08:12:29 UTC
*ping*

See http://lists.freedesktop.org/archives/fontconfig/2010-March/003396.html
Comment 5 Behdad Esfahbod 2011-01-05 09:38:22 UTC
Using alphasort() is wrong since it uses strcoll and hence is locale-dependent and not stable.  Use a strcmp()-based sort routine.
Comment 6 Behdad Esfahbod 2011-01-05 09:40:44 UTC
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.
Comment 7 Mikhail Gusarov 2011-01-05 09:48:08 UTC
Ack, going to work on updated patch.
Comment 8 Behdad Esfahbod 2011-01-05 09:56:19 UTC
Thanks.
Comment 9 Mikhail Gusarov 2011-02-22 10:18:44 UTC
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.
Comment 10 Mikhail Gusarov 2011-02-22 10:19:36 UTC
Created attachment 43672 [details] [review]
Add new entries to .gitignore

A few new .gitignore entries found out during developing the previous patch.
Comment 11 Mikhail Gusarov 2011-02-22 10:21:16 UTC
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.
Comment 12 Mikhail Gusarov 2011-03-28 07:10:16 UTC
*ping*
Comment 13 Behdad Esfahbod 2011-04-12 19:21:22 UTC
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.
Comment 14 Akira TAGOH 2012-04-09 00:19:59 UTC
Does this still persist?
Comment 15 Mikhail Gusarov 2012-04-09 01:16:08 UTC
Sure it does. The problem still exists, and patch is not yet applied.
Comment 16 Akira TAGOH 2012-04-09 01:29:47 UTC
(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.
Comment 17 Mikhail Gusarov 2012-04-09 01:43:24 UTC
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.
Comment 18 Mikhail Gusarov 2012-04-29 05:14:48 UTC
Created attachment 60763 [details] [review]
Move-FcStat-to-separate-compilation-unit
Comment 19 Mikhail Gusarov 2012-04-29 05:15:11 UTC
Created attachment 60764 [details] [review]
Fix-cache-aging-for-fonts-on-FAT-filesystem-under-Linux
Comment 20 Mikhail Gusarov 2012-04-29 05:15:24 UTC
Updated patchset is attached.
Comment 21 Akira TAGOH 2012-05-01 01:00:38 UTC
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.
Comment 22 Mikhail Gusarov 2012-05-01 02:46:35 UTC
(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.
Comment 23 Akira TAGOH 2012-05-01 04:30:12 UTC
(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.
Comment 24 Mikhail Gusarov 2012-05-01 04:36:19 UTC
(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.
Comment 25 Akira TAGOH 2012-05-01 19:28:08 UTC
Okay. got it.
Comment 26 Mikhail Gusarov 2012-05-12 16:24:01 UTC
Created attachment 61532 [details] [review]
0001-Move-statfs-statvfs-wrapper-to-fcstat.c-and-add-a-te.patch
Comment 27 Mikhail Gusarov 2012-05-12 16:24:26 UTC
Created attachment 61533 [details] [review]
0002-Fix-cache-aging-for-fonts-on-FAT-filesystem-under-Li.patch
Comment 28 Mikhail Gusarov 2012-05-12 16:25:34 UTC
Updated patch is attached. fcstat is so convoluted to avoid combinatorial explosion of #ifdefs for new checks.
Comment 29 Mikhail Gusarov 2012-05-12 16:27:29 UTC
Created attachment 61534 [details] [review]
0001-Move-statfs-statvfs-wrapper-to-fcstat.c-and-add-a-te.patch
Comment 30 Mikhail Gusarov 2012-05-12 16:32:26 UTC
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/
Comment 31 Akira TAGOH 2012-05-13 19:51:32 UTC
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.
Comment 32 Akira TAGOH 2012-05-17 19:33:21 UTC
BTW I'm planning to have new release next month and want to contain this fix if possible. please update the patch.
Comment 33 Mikhail Gusarov 2012-05-27 04:03:36 UTC
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?
Comment 34 Akira TAGOH 2012-05-27 05:47:52 UTC
(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.
Comment 35 Akira TAGOH 2012-05-28 00:09:48 UTC
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.
Comment 36 Mikhail Gusarov 2012-05-28 00:26:10 UTC
(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.
Comment 37 Mikhail Gusarov 2012-05-28 00:26:46 UTC
(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.
Comment 38 Akira TAGOH 2012-05-28 00:51:58 UTC
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.