| Message ID | 7af8f80173fab5408da86f5b3393e787659a81d6.1731683116.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fstests: refine filesystem module reload handling | expand |
On Fri, Nov 15, 2024 at 11:20:51PM +0800, Anand Jain wrote: > Reload the module before each test, instead of later in run_section. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > check | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/check b/check > index 9222cd7e4f81..d8ee73f48c77 100755 > --- a/check > +++ b/check > @@ -935,6 +935,15 @@ function run_section() > continue > fi > > + # Reload the module after each test to check for leaks or > + # other problems. Hrmm. The nice thing about doing the reload /after/ each test is that unloading the module will purge any per-fs slab caches that the module creates. The slab teardown logs any forgotten objects while $seq still points to the test that leaked those objects. Granted it's not a 100% solution (that job falls to kmemcheck) but it's a cheap check. This change makes it so that if test N leaks something, fstests reports them for test N+1. --D > + if [ -n "${TEST_FS_MODULE_RELOAD}" ]; then > + _test_unmount 2> /dev/null > + _scratch_unmount 2> /dev/null > + modprobe -r fs-$FSTYP > + modprobe fs-$FSTYP > + fi > + > # record that we really tried to run this test. > if ((!${#loop_status[*]})); then > try+=("$seqnum") > @@ -1033,15 +1042,6 @@ function run_section() > done > fi > > - # Reload the module after each test to check for leaks or > - # other problems. > - if [ -n "${TEST_FS_MODULE_RELOAD}" ]; then > - _test_unmount 2> /dev/null > - _scratch_unmount 2> /dev/null > - modprobe -r fs-$FSTYP > - modprobe fs-$FSTYP > - fi > - > # Scan for memory leaks after every test so that associating > # a leak to a particular test will be as accurate as possible. > _check_kmemleak || tc_status="fail" > -- > 2.46.1 > >
On Fri, Nov 15, 2024 at 3:22 PM Anand Jain <anand.jain@oracle.com> wrote: > > Reload the module before each test, instead of later in run_section. Why? The change log should give a justification for the change. It's just stating what the code changes are doing but without any reasoning. Thanks. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > check | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/check b/check > index 9222cd7e4f81..d8ee73f48c77 100755 > --- a/check > +++ b/check > @@ -935,6 +935,15 @@ function run_section() > continue > fi > > + # Reload the module after each test to check for leaks or > + # other problems. > + if [ -n "${TEST_FS_MODULE_RELOAD}" ]; then > + _test_unmount 2> /dev/null > + _scratch_unmount 2> /dev/null > + modprobe -r fs-$FSTYP > + modprobe fs-$FSTYP > + fi > + > # record that we really tried to run this test. > if ((!${#loop_status[*]})); then > try+=("$seqnum") > @@ -1033,15 +1042,6 @@ function run_section() > done > fi > > - # Reload the module after each test to check for leaks or > - # other problems. > - if [ -n "${TEST_FS_MODULE_RELOAD}" ]; then > - _test_unmount 2> /dev/null > - _scratch_unmount 2> /dev/null > - modprobe -r fs-$FSTYP > - modprobe fs-$FSTYP > - fi > - > # Scan for memory leaks after every test so that associating > # a leak to a particular test will be as accurate as possible. > _check_kmemleak || tc_status="fail" > -- > 2.46.1 > >
On 16/11/24 00:39, Darrick J. Wong wrote: > On Fri, Nov 15, 2024 at 11:20:51PM +0800, Anand Jain wrote: >> Reload the module before each test, instead of later in run_section. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> check | 18 +++++++++--------- >> 1 file changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/check b/check >> index 9222cd7e4f81..d8ee73f48c77 100755 >> --- a/check >> +++ b/check >> @@ -935,6 +935,15 @@ function run_section() >> continue >> fi >> >> + # Reload the module after each test to check for leaks or >> + # other problems. > > Hrmm. The nice thing about doing the reload /after/ each test is that > unloading the module will purge any per-fs slab caches that the module > creates. The slab teardown logs any forgotten objects while $seq still > points to the test that leaked those objects. Granted it's not a 100% > solution (that job falls to kmemcheck) but it's a cheap check. > > This change makes it so that if test N leaks something, fstests reports > them for test N+1. > Agreed. It's better to report the issue at test N. I'll add this comment to the code in v2. Thanks! Anand > --D > >> + if [ -n "${TEST_FS_MODULE_RELOAD}" ]; then >> + _test_unmount 2> /dev/null >> + _scratch_unmount 2> /dev/null >> + modprobe -r fs-$FSTYP >> + modprobe fs-$FSTYP >> + fi >> + >> # record that we really tried to run this test. >> if ((!${#loop_status[*]})); then >> try+=("$seqnum") >> @@ -1033,15 +1042,6 @@ function run_section() >> done >> fi >> >> - # Reload the module after each test to check for leaks or >> - # other problems. >> - if [ -n "${TEST_FS_MODULE_RELOAD}" ]; then >> - _test_unmount 2> /dev/null >> - _scratch_unmount 2> /dev/null >> - modprobe -r fs-$FSTYP >> - modprobe fs-$FSTYP >> - fi >> - >> # Scan for memory leaks after every test so that associating >> # a leak to a particular test will be as accurate as possible. >> _check_kmemleak || tc_status="fail" >> -- >> 2.46.1 >> >>
diff --git a/check b/check index 9222cd7e4f81..d8ee73f48c77 100755 --- a/check +++ b/check @@ -935,6 +935,15 @@ function run_section() continue fi + # Reload the module after each test to check for leaks or + # other problems. + if [ -n "${TEST_FS_MODULE_RELOAD}" ]; then + _test_unmount 2> /dev/null + _scratch_unmount 2> /dev/null + modprobe -r fs-$FSTYP + modprobe fs-$FSTYP + fi + # record that we really tried to run this test. if ((!${#loop_status[*]})); then try+=("$seqnum") @@ -1033,15 +1042,6 @@ function run_section() done fi - # Reload the module after each test to check for leaks or - # other problems. - if [ -n "${TEST_FS_MODULE_RELOAD}" ]; then - _test_unmount 2> /dev/null - _scratch_unmount 2> /dev/null - modprobe -r fs-$FSTYP - modprobe fs-$FSTYP - fi - # Scan for memory leaks after every test so that associating # a leak to a particular test will be as accurate as possible. _check_kmemleak || tc_status="fail"
Reload the module before each test, instead of later in run_section. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- check | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)