Re: [PATCH blktests 2/2] Add a test that triggers blk_mq_update_nr_hw_queues()

[Omar Sandoval <osandov@osandov.com>]

On Thu, Oct 24, 2019 at 10:55:06AM -0700, Bart Van Assche wrote:
> On 10/24/19 10:42 AM, Omar Sandoval wrote:
> > On Mon, Oct 21, 2019 at 03:57:19PM -0700, Bart Van Assche wrote:
> > > +# Configure one null_blk instance.
> > > +configure_null_blk() {
> > > +	(
> > > +		cd /sys/kernel/config/nullb || return $?
> > > +		(
> > > +			mkdir -p nullb0 &&
> > > +				cd nullb0 &&
> > > +				echo 0 > completion_nsec &&
> > > +				echo 512 > blocksize &&
> > > +				echo 16 > size &&
> > > +				echo 1 > memory_backed &&
> > > +				echo 1 > power
> > > +		)
> > > +	) &&
> > > +	ls -l /dev/nullb* &>>"$FULL"
> > 
> > What's the point of these nested subshells? Can't this just be:
> > 
> > configure_null_blk() {
> > 	cd /sys/kernel/config/nullb &&
> > 	mkdir -p nullb0 &&
> > 	cd nullb0 &&
> > 	echo 0 > completion_nsec &&
> > 	echo 512 > blocksize &&
> > 	echo 16 > size &&
> > 	echo 1 > memory_backed &&
> > 	echo 1 > power &&
> > 	ls -l /dev/nullb* &>>"$FULL"
> > }
> 
> The above code is not equivalent to the original code because it does not
> restore the original working directory.
> 
> When using 'cd' inside a subshell, the working directory that was effective
> before the 'cd' command is restored automatically when exiting from the
> subshell. I prefer using 'cd' in a subshell instead of using pushd / popd
> because with the former approach the old working directory is restored no
> matter how the exit from the subshell happens.

Ah, right, I didn't pay enough attention to the cd. In that case, I'd
prefer not doing the cd at all. Something like:

configure_null_blk() {
	local nullb0="/sys/kernel/config/nullb/nullb0"
	mkdir -p "$nullb0" &&
	echo 0 > "$nullb0/completion_nsec" &&
	echo 512 > "$nullb0/blocksize" &&
	echo 16 > "$nullb0/size" &&
	echo 1 > "$nullb0/memory_backed" &&
	echo 1 > "$nullb0/power" &&
	ls -l /dev/nullb* &>>"$FULL"
}

> > > +modify_nr_hw_queues() {
> > > +	local deadline num_cpus
> > > +
> > > +	deadline=$(($(_uptime_s) + TIMEOUT))
> > > +	num_cpus=$(find /sys/devices/system/cpu -maxdepth 1 -name 'cpu[0-9]*' |
> > > +			   wc -l)
> > 
> > Please just use nproc. Or even better, can you just read the original
> > value of /sys/kernel/config/nullb/nullb0/submit_queues? Or does that
> > start at 1?
> 
> I will have a closer look and see which alternative works best.

Thanks!