On Fri, Jul 01, 2022 at 11:13:21AM -0700, Lucas De Marchi wrote: > On Fri, Jul 01, 2022 at 10:49:10AM -0700, Luis Chamberlain wrote: > > On Thu, Jun 30, 2022 at 03:33:23PM -0700, Lucas De Marchi wrote: > > > On Thu, Jun 30, 2022 at 03:09:32PM -0700, Luis Chamberlain wrote: > > > > Sorry for the super late review, I was swamped. OK so the only issue > > > > I can think of is that rmmod *used* to support the kernel wait support > > > > with $(rmmod --wait) so wouldn't this be odd? > > > > > > any reason not to use modprobe -r? > > > > I was referring to old scripts which may have used $(rmmod --wait) before. > > But since support for that was ripped, then yeah I can see that should > > not be an issue. > > > > However I can think of *one* issue, did we ever support `modprobe--wait`? > > no > > > > > Because the way fstests / blktests would implement this feature > > detection is with something like this now: > > > > _has_modprobe_patient() > > { > > modprobe --help >& /dev/null || return 1 > > modprobe --help | grep -q -1 "remove-patiently" || return 1 > > return 0 > > } > > the grep would need to be changed to something like "-w, --wait" Yes of course, that's easy, but we modprobe ever supportedl "--wait" this would be a problem. Since it did not, we are good then! > > > > It is why I had gone with: > > > > > > > > -p | --remove-patiently patiently removes the module > > > > -t | --timeout timeout in ms to remove the module > > > > > > > > You would know better though. > > > > > > > > Also just curious, is it really terrible to just support waiting > > > > forever? > > > > > > is there a use case for that? If we are trying to cover some races, I > > > imagine a small timeout would be sufficient. Also notice that if the > > > timeout is too big, so will be the interval between the retries. On > > > your v2 I had suggested polling the refcnt so we would get notificed > > > on changes, but as you also noticed, that didn't work very well. So I > > > went back to a time-based retry solution. > > > > > > if there is a use-case, should we cap the interval between retries? > > > > I really can't think of a use case except for catching glaring > > unexpected bugs in test suites where the kernel developer would > > really like to know something really bad happened, but even then > > a timeout is likely desirable. > > yeah... if developer wants to possibly wait for a long time, `--wait -1` > is available to wait for years. :) good call > > So just a heads up the timeout I'll use for fstests / blktests will be > > of 100 seconds. > > yeah... 100 seconds still make more sense than -1 IMO Great we'll go with that. Hopefully this will put a nail on the flaky modules issue for good! Luis