Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 5 May 2023 00:00:37 +0200
From:   Petr Vorel <pvorel@suse.cz>
To:     Cyril Hrubis <chrubis@suse.cz>
Cc:     ltp@lists.linux.it, NeilBrown <neilb@suse.de>,
        linux-nfs@vger.kernel.org, Jeff Layton <jlayton@kernel.org>
Subject: Re: [PATCH v5 5/5] nfs: Run on btrfs, ext4, xfs
Message-ID: <20230504220037.GB4244@pevik>
Reply-To: Petr Vorel <pvorel@suse.cz>
References: <20230504131414.3826283-1-pvorel@suse.cz>
 <20230504131414.3826283-6-pvorel@suse.cz>
 <ZFO6ywouPkmNKtkr@yuki>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZFO6ywouPkmNKtkr@yuki>
Precedence: bulk

> Hi!
> >  nfs_get_remote_path()
> > @@ -210,6 +213,7 @@ nfs_cleanup()
> >  		grep -q "$local_dir" /proc/mounts && umount $local_dir
> >  		n=$(( n + 1 ))
> >  	done
> > +	sleep 2

> >  	n=0
> >  	for i in $VERSION; do
> > @@ -219,12 +223,15 @@ nfs_cleanup()
> >  		if tst_net_use_netns; then
> >  			if test -d $remote_dir; then
> >  				exportfs -u *:$remote_dir
> > +				sleep 1
> >  				rm -rf $remote_dir
> >  			fi
> >  		else
> >  			tst_rhost_run -c "test -d $remote_dir && exportfs -u *:$remote_dir"
> > +			sleep 1
> >  			tst_rhost_run -c "test -d $remote_dir && rm -rf $remote_dir"
> >  		fi
> > +
> >  		n=$(( n + 1 ))
> >  	done

> Generally I'm not happy about the sleeps in the code, the main problem
> is that the test still may fail in a case that something was slower than
> usuall and we decided to proceed after we slept for a pre-defined
> interval. Ideally these should be replaced with a active waiting, i.e.
> loop that checks some condition 10 times in a second or so. Hoewever I'm
> okay with getting this in so that we manage to have these tests in
> before the release and fixing it later on.

I wonder myself what is wrong and if there is a problem with my code (likely) or
with nfsd or with loop device (Christoph Hellwig has been changing
drivers/block/loop.c quite a lot).

Because if first umount is too early, it does not recover (i.e. tst_umount
contain 50 times trying to umount with 100ms, and that does not help).

I also wonder how should I detect it's ready to be umounted.
I'll have look if losetup "fuser -vm /dev/loop0" would help, but instead of
depending on the device, we'd probably want to look into /proc/mounts, right?
Tomorrow I'll test this:

check_umount()
{
	local i
	local dir="$1"
	local type="${2:-lhost}"
	local cmd="grep -q $dir /proc/mounts"

	for i in $(seq 50); do
		if [ "$type" = "lhost" ]; then
			$cmd || return
		else
			tst_rhost_run -c "$cmd" || return
		fi
		tst_sleep 100ms
		tst_res TWARN "failed to umount '$dir'"
	done
}

	for i in $VERSION; do
		local_dir="$(get_local_dir $i $n)"
		grep -q "$local_dir" /proc/mounts && umount $local_dir

		# instead of 'sleep 2' below check here:
		check_umount "$local_dir"

		n=$(( n + 1 ))
	done

	n=0
	for i in $VERSION; do
		type=$(get_socket_type $n)
		remote_dir="$(get_remote_dir $i $type)"

		if tst_net_use_netns; then
			if test -d $remote_dir; then
				exportfs -u *:$remote_dir
				check_umount "$remote_dir" # instead of sleep 1
				rm -rf $remote_dir
			fi
		else
			tst_rhost_run -c "test -d $remote_dir && exportfs -u *:$remote_dir"
			check_umount "$remote_dir" rhost # instead of sleep 1
			tst_rhost_run -c "test -d $remote_dir && rm -rf $remote_dir"
		fi

		n=$(( n + 1 ))
	done

[1] https://gitlab.com/psmisc/psmisc/-/blob/master/src/fuser.c