On 2021/8/16 下午5:28, Nikolay Borisov wrote:

[...]

quoted

  	if (sectorsize != PAGE_SIZE) {
+		struct btrfs_subpage_info *subpage_info;
+
+		ASSERT(sectorsize < PAGE_SIZE);

nit: Simply make the check sectorsize < PAGE_SIZE and that renders the
assert redundant.

I guess that's the way to go.

Since I don't believe we will support multi-page sectorsize anyway, we
don't need to bother sectorsize > PAGE_SIZE forever.
Thus we can forget the sectorsize > PAGE_SIZE case.

[...]

quoted

+
+	subpage_info->total_nr_bits = cur;

So those values are really consts, however due to them being allocated
on the heap you can't simply define them as const and initialize them.
What a bummer...

Yeah, that's the limit of C.

On the other hand the namings are a bit generic, those are really
offsets into subpage->bitmaps. As such I'd prefer something along the
lines of
writeback_bitmap_offset
ordered_bitmap_offset etc.

The *_bitmap_offset seems a little long, especially in the 2nd patch
we're already introducing too many new lines just to handle the *_start
naming.

Especially all these members are used with thing like
fs_info->subpage_info->uptodate_start.

What about the name "uptodate_offset"?

Also I believe a graphical representation is in order i.e


[u][u][u][u][e][e][e][e][e]
^			  ^
|-uptodate_start	  |- error_start etc

That looks awesome.

Since it's a bit unexpected to have multiple, logically independent
bitmaps be tracked in the same physical location.

That's also I'm concerning of.

I haven't seen other code sides doing the same behavior.
IOmap just waste all the memory by going full u32 bitmap even for case
like 16K page size and 4K sectorsize, exactly I want to avoid.

Thanks,
Qu

quoted

+}
+
  int btrfs_attach_subpage(const struct btrfs_fs_info *fs_info,
  			 struct page *page, enum btrfs_subpage_type type)
  {

diff --git a/fs/btrfs/subpage.h b/fs/btrfs/subpage.h
index 9aa40d795ba9..ea90ba42c97b 100644
--- a/fs/btrfs/subpage.h
+++ b/fs/btrfs/subpage.h

@@ -11,6 +11,32 @@
   */
  #define BTRFS_SUBPAGE_BITMAP_SIZE	16

+/*
+ * Extra info for subpapge bitmap.
+ *
+ * For subpage we integrate all uptodate/error/dirty/writeback/ordered
+ * bitmaps into one larger bitmap.
+ * This structure records the basic info.
+ */
+struct btrfs_subpage_info {
+	/* Number of bits for each bitmap*/
+	unsigned int bitmap_nr_bits;
+
+	/* Total number of bits for the whole bitmap */
+	unsigned int total_nr_bits;
+
+	/*
+	 * *_start indicates where the bitmap starts, the length
+	 * is always @bitmap_size, which is calculated from
+	 * PAGE_SIZE / sectorsize.
+	 */
+	unsigned int uptodate_start;
+	unsigned int error_start;
+	unsigned int dirty_start;
+	unsigned int writeback_start;
+	unsigned int ordered_start;
+};
+
  /*
   * Structure to trace status of each sector inside a page, attached to
   * page::private for both data and metadata inodes.

@@ -53,6 +79,8 @@ enum btrfs_subpage_type {
  	BTRFS_SUBPAGE_DATA,
  };

+void btrfs_init_subpage_info(struct btrfs_subpage_info *subpage_info,
+			     u32 sectorsize);
  int btrfs_attach_subpage(const struct btrfs_fs_info *fs_info,
  			 struct page *page, enum btrfs_subpage_type type);
  void btrfs_detach_subpage(const struct btrfs_fs_info *fs_info,