| Message ID | 20190103100135.24748-1-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | common/config: Always create RESULT_BASE | expand |
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 >
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 > >
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.
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
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 >
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
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
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
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
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(-)