On 2021/1/24 上午3:37, David Sterba wrote:

On Wed, Jan 20, 2021 at 08:19:14AM +0800, Qu Wenruo wrote:

quoted

On 2021/1/20 上午12:06, David Sterba wrote:

quoted

On Tue, Jan 19, 2021 at 04:51:45PM +0100, David Sterba wrote:

quoted

On Tue, Jan 19, 2021 at 06:54:28AM +0800, Qu Wenruo wrote:

quoted

On 2021/1/19 上午6:46, David Sterba wrote:

quoted

On Sat, Jan 16, 2021 at 03:15:18PM +0800, Qu Wenruo wrote:

quoted

+		return;
+
+	subpage = (struct btrfs_subpage *)detach_page_private(page);
+	ASSERT(subpage);
+	kfree(subpage);
+}

diff --git a/fs/btrfs/subpage.h b/fs/btrfs/subpage.h
new file mode 100644
index 000000000000..96f3b226913e
--- /dev/null
+++ b/fs/btrfs/subpage.h

@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef BTRFS_SUBPAGE_H
+#define BTRFS_SUBPAGE_H
+
+#include <linux/spinlock.h>
+#include "ctree.h"

So subpage.h would pull the whole ctree.h, that's not very nice. If
anything, the .c could include ctree.h because there are lots of the
common structure and function definitions, but not the .h. This creates
unnecessary include dependencies.

Any pointer type you'd need in structures could be forward declared.

Unfortunately, the main needed pointer is fs_info, and we're accessing
it pretty frequently (mostly for sector/node size).

I don't believe forward declaration would help in this case.

I've looked at the final subpage.h and you add way too many static
inlines that don't seem to be necessary for the reasons the static
inlines are supposed to be used.

The only file that includes subpage.h is extent_io.c, so as long as it
stays like that it's manageable. But untangling the include hell still
needs to hapen some day and new code that makes it harder worries me.

If going through the github branch, you will see there are more files
using subpage.h:
- extent_io.c
- disk-io.c
- file.c
- inode.c
- reflink.c
- relocation.c

And furthermore, about the static inline abuse, the part really need
that static inline is the check against regular sector size, and
unfortunately, most outside callers need such check.

I can put the pure subpage callers into subpage.c, but the generic
helpers handling both cases still need that.

I had a look and this is too much. Just by counting 'static inline'
(wher it's also part of the btrfs_page_clamp_* helpers) it's 30 and not
all the functions are short enough for static inlines. Please make them
all regular functions and put them to subpage.c and don't include
ctree.h.

OK, I'll go that direction for next update.

Thanks,
Qu