Re: [PATCH 3/3] btrfs-progs: rootdir: reject hard links

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2024/8/1 07:47, Boris Burkov 写道:
> On Wed, Jul 31, 2024 at 07:08:48PM +0930, Qu Wenruo wrote:
>> Hard link detection needs extra memory to save all the inodes hits with
>> extra nlinks.
>>
>> There is no such support from the very beginning, and our code will just
>> create two different inodes, ignoring the extra links completely.
>
> I don't understand exactly what this means.

I mean, if we have the following layout as source root dir

rootdir
|- dir1
|  |- file1 (ino 1024)
|- dir2
    |- file2 (ino 1024)

Then the resulted btrfs would looks like this:

.
|- dir1
|  |- file 1 (ino 257)
|- dir2
    |- file 2 (ino 259)

Making them different inodes.

>
> How does the code you are replacing handle hardlinks? As far as I can
> tell (far from an expert...) it looks like it does handle them, so
> explicitly rejecting them now is a regression?

Sorry, the new code doesn't handle hardlinks, they are treated as
different inodes.

As the new code always grab a new ino number for each file.

Although things like btrfs_add_link() can handle extra hard links, we
always treat every inode we hit as a new one.


And yes, it is a regression. The old code skips the ino number detection
by reusing the old ino from the host fs, which introduced two bugs (that
we didn't notice until now):

- If rootdir crosses mount points, we can have conflicting ino
   And screw up things easily

- If rootdir has an inode with hardlinks out of rootdir
   Then the resulted fs will have the same nlink number, mismatching
   the correct number.

   Just like this:

   # mkfs rootfs
   # touch rootfs/file1
   # ln rootfs/file1 file1
   # mkfs.btrfs -f --rootdir rootfs $dev
   # btrfs check #dev
   ...
   [4/7] checking fs roots
   root 5 inode 88347536 errors 2000, link count wrong
	unresolved ref dir 256 index 2 namelen 5 name file1 filetype 1 errors 0
   ERROR: errors found in fs roots
   found 147456 bytes used, error(s) found

So for now, I'll go with the more robust and correct code, with the cost
of buggy hard links support.

>
>>
>> Considering the most common use case (populating a rootfs), there is not
>> much distro doing hard links intentionally, it should be pretty safe.
>>
>> Just error out if we hit such hard links.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   mkfs/rootdir.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/mkfs/rootdir.c b/mkfs/rootdir.c
>> index d8bd2ce29d5a..babb9d102d6a 100644
>> --- a/mkfs/rootdir.c
>> +++ b/mkfs/rootdir.c
>> @@ -418,6 +418,18 @@ static int ftw_add_inode(const char *full_path, const struct stat *st,
>>   	u64 ino;
>>   	int ret;
>>
>> +	/*
>> +	 * Hard link need extra detection code, not support for now.
>> +	 *
>> +	 * st_nlink on directories is fs specific, so only check it on
>> +	 * non-directory inodes.
>> +	 */
>> +	if (unlikely(!S_ISDIR(st->st_mode) && st->st_nlink > 1)) {
>> +		error("'%s' has extra hard links, not supported for now",
>> +			full_path);
>
> I would change the message to something like:
> "inodes with hard links are not supported"

Thanks for the advice, I'll go with this new message, along with some
new test cases for the existing hardlink and mount point bugs.

Thanks,
Qu

>
> Thanks,
> Boris
>
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>>   	/* The rootdir itself. */
>>   	if (unlikely(ftwbuf->level == 0)) {
>>   		u64 root_ino = btrfs_root_dirid(&root->root_item);
>> --
>> 2.45.2
>>
>