On Mon, Feb 07, 2022 at 05:10:24PM -0800, Bart Van Assche wrote:
> On 11/16/21 09:29, Luis Chamberlain wrote:
> > for a while now. More wider testig is welcomed.
> ^^^^^^
> testing?
>
> > @@ -427,14 +428,8 @@ stop_soft_rdma() {
> > echo "$i ..."
> > rdma link del "${i}" || echo "Failed to remove ${i}"
> > done
> > - if ! unload_module rdma_rxe 10; then
> > - echo "Unloading rdma_rxe failed"
> > - return 1
> > - fi
> > - if ! unload_module siw 10; then
> > - echo "Unloading siw failed"
> > - return 1
> > - fi
> > + _patient_rmmod rdma_rxe || return 1
> > + _patient_rmmod 1siw || return 1
>
> The above clearly has not been tested. There is an easy to spot typo in the
> above change. Please test your changes before publishing these.
Thanks, fixed. Please I stated:
I've tested blktests with this for things which I can run virtually
for a while now. More wider testig is welcomed.
> > + if [[ ! -z "$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS" ]]; then
>
> Please use -n instead of ! -z.
Sure...
> > + if [[ -f /sys/module/$module/refcnt ]]; then
>
> Has this patch been analyzed with 'make check'? I expect checkpatch to
> complain about missing double quotes for the above statement.
No, I was not aware this was a thing. I fixed all the stupid complaints
however I should note at least tests/srp/rc has existing complaints
before my changes. I won't address those.
I can't say I agree with the changes to remove checks for $?, but
whatever. Consinstancy is better than not having one. Perhaps we should
embrace this on fstests as well. Who knows how many crufty bugs lie
there.
> > +# Patiently tries to wait to remove a module by ensuring first
> ^^^^^^^^^^^^^^^^^^^^^^^
> Tries to wait patiently?
Sure..
> > + if [[ ! -z $MODPROBE_REMOVE_PATIENT ]]; then
>
> Please use -n instead of ! -z and surround the variable expansion with
> double quotes.
Sure..
> > + # If we have extra time left. Use the time left to now try to
> > + # persistently remove the module. We do this because although through
>
> What is persistent module removal? Is that perhaps removing a module such
> that it cannot be loaded again?
No, I'll clarify to this:
# Tries to wait patiently to remove a module by ensuring first
# the refcnt is 0 and then trying to remove the module over and over
# again within the time allowed
> > for m in ib_srpt iscsi_target_mod target_core_pscsi target_core_iblock \
> > target_core_file target_core_stgt target_core_user \
> > target_core_mod
> > - do
> > - unload_module $m 10 || return $?
> > - done
> > + _patient_rmmod $m || return $1
>
> Please proofread your changes and test your changes before posting these. I
> think that both "do" and "done" should have been preserved above.
And the return value shoudl be $? not $1. I couldn't test srp so wanted
someone who would to help review, but sure, I should have caught this.
Thanks for the review, I'll post a v2.
Luis
