[PATCH] staging: lustre: ldlm: pl_recalc time handling is wrong

[PATCH] staging: lustre: ldlm: pl_recalc time handling is wrong

[James Simmons]

The ldlm_pool field pl_recalc_time is set to the current
monotonic clock value but the interval period is calculated
with the wall clock. This means the interval period will
always be far larger than the pl_recalc_period, which is
just a small interval time period. The correct thing to
do is to use monotomic clock current value instead of the
wall clocks value when calculating recalc_interval_sec.

Signed-off-by: James Simmons <jsimmons@infradead.org>
---
 drivers/staging/lustre/lustre/ldlm/ldlm_pool.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
index 19831c5..30d4f80 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
@@ -256,7 +256,7 @@ static int ldlm_cli_pool_recalc(struct ldlm_pool *pl)
 	time64_t recalc_interval_sec;
 	int ret;
 
-	recalc_interval_sec = ktime_get_real_seconds() - pl->pl_recalc_time;
+	recalc_interval_sec = ktime_get_seconds() - pl->pl_recalc_time;
 	if (recalc_interval_sec < pl->pl_recalc_period)
 		return 0;
 
@@ -264,7 +264,7 @@ static int ldlm_cli_pool_recalc(struct ldlm_pool *pl)
 	/*
 	 * Check if we need to recalc lists now.
 	 */
-	recalc_interval_sec = ktime_get_real_seconds() - pl->pl_recalc_time;
+	recalc_interval_sec = ktime_get_seconds() - pl->pl_recalc_time;
 	if (recalc_interval_sec < pl->pl_recalc_period) {
 		spin_unlock(&pl->pl_lock);
 		return 0;
@@ -301,7 +301,7 @@ static int ldlm_cli_pool_recalc(struct ldlm_pool *pl)
 	 * Time of LRU resizing might be longer than period,
 	 * so update after LRU resizing rather than before it.
 	 */
-	pl->pl_recalc_time = ktime_get_real_seconds();
+	pl->pl_recalc_time = ktime_get_seconds();
 	lprocfs_counter_add(pl->pl_stats, LDLM_POOL_TIMING_STAT,
 			    recalc_interval_sec);
 	spin_unlock(&pl->pl_lock);
-- 
1.7.1

Re: [lustre-devel] [PATCH] staging: lustre: ldlm: pl_recalc time handling is wrong

[Dilger, Andreas]

On Nov 7, 2016, at 19:47, James Simmons <jsimmons@infradead.org> wrote:
> 
> The ldlm_pool field pl_recalc_time is set to the current
> monotonic clock value but the interval period is calculated
> with the wall clock. This means the interval period will
> always be far larger than the pl_recalc_period, which is
> just a small interval time period. The correct thing to
> do is to use monotomic clock current value instead of the
> wall clocks value when calculating recalc_interval_sec.

It looks like this was introduced by commit 8f83409cf
"staging/lustre: use 64-bit time for pl_recalc" but that patch changed
get_seconds() to a mix of ktime_get_seconds() and ktime_get_real_seconds()
for an unknown reason.  It doesn't appear that there is any difference
in overhead between the two (on 64-bit at least).

Since the ldlm pool recalculation interval is actually driven in response to
load on the server, it makes sense to use the "real" time instead of the
monotonic time (if I understand correctly) if the client is in a VM that
may periodically be blocked and "miss time" compared to the outside world.
Using the "real" clock, the recalc_interval_sec will correctly reflect the
actual elapsed time rather than just the number of ticks inside the VM.

Is my understanding of these different clocks correct?

Cheers, Andreas

> 
> Signed-off-by: James Simmons <jsimmons@infradead.org>
> ---
> drivers/staging/lustre/lustre/ldlm/ldlm_pool.c |    6 +++---
> 1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
> index 19831c5..30d4f80 100644
> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
> @@ -256,7 +256,7 @@ static int ldlm_cli_pool_recalc(struct ldlm_pool *pl)
> 	time64_t recalc_interval_sec;
> 	int ret;
> 
> -	recalc_interval_sec = ktime_get_real_seconds() - pl->pl_recalc_time;
> +	recalc_interval_sec = ktime_get_seconds() - pl->pl_recalc_time;
> 	if (recalc_interval_sec < pl->pl_recalc_period)
> 		return 0;
> 
> @@ -264,7 +264,7 @@ static int ldlm_cli_pool_recalc(struct ldlm_pool *pl)
> 	/*
> 	 * Check if we need to recalc lists now.
> 	 */
> -	recalc_interval_sec = ktime_get_real_seconds() - pl->pl_recalc_time;
> +	recalc_interval_sec = ktime_get_seconds() - pl->pl_recalc_time;
> 	if (recalc_interval_sec < pl->pl_recalc_period) {
> 		spin_unlock(&pl->pl_lock);
> 		return 0;
> @@ -301,7 +301,7 @@ static int ldlm_cli_pool_recalc(struct ldlm_pool *pl)
> 	 * Time of LRU resizing might be longer than period,
> 	 * so update after LRU resizing rather than before it.
> 	 */
> -	pl->pl_recalc_time = ktime_get_real_seconds();
> +	pl->pl_recalc_time = ktime_get_seconds();
> 	lprocfs_counter_add(pl->pl_stats, LDLM_POOL_TIMING_STAT,
> 			    recalc_interval_sec);
> 	spin_unlock(&pl->pl_lock);
> -- 
> 1.7.1
> 
> _______________________________________________
> lustre-devel mailing list
> lustre-devel@lists.lustre.org
> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org

Re: [lustre-devel] [PATCH] staging: lustre: ldlm: pl_recalc time handling is wrong

[Arnd Bergmann]

On Wednesday, November 9, 2016 3:50:29 AM CET Dilger, Andreas wrote:
> On Nov 7, 2016, at 19:47, James Simmons <jsimmons@infradead.org> wrote:
> > 
> > The ldlm_pool field pl_recalc_time is set to the current
> > monotonic clock value but the interval period is calculated
> > with the wall clock. This means the interval period will
> > always be far larger than the pl_recalc_period, which is
> > just a small interval time period. The correct thing to
> > do is to use monotomic clock current value instead of the
> > wall clocks value when calculating recalc_interval_sec.
> 
> It looks like this was introduced by commit 8f83409cf
> "staging/lustre: use 64-bit time for pl_recalc" but that patch changed
> get_seconds() to a mix of ktime_get_seconds() and ktime_get_real_seconds()
> for an unknown reason.  It doesn't appear that there is any difference
> in overhead between the two (on 64-bit at least).

It was meant to use ktime_get_real_seconds() consistently, very sorry
about the mistake. I don't remember exactly how we got there, I assume
I had started out using ktime_get_seconds() and then moved to
ktime_get_real_seconds() later but missed the last three instances.

> Since the ldlm pool recalculation interval is actually driven in response to
> load on the server, it makes sense to use the "real" time instead of the
> monotonic time (if I understand correctly) if the client is in a VM that
> may periodically be blocked and "miss time" compared to the outside world.
> Using the "real" clock, the recalc_interval_sec will correctly reflect the
> actual elapsed time rather than just the number of ticks inside the VM.
> 
> Is my understanding of these different clocks correct?

No, this is not the difference: monotonic and real time always tick
at exactly the same rate, the only difference is the starting point.
monotonic time starts at boot and can not be adjusted, while real
time is set to reflect the UTC time base and gets initialized
from the real time clock at boot, or using settimeofday(2) or
NTP later on.

In my changelog text, I wrote

    keeping the 'real' instead of 'monotonic' time because of the
    debug prints.

the intention here is simply to have the console log keep the
same numbers as "date +%s" for absolute values. The patch that
James suggested converting everything to ktime_get_seconds()
would result in the same intervals that have always been there
(until I broke it by using time domains inconsistently), but
would mean we could use a u32 type for pl_recalc_time and
pl_recalc_period because that doesn't overflow until 136 years
after booting. (signed time_t overflows 68 years after 1970,
i.e 2038).

	Arnd

Re: [lustre-devel] [PATCH] staging: lustre: ldlm: pl_recalc time handling is wrong

[Greg Kroah-Hartman]

On Wed, Nov 09, 2016 at 05:00:42PM +0100, Arnd Bergmann wrote:
> On Wednesday, November 9, 2016 3:50:29 AM CET Dilger, Andreas wrote:
> > On Nov 7, 2016, at 19:47, James Simmons <jsimmons@infradead.org> wrote:
> > > 
> > > The ldlm_pool field pl_recalc_time is set to the current
> > > monotonic clock value but the interval period is calculated
> > > with the wall clock. This means the interval period will
> > > always be far larger than the pl_recalc_period, which is
> > > just a small interval time period. The correct thing to
> > > do is to use monotomic clock current value instead of the
> > > wall clocks value when calculating recalc_interval_sec.
> > 
> > It looks like this was introduced by commit 8f83409cf
> > "staging/lustre: use 64-bit time for pl_recalc" but that patch changed
> > get_seconds() to a mix of ktime_get_seconds() and ktime_get_real_seconds()
> > for an unknown reason.  It doesn't appear that there is any difference
> > in overhead between the two (on 64-bit at least).
> 
> It was meant to use ktime_get_real_seconds() consistently, very sorry
> about the mistake. I don't remember exactly how we got there, I assume
> I had started out using ktime_get_seconds() and then moved to
> ktime_get_real_seconds() later but missed the last three instances.
> 
> > Since the ldlm pool recalculation interval is actually driven in response to
> > load on the server, it makes sense to use the "real" time instead of the
> > monotonic time (if I understand correctly) if the client is in a VM that
> > may periodically be blocked and "miss time" compared to the outside world.
> > Using the "real" clock, the recalc_interval_sec will correctly reflect the
> > actual elapsed time rather than just the number of ticks inside the VM.
> > 
> > Is my understanding of these different clocks correct?
> 
> No, this is not the difference: monotonic and real time always tick
> at exactly the same rate, the only difference is the starting point.
> monotonic time starts at boot and can not be adjusted, while real
> time is set to reflect the UTC time base and gets initialized
> from the real time clock at boot, or using settimeofday(2) or
> NTP later on.
> 
> In my changelog text, I wrote
> 
>     keeping the 'real' instead of 'monotonic' time because of the
>     debug prints.
> 
> the intention here is simply to have the console log keep the
> same numbers as "date +%s" for absolute values. The patch that
> James suggested converting everything to ktime_get_seconds()
> would result in the same intervals that have always been there
> (until I broke it by using time domains inconsistently), but
> would mean we could use a u32 type for pl_recalc_time and
> pl_recalc_period because that doesn't overflow until 136 years
> after booting. (signed time_t overflows 68 years after 1970,
> i.e 2038).

So, is this patch correct and should be merged to the tree, or not?

confused,

greg k-h

Re: [lustre-devel] [PATCH] staging: lustre: ldlm: pl_recalc time handling is wrong

[Arnd Bergmann]

On Thursday, November 10, 2016 1:21:08 PM CET Greg Kroah-Hartman wrote:
> > 
> > the intention here is simply to have the console log keep the
> > same numbers as "date +%s" for absolute values. The patch that
> > James suggested converting everything to ktime_get_seconds()
> > would result in the same intervals that have always been there
> > (until I broke it by using time domains inconsistently), but
> > would mean we could use a u32 type for pl_recalc_time and
> > pl_recalc_period because that doesn't overflow until 136 years
> > after booting. (signed time_t overflows 68 years after 1970,
> > i.e 2038).
> 
> So, is this patch correct and should be merged to the tree, or not?
> 

No, I think it's wrong in a different way as before. After my
patch, there were six instances of ktime_get_real_seconds() and
three instances of ktime_get_seconds(), and that was wrong.

James's patch converts three of the six instances to
ktime_get_seconds(), which is equally wrong, just different.

We can either convert the six ktime_get_real_seconds()
to ktime_get_seconds(), or convert the four (one was added
in the meantime) ktime_get_seconds() to ktime_get_real_seconds().

I'll follow up with a patch for the latter as it is what I
had originally intended. We can also do what James intended
instead.

	Arnd

[PATCH v2] staging: lustre: ldlm: pl_recalc time handling is wrong

[Arnd Bergmann]

James Simmons reports:
> The ldlm_pool field pl_recalc_time is set to the current
> monotonic clock value but the interval period is calculated
> with the wall clock. This means the interval period will
> always be far larger than the pl_recalc_period, which is
> just a small interval time period. The correct thing to
> do is to use monotomic clock current value instead of the
> wall clocks value when calculating recalc_interval_sec.

This broke when I converted the 32-bit get_seconds() into
ktime_get_{real_,}seconds() inconsistently. Either
one of those two would have worked, but mixing them
does not.

Staying with the original intention of the patch, this
changes the ktime_get_seconds() calls into ktime_get_real_seconds(),
using real time instead of mononic time.

Cc: stable@vger.kernel.org # v4.4+
Fixes: 8f83409cf238 ("staging/lustre: use 64-bit time for pl_recalc")
Reported-by: James Simmons <jsimmons@infradead.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v2: James' patch was similarly incomplete to mine, as it only
addressed some of the calls. With this new version, all ktime
accessors use the same time domain.

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
index 19831c555c49..b820309d70e3 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
@@ -356,10 +356,10 @@ static int ldlm_pool_recalc(struct ldlm_pool *pl)
 	u32 recalc_interval_sec;
 	int count;
 
-	recalc_interval_sec = ktime_get_seconds() - pl->pl_recalc_time;
+	recalc_interval_sec = ktime_get_real_seconds() - pl->pl_recalc_time;
 	if (recalc_interval_sec > 0) {
 		spin_lock(&pl->pl_lock);
-		recalc_interval_sec = ktime_get_seconds() - pl->pl_recalc_time;
+		recalc_interval_sec = ktime_get_real_seconds() - pl->pl_recalc_time;
 
 		if (recalc_interval_sec > 0) {
 			/*
@@ -382,7 +382,7 @@ static int ldlm_pool_recalc(struct ldlm_pool *pl)
 				    count);
 	}
 
-	recalc_interval_sec = pl->pl_recalc_time - ktime_get_seconds() +
+	recalc_interval_sec = pl->pl_recalc_time - ktime_get_real_seconds() +
 			      pl->pl_recalc_period;
 	if (recalc_interval_sec <= 0) {
 		/* DEBUG: should be re-removed after LU-4536 is fixed */
@@ -657,7 +657,7 @@ int ldlm_pool_init(struct ldlm_pool *pl, struct ldlm_namespace *ns,
 
 	spin_lock_init(&pl->pl_lock);
 	atomic_set(&pl->pl_granted, 0);
-	pl->pl_recalc_time = ktime_get_seconds();
+	pl->pl_recalc_time = ktime_get_real_seconds();
 	atomic_set(&pl->pl_lock_volume_factor, 1);
 
 	atomic_set(&pl->pl_grant_rate, 0);

Re: [lustre-devel] [PATCH] staging: lustre: ldlm: pl_recalc time handling is wrong

[James Simmons]

> On Thursday, November 10, 2016 1:21:08 PM CET Greg Kroah-Hartman wrote:
> > > 
> > > the intention here is simply to have the console log keep the
> > > same numbers as "date +%s" for absolute values. The patch that
> > > James suggested converting everything to ktime_get_seconds()
> > > would result in the same intervals that have always been there
> > > (until I broke it by using time domains inconsistently), but
> > > would mean we could use a u32 type for pl_recalc_time and
> > > pl_recalc_period because that doesn't overflow until 136 years
> > > after booting. (signed time_t overflows 68 years after 1970,
> > > i.e 2038).
> > 
> > So, is this patch correct and should be merged to the tree, or not?
> > 
> 
> No, I think it's wrong in a different way as before. After my
> patch, there were six instances of ktime_get_real_seconds() and
> three instances of ktime_get_seconds(), and that was wrong.
> 
> James's patch converts three of the six instances to
> ktime_get_seconds(), which is equally wrong, just different.
> 
> We can either convert the six ktime_get_real_seconds()
> to ktime_get_seconds(), or convert the four (one was added
> in the meantime) ktime_get_seconds() to ktime_get_real_seconds().
> 
> I'll follow up with a patch for the latter as it is what I
> had originally intended. We can also do what James intended
> instead.

I tought about it and yes ktime_get_real_seconds() is the
right way to go due to the debug message. Especially if the
node happens to reboot. Having a debug message after a reboot
with a smaller time reported in the debug message compared to
a earlier debug message might be confusing to someone.

Re: [PATCH v2] staging: lustre: ldlm: pl_recalc time handling is wrong

[James Simmons]

> James Simmons reports:
> > The ldlm_pool field pl_recalc_time is set to the current
> > monotonic clock value but the interval period is calculated
> > with the wall clock. This means the interval period will
> > always be far larger than the pl_recalc_period, which is
> > just a small interval time period. The correct thing to
> > do is to use monotomic clock current value instead of the
> > wall clocks value when calculating recalc_interval_sec.
> 
> This broke when I converted the 32-bit get_seconds() into
> ktime_get_{real_,}seconds() inconsistently. Either
> one of those two would have worked, but mixing them
> does not.
> 
> Staying with the original intention of the patch, this
> changes the ktime_get_seconds() calls into ktime_get_real_seconds(),
> using real time instead of mononic time.

Reviewed-by: James Simmons <jsimmons@infradead.org>
 
> Cc: stable@vger.kernel.org # v4.4+
> Fixes: 8f83409cf238 ("staging/lustre: use 64-bit time for pl_recalc")
> Reported-by: James Simmons <jsimmons@infradead.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2: James' patch was similarly incomplete to mine, as it only
> addressed some of the calls. With this new version, all ktime
> accessors use the same time domain.
> 
> diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
> index 19831c555c49..b820309d70e3 100644
> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
> @@ -356,10 +356,10 @@ static int ldlm_pool_recalc(struct ldlm_pool *pl)
>  	u32 recalc_interval_sec;
>  	int count;
>  
> -	recalc_interval_sec = ktime_get_seconds() - pl->pl_recalc_time;
> +	recalc_interval_sec = ktime_get_real_seconds() - pl->pl_recalc_time;
>  	if (recalc_interval_sec > 0) {
>  		spin_lock(&pl->pl_lock);
> -		recalc_interval_sec = ktime_get_seconds() - pl->pl_recalc_time;
> +		recalc_interval_sec = ktime_get_real_seconds() - pl->pl_recalc_time;
>  
>  		if (recalc_interval_sec > 0) {
>  			/*
> @@ -382,7 +382,7 @@ static int ldlm_pool_recalc(struct ldlm_pool *pl)
>  				    count);
>  	}
>  
> -	recalc_interval_sec = pl->pl_recalc_time - ktime_get_seconds() +
> +	recalc_interval_sec = pl->pl_recalc_time - ktime_get_real_seconds() +
>  			      pl->pl_recalc_period;
>  	if (recalc_interval_sec <= 0) {
>  		/* DEBUG: should be re-removed after LU-4536 is fixed */
> @@ -657,7 +657,7 @@ int ldlm_pool_init(struct ldlm_pool *pl, struct ldlm_namespace *ns,
>  
>  	spin_lock_init(&pl->pl_lock);
>  	atomic_set(&pl->pl_granted, 0);
> -	pl->pl_recalc_time = ktime_get_seconds();
> +	pl->pl_recalc_time = ktime_get_real_seconds();
>  	atomic_set(&pl->pl_lock_volume_factor, 1);
>  
>  	atomic_set(&pl->pl_grant_rate, 0);
> 
>