* Re: [PATCH] t7063: work around FreeBSD's lazy mtime update feature
2016-07-30 18:20 ` [PATCH] t7063: work around FreeBSD's lazy mtime update feature Nguyễn Thái Ngọc Duy
@ 2016-07-31 0:15 ` Eric Wong
2016-07-31 1:07 ` Eric Wong
2016-08-01 1:37 ` Torstem Bögershausen
2016-08-03 16:05 ` [PATCH v2] " Nguyễn Thái Ngọc Duy
2 siblings, 1 reply; 24+ messages in thread
From: Eric Wong @ 2016-07-31 0:15 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> Let's start with the commit message of [1] from freebsd.git [2]
>
> Sync timestamp changes for inodes of special files to disk as late
> as possible (when the inode is reclaimed). Temporarily only do
> this if option UFS_LAZYMOD configured and softupdates aren't
> enabled. UFS_LAZYMOD is intentionally left out of
> /sys/conf/options.
>
> This is mainly to avoid almost useless disk i/o on battery powered
> machines. It's silly to write to disk (on the next sync or when
> the inode becomes inactive) just because someone hit a key or
> something wrote to the screen or /dev/null.
>
> PR: 5577 [3]
>
> The short version of that, in the context of t7063, is that when a
> directory is updated, its mtime may be updated later, not
> immediately. This can be shown with a simple command sequence
>
> date; sleep 1; touch abc; rm abc; sleep 10; ls -lTd .
Yikes. I guess FreeBSD doesn't have an in-memory inode cache it
can keep up-to-date without flushing to disk?
> One would expect that the date shown in `ls` would be one second from
> `date`, but it's 10 seconds later. If we put another `ls -lTd .` in
> front of `sleep 10`, then the date of the last `ls` comes as
> expected. The first `ls` somehow forces mtime to be updated.
Fwiw, `stat .` seems to have the same effect as `ls -lTd .`...
> t7063 is really sensitive to directory mtime. When mtime is too "new",
> git code suspects racy timestamps and will not trigger the shortcut in
> untracked cache, in t7063.26 (or t7063.25 before this patch) and
> eventually be detected in t7063.28
>
> We have two options thanks to this special FreeBSD feature:
>
> 1) Stop supporting untracked cache on FreeBSD. Skip t7063 entirely
> when running on FreeBSD
>
> 2) Work around this problem (using the same 'ls' trick) and continue
> to support untracked cache on FreeBSD
>
> I initially wanted to go with 1) because I didn't know the exact
> nature of this feature and feared that it would make untracked cache
> work unreliably, using the cached version when it should not.
>
> Since the behavior of this thing is clearer now. The picture is not
> that bad. If this indeed happens often, untracked cache would assume
> racy condition more often and _fall back_ to non-untracked cache code
> paths. Which means it may be less effective, but it will not show
> wrong things.
>
> This patch goes with option 2.
>
> PS. For those who want to look further in FreeBSD source code, this
> flag is now called IN_LAZYMOD. I can see it's effective in ext2 and
> ufs. zfs is questionable.
>
> [1] 660e6408e6df99a20dacb070c5e7f9739efdf96d
> [2] git://github.com/freebsd/freebsd.git
> [3] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=5577
>
> Reported-by: Eric Wong <e@80x24.org>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> This is only of those commits that commit messages are more important
> than the patch itself. One of the good notes about directory mtime,
> if we start to use it in more places in git.
Thanks, Tested-by: Eric Wong <e@80x24.org>
> t/t7063-status-untracked-cache.sh | 4 ++++
> t/test-lib.sh | 6 ++++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
> index a971884..08fc586 100755
> --- a/t/t7063-status-untracked-cache.sh
> +++ b/t/t7063-status-untracked-cache.sh
> @@ -419,6 +419,10 @@ test_expect_success 'create/modify files, some of which are gitignored' '
> rm base
> '
>
> +test_expect_success FREEBSD 'Work around lazy mtime update' '
> + ls -ld . >/dev/null
> +'
stat . >/dev/null
would be more to the point of what is going on, here. But I
also wonder if untracked cache itself could/should be doing this
internally.
(I'm not familiar with that code, of course)
Thanks again for looking into this.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] t7063: work around FreeBSD's lazy mtime update feature
2016-07-31 0:15 ` Eric Wong
@ 2016-07-31 1:07 ` Eric Wong
2016-07-31 14:30 ` Duy Nguyen
0 siblings, 1 reply; 24+ messages in thread
From: Eric Wong @ 2016-07-31 1:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Nguyễn Thái Ngọc Duy
Eric Wong <e@80x24.org> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> > +test_expect_success FREEBSD 'Work around lazy mtime update' '
> > + ls -ld . >/dev/null
> > +'
>
> stat . >/dev/null
If there's some older FreeBSD w/o stat(1); "test -x ."
ought to work, too, and it's faster being a shell builtin.
I suspect some shell might be clever about optimizing away
a more-obvious "test -d .", so I choose "test -x ."
> would be more to the point of what is going on, here. But I
> also wonder if untracked cache itself could/should be doing this
> internally.
Still wondering :>
> (I'm not familiar with that code, of course)
>
> Thanks again for looking into this.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] t7063: work around FreeBSD's lazy mtime update feature
2016-07-31 1:07 ` Eric Wong
@ 2016-07-31 14:30 ` Duy Nguyen
0 siblings, 0 replies; 24+ messages in thread
From: Duy Nguyen @ 2016-07-31 14:30 UTC (permalink / raw)
To: Eric Wong; +Cc: Junio C Hamano, Git Mailing List
On Sun, Jul 31, 2016 at 3:07 AM, Eric Wong <e@80x24.org> wrote:
>> would be more to the point of what is going on, here. But I
>> also wonder if untracked cache itself could/should be doing this
>> internally.
>
> Still wondering :>
There's nothing we can do besides maybe run a cron job executing
'sync' every second or so. We need to force mtime to be written down
close to.. you know.. mtime. By the time git is executed, it's already
late. You can execute 'sync' inside git then wait a couple seconds..
but that's just stupid. And removing the racy check is even more
dangerous, now you can get false output.
--
Duy
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] t7063: work around FreeBSD's lazy mtime update feature
2016-07-30 18:20 ` [PATCH] t7063: work around FreeBSD's lazy mtime update feature Nguyễn Thái Ngọc Duy
2016-07-31 0:15 ` Eric Wong
@ 2016-08-01 1:37 ` Torstem Bögershausen
2016-08-01 17:10 ` Duy Nguyen
2016-08-03 16:05 ` [PATCH v2] " Nguyễn Thái Ngọc Duy
2 siblings, 1 reply; 24+ messages in thread
From: Torstem Bögershausen @ 2016-08-01 1:37 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git@vger.kernel.org, Junio C Hamano, e@80x24.org
> Am 30.07.2016 um 15:20 schrieb Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
>
> Let's start with the commit message of [1] from freebsd.git [2]
>
> Sync timestamp changes for inodes of special files to disk as late
> as possible (when the inode is reclaimed). Temporarily only do
> this if option UFS_LAZYMOD configured and softupdates aren't
> enabled. UFS_LAZYMOD is intentionally left out of
> /sys/conf/options.
>
> This is mainly to avoid almost useless disk i/o on battery powered
> machines. It's silly to write to disk (on the next sync or when
> the inode becomes inactive) just because someone hit a key or
> something wrote to the screen or /dev/null.
>
> PR: 5577 [3]
>
> The short version of that, in the context of t7063, is that when a
> directory is updated, its mtime may be updated later, not
> immediately. This can be shown with a simple command sequence
>
> date; sleep 1; touch abc; rm abc; sleep 10; ls -lTd .
>
> One would expect that the date shown in `ls` would be one second from
> `date`, but it's 10 seconds later. If we put another `ls -lTd .` in
> front of `sleep 10`, then the date of the last `ls` comes as
> expected. The first `ls` somehow forces mtime to be updated.
>
> t7063 is really sensitive to directory mtime. When mtime is too "new",
> git code suspects racy timestamps and will not trigger the shortcut in
> untracked cache, in t7063.26 (or t7063.25 before this patch) and
> eventually be detected in t7063.28
>
> We have two options thanks to this special FreeBSD feature:
>
> 1) Stop supporting untracked cache on FreeBSD. Skip t7063 entirely
> when running on FreeBSD
>
> 2) Work around this problem (using the same 'ls' trick) and continue
> to support untracked cache on FreeBSD
>
> I initially wanted to go with 1) because I didn't know the exact
> nature of this feature and feared that it would make untracked cache
> work unreliably, using the cached version when it should not.
>
> Since the behavior of this thing is clearer now. The picture is not
> that bad. If this indeed happens often, untracked cache would assume
> racy condition more often and _fall back_ to non-untracked cache code
> paths. Which means it may be less effective, but it will not show
> wrong things.
>
> This patch goes with option 2.
>
> PS. For those who want to look further in FreeBSD source code, this
> flag is now called IN_LAZYMOD. I can see it's effective in ext2 and
> ufs. zfs is questionable.
>
> [1] 660e6408e6df99a20dacb070c5e7f9739efdf96d
> [2] git://github.com/freebsd/freebsd.git
> [3] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=5577
>
> Reported-by: Eric Wong <e@80x24.org>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> This is only of those commits that commit messages are more important
> than the patch itself. One of the good notes about directory mtime,
> if we start to use it in more places in git.
>
> t/t7063-status-untracked-cache.sh | 4 ++++
> t/test-lib.sh | 6 ++++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
> index a971884..08fc586 100755
> --- a/t/t7063-status-untracked-cache.sh
> +++ b/t/t7063-status-untracked-cache.sh
> @@ -419,6 +419,10 @@ test_expect_success 'create/modify files, some of which are gitignored' '
> rm base
> '
>
> +test_expect_success FREEBSD 'Work around lazy mtime update' '
> + ls -ld . >/dev/null
> +'
> +
the term FREEBSD may be too generic to point out a single feature
in an OS distributution.
Following your investigations, it may even be possible that
other systems adapt this "feature"?
How about
LAZY_DIR_MTIME_UPDATE
(or similar)
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] t7063: work around FreeBSD's lazy mtime update feature
2016-08-01 1:37 ` Torstem Bögershausen
@ 2016-08-01 17:10 ` Duy Nguyen
2016-08-01 21:04 ` Junio C Hamano
0 siblings, 1 reply; 24+ messages in thread
From: Duy Nguyen @ 2016-08-01 17:10 UTC (permalink / raw)
To: Torstem Bögershausen
Cc: git@vger.kernel.org, Junio C Hamano, e@80x24.org
On Mon, Aug 1, 2016 at 3:37 AM, Torstem Bögershausen <tboegi@web.de> wrote:
> the term FREEBSD may be too generic to point out a single feature
> in an OS distributution.
> Following your investigations, it may even be possible that
> other systems adapt this "feature"?
>
> How about
> LAZY_DIR_MTIME_UPDATE
> (or similar)
This feature was added in 1998, so yes there's a chance it has spread
to a few fbsd derivatives (not sure if openbsd or netbsd is close
enough and they ever exchange changes). But I'd rather wait for the
second OS to expose the same feature before renaming it.
--
Duy
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] t7063: work around FreeBSD's lazy mtime update feature
2016-08-01 17:10 ` Duy Nguyen
@ 2016-08-01 21:04 ` Junio C Hamano
2016-08-02 15:37 ` Duy Nguyen
0 siblings, 1 reply; 24+ messages in thread
From: Junio C Hamano @ 2016-08-01 21:04 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Torstem Bögershausen, git@vger.kernel.org, e@80x24.org
Duy Nguyen <pclouds@gmail.com> writes:
> On Mon, Aug 1, 2016 at 3:37 AM, Torstem Bögershausen <tboegi@web.de> wrote:
>> the term FREEBSD may be too generic to point out a single feature
>> in an OS distributution.
>> Following your investigations, it may even be possible that
>> other systems adapt this "feature"?
>>
>> How about
>> LAZY_DIR_MTIME_UPDATE
>> (or similar)
>
> This feature was added in 1998, so yes there's a chance it has spread
> to a few fbsd derivatives (not sure if openbsd or netbsd is close
> enough and they ever exchange changes). But I'd rather wait for the
> second OS to expose the same feature before renaming it.
I think a name based on the observed behaviour ("feature") would be
more appropriate because I'd be more worried about us finding other
glitches we see (initially) only on FBSD. People who need to adjust
tests that use the same FBSD prereq would have to wonder which
prereq-skip is due to which glitch.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH] t7063: work around FreeBSD's lazy mtime update feature
2016-08-01 21:04 ` Junio C Hamano
@ 2016-08-02 15:37 ` Duy Nguyen
2016-08-02 17:17 ` Junio C Hamano
0 siblings, 1 reply; 24+ messages in thread
From: Duy Nguyen @ 2016-08-02 15:37 UTC (permalink / raw)
To: Junio C Hamano
Cc: Torstem Bögershausen, git@vger.kernel.org, e@80x24.org
On Mon, Aug 01, 2016 at 02:04:44PM -0700, Junio C Hamano wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
> > On Mon, Aug 1, 2016 at 3:37 AM, Torstem Bögershausen <tboegi@web.de> wrote:
> >> the term FREEBSD may be too generic to point out a single feature
> >> in an OS distributution.
> >> Following your investigations, it may even be possible that
> >> other systems adapt this "feature"?
> >>
> >> How about
> >> LAZY_DIR_MTIME_UPDATE
> >> (or similar)
> >
> > This feature was added in 1998, so yes there's a chance it has spread
> > to a few fbsd derivatives (not sure if openbsd or netbsd is close
> > enough and they ever exchange changes). But I'd rather wait for the
> > second OS to expose the same feature before renaming it.
>
> I think a name based on the observed behaviour ("feature") would be
> more appropriate because I'd be more worried about us finding other
> glitches we see (initially) only on FBSD. People who need to adjust
> tests that use the same FBSD prereq would have to wonder which
> prereq-skip is due to which glitch.
OK how about this squashed in? The name was taken from fbsd definition
IN_LAZYMOD.
Off topic. Since I found this macro defined twice, in ext2 and ufs,
but not in zfs (found its source!), I assume zfs does not have this
particular feature (but I didn't test it). Untracked cache may be more
effecient there.
-- 8< --
diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index 08fc586..8bb048a 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -419,7 +419,7 @@ test_expect_success 'create/modify files, some of which are gitignored' '
rm base
'
-test_expect_success FREEBSD 'Work around lazy mtime update' '
+test_expect_success LAZYMOD 'Work around lazy mtime update' '
ls -ld . >/dev/null
'
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3c730a2..1fc5266 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -962,7 +962,7 @@ case $(uname -s) in
test_set_prereq GREP_STRIPS_CR
;;
*FreeBSD*)
- test_set_prereq FREEBSD
+ test_set_prereq LAZYMOD
test_set_prereq POSIXPERM
test_set_prereq BSLASHPSPEC
test_set_prereq EXECKEEPSPID
-- 8< --
--
Duy
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH] t7063: work around FreeBSD's lazy mtime update feature
2016-08-02 15:37 ` Duy Nguyen
@ 2016-08-02 17:17 ` Junio C Hamano
0 siblings, 0 replies; 24+ messages in thread
From: Junio C Hamano @ 2016-08-02 17:17 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Torstem Bögershausen, git@vger.kernel.org, e@80x24.org
Duy Nguyen <pclouds@gmail.com> writes:
> OK how about this squashed in? The name was taken from fbsd definition
> IN_LAZYMOD.
I am sorry that I didn't spot this possiblity earlier, but do we
need anything conditional? Either FREEBSD or LAZYMOD prerequisite
tells very little what the "Work around lazy mtime update" is and
where it triggers (I think the conclusion of your investigation was
that the timestamp on the containing directory, but that is ONLY
recorded in the log message, i.e. readers need to run 'git blame'
to find out).
It might be a better approach to have a helper function with
descriptive name and comment early in t7063, e.g.
# On some filesystems (e.g. FreeBSD's ext2 and ufs) this
# and that happens when we do blah, which forces the
# untracked cache code to take the slow path. A test
# that wants to make sure the fast path works correctly
# should call this helper to make mtime of the containing
# directory in sync with the reality after doing blah and
# before checking the fast path behaviour
test_sync_directory_mtime () {
ls -ld . >/dev/null
}
and then call it at strategic places without any prerequisite.
The helper may turn out to be useful outside the context of 7063
later, at which time we can move it to test-lib-functions, but that
is a separate step.
> Off topic. Since I found this macro defined twice, in ext2 and ufs,
> but not in zfs (found its source!), I assume zfs does not have this
> particular feature (but I didn't test it). Untracked cache may be more
> effecient there.
> -- 8< --
> diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
> index 08fc586..8bb048a 100755
> --- a/t/t7063-status-untracked-cache.sh
> +++ b/t/t7063-status-untracked-cache.sh
> @@ -419,7 +419,7 @@ test_expect_success 'create/modify files, some of which are gitignored' '
> rm base
> '
>
> -test_expect_success FREEBSD 'Work around lazy mtime update' '
> +test_expect_success LAZYMOD 'Work around lazy mtime update' '
> ls -ld . >/dev/null
> '
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 3c730a2..1fc5266 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -962,7 +962,7 @@ case $(uname -s) in
> test_set_prereq GREP_STRIPS_CR
> ;;
> *FreeBSD*)
> - test_set_prereq FREEBSD
> + test_set_prereq LAZYMOD
> test_set_prereq POSIXPERM
> test_set_prereq BSLASHPSPEC
> test_set_prereq EXECKEEPSPID
> -- 8< --
>
> --
> Duy
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2] t7063: work around FreeBSD's lazy mtime update feature
2016-07-30 18:20 ` [PATCH] t7063: work around FreeBSD's lazy mtime update feature Nguyễn Thái Ngọc Duy
2016-07-31 0:15 ` Eric Wong
2016-08-01 1:37 ` Torstem Bögershausen
@ 2016-08-03 16:05 ` Nguyễn Thái Ngọc Duy
2016-08-03 16:16 ` Junio C Hamano
2016-08-03 17:45 ` [PATCH v3] " Nguyễn Thái Ngọc Duy
2 siblings, 2 replies; 24+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2016-08-03 16:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, e, tboegi, Nguyễn Thái Ngọc Duy
Let's start with the commit message of [1] from freebsd.git [2]
Sync timestamp changes for inodes of special files to disk as late
as possible (when the inode is reclaimed). Temporarily only do
this if option UFS_LAZYMOD configured and softupdates aren't
enabled. UFS_LAZYMOD is intentionally left out of
/sys/conf/options.
This is mainly to avoid almost useless disk i/o on battery powered
machines. It's silly to write to disk (on the next sync or when
the inode becomes inactive) just because someone hit a key or
something wrote to the screen or /dev/null.
PR: 5577 [3]
The short version of that, in the context of t7063, is that when a
directory is updated, its mtime may be updated later, not
immediately. This can be shown with a simple command sequence
date; sleep 1; touch abc; rm abc; sleep 10; ls -lTd .
One would expect that the date shown in `ls` would be one second from
`date`, but it's 10 seconds later. If we put another `ls -lTd .` in
front of `sleep 10`, then the date of the last `ls` comes as
expected. The first `ls` somehow forces mtime to be updated.
t7063 is really sensitive to directory mtime. When mtime is too "new",
git code suspects racy timestamps and will not trigger the shortcut in
untracked cache, in t7063.24 and eventually be detected in t7063.27
We have two options thanks to this special FreeBSD feature:
1) Stop supporting untracked cache on FreeBSD. Skip t7063 entirely
when running on FreeBSD
2) Work around this problem (using the same 'ls' trick) and continue
to support untracked cache on FreeBSD
I initially wanted to go with 1) because I didn't know the exact
nature of this feature and feared that it would make untracked cache
work unreliably, using the cached version when it should not.
Since the behavior of this thing is clearer now. The picture is not
that bad. If this indeed happens often, untracked cache would assume
racy condition more often and _fall back_ to non-untracked cache code
paths. Which means it may be less effective, but it will not show
wrong things.
This patch goes with option 2.
PS. For those who want to look further in FreeBSD source code, this
flag is now called IN_LAZYMOD. I can see it's effective in ext2 and
ufs. zfs is not affected.
[1] 660e6408e6df99a20dacb070c5e7f9739efdf96d
[2] git://github.com/freebsd/freebsd.git
[3] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=5577
Reported-by: Eric Wong <e@80x24.org>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
v2 goes with Junio's suggestion (good one!). And since there is an
intention to reuse this new function, I make sure all directories are
stat'd to pin down their mtime.
t/t7063-status-untracked-cache.sh | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index a971884..7dd4de0 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -4,6 +4,16 @@ test_description='test untracked cache'
. ./test-lib.sh
+# On some filesystems (e.g. FreeBSD's ext2 and ufs) this and that
+# happens when we do blah, which forces the untracked cache code to
+# take the slow path. A test that wants to make sure the fast path
+# works correctly should call this helper to make mtime of the
+# containing directory in sync with the reality after doing blah and
+# before checking the fast path behaviour
+sync_mtime () {
+ find . -type d -exec ls -ld {} \; >/dev/null
+}
+
avoid_racy() {
sleep 1
}
@@ -416,7 +426,8 @@ test_expect_success 'create/modify files, some of which are gitignored' '
echo four >done/four && # four is gitignored at a higher level
echo five >done/five && # five is not gitignored
echo test >base && #we need to ensure that the root dir is touched
- rm base
+ rm base &&
+ sync_mtime
'
test_expect_success 'test sparse status with untracked cache' '
--
2.9.1.566.gbd532d4
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v2] t7063: work around FreeBSD's lazy mtime update feature
2016-08-03 16:05 ` [PATCH v2] " Nguyễn Thái Ngọc Duy
@ 2016-08-03 16:16 ` Junio C Hamano
2016-08-03 16:25 ` Duy Nguyen
2016-08-03 17:45 ` [PATCH v3] " Nguyễn Thái Ngọc Duy
1 sibling, 1 reply; 24+ messages in thread
From: Junio C Hamano @ 2016-08-03 16:16 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, e, tboegi
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> v2 goes with Junio's suggestion (good one!).
Ehh, have you even read what you copied and pasted? "this and that"
and "blah" are meant to be placeholders for you to fill in.
I am not sure if "-exec ls -ld" is a good idea. Doesn't "find" by
itself does enough lstat(2) call to work already? Even if it were
necessary to trigger a separate stat(2) call, wouldn't "-ls" be
sufficient?
> +# On some filesystems (e.g. FreeBSD's ext2 and ufs) this and that
> +# happens when we do blah, which forces the untracked cache code to
> +# take the slow path. A test that wants to make sure the fast path
> +# works correctly should call this helper to make mtime of the
> +# containing directory in sync with the reality after doing blah and
> +# before checking the fast path behaviour
> +sync_mtime () {
> + find . -type d -exec ls -ld {} \; >/dev/null
> +}
> +
> avoid_racy() {
> sleep 1
> }
> @@ -416,7 +426,8 @@ test_expect_success 'create/modify files, some of which are gitignored' '
> echo four >done/four && # four is gitignored at a higher level
> echo five >done/five && # five is not gitignored
> echo test >base && #we need to ensure that the root dir is touched
> - rm base
> + rm base &&
> + sync_mtime
> '
>
> test_expect_success 'test sparse status with untracked cache' '
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2] t7063: work around FreeBSD's lazy mtime update feature
2016-08-03 16:16 ` Junio C Hamano
@ 2016-08-03 16:25 ` Duy Nguyen
0 siblings, 0 replies; 24+ messages in thread
From: Duy Nguyen @ 2016-08-03 16:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Eric Wong, Torsten Bögershausen
On Wed, Aug 3, 2016 at 6:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> v2 goes with Junio's suggestion (good one!).
>
> Ehh, have you even read what you copied and pasted? "this and that"
> and "blah" are meant to be placeholders for you to fill in.
I did. But after a bit of consideration, I think the confusion caused
by "this and that" would force the reader to blame and read the entire
commit message.
> I am not sure if "-exec ls -ld" is a good idea. Doesn't "find" by
> itself does enough lstat(2) call to work already?
Hmm.. because in some cases we could have dtype from readdir()?
> Even if it were
> necessary to trigger a separate stat(2) call, wouldn't "-ls" be
> sufficient?
Yep. Let me run some tests before sending v3.
--
Duy
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3] t7063: work around FreeBSD's lazy mtime update feature
2016-08-03 16:05 ` [PATCH v2] " Nguyễn Thái Ngọc Duy
2016-08-03 16:16 ` Junio C Hamano
@ 2016-08-03 17:45 ` Nguyễn Thái Ngọc Duy
2016-08-03 17:52 ` Junio C Hamano
1 sibling, 1 reply; 24+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2016-08-03 17:45 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, e, tboegi, Nguyễn Thái Ngọc Duy
Let's start with the commit message of [1] from freebsd.git [2]
Sync timestamp changes for inodes of special files to disk as late
as possible (when the inode is reclaimed). Temporarily only do
this if option UFS_LAZYMOD configured and softupdates aren't
enabled. UFS_LAZYMOD is intentionally left out of
/sys/conf/options.
This is mainly to avoid almost useless disk i/o on battery powered
machines. It's silly to write to disk (on the next sync or when
the inode becomes inactive) just because someone hit a key or
something wrote to the screen or /dev/null.
PR: 5577 [3]
The short version of that, in the context of t7063, is that when a
directory is updated, its mtime may be updated later, not
immediately. This can be shown with a simple command sequence
date; sleep 1; touch abc; rm abc; sleep 10; ls -lTd .
One would expect that the date shown in `ls` would be one second from
`date`, but it's 10 seconds later. If we put another `ls -lTd .` in
front of `sleep 10`, then the date of the last `ls` comes as
expected. The first `ls` somehow forces mtime to be updated.
t7063 is really sensitive to directory mtime. When mtime is too "new",
git code suspects racy timestamps and will not trigger the shortcut in
untracked cache, in t7063.24 and eventually be detected in t7063.27
We have two options thanks to this special FreeBSD feature:
1) Stop supporting untracked cache on FreeBSD. Skip t7063 entirely
when running on FreeBSD
2) Work around this problem (using the same 'ls' trick) and continue
to support untracked cache on FreeBSD
I initially wanted to go with 1) because I didn't know the exact
nature of this feature and feared that it would make untracked cache
work unreliably, using the cached version when it should not.
Since the behavior of this thing is clearer now. The picture is not
that bad. If this indeed happens often, untracked cache would assume
racy condition more often and _fall back_ to non-untracked cache code
paths. Which means it may be less effective, but it will not show
wrong things.
This patch goes with option 2.
PS. For those who want to look further in FreeBSD source code, this
flag is now called IN_LAZYMOD. I can see it's effective in ext2 and
ufs. zfs is not affected.
[1] 660e6408e6df99a20dacb070c5e7f9739efdf96d
[2] git://github.com/freebsd/freebsd.git
[3] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=5577
Reported-by: Eric Wong <e@80x24.org>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
v3 differs from v2 in one line
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -11,7 +11,7 @@ test_description='test untracked cache'
# containing directory in sync with the reality after doing blah and
# before checking the fast path behaviour
sync_mtime () {
- find . -type d -exec ls -ld {} \; >/dev/null
+ find . -type d -ls >/dev/null
}
avoid_racy() {
t/t7063-status-untracked-cache.sh | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index a971884..d31d080 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -4,6 +4,16 @@ test_description='test untracked cache'
. ./test-lib.sh
+# On some filesystems (e.g. FreeBSD's ext2 and ufs) this and that
+# happens when we do blah, which forces the untracked cache code to
+# take the slow path. A test that wants to make sure the fast path
+# works correctly should call this helper to make mtime of the
+# containing directory in sync with the reality after doing blah and
+# before checking the fast path behaviour
+sync_mtime () {
+ find . -type d -ls >/dev/null
+}
+
avoid_racy() {
sleep 1
}
@@ -416,7 +426,8 @@ test_expect_success 'create/modify files, some of which are gitignored' '
echo four >done/four && # four is gitignored at a higher level
echo five >done/five && # five is not gitignored
echo test >base && #we need to ensure that the root dir is touched
- rm base
+ rm base &&
+ sync_mtime
'
test_expect_success 'test sparse status with untracked cache' '
--
2.9.1.566.gbd532d4
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v3] t7063: work around FreeBSD's lazy mtime update feature
2016-08-03 17:45 ` [PATCH v3] " Nguyễn Thái Ngọc Duy
@ 2016-08-03 17:52 ` Junio C Hamano
2016-08-03 19:07 ` Junio C Hamano
0 siblings, 1 reply; 24+ messages in thread
From: Junio C Hamano @ 2016-08-03 17:52 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: Git Mailing List, Eric Wong, Torsten Bögershausen
On Wed, Aug 3, 2016 at 10:45 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> v3 differs from v2 in one line
>
> --- a/t/t7063-status-untracked-cache.sh
> +++ b/t/t7063-status-untracked-cache.sh
> @@ -11,7 +11,7 @@ test_description='test untracked cache'
> # containing directory in sync with the reality after doing blah and
> # before checking the fast path behaviour
> sync_mtime () {
> - find . -type d -exec ls -ld {} \; >/dev/null
> + find . -type d -ls >/dev/null
> }
"this and that" and "blah" are still there.
If you mean to tell the user "I won't describe it in detail, if you
really want to know,
go run blame yourself", spell it out like so. I was hoping that you
can summarize
in-line there to help the readers here.
> +# On some filesystems (e.g. FreeBSD's ext2 and ufs) this and that
> +# happens when we do blah, which forces the untracked cache code to
> +# take the slow path. A test that wants to make sure the fast path
> +# works correctly should call this helper to make mtime of the
> +# containing directory in sync with the reality after doing blah and
> +# before checking the fast path behaviour
> +sync_mtime () {
> + find . -type d -ls >/dev/null
> +}
> +
> avoid_racy() {
> sleep 1
> }
> @@ -416,7 +426,8 @@ test_expect_success 'create/modify files, some of which are gitignored' '
> echo four >done/four && # four is gitignored at a higher level
> echo five >done/five && # five is not gitignored
> echo test >base && #we need to ensure that the root dir is touched
> - rm base
> + rm base &&
> + sync_mtime
> '
>
> test_expect_success 'test sparse status with untracked cache' '
> --
> 2.9.1.566.gbd532d4
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3] t7063: work around FreeBSD's lazy mtime update feature
2016-08-03 17:52 ` Junio C Hamano
@ 2016-08-03 19:07 ` Junio C Hamano
2016-08-04 15:46 ` Duy Nguyen
0 siblings, 1 reply; 24+ messages in thread
From: Junio C Hamano @ 2016-08-03 19:07 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: Git Mailing List, Eric Wong, Torsten Bögershausen
Junio C Hamano <gitster@pobox.com> writes:
> If you mean to tell the user "I won't describe it in detail, if you
> really want to know,
> go run blame yourself", spell it out like so. I was hoping that you
> can summarize
> in-line there to help the readers here.
Here is a proposed fixup.
t/t7063-status-untracked-cache.sh | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index d31d080..e0a8228 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -4,12 +4,16 @@ test_description='test untracked cache'
. ./test-lib.sh
-# On some filesystems (e.g. FreeBSD's ext2 and ufs) this and that
-# happens when we do blah, which forces the untracked cache code to
-# take the slow path. A test that wants to make sure the fast path
-# works correctly should call this helper to make mtime of the
-# containing directory in sync with the reality after doing blah and
-# before checking the fast path behaviour
+# On some filesystems (e.g. FreeBSD's ext2 and ufs) directory mtime
+# is updated lazily after contents in the directory changes, which
+# forces the untracked cache code to take the slow path. A test
+# that wants to make sure that the fast path works correctly should
+# call this helper to make mtime of the containing directory in sync
+# with the reality before checking the fast path behaviour.
+#
+# See <20160803174522.5571-1-pclouds@gmail.com> if you want to know
+# more.
+
sync_mtime () {
find . -type d -ls >/dev/null
}
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v3] t7063: work around FreeBSD's lazy mtime update feature
2016-08-03 19:07 ` Junio C Hamano
@ 2016-08-04 15:46 ` Duy Nguyen
0 siblings, 0 replies; 24+ messages in thread
From: Duy Nguyen @ 2016-08-04 15:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Eric Wong, Torsten Bögershausen
On Wed, Aug 3, 2016 at 9:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> If you mean to tell the user "I won't describe it in detail, if you
>> really want to know,
>> go run blame yourself", spell it out like so. I was hoping that you
>> can summarize
>> in-line there to help the readers here.
>
> Here is a proposed fixup.
Great! Sorry I only have one or two hours these days and could not
propose something else quicker.
> t/t7063-status-untracked-cache.sh | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
> index d31d080..e0a8228 100755
> --- a/t/t7063-status-untracked-cache.sh
> +++ b/t/t7063-status-untracked-cache.sh
> @@ -4,12 +4,16 @@ test_description='test untracked cache'
>
> . ./test-lib.sh
>
> -# On some filesystems (e.g. FreeBSD's ext2 and ufs) this and that
> -# happens when we do blah, which forces the untracked cache code to
> -# take the slow path. A test that wants to make sure the fast path
> -# works correctly should call this helper to make mtime of the
> -# containing directory in sync with the reality after doing blah and
> -# before checking the fast path behaviour
> +# On some filesystems (e.g. FreeBSD's ext2 and ufs) directory mtime
> +# is updated lazily after contents in the directory changes, which
> +# forces the untracked cache code to take the slow path. A test
> +# that wants to make sure that the fast path works correctly should
> +# call this helper to make mtime of the containing directory in sync
> +# with the reality before checking the fast path behaviour.
> +#
> +# See <20160803174522.5571-1-pclouds@gmail.com> if you want to know
> +# more.
> +
> sync_mtime () {
> find . -type d -ls >/dev/null
> }
--
Duy
^ permalink raw reply [flat|nested] 24+ messages in thread