[PATCH 0/1 NOT FOR MERGE] Basic subdir tracking in nlink

[PATCH 0/1 NOT FOR MERGE] Basic subdir tracking in nlink

[Nikolay Borisov]

Here are the various bits and pieces needed to have a full-fledged subdir
tracking in nlink. The kernel code is complete and the accompanying fstests
proves that. Nevertheless, progs would need a bit more work, with the attached
progs patch I get only the following tests failing due to progs' nlink code:
	generic/077, generic/107, generic/498 and generic/547

At this point I won't be working anymore on this unless there is a really
compelling argument to do so, however I'm sending the patches now so that if
anyone should feel inclined to finish the work they they can base their efforts
on mine. At this point what's left is:

* Decide how old kernels are supposed to be supported - tree-checker won't allow
them too mount an fs whose hard link counter for directories is != 1. This would
either require an incompat_ro flag or a coordinated effort to backport the patch
to LTS kernels.

* The remaining fstests failures need to be investigated and fixed.

* btrfs-progs lowmem mode would likely consider a filesystem broken so it will
also need to be adjusted.

Nikolay Borisov (1):
  btrfs: Track subdirectories in nlink

 fs/btrfs/inode.c        | 13 +++++++++++--
 fs/btrfs/ioctl.c        | 10 +++++++---
 fs/btrfs/transaction.c  | 12 ++++++++----
 fs/btrfs/tree-checker.c |  7 +------
 4 files changed, 27 insertions(+), 15 deletions(-)

--
2.17.1

[PATCH] btrfs/218: Test for i_nlink tracking

[Nikolay Borisov]

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 tests/btrfs/218     | 170 ++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/218.out |   2 +
 tests/btrfs/group   |   1 +
 3 files changed, 173 insertions(+)
 create mode 100755 tests/btrfs/218
 create mode 100644 tests/btrfs/218.out

diff --git a/tests/btrfs/218 b/tests/btrfs/218
new file mode 100755
index 000000000000..571eca8a20e2
--- /dev/null
+++ b/tests/btrfs/218
@@ -0,0 +1,170 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2020 SUSE Linux Products GmbH. All Rights Reserved.
+#
+# FS QA Test 218
+#
+# Test that btrfs is correctly keeping track of link count for directories.
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs btrfs
+_supported_os Linux
+_require_test
+
+_scratch_mkfs > /dev/null 2>&1
+_scratch_mount
+
+# Ensure we have sane start value of 1
+linkcount=$(stat -c %h $SCRATCH_MNT)
+if [ $linkcount != 1 ]; then
+	echo "Unexpected default link count: $linkcount expected: 1"
+fi
+
+# moving/copying/deleting files shouldn't affect link count
+touch $SCRATCH_MNT/foo
+linkcount=$(stat -c %h $SCRATCH_MNT)
+if [ $linkcount != 1 ]; then
+	echo "Unexpected link count after file creation: $linkcount expected: 1"
+fi
+
+cp $SCRATCH_MNT/foo $SCRATCH_MNT/bar
+linkcount=$(stat -c %h $SCRATCH_MNT)
+if [ $linkcount != 1 ]; then
+	echo "Unexpected link count after file copy: $linkcount expected: 1"
+fi
+
+rm $SCRATCH_MNT/bar
+linkcount=$(stat -c %h $SCRATCH_MNT)
+if [ $linkcount != 1 ]; then
+	echo "Unexpected link count after file delete: $linkcount expected: 1"
+fi
+
+# move the file to oblivion
+mv $SCRATCH_MNT/foo /dev/null
+linkcount=$(stat -c %h $SCRATCH_MNT)
+if [ $linkcount != 1 ]; then
+	echo "Unexpected link count after file move: $linkcount expected: 1"
+fi
+
+# Add a dir and verify link count is incremented
+mkdir $SCRATCH_MNT/foo
+linkcount=$(stat -c %h $SCRATCH_MNT)
+if [ $linkcount != 2 ]; then
+	echo "Unexpected link count after dir create: $linkcount expected: 2"
+fi
+
+# Check if copy also increments the link count
+cp -r $SCRATCH_MNT/foo $SCRATCH_MNT/bar
+linkcount=$(stat -c %h $SCRATCH_MNT)
+if [ $linkcount != 3 ]; then
+	echo "Unexpected link count after dir copy: $linkcount expected: 3"
+fi
+
+# Now see if rename decrements the count of the src and increments the count 
+# of the dst
+mv $SCRATCH_MNT/bar $SCRATCH_MNT/foo/
+linkcount=$(stat -c %h $SCRATCH_MNT)
+if [ $linkcount != 2 ]; then
+	echo "Unexpected link count at src after dir move: $linkcount expected: 2"
+fi
+
+
+linkcount=$(stat -c %h $SCRATCH_MNT/foo)
+if [ $linkcount != 2 ]; then
+	echo "Unexpected link count at dst after dir move: $linkcount expected: 2"
+fi
+
+# Now delete a directory and see if count is correctly decremented
+mkdir $SCRATCH_MNT/bar
+rmdir $SCRATCH_MNT/bar
+linkcount=$(stat -c %h $SCRATCH_MNT)
+if [ $linkcount != 2 ]; then
+	echo "Unexpected link count after dir removal: $linkcount expected: 2"
+fi
+
+# Now create a subvolume, it's also a dir
+$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/subvol > /dev/null
+linkcount=$(stat -c %h $SCRATCH_MNT)
+if [ $linkcount != 3 ]; then
+	echo "Unexpected link count after subvol create: $linkcount expected: 3"
+fi
+
+mv $SCRATCH_MNT/subvol $SCRATCH_MNT/subvol1
+linkcount=$(stat -c %h $SCRATCH_MNT)
+if [ $linkcount != 3 ]; then
+	echo "Unexpected link count after subvol move in same dir: $linkcount expected: 3"
+fi
+
+# Move subvolume and see if src/dest counts are updated accordingly
+mv $SCRATCH_MNT/subvol1 $SCRATCH_MNT/foo/
+linkcount=$(stat -c %h $SCRATCH_MNT)
+if [ $linkcount != 2 ]; then
+	echo "Unexpected link count at src after subvol move: $linkcount expected: 2"
+fi
+
+linkcount=$(stat -c %h $SCRATCH_MNT/foo)
+if [ $linkcount != 3 ]; then
+	echo "Unexpected link count at dst after subvol move: $linkcount expected: 3"
+fi
+
+# Now delete it
+$BTRFS_UTIL_PROG subvolume delete $SCRATCH_MNT/foo/subvol1 > /dev/null
+linkcount=$(stat -c %h $SCRATCH_MNT/foo)
+if [ $linkcount != 2 ]; then
+	echo "Unexpected link count after subvol delete: $linkcount expected: 2"
+fi
+
+# snapshots are created in a different path
+$BTRFS_UTIL_PROG subvolume snapshot $SCRATCH_MNT $SCRATCH_MNT/snapshot1 > /dev/null
+linkcount=$(stat -c %h $SCRATCH_MNT)
+if [ $linkcount != 3 ]; then
+	echo "Unexpected link count after snapshot create: $linkcount expected: 3"
+fi
+
+# move snapshot 
+mv $SCRATCH_MNT/snapshot1 $SCRATCH_MNT/foo
+linkcount=$(stat -c %h $SCRATCH_MNT)
+if [ $linkcount != 2 ]; then 
+	echo "Unexpected link count after at src after snapshot move: $linkcount expected: 2"
+fi
+
+linkcount=$(stat -c %h $SCRATCH_MNT/foo)
+if [ $linkcount != 3 ]; then 
+	echo "Unexpected link count after at dst after snapshot move: $linkcount expected: 2"
+fi
+
+# delete snapshot
+$BTRFS_UTIL_PROG subvolume delete $SCRATCH_MNT/foo/snapshot1 > /dev/null
+linkcount=$(stat -c %h $SCRATCH_MNT/foo)
+if [ $linkcount != 2 ]; then 
+	echo "Unexpected link count after after snapshot delete: $linkcount expected: 2"
+fi
+echo "Silence is golden"
+
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/218.out b/tests/btrfs/218.out
new file mode 100644
index 000000000000..1ef372a2a24b
--- /dev/null
+++ b/tests/btrfs/218.out
@@ -0,0 +1,2 @@
+QA output created by 218
+Silence is golden
diff --git a/tests/btrfs/group b/tests/btrfs/group
index 8dcb23156236..94c1ec2806dc 100644
--- a/tests/btrfs/group
+++ b/tests/btrfs/group
@@ -220,3 +220,4 @@
 215 auto quick
 216 auto quick seed
 217 auto quick trim dangerous
+218 auto quick subvol
-- 
2.17.1

[PATCH 1/1] btrfs: Track subdirectories in nlink

[Nikolay Borisov]

This adds the necessary calls to inc_nlink so that the number of
subdirectories are accounted for in the i_nlink count of a directory. It
works also for subvolumes and snapshots. Unfortunately the state of the
on-disk i_nlink is codified in the tree checker so I had to remove the
check but such filesystems will refuse to mount on older kernels.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c        | 13 +++++++++++--
 fs/btrfs/ioctl.c        | 10 +++++++---
 fs/btrfs/transaction.c  | 12 ++++++++----
 fs/btrfs/tree-checker.c |  7 +------
 4 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 40fed3285f62..e82eb501fe0d 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3635,6 +3635,8 @@ static int __btrfs_unlink_inode(struct btrfs_trans_handle *trans,
 	inode_inc_iversion(&dir->vfs_inode);
 	inode->vfs_inode.i_ctime = dir->vfs_inode.i_mtime =
 		dir->vfs_inode.i_ctime = current_time(&inode->vfs_inode);
+	if (S_ISDIR(inode->vfs_inode.i_mode))
+		drop_nlink(&dir->vfs_inode);
 	ret = btrfs_update_inode(trans, root, &dir->vfs_inode);
 out:
 	return ret;
@@ -3798,9 +3800,12 @@ static int btrfs_unlink_subvol(struct btrfs_trans_handle *trans,
 	btrfs_i_size_write(BTRFS_I(dir), dir->i_size - name_len * 2);
 	inode_inc_iversion(dir);
 	dir->i_mtime = dir->i_ctime = current_time(dir);
+	drop_nlink(dir);
 	ret = btrfs_update_inode_fallback(trans, root, dir);
-	if (ret)
+	if (ret) {
 		btrfs_abort_transaction(trans, ret);
+		inc_nlink(dir);
+	}
 out:
 	btrfs_free_path(path);
 	return ret;
@@ -6137,9 +6142,13 @@ int btrfs_add_link(struct btrfs_trans_handle *trans,
 		parent_inode->vfs_inode.i_mtime = now;
 		parent_inode->vfs_inode.i_ctime = now;
 	}
+	if (S_ISDIR(inode->vfs_inode.i_mode))
+		inc_nlink(&parent_inode->vfs_inode);
 	ret = btrfs_update_inode(trans, root, &parent_inode->vfs_inode);
-	if (ret)
+	if (ret) {
 		btrfs_abort_transaction(trans, ret);
+		drop_nlink(&parent_inode->vfs_inode);
+	}
 	return ret;
 
 fail_dir_item:
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index f12c3df3c216..96b8daa8e8e1 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -716,21 +716,22 @@ static noinline int create_subvol(struct inode *dir,
 				    BTRFS_FT_DIR, index);
 	if (ret) {
 		btrfs_abort_transaction(trans, ret);
-		goto fail;
+		goto fail_drop_nlink;
 	}
 
 	btrfs_i_size_write(BTRFS_I(dir), dir->i_size + namelen * 2);
+	inc_nlink(dir);
 	ret = btrfs_update_inode(trans, root, dir);
 	if (ret) {
 		btrfs_abort_transaction(trans, ret);
-		goto fail;
+		goto fail_drop_nlink;
 	}
 
 	ret = btrfs_add_root_ref(trans, objectid, root->root_key.objectid,
 				 btrfs_ino(BTRFS_I(dir)), index, name, namelen);
 	if (ret) {
 		btrfs_abort_transaction(trans, ret);
-		goto fail;
+		goto fail_drop_nlink;
 	}
 
 	ret = btrfs_uuid_tree_add(trans, root_item->uuid,
@@ -738,6 +739,9 @@ static noinline int create_subvol(struct inode *dir,
 	if (ret)
 		btrfs_abort_transaction(trans, ret);
 
+fail_drop_nlink:
+	if (ret)
+		drop_nlink(dir);
 fail:
 	kfree(root_item);
 	trans->block_rsv = NULL;
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index a1a8a35263a9..7717c4f522aa 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1677,17 +1677,18 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
 					 dentry->d_name.len * 2);
 	parent_inode->i_mtime = parent_inode->i_ctime =
 		current_time(parent_inode);
+	inc_nlink(parent_inode);
 	ret = btrfs_update_inode_fallback(trans, parent_root, parent_inode);
 	if (ret) {
 		btrfs_abort_transaction(trans, ret);
-		goto fail;
+		goto fail_drop_nlink;
 	}
 	ret = btrfs_uuid_tree_add(trans, new_root_item->uuid,
 				  BTRFS_UUID_KEY_SUBVOL,
 				  objectid);
 	if (ret) {
 		btrfs_abort_transaction(trans, ret);
-		goto fail;
+		goto fail_drop_nlink;
 	}
 	if (!btrfs_is_empty_uuid(new_root_item->received_uuid)) {
 		ret = btrfs_uuid_tree_add(trans, new_root_item->received_uuid,
@@ -1695,16 +1696,19 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
 					  objectid);
 		if (ret && ret != -EEXIST) {
 			btrfs_abort_transaction(trans, ret);
-			goto fail;
+			goto fail_drop_nlink;
 		}
 	}
 
 	ret = btrfs_run_delayed_refs(trans, (unsigned long)-1);
 	if (ret) {
 		btrfs_abort_transaction(trans, ret);
-		goto fail;
+		goto fail_drop_nlink;
 	}
 
+fail_drop_nlink:
+	if (ret)
+		drop_nlink(parent_inode);
 fail:
 	pending->error = ret;
 dir_item_existed:
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index 517b44300a05..c353700b03c5 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -1015,12 +1015,7 @@ static int check_inode_item(struct extent_buffer *leaf,
 			return -EUCLEAN;
 		}
 	}
-	if (S_ISDIR(mode) && btrfs_inode_nlink(leaf, iitem) > 1) {
-		inode_item_err(leaf, slot,
-		       "invalid nlink: has %u expect no more than 1 for dir",
-			btrfs_inode_nlink(leaf, iitem));
-		return -EUCLEAN;
-	}
+
 	if (btrfs_inode_flags(leaf, iitem) & ~BTRFS_INODE_FLAG_MASK) {
 		inode_item_err(leaf, slot,
 			       "unknown flags detected: 0x%llx",
-- 
2.17.1

[PATCH] btrfs-progs: check: Support precise nlink tracking

[Nikolay Borisov]

This commit makes progs compatible with a kernel that has implemented
subdirectory tracking in nlink. To achieve this the logic is modified
such that found_link is incremented for the parent dir for every
DIR_ITEM found which points to a subdirectory. Another change is to
always set found_link to 1 when parsing INODE_ITEM which corresponds to
a subvolume/snapshot root. This is to account for the fact that such
inodes are the root in their respective fs trees so they won't have
their found_link count bumped when parsing their DIR_ITEM in the parent
directory.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 check/main.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/check/main.c b/check/main.c
index c622caa8e04f..eec18132f8e5 100644
--- a/check/main.c
+++ b/check/main.c
@@ -908,6 +908,8 @@ static int process_inode_item(struct extent_buffer *eb,
 	if (S_ISLNK(rec->imode) &&
 	    flags & (BTRFS_INODE_IMMUTABLE | BTRFS_INODE_APPEND))
 		rec->errors |= I_ERR_ODD_INODE_FLAGS;
+	if (S_ISDIR(rec->imode) && rec->ino == BTRFS_FIRST_FREE_OBJECTID)
+		rec->found_link = 1;
 	/*
 	 * We don't have accurate root info to determine the correct
 	 * inode generation uplimit, use super_generation + 1 anyway
@@ -1421,6 +1423,16 @@ static int process_dir_item(struct extent_buffer *eb,
 			goto next;
 		}
 
+		if ((location.type == BTRFS_INODE_ITEM_KEY ||
+			location.type == BTRFS_ROOT_ITEM_KEY) && filetype == BTRFS_FT_DIR &&
+				key->type == BTRFS_DIR_ITEM_KEY) {
+			struct inode_record *dir_rec = get_inode_rec(inode_cache,
+					key->objectid, 1);
+			BUG_ON(IS_ERR(rec));
+			dir_rec->found_link++;
+			maybe_free_inode_rec(inode_cache, dir_rec);
+		}
+
 		if (location.type == BTRFS_INODE_ITEM_KEY) {
 			add_inode_backref(inode_cache, location.objectid,
 					  key->objectid, key->offset, namebuf,
@@ -1893,7 +1905,7 @@ static int check_root_dir(struct inode_record *rec)
 		goto out;
 	}
 
-	if (rec->nlink != 1 || rec->found_link != 0) {
+	if (rec->nlink != rec->found_link) {
 		rec->errors |= I_ERR_LINK_COUNT_WRONG;
 		goto out;
 	}
-- 
2.17.1

Re: [PATCH 1/1] btrfs: Track subdirectories in nlink

[David Sterba]

On Thu, Aug 27, 2020 at 06:04:25PM +0300, Nikolay Borisov wrote:
> This adds the necessary calls to inc_nlink so that the number of
> subdirectories are accounted for in the i_nlink count of a directory. It
> works also for subvolumes and snapshots. Unfortunately the state of the
> on-disk i_nlink is codified in the tree checker so I had to remove the
> check but such filesystems will refuse to mount on older kernels.

Not refusing to mount but it would return EUCLEAN when accessing the
directory and probably turning the fs to read-only, aborting any
running transactions on the way.

> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/inode.c        | 13 +++++++++++--
>  fs/btrfs/ioctl.c        | 10 +++++++---
>  fs/btrfs/transaction.c  | 12 ++++++++----
>  fs/btrfs/tree-checker.c |  7 +------
>  4 files changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 40fed3285f62..e82eb501fe0d 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -3635,6 +3635,8 @@ static int __btrfs_unlink_inode(struct btrfs_trans_handle *trans,
>  	inode_inc_iversion(&dir->vfs_inode);
>  	inode->vfs_inode.i_ctime = dir->vfs_inode.i_mtime =
>  		dir->vfs_inode.i_ctime = current_time(&inode->vfs_inode);
> +	if (S_ISDIR(inode->vfs_inode.i_mode))
> +		drop_nlink(&dir->vfs_inode);

This should not be unconditional, only decrease links if it's not 1,
similarly the inc_nlink calls.