All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] common/config: Always create RESULT_BASE
@ 2019-01-03 10:01 Nikolay Borisov
  2019-01-03 12:29 ` Nikolay Borisov
  0 siblings, 1 reply; 9+ messages in thread
From: Nikolay Borisov @ 2019-01-03 10:01 UTC (permalink / raw)
  To: fstests; +Cc: guaneryu, Nikolay Borisov

Commit 7fc034868d5d ("common/config: create $RESULT_BASE before dumping kmemleak leaks")
inadvertently broke $RESULT_BASE dir creation since it changed the logic
to only create the directory only if this variable is not explicitly set
by the user. Fix this by ensuring RESULT_BASE is always created.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 common/config | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/common/config b/common/config
index fb664cf0a967..e2da9cfacbb5 100644
--- a/common/config
+++ b/common/config
@@ -663,11 +663,12 @@ get_next_config() {
 	# set default RESULT_BASE
 	if [ -z "$RESULT_BASE" ]; then
 		export RESULT_BASE="$here/results/"
-		mkdir -p ${RESULT_BASE}
-		if [ ! -d ${RESULT_BASE} ]; then
-			echo "failed to create results directory $RESULT_BASE"
-			exit 1
-		fi
+	fi
+
+	mkdir -p ${RESULT_BASE}
+	if [ ! -d ${RESULT_BASE} ]; then
+		echo "failed to create results directory $RESULT_BASE"
+		exit 1
 	fi
 
 	if [ "$FSTYP" == "tmpfs" ]; then
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] common/config: Always create RESULT_BASE
  2019-01-03 10:01 [PATCH] common/config: Always create RESULT_BASE Nikolay Borisov
@ 2019-01-03 12:29 ` Nikolay Borisov
  2019-01-03 15:22   ` Eryu Guan
  2019-01-03 21:56   ` Dave Chinner
  0 siblings, 2 replies; 9+ messages in thread
From: Nikolay Borisov @ 2019-01-03 12:29 UTC (permalink / raw)
  To: fstests; +Cc: guaneryu, Dave Chinner



On 3.01.19 г. 12:01 ч., Nikolay Borisov wrote:
> Commit 7fc034868d5d ("common/config: create $RESULT_BASE before dumping kmemleak leaks")
> inadvertently broke $RESULT_BASE dir creation since it changed the logic
> to only create the directory only if this variable is not explicitly set
> by the user. Fix this by ensuring RESULT_BASE is always created.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Eryu,

I think Johannes' commit should actually be reverted. Currently
get_next_config is called at the beginning of the section code _AFTER_
_init_kmemleak so it's not really fixing the problem that Johannes
described. So care to revert his commit ?

> ---
>  common/config | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/common/config b/common/config
> index fb664cf0a967..e2da9cfacbb5 100644
> --- a/common/config
> +++ b/common/config
> @@ -663,11 +663,12 @@ get_next_config() {
>  	# set default RESULT_BASE
>  	if [ -z "$RESULT_BASE" ]; then
>  		export RESULT_BASE="$here/results/"
> -		mkdir -p ${RESULT_BASE}
> -		if [ ! -d ${RESULT_BASE} ]; then
> -			echo "failed to create results directory $RESULT_BASE"
> -			exit 1
> -		fi
> +	fi
> +
> +	mkdir -p ${RESULT_BASE}
> +	if [ ! -d ${RESULT_BASE} ]; then
> +		echo "failed to create results directory $RESULT_BASE"
> +		exit 1
>  	fi
>  
>  	if [ "$FSTYP" == "tmpfs" ]; then
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] common/config: Always create RESULT_BASE
  2019-01-03 12:29 ` Nikolay Borisov
@ 2019-01-03 15:22   ` Eryu Guan
  2019-01-03 21:56   ` Dave Chinner
  1 sibling, 0 replies; 9+ messages in thread
From: Eryu Guan @ 2019-01-03 15:22 UTC (permalink / raw)
  To: Nikolay Borisov; +Cc: fstests, Dave Chinner

On Thu, Jan 03, 2019 at 02:29:08PM +0200, Nikolay Borisov wrote:
> 
> 
> On 3.01.19 г. 12:01 ч., Nikolay Borisov wrote:
> > Commit 7fc034868d5d ("common/config: create $RESULT_BASE before dumping kmemleak leaks")
> > inadvertently broke $RESULT_BASE dir creation since it changed the logic
> > to only create the directory only if this variable is not explicitly set
> > by the user. Fix this by ensuring RESULT_BASE is always created.
> > 
> > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> 
> Eryu,
> 
> I think Johannes' commit should actually be reverted. Currently
> get_next_config is called at the beginning of the section code _AFTER_
> _init_kmemleak so it's not really fixing the problem that Johannes
> described. So care to revert his commit ?

get_next_config is always called at the end of common/config as well,
which is sourced before _init_kmemleak, so $RESULT_BASE is created. But
I missed the user-specified RESULT_BASE case, so I think everything is
fine after applying your patch. Thanks for the fix!

Eryu

> 
> > ---
> >  common/config | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/common/config b/common/config
> > index fb664cf0a967..e2da9cfacbb5 100644
> > --- a/common/config
> > +++ b/common/config
> > @@ -663,11 +663,12 @@ get_next_config() {
> >  	# set default RESULT_BASE
> >  	if [ -z "$RESULT_BASE" ]; then
> >  		export RESULT_BASE="$here/results/"
> > -		mkdir -p ${RESULT_BASE}
> > -		if [ ! -d ${RESULT_BASE} ]; then
> > -			echo "failed to create results directory $RESULT_BASE"
> > -			exit 1
> > -		fi
> > +	fi
> > +
> > +	mkdir -p ${RESULT_BASE}
> > +	if [ ! -d ${RESULT_BASE} ]; then
> > +		echo "failed to create results directory $RESULT_BASE"
> > +		exit 1
> >  	fi
> >  
> >  	if [ "$FSTYP" == "tmpfs" ]; then
> > 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] common/config: Always create RESULT_BASE
  2019-01-03 12:29 ` Nikolay Borisov
  2019-01-03 15:22   ` Eryu Guan
@ 2019-01-03 21:56   ` Dave Chinner
  2019-01-04  9:43     ` Eryu Guan
  2019-01-06 17:47     ` Darrick J. Wong
  1 sibling, 2 replies; 9+ messages in thread
From: Dave Chinner @ 2019-01-03 21:56 UTC (permalink / raw)
  To: Nikolay Borisov; +Cc: fstests, guaneryu, Dave Chinner

On Thu, Jan 03, 2019 at 02:29:08PM +0200, Nikolay Borisov wrote:
> 
> 
> On 3.01.19 г. 12:01 ч., Nikolay Borisov wrote:
> > Commit 7fc034868d5d ("common/config: create $RESULT_BASE before dumping kmemleak leaks")
> > inadvertently broke $RESULT_BASE dir creation since it changed the logic
> > to only create the directory only if this variable is not explicitly set
> > by the user. Fix this by ensuring RESULT_BASE is always created.
> > 
> > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> 
> Eryu,
> 
> I think Johannes' commit should actually be reverted. Currently
> get_next_config is called at the beginning of the section code _AFTER_
> _init_kmemleak so it's not really fixing the problem that Johannes
> described. So care to revert his commit ?

Yes, the original commit should be reverted. Using RESULT_BASE in
the way that init_kmemleak is using it is fundamentally broken.
Using RESULTS_BASE is incorrect - harness run state is supposed to
be stored in the section-aware RESULTS_DIR (e.g. see
_require_scratch()) which is constant and always exists for each
section that is run.

The code was architectected this way because section definitions can
change RESULT_BASE, and that means the RESULT_BASE defined when
init_kmemleak() is called may not point to the same location as when
check_memleak() is called. Hence check_memleak will never run if a
section definition changes RESULT_BASE.

IOWs, the init_kmemleak() call needs to be done inside the
section iteration loop, after the section config has been parsed and
we've determined if the the section will be run. This is where the
original code created RESULT_BASE if it didn't exist. Further, all
the section run state is then stored in RESULTS_DIR, which is
created from the current RESULT_BASE (e.g. see _require_scratch,
check_dmesg, etc).

Hence init_kmemleak() and check_kmemleak() should have a scope
inside a valid RESULT_DIR path. And once this is done, you can then
add a USE_KMEMLEAK section variable to turn kmemleak detection on
and off via the config file on a per-section basis.....

So, IMO, the correct thing to do here is revert the original broken
change and fix the kmemleak infrastructure to be properly aware of
config sections.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] common/config: Always create RESULT_BASE
  2019-01-03 21:56   ` Dave Chinner
@ 2019-01-04  9:43     ` Eryu Guan
  2019-01-04  9:54       ` Nikolay Borisov
  2019-01-06 17:47     ` Darrick J. Wong
  1 sibling, 1 reply; 9+ messages in thread
From: Eryu Guan @ 2019-01-04  9:43 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Nikolay Borisov, fstests, Dave Chinner

On Fri, Jan 04, 2019 at 08:56:09AM +1100, Dave Chinner wrote:
> On Thu, Jan 03, 2019 at 02:29:08PM +0200, Nikolay Borisov wrote:
> > 
> > 
> > On 3.01.19 г. 12:01 ч., Nikolay Borisov wrote:
> > > Commit 7fc034868d5d ("common/config: create $RESULT_BASE before dumping kmemleak leaks")
> > > inadvertently broke $RESULT_BASE dir creation since it changed the logic
> > > to only create the directory only if this variable is not explicitly set
> > > by the user. Fix this by ensuring RESULT_BASE is always created.
> > > 
> > > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> > 
> > Eryu,
> > 
> > I think Johannes' commit should actually be reverted. Currently
> > get_next_config is called at the beginning of the section code _AFTER_
> > _init_kmemleak so it's not really fixing the problem that Johannes
> > described. So care to revert his commit ?
> 
> Yes, the original commit should be reverted. Using RESULT_BASE in
> the way that init_kmemleak is using it is fundamentally broken.
> Using RESULTS_BASE is incorrect - harness run state is supposed to
> be stored in the section-aware RESULTS_DIR (e.g. see
> _require_scratch()) which is constant and always exists for each
> section that is run.
> 
> The code was architectected this way because section definitions can
> change RESULT_BASE, and that means the RESULT_BASE defined when
> init_kmemleak() is called may not point to the same location as when
> check_memleak() is called. Hence check_memleak will never run if a
> section definition changes RESULT_BASE.
> 
> IOWs, the init_kmemleak() call needs to be done inside the
> section iteration loop, after the section config has been parsed and
> we've determined if the the section will be run. This is where the
> original code created RESULT_BASE if it didn't exist. Further, all
> the section run state is then stored in RESULTS_DIR, which is
> created from the current RESULT_BASE (e.g. see _require_scratch,
> check_dmesg, etc).

This makes more sense, thanks for the suggestion!

> 
> Hence init_kmemleak() and check_kmemleak() should have a scope
> inside a valid RESULT_DIR path. And once this is done, you can then
> add a USE_KMEMLEAK section variable to turn kmemleak detection on
> and off via the config file on a per-section basis.....
> 
> So, IMO, the correct thing to do here is revert the original broken
> change and fix the kmemleak infrastructure to be properly aware of
> config sections.

I think we can take Nikolay's patch for now so we don't leave
user-specified RESULT_BASE not created for any longer, and revert it and
7fc034868d5d when we rework kmemleak infrastructure.

Thanks,
Eryu

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] common/config: Always create RESULT_BASE
  2019-01-04  9:43     ` Eryu Guan
@ 2019-01-04  9:54       ` Nikolay Borisov
  2019-01-04 13:19         ` Eryu Guan
  0 siblings, 1 reply; 9+ messages in thread
From: Nikolay Borisov @ 2019-01-04  9:54 UTC (permalink / raw)
  To: Eryu Guan, Dave Chinner; +Cc: fstests, Dave Chinner, Darrick J. Wong



On 4.01.19 г. 11:43 ч., Eryu Guan wrote:
> On Fri, Jan 04, 2019 at 08:56:09AM +1100, Dave Chinner wrote:
>> On Thu, Jan 03, 2019 at 02:29:08PM +0200, Nikolay Borisov wrote:
>>>
>>>
>>> On 3.01.19 г. 12:01 ч., Nikolay Borisov wrote:
>>>> Commit 7fc034868d5d ("common/config: create $RESULT_BASE before dumping kmemleak leaks")
>>>> inadvertently broke $RESULT_BASE dir creation since it changed the logic
>>>> to only create the directory only if this variable is not explicitly set
>>>> by the user. Fix this by ensuring RESULT_BASE is always created.
>>>>
>>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>>
>>> Eryu,
>>>
>>> I think Johannes' commit should actually be reverted. Currently
>>> get_next_config is called at the beginning of the section code _AFTER_
>>> _init_kmemleak so it's not really fixing the problem that Johannes
>>> described. So care to revert his commit ?
>>
>> Yes, the original commit should be reverted. Using RESULT_BASE in
>> the way that init_kmemleak is using it is fundamentally broken.
>> Using RESULTS_BASE is incorrect - harness run state is supposed to
>> be stored in the section-aware RESULTS_DIR (e.g. see
>> _require_scratch()) which is constant and always exists for each
>> section that is run.
>>
>> The code was architectected this way because section definitions can
>> change RESULT_BASE, and that means the RESULT_BASE defined when
>> init_kmemleak() is called may not point to the same location as when
>> check_memleak() is called. Hence check_memleak will never run if a
>> section definition changes RESULT_BASE.
>>
>> IOWs, the init_kmemleak() call needs to be done inside the
>> section iteration loop, after the section config has been parsed and
>> we've determined if the the section will be run. This is where the
>> original code created RESULT_BASE if it didn't exist. Further, all
>> the section run state is then stored in RESULTS_DIR, which is
>> created from the current RESULT_BASE (e.g. see _require_scratch,
>> check_dmesg, etc).
> 
> This makes more sense, thanks for the suggestion!
> 
>>
>> Hence init_kmemleak() and check_kmemleak() should have a scope
>> inside a valid RESULT_DIR path. And once this is done, you can then
>> add a USE_KMEMLEAK section variable to turn kmemleak detection on
>> and off via the config file on a per-section basis.....
>>
>> So, IMO, the correct thing to do here is revert the original broken
>> change and fix the kmemleak infrastructure to be properly aware of
>> config sections.
> 
> I think we can take Nikolay's patch for now so we don't leave
> user-specified RESULT_BASE not created for any longer, and revert it and
> 7fc034868d5d when we rework kmemleak infrastructure.

Actually no, for the following RESULT_BASE pattern: 
RESULT_BASE="$PWD/results/$HOST/$(uname -r)"/$(date +%Y-%m-%d-%H-%M-%N)/


Running a test I can see the following mkdirs happening: 

created RESULT_BASE: /root/xfstests-dev/results/linux/4.12.14-nikbor/2019-01-04-09-48-027249346/
created RESULT_BASE: /root/xfstests-dev/results/linux/4.12.14-nikbor/2019-01-04-09-48-093731409/
created RESULT_BASE: /root/xfstests-dev/results/linux/4.12.14-nikbor/2019-01-04-09-48-093731409/

In my case for every test run it potentially creates 2 directories, where only one is really 
populated with data. This is no good. I will strongly suggest that you revert the original patch and 
let kmemleak be broken rather than piling hacks (which have funky side effects as I have shown above)
or leaving dir creation broken altogether.

While at it you might also consider reverting 074740a32c6a36c5ba7d4be66dd4ee63e9f744f3 until Darick 
(or anyone else interested) properly integrated kmemleak support. 



> 
> Thanks,
> Eryu
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] common/config: Always create RESULT_BASE
  2019-01-04  9:54       ` Nikolay Borisov
@ 2019-01-04 13:19         ` Eryu Guan
  0 siblings, 0 replies; 9+ messages in thread
From: Eryu Guan @ 2019-01-04 13:19 UTC (permalink / raw)
  To: Nikolay Borisov; +Cc: Dave Chinner, fstests, Dave Chinner, Darrick J. Wong

On Fri, Jan 04, 2019 at 11:54:11AM +0200, Nikolay Borisov wrote:
> 
> 
> On 4.01.19 г. 11:43 ч., Eryu Guan wrote:
> > On Fri, Jan 04, 2019 at 08:56:09AM +1100, Dave Chinner wrote:
> >> On Thu, Jan 03, 2019 at 02:29:08PM +0200, Nikolay Borisov wrote:
> >>>
> >>>
> >>> On 3.01.19 г. 12:01 ч., Nikolay Borisov wrote:
> >>>> Commit 7fc034868d5d ("common/config: create $RESULT_BASE before dumping kmemleak leaks")
> >>>> inadvertently broke $RESULT_BASE dir creation since it changed the logic
> >>>> to only create the directory only if this variable is not explicitly set
> >>>> by the user. Fix this by ensuring RESULT_BASE is always created.
> >>>>
> >>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> >>>
> >>> Eryu,
> >>>
> >>> I think Johannes' commit should actually be reverted. Currently
> >>> get_next_config is called at the beginning of the section code _AFTER_
> >>> _init_kmemleak so it's not really fixing the problem that Johannes
> >>> described. So care to revert his commit ?
> >>
> >> Yes, the original commit should be reverted. Using RESULT_BASE in
> >> the way that init_kmemleak is using it is fundamentally broken.
> >> Using RESULTS_BASE is incorrect - harness run state is supposed to
> >> be stored in the section-aware RESULTS_DIR (e.g. see
> >> _require_scratch()) which is constant and always exists for each
> >> section that is run.
> >>
> >> The code was architectected this way because section definitions can
> >> change RESULT_BASE, and that means the RESULT_BASE defined when
> >> init_kmemleak() is called may not point to the same location as when
> >> check_memleak() is called. Hence check_memleak will never run if a
> >> section definition changes RESULT_BASE.
> >>
> >> IOWs, the init_kmemleak() call needs to be done inside the
> >> section iteration loop, after the section config has been parsed and
> >> we've determined if the the section will be run. This is where the
> >> original code created RESULT_BASE if it didn't exist. Further, all
> >> the section run state is then stored in RESULTS_DIR, which is
> >> created from the current RESULT_BASE (e.g. see _require_scratch,
> >> check_dmesg, etc).
> > 
> > This makes more sense, thanks for the suggestion!
> > 
> >>
> >> Hence init_kmemleak() and check_kmemleak() should have a scope
> >> inside a valid RESULT_DIR path. And once this is done, you can then
> >> add a USE_KMEMLEAK section variable to turn kmemleak detection on
> >> and off via the config file on a per-section basis.....
> >>
> >> So, IMO, the correct thing to do here is revert the original broken
> >> change and fix the kmemleak infrastructure to be properly aware of
> >> config sections.
> > 
> > I think we can take Nikolay's patch for now so we don't leave
> > user-specified RESULT_BASE not created for any longer, and revert it and
> > 7fc034868d5d when we rework kmemleak infrastructure.
> 
> Actually no, for the following RESULT_BASE pattern: 
> RESULT_BASE="$PWD/results/$HOST/$(uname -r)"/$(date +%Y-%m-%d-%H-%M-%N)/
> 
> 
> Running a test I can see the following mkdirs happening: 
> 
> created RESULT_BASE: /root/xfstests-dev/results/linux/4.12.14-nikbor/2019-01-04-09-48-027249346/
> created RESULT_BASE: /root/xfstests-dev/results/linux/4.12.14-nikbor/2019-01-04-09-48-093731409/
> created RESULT_BASE: /root/xfstests-dev/results/linux/4.12.14-nikbor/2019-01-04-09-48-093731409/
> 
> In my case for every test run it potentially creates 2 directories, where only one is really 

OK, I see it now, it only happens when defining RESULT_BASE as above in
section based config file, non-section based config is not affected and
it worked if RESULT_BASE is defined from the command line.

> populated with data. This is no good. I will strongly suggest that you revert the original patch and 
> let kmemleak be broken rather than piling hacks (which have funky side effects as I have shown above)
> or leaving dir creation broken altogether.

Would you like to send a revert patch?

> 
> While at it you might also consider reverting 074740a32c6a36c5ba7d4be66dd4ee63e9f744f3 until Darick 
> (or anyone else interested) properly integrated kmemleak support. 

It's been broken for almost one year, so I think it's fine to just
revert 7fc034868d5d. I'll look into rework kmemleak next week.

Thanks,
Eryu

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] common/config: Always create RESULT_BASE
  2019-01-03 21:56   ` Dave Chinner
  2019-01-04  9:43     ` Eryu Guan
@ 2019-01-06 17:47     ` Darrick J. Wong
  2019-01-06 18:08       ` Darrick J. Wong
  1 sibling, 1 reply; 9+ messages in thread
From: Darrick J. Wong @ 2019-01-06 17:47 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Nikolay Borisov, fstests, guaneryu, Dave Chinner

On Fri, Jan 04, 2019 at 08:56:09AM +1100, Dave Chinner wrote:
> On Thu, Jan 03, 2019 at 02:29:08PM +0200, Nikolay Borisov wrote:
> > 
> > 
> > On 3.01.19 г. 12:01 ч., Nikolay Borisov wrote:
> > > Commit 7fc034868d5d ("common/config: create $RESULT_BASE before dumping kmemleak leaks")
> > > inadvertently broke $RESULT_BASE dir creation since it changed the logic
> > > to only create the directory only if this variable is not explicitly set
> > > by the user. Fix this by ensuring RESULT_BASE is always created.
> > > 
> > > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> > 
> > Eryu,
> > 
> > I think Johannes' commit should actually be reverted. Currently
> > get_next_config is called at the beginning of the section code _AFTER_
> > _init_kmemleak so it's not really fixing the problem that Johannes
> > described. So care to revert his commit ?
> 
> Yes, the original commit should be reverted. Using RESULT_BASE in
> the way that init_kmemleak is using it is fundamentally broken.
> Using RESULTS_BASE is incorrect - harness run state is supposed to
> be stored in the section-aware RESULTS_DIR (e.g. see
> _require_scratch()) which is constant and always exists for each
> section that is run.
> 
> The code was architectected this way because section definitions can
> change RESULT_BASE, and that means the RESULT_BASE defined when
> init_kmemleak() is called may not point to the same location as when
> check_memleak() is called. Hence check_memleak will never run if a
> section definition changes RESULT_BASE.
> 
> IOWs, the init_kmemleak() call needs to be done inside the
> section iteration loop, after the section config has been parsed and
> we've determined if the the section will be run. This is where the
> original code created RESULT_BASE if it didn't exist. Further, all
> the section run state is then stored in RESULTS_DIR, which is
> created from the current RESULT_BASE (e.g. see _require_scratch,
> check_dmesg, etc).
> 
> Hence init_kmemleak() and check_kmemleak() should have a scope
> inside a valid RESULT_DIR path. And once this is done, you can then

However, the scope of RESULT_DIR is per-test, not per-section.  Either
we'd have to hoist the creation of RESULT_DIR above the "for seq in
$list..." or engage in some egregious hacks to get this set up before
we loop through the tests.

The other problem with kmemleak is that it takes a while for the scanner
to notice leaks (hence all the "read report twice" crap code) which
means we'd really don't want to be cycling it on and off repeatedly,
because that causes it to lose leaks periodically.  It works best when
you turn it on once and sample every now and then.

Frankly, the kmemleak mechanism is just grody and unreliable enough that
I'm tempted to ask Eryu to rip it out entirely.  But, I'll give one go
at fixing it, since I wrote it...

--D

> add a USE_KMEMLEAK section variable to turn kmemleak detection on
> and off via the config file on a per-section basis.....
> 
> So, IMO, the correct thing to do here is revert the original broken
> change and fix the kmemleak infrastructure to be properly aware of
> config sections.
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] common/config: Always create RESULT_BASE
  2019-01-06 17:47     ` Darrick J. Wong
@ 2019-01-06 18:08       ` Darrick J. Wong
  0 siblings, 0 replies; 9+ messages in thread
From: Darrick J. Wong @ 2019-01-06 18:08 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Nikolay Borisov, fstests, guaneryu, Dave Chinner

On Sun, Jan 06, 2019 at 09:47:34AM -0800, Darrick J. Wong wrote:
> On Fri, Jan 04, 2019 at 08:56:09AM +1100, Dave Chinner wrote:
> > On Thu, Jan 03, 2019 at 02:29:08PM +0200, Nikolay Borisov wrote:
> > > 
> > > 
> > > On 3.01.19 г. 12:01 ч., Nikolay Borisov wrote:
> > > > Commit 7fc034868d5d ("common/config: create $RESULT_BASE before dumping kmemleak leaks")
> > > > inadvertently broke $RESULT_BASE dir creation since it changed the logic
> > > > to only create the directory only if this variable is not explicitly set
> > > > by the user. Fix this by ensuring RESULT_BASE is always created.
> > > > 
> > > > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> > > 
> > > Eryu,
> > > 
> > > I think Johannes' commit should actually be reverted. Currently
> > > get_next_config is called at the beginning of the section code _AFTER_
> > > _init_kmemleak so it's not really fixing the problem that Johannes
> > > described. So care to revert his commit ?
> > 
> > Yes, the original commit should be reverted. Using RESULT_BASE in
> > the way that init_kmemleak is using it is fundamentally broken.
> > Using RESULTS_BASE is incorrect - harness run state is supposed to
> > be stored in the section-aware RESULTS_DIR (e.g. see
> > _require_scratch()) which is constant and always exists for each
> > section that is run.
> > 
> > The code was architectected this way because section definitions can
> > change RESULT_BASE, and that means the RESULT_BASE defined when
> > init_kmemleak() is called may not point to the same location as when
> > check_memleak() is called. Hence check_memleak will never run if a
> > section definition changes RESULT_BASE.
> > 
> > IOWs, the init_kmemleak() call needs to be done inside the
> > section iteration loop, after the section config has been parsed and
> > we've determined if the the section will be run. This is where the
> > original code created RESULT_BASE if it didn't exist. Further, all
> > the section run state is then stored in RESULTS_DIR, which is
> > created from the current RESULT_BASE (e.g. see _require_scratch,
> > check_dmesg, etc).
> > 
> > Hence init_kmemleak() and check_kmemleak() should have a scope
> > inside a valid RESULT_DIR path. And once this is done, you can then
> 
> However, the scope of RESULT_DIR is per-test, not per-section.  Either
> we'd have to hoist the creation of RESULT_DIR above the "for seq in
> $list..." or engage in some egregious hacks to get this set up before
> we loop through the tests.
> 
> The other problem with kmemleak is that it takes a while for the scanner
> to notice leaks (hence all the "read report twice" crap code) which
> means we'd really don't want to be cycling it on and off repeatedly,
> because that causes it to lose leaks periodically.  It works best when
> you turn it on once and sample every now and then.

Ahh, we don't cycle the background scanner anymore, we use the
_init_kmemleak method to disable background scanning and discard all the
leaks that happened before the user even ran ./check.  So that only
needs to run once at the very beginning, which is how it behaves now.

As for _check_kmemleak, we actually need it to run after ever test, if
nothing else to clean out corruption reports.  I can make it discard the
results if USE_KMEMLEAK=no or CONFIG_DEBUG_KMEMLEAK=n.  I will also
change it to drop the report in the $RESULT_DIR instead of $RESULT_BASE,
as discussed.

--D

> Frankly, the kmemleak mechanism is just grody and unreliable enough that
> I'm tempted to ask Eryu to rip it out entirely.  But, I'll give one go
> at fixing it, since I wrote it...
> 
> --D
> 
> > add a USE_KMEMLEAK section variable to turn kmemleak detection on
> > and off via the config file on a per-section basis.....
> > 
> > So, IMO, the correct thing to do here is revert the original broken
> > change and fix the kmemleak infrastructure to be properly aware of
> > config sections.
> > 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-01-06 18:09 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-03 10:01 [PATCH] common/config: Always create RESULT_BASE Nikolay Borisov
2019-01-03 12:29 ` Nikolay Borisov
2019-01-03 15:22   ` Eryu Guan
2019-01-03 21:56   ` Dave Chinner
2019-01-04  9:43     ` Eryu Guan
2019-01-04  9:54       ` Nikolay Borisov
2019-01-04 13:19         ` Eryu Guan
2019-01-06 17:47     ` Darrick J. Wong
2019-01-06 18:08       ` Darrick J. Wong

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.