Re: [PATCH RFC] btrfs-progs: mkfs: optimize file descriptor usage in mkfs.btrfs

[Josef Bacik <josef@toxicpanda.com>]

On Wed, Jan 31, 2024 at 11:49:28PM +0800, Anand Jain wrote:
> I've reproduced the missing udev events issue without device partitions
> using the test case as below. The test waits for the creation of 'by-uuid'
> and, waiting indefinitely means successful reproduction. as below:
> 
> --------------------------------------------------
> #!/bin/bash
> usage()
> {
> 	echo
> 	echo "Usage: ./t1 sdb btrfs"
> 	exit 1
> }
> 
> : ${1?"arg1 <dev> is missing"} || usage
> dev=$1
> 
> : ${2?"arg2 <fstype> is missing"} || usage
> fstype=$2
> 
> systemd=$(rpm -q --queryformat='%{NAME}-%{VERSION}-%{RELEASE}' systemd)
> 
> run_testcase()
> {
> 	local cnt=$1
> 	local ret=0
> 	local sleepcnt=0
> 
> 	local newuuid=""
> 	local logpid=""
> 	local log=""
> 	local logfile="./udev_log_${systemd}_${fstype}.out"
> 
> 	>$logfile
> 
> 	wipefs -a /dev/${dev}* &>/dev/null
> 	sync
> 	udevadm settle /dev/$dev
> 
> 	udevadm monitor -k -u -p > $logfile &
> 	logpid=$!
> 	>strace.out
> 	run "strace -f -ttT -o strace.out mkfs.$fstype -q -f /dev/$dev"
> 
> 	newuuid=$(blkid -p /dev/$dev | awk '{print $2}' | sed -e 's/UUID=//' -e 's/\"//g')
> 
> 	kill $logpid
> 	sync $logfile
> 
> 	ret=-1
> 	while [ $ret != 0 ]
> 	do
> 		run -s -q "ls -lt /dev/disk/by-uuid | grep $newuuid"
> 		ret=$?
> 		((sleepcnt++))
> 		sleep 1
> 	done
> 
> 	#for systemd-239-78.0.3.el8
> 	log=$(cat $logfile|grep ID_FS_TYPE_NEW)
> 	#for systemd-252-18.0.1.el9.x86_64
> 	#log=$(grep --text "ID_FS_UUID=${newuuid}" $logfile)
> 
> 	echo $cnt sleepcnt=$sleepcnt newuuid=$newuuid ret=$ret log=$log
> }
> 
> echo Test case: t1: version 3.
> echo
> 
> run -o cat /etc/system-release
> run -o uname -a
> run -o rpm -q systemd
> if [ $fstype == "btrfs" ]; then
> 	run -o mkfs.btrfs --version
> elif [ $fstype == "xfs" ]; then
> 	run -o mkfs.xfs -V
> else
> 	echo unknown fstype $fstype
> fi
> echo
> 
> for ((cnt = 0; cnt < 13; cnt++)); do
> 	run_testcase $cnt
> done
> -----------------------------------------------------------------
> 
> 
> It appears that the problem is due to the convoluted nested device open
> and device close in mkfs.btrfs as shown below:
> 
> -------------
>  prepare_ctx opens all devices <-- fd1
>    zero the super-block
>    writes temp-sb to the first device.
> 
>  open_ctree opens the first device again <-- fd2
> 
>  prepare_ctx writes temp-sb to all the remaining devs <-- fd1
> 
>  fs_info->finalize_on_close = 1;
>  close_ctree_fs_info()<-- writes valid <--- fd2
> 
>  prepare_ctx is closed <--- fd1.
> -------------
> 
> This cleanup patch reuses the main file descriptor (fd1) in open_ctree(),
> and with this change both the test cases (with partition and without
> partition) now runs fine.
> 
> I've done an initial tests only, not validated with the multi-device mkfs.
> More cleanup is possible but pending feedback;  marking this patch as an RFC.

I'd like to see the cleaned up version of this patch, but I have a few comments.

1) I think re-using the fd is reasonable, tho could this just be reworked to
   create the temp-sb and write this to all the devs, close the file
   descriptors, and then call open_ctree?

2) I hate adding another thing into a core file that we'll have to figure out
   how to undo later as we sync more code from the kernel into btrfs-progs, I'm
   not sure if there's a way around this, but thinking harder about adding
   something to disk-io.c that is for userspace only would be good.

Thanks,

Josef