Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 18 Aug 2021 07:02:56 -0700
From:   Luis Chamberlain <mcgrof@kernel.org>
To:     Eryu Guan <guan@eryu.me>
Cc:     fstests@vger.kernel.org, hare@suse.de, dgilbert@interlog.com,
        jeyu@kernel.org, lucas.demarchi@intel.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/3] common/module: add patient module rmmod support
Message-ID: <YR0TEE8lUwo6QlHw@bombadil.infradead.org>
References: <20210811154512.1813622-1-mcgrof@kernel.org>
 <20210811154512.1813622-3-mcgrof@kernel.org>
 <YRkIttM75q3gLxpN@desktop>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YRkIttM75q3gLxpN@desktop>
Sender: Luis Chamberlain <mcgrof@infradead.org>
Precedence: bulk

On Sun, Aug 15, 2021 at 08:29:42PM +0800, Eryu Guan wrote:
> On Wed, Aug 11, 2021 at 08:45:11AM -0700, Luis Chamberlain wrote:
> >  common/config |  31 +++++++++++++++
> >  common/module | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 
> Please also update README to document the new configurable variables.

Got it.

> >  2 files changed, 138 insertions(+)
> > 
> > diff --git a/common/config b/common/config
> > index 005fd50a..9b8a2bc4 100644
> > --- a/common/config
> > +++ b/common/config
> 
> 100s as default seems a bit long to me, use 10s as in v1 patch?

In practice I tried using 10s and from my observations we *still* ran
into false positives. So from my own testing peace of mind I'd prefer at
least something higher, and if its going to be higher might as well go
with something which at least makes painfully safe. I'll go with 50s
for my next submission.

> > +	fi
> > +else
> > +	MODPROBE_RM_PATIENT_TIMEOUT_ARGS=""
> > +	if [[ ! -z "$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS" ]]; then
> > +		if [[ "$MODPROBE_PATIENT_RM_TIMEOUT_MS" != "forever" ]]; then
> 
> Should check MODPROBE_PATIENT_RM_TIMEOUT_SECONDS instead?

Indeed will fix.

> > diff --git a/common/module b/common/module
> > index 39e4e793..03953fa1 100644
> > --- a/common/module
> > +++ b/common/module
> > @@ -4,6 +4,8 @@
> >  #
> >  # Routines for messing around with loadable kernel modules
> >  
> > +source common/config
> > +
> 
> Seems there's no need to source common/config here, as it's sourced in
> common/rc, which is sourced by every test.

OK.

> > +	local max_tries_max=$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS
<-- snip -->

> > +	local max_tries=0
<-- snip -->

> > +	max_tries=$max_tries_max
> > +
> > +	while [[ "$max_tries" != "0" ]]; do
> 
> Use "$max_tries -ne 0" to check inters seems better.

max_tries can be "forever", in which case this is -eq 0:

$ foo="forever"; if [[ $foo -eq 0 ]]; then echo buggy; else echo ok; fi
buggy

> > +			refcnt_is_zero=1
> > +			break
> > +		fi
> > +		sleep 1
> > +		if [[ "$max_tries" == "forever" ]]; then
> > +			continue
> > +		fi
> > +		let max_tries=$max_tries-1
> > +	done
> > +
> > +	if [[ $refcnt_is_zero -ne 1 ]]; then
> > +		echo "custom patient module removal for $module timed out waiting for refcnt to become 0 using timeout of $max_tries_max"
> > +		return -1
> > +	fi
> > +
> > +	# If we ran out of time but our refcnt check confirms we had
> > +	# a refcnt of 0, just try to remove the module once.
> > +	if [[ "$max_tries" == "0" ]]; then
> 
> $max_tries -eq 0

Same issue.

  Luis