| Message ID | 7984ff20beeb81268f786234d30d3c24d90a9a5b.1722418505.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: rework how we traverse rootdir | expand |
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. 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? > > 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, Boris > + return -EOPNOTSUPP; > + } > + > /* The rootdir itself. */ > if (unlikely(ftwbuf->level == 0)) { > u64 root_ino = btrfs_root_dirid(&root->root_item); > -- > 2.45.2 >
在 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 >> >
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); + return -EOPNOTSUPP; + } + /* The rootdir itself. */ if (unlikely(ftwbuf->level == 0)) { u64 root_ino = btrfs_root_dirid(&root->root_item);
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. 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(+)