Skip to contentSkip to conversation

btrfs: Embed sector size check into BTRFS_MAX_INLINE_DATA_SIZE()

PATCH In review Qu Wenruo 2 participants 4 messages Mar 2, 2018 Btrfs file system

lore
Share & tools
lore

Qu Wenruo

author
#0 lore raw

PATCH 1/3 btrfs: Embed sector size check into BTRFS_MAX_INLINE_DATA_SIZE()

We have extra sector size check in cow_file_range_inline(), but doesn't implement it in BTRFS_MAX_INLINE_DATA_SIZE().

The biggest reason is that btrfs_symlink() also uses this macro to check name length.

In fact such behavior makes max_inline calculation quite confusing, and cause unexpected large extent for symbol link.

Here we embed sector size check into BTRFS_MAX_INLINE_DATA_SIZE() so that it will never exceed sector size.

The downside is, for symbol link, we will reduce max symbol link length from 16K- to 4095, but it won't affect current system using that long name, but only prevent later creation.

---
 fs/btrfs/ctree.h | 5 +++--
 fs/btrfs/inode.c | 1 -
 2 files changed, 3 insertions(+), 3 deletions(-)
fs/btrfs/ctree.h +3 -2
index 13c260b525a1..90948096c00f 100644
@@ -1297,8 +1297,9 @@ static inline u32 BTRFS_NODEPTRS_PER_BLOCK(const struct btrfs_fs_info *info)
 		(offsetof(struct btrfs_file_extent_item, disk_bytenr))
 static inline u32 BTRFS_MAX_INLINE_DATA_SIZE(const struct btrfs_fs_info *info)
 {
-	return BTRFS_MAX_ITEM_SIZE(info) -
-	       BTRFS_FILE_EXTENT_INLINE_DATA_START;
+	return min_t(u32, info->sectorsize - 1,
+		     BTRFS_MAX_ITEM_SIZE(info) -
+		     BTRFS_FILE_EXTENT_INLINE_DATA_START);
 }
 
 static inline u32 BTRFS_MAX_XATTR_SIZE(const struct btrfs_fs_info *info)
fs/btrfs/inode.c +0 -1
index e1a7f3cb5be9..0f7041e10c67 100644
@@ -299,7 +299,6 @@ static noinline int cow_file_range_inline(struct btrfs_root *root,
 
 	if (start > 0 ||
 	    actual_end > fs_info->sectorsize ||
-	    data_len > BTRFS_MAX_INLINE_DATA_SIZE(fs_info) ||
 	    (!compressed_size &&
 	    (actual_end & (fs_info->sectorsize - 1)) == 0) ||
 	    end + 1 < isize ||
Signature · 1 line
2.16.2
author
#1 lore raw

PATCH 2/3 btrfs: Unify inline extent creation condition for plain and compressed data

cow_file_range_inline() used different condition for plain and compressed data.

For compressed data, it's allowed to have inline extent equal to sectorsize, while for plain data, it's not allowed to have inline extent equal to sectorsize.

But since we limit BTRFS_MAX_INLINE_DATA_SIZE() to (sectorsize - 1), there is no such difference any long, just remove the extra check.

---
 fs/btrfs/inode.c | 2 --
 1 file changed, 2 deletions(-)
fs/btrfs/inode.c +0 -2
index 0f7041e10c67..8c5e69bdbfb5 100644
@@ -299,8 +299,6 @@ static noinline int cow_file_range_inline(struct btrfs_root *root,
 
 	if (start > 0 ||
 	    actual_end > fs_info->sectorsize ||
-	    (!compressed_size &&
-	    (actual_end & (fs_info->sectorsize - 1)) == 0) ||
 	    end + 1 < isize ||
 	    data_len > fs_info->max_inline) {
 		return 1;
Signature · 1 line
2.16.2
author
#2 lore raw

PATCH 3/3 btrfs: Show more accurate max_inline

In reply to Qu Wenruo

Btrfs shows max_inline option into kernel message, but for max_inline=4096, btrfs won't really inline 4096 bytes inline data if it's not compressed.

Since we have unified the behavior and now BTRFS_MAX_INLINE_DATA_SIZE() should handle most of the condition check, just limit fs_info->max_inline to BTRFS_MAX_INLINE_DATA_SIZE(), so we could have more accurate max_inline output.

---
 fs/btrfs/super.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/btrfs/super.c +2 -2
index 3a4dce153645..6685016bc0ec 100644
@@ -618,8 +618,8 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 
 				if (info->max_inline) {
 					info->max_inline = min_t(u64,
-						info->max_inline,
-						info->sectorsize);
+					info->max_inline,
+					BTRFS_MAX_INLINE_DATA_SIZE(info));
 				}
 				btrfs_info(info, "max_inline at %llu",
 					   info->max_inline);
Signature · 1 line
2.16.2
#3 lore raw
In reply to Qu Wenruo

This patch along with any one uses sector/node size in btrfs_parse_options() should only use them after it get initialized.

Unfortunately at this point, nodesize is not initialized in open_ctree().

So unfortunately, this patch needs extra work to move btrfs_parse_options() after basic fs_info initialization, and please ignore this version.

Thanks,
Qu

Quoted 51 lines · Qu Wenruo

We have extra sector size check in cow_file_range_inline(), but doesn't implement it in BTRFS_MAX_INLINE_DATA_SIZE().

The biggest reason is that btrfs_symlink() also uses this macro to check name length.

In fact such behavior makes max_inline calculation quite confusing, and cause unexpected large extent for symbol link.

Here we embed sector size check into BTRFS_MAX_INLINE_DATA_SIZE() so that it will never exceed sector size.

The downside is, for symbol link, we will reduce max symbol link length from 16K- to 4095, but it won't affect current system using that long name, but only prevent later creation.

---
 fs/btrfs/ctree.h | 5 +++--
 fs/btrfs/inode.c | 1 -
 2 files changed, 3 insertions(+), 3 deletions(-)
fs/btrfs/ctree.h +3 -2
index 13c260b525a1..90948096c00f 100644
@@ -1297,8 +1297,9 @@ static inline u32 BTRFS_NODEPTRS_PER_BLOCK(const struct btrfs_fs_info *info)
 		(offsetof(struct btrfs_file_extent_item, disk_bytenr))
 static inline u32 BTRFS_MAX_INLINE_DATA_SIZE(const struct btrfs_fs_info *info)
 {
-	return BTRFS_MAX_ITEM_SIZE(info) -
-	       BTRFS_FILE_EXTENT_INLINE_DATA_START;
+	return min_t(u32, info->sectorsize - 1,
+		     BTRFS_MAX_ITEM_SIZE(info) -
+		     BTRFS_FILE_EXTENT_INLINE_DATA_START);
 }
 
 static inline u32 BTRFS_MAX_XATTR_SIZE(const struct btrfs_fs_info *info)
fs/btrfs/inode.c +0 -1
index e1a7f3cb5be9..0f7041e10c67 100644
@@ -299,7 +299,6 @@ static noinline int cow_file_range_inline(struct btrfs_root *root,
 
 	if (start > 0 ||
 	    actual_end > fs_info->sectorsize ||
-	    data_len > BTRFS_MAX_INLINE_DATA_SIZE(fs_info) ||
 	    (!compressed_size &&
 	    (actual_end & (fs_info->sectorsize - 1)) == 0) ||
 	    end + 1 < isize ||