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

[Anand Jain <anand.jain@oracle.com>]

On 2/2/24 00:48, David Sterba wrote:
> On Wed, Jan 31, 2024 at 03:48:00PM -0500, Josef Bacik wrote:
>> On Wed, Jan 31, 2024 at 11:49:28PM +0800, Anand Jain wrote:
>>> 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?


Multiple sequential open-close maybe fine.

> This would trigger the udev unnecessarily and could let other processes
> to eg. try to mount the device (like systemd did or maybe still does).

> This could fail the second open_ctree.

I think you are missing the point that when we write the temp-sb, there 
is no valid BTRFS magic. As a result, the kernel won't get the scan, and 
systemd won't attempt to mount.

However, there will be a udev event (due to the write-close), and in 
this udev event, we shall have:

ID_FS_UUID=<blank>

This action cleans the /dev/disk/by-uuid directory, which is fine.

> That it's all done with one fd
> open reduces possible interactions that could be problematic.

Hmm reduced possible interactions is problematic? I didn't get this.

Here is the sequence of events to help discuss:

Consider a single device, for now:

  fd1 writes zero and temp-sb.
  fd2 writes good-sb.
  fd2 closes.
  fd1 closes.

Please note that the fd which wrote zero/temp-sb (fd1) closes last.

Per udevadm monitor the change event OR the UUID is missing (depending 
on the systemd version).

ID_FS_UUID=<blank> instead of ID_FS_UUID=<UUID>

>> 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.
> 
> Yeah the open_ctree functions in progs are misplaced in disk-io.c and
> are there for historical reasons. We'd need separate file (or maybe
> whole compat directory) but for now let it be there, the cleanup is
> going to be big.


Thanks, Anand