Btrfs: set default max_inline to 8KiB instead of 8MiB

PATCH In review Filipe David Borba Manana 4 participants 6 messages Aug 8, 2013 – Aug 14, 2013 Btrfs file system

Share & tools

Filipe David Borba Manana

author

2013-08-08 21:45 UTC #0 lore raw

PATCH 1/1 Btrfs: set default max_inline to 8KiB instead of 8MiB

8MiB is way too large and likely set by mistake. This is not a significant issue as in practice the max amount of data added to an inline extent is also limited by the page cache and btree leaf sizes.

Signed-off-by Filipe David Borba Manana <[email protected]>

---
 fs/btrfs/disk-io.c |    2 +-
 fs/btrfs/disk-io.h |    2 ++
 fs/btrfs/super.c   |    2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

fs/btrfs/disk-io.c +1 -1

index 5de9ad7..aff37bd 100644
@@ -2189,7 +2189,7 @@ int open_ctree(struct super_block *sb,
 	atomic_set(&fs_info->defrag_running, 0);
 	atomic64_set(&fs_info->tree_mod_seq, 0);
 	fs_info->sb = sb;
-	fs_info->max_inline = 8192 * 1024;
+	fs_info->max_inline = BTRFS_DEFAULT_MAX_INLINE;
 	fs_info->metadata_ratio = 0;
 	fs_info->defrag_inodes = RB_ROOT;
 	fs_info->free_chunk_space = 0;

fs/btrfs/disk-io.h +2 -0

index b71acd6e..e76c1a2 100644
@@ -25,6 +25,8 @@
 #define BTRFS_SUPER_MIRROR_MAX	 3
 #define BTRFS_SUPER_MIRROR_SHIFT 12
 
+#define BTRFS_DEFAULT_MAX_INLINE 8192
+
 enum {
 	BTRFS_WQ_ENDIO_DATA = 0,
 	BTRFS_WQ_ENDIO_METADATA = 1,

fs/btrfs/super.c +1 -1

index 1967903..7359a9e 100644
@@ -941,7 +941,7 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry)
 		seq_puts(seq, ",nodatacow");
 	if (btrfs_test_opt(root, NOBARRIER))
 		seq_puts(seq, ",nobarrier");
-	if (info->max_inline != 8192 * 1024)
+	if (info->max_inline != BTRFS_DEFAULT_MAX_INLINE)
 		seq_printf(seq, ",max_inline=%llu",
 			   (unsigned long long)info->max_inline);
 	if (info->alloc_start != 0)

Signature · 1 line

1.7.9.5

David Sterba

Reviewed-by on v1 1/1

2013-08-08 22:05 UTC #1 lore raw

Quoted 6 lines · Filipe David Borba Manana

8MiB is way too large and likely set by mistake. This is not a significant issue as in practice the max amount of data added to an inline extent is also limited by the page cache and btree leaf sizes.
Signed-off-by Filipe David Borba Manana <[email protected]>

Reviewed-by David Sterba <[email protected]>

Miao Xie

2013-08-09 05:46 UTC #2 lore raw

In reply to Filipe David Borba Manana

Quoted 4 lines · Filipe David Borba Manana

8MiB is way too large and likely set by mistake. This is not a significant issue as in practice the max amount of data added to an inline extent is also limited by the page cache and btree leaf sizes.

I don't think 8KB is a reasonable value of the default max inline size because it makes no sense on the machine whose page size is 4KB.

I think 4KB is a reasonable value, because we may mount the fs on the machines with the different page size in the future, in order to avoid the compatible problem, we should use the min page size as the max inline size.

Thanks
Miao

Quoted 47 lines

Signed-off-by Filipe David Borba Manana <[email protected]>

---
 fs/btrfs/disk-io.c |    2 +-
 fs/btrfs/disk-io.h |    2 ++
 fs/btrfs/super.c   |    2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

fs/btrfs/disk-io.c +1 -1

index 5de9ad7..aff37bd 100644
@@ -2189,7 +2189,7 @@ int open_ctree(struct super_block *sb,
 	atomic_set(&fs_info->defrag_running, 0);
 	atomic64_set(&fs_info->tree_mod_seq, 0);
 	fs_info->sb = sb;
-	fs_info->max_inline = 8192 * 1024;
+	fs_info->max_inline = BTRFS_DEFAULT_MAX_INLINE;
 	fs_info->metadata_ratio = 0;
 	fs_info->defrag_inodes = RB_ROOT;
 	fs_info->free_chunk_space = 0;

fs/btrfs/disk-io.h +2 -0

index b71acd6e..e76c1a2 100644
@@ -25,6 +25,8 @@
 #define BTRFS_SUPER_MIRROR_MAX	 3
 #define BTRFS_SUPER_MIRROR_SHIFT 12
 
+#define BTRFS_DEFAULT_MAX_INLINE 8192
+
 enum {
 	BTRFS_WQ_ENDIO_DATA = 0,
 	BTRFS_WQ_ENDIO_METADATA = 1,

fs/btrfs/super.c +1 -1

index 1967903..7359a9e 100644
@@ -941,7 +941,7 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry)
 		seq_puts(seq, ",nodatacow");
 	if (btrfs_test_opt(root, NOBARRIER))
 		seq_puts(seq, ",nobarrier");
-	if (info->max_inline != 8192 * 1024)
+	if (info->max_inline != BTRFS_DEFAULT_MAX_INLINE)
 		seq_printf(seq, ",max_inline=%llu",
 			   (unsigned long long)info->max_inline);
 	if (info->alloc_start != 0)

David Sterba

2013-08-09 14:46 UTC #3 lore raw

Quoted 8 lines · Miao Xie

Quoted 4 lines · Filipe David Borba Manana
8MiB is way too large and likely set by mistake. This is not a significant issue as in practice the max amount of data added to an inline extent is also limited by the page cache and btree leaf sizes.
I don't think 8KB is a reasonable value of the default max inline size because it makes no sense on the machine whose page size is 4KB.

Page size limit is artificial implied by the implementation and on a 4k page machine max_inline does not take place. Even if it's 4k, it does not apply because the leaf space is smaller.

Quoted 4 lines

I think 4KB is a reasonable value, because we may mount the fs on the machines with the different page size in the future, in order to avoid the compatible problem, we should use the min page size as the max inline size.

If there's support for mounting a fs with sectorsize != pagesize, this will work automatically. Currently such filesystem fails to mount, as we know.

Whether it's 4k or 8k could be decided by the performance impact, but I have no numbers to vote for either.

david

Mitch Harder

2013-08-10 21:30 UTC #4 lore raw

Quoted 23 lines · David Sterba

Quoted 8 lines · Miao Xie
Quoted 4 lines · Filipe David Borba Manana
8MiB is way too large and likely set by mistake. This is not a significant issue as in practice the max amount of data added to an inline extent is also limited by the page cache and btree leaf sizes.
I don't think 8KB is a reasonable value of the default max inline size because it makes no sense on the machine whose page size is 4KB.
Page size limit is artificial implied by the implementation and on a 4k page machine max_inline does not take place. Even if it's 4k, it does not apply because the leaf space is smaller.
Quoted 4 lines
I think 4KB is a reasonable value, because we may mount the fs on the machines with the different page size in the future, in order to avoid the compatible problem, we should use the min page size as the max inline size.
If there's support for mounting a fs with sectorsize != pagesize, this will work automatically. Currently such filesystem fails to mount, as we know.

Quoted 8 lines

Whether it's 4k or 8k could be decided by the performance impact, but I have no numbers to vote for either.
david
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I did some research on max_inline when I was playing with an experimental patch to allow inline extents to be greater than sectorsize (but still < leafsize and inodesize).

I concluded that that max_inline doesn't matter as long as it is > PAGE_CACHE_SIZE.

My guess is that at some point early in BTRFS history, they wanted to allow for inlining of really large extents (essentially inlining almost the entire filesystem except for really large files).

When Chris incorporated compression (Btrfs: Add zlib compression support, 2008-10-29, commit c8b978188c9a0fd3d535c13debd19d522b726f1f, and a follow-up patch: Btrfs: Compression corner fixes, 2008-10-31, 70b99e6959a4c28ae1b314985eca731f3db72f1d), the new cow_file_range_inline() function limited inline extent size to PAGE_CACHE_SIZE.

if (start > 0 ||
actual_end >= PAGE_CACHE_SIZE || data_len >= BTRFS_MAX_INLINE_DATA_SIZE(root) || (!compressed_size &&
(actual_end & (root->sectorsize - 1)) == 0) || end + 1 < isize ||
data_len > root->fs_info->max_inline) {
return 1;
}

So, this is why we could have a really large max_inline setting for all these years that didn't make anything go BOOM.

As Miao Xie suggested, if you use fs_info->max_inline = PAGE_CACHE_SIZE, the code will be ready if the issue with 64k page size is ever addressed.

You could probably also refactor fs_info->max_inline out completely with some additional work.

Filipe David Manana

author

2013-08-14 13:04 UTC #5 lore raw

Quoted 61 lines · Mitch Harder

Quoted 23 lines · David Sterba
Quoted 8 lines · Miao Xie
Quoted 4 lines · Filipe David Borba Manana
8MiB is way too large and likely set by mistake. This is not a significant issue as in practice the max amount of data added to an inline extent is also limited by the page cache and btree leaf sizes.
I don't think 8KB is a reasonable value of the default max inline size because it makes no sense on the machine whose page size is 4KB.
Page size limit is artificial implied by the implementation and on a 4k page machine max_inline does not take place. Even if it's 4k, it does not apply because the leaf space is smaller.
Quoted 4 lines
I think 4KB is a reasonable value, because we may mount the fs on the machines with the different page size in the future, in order to avoid the compatible problem, we should use the min page size as the max inline size.
If there's support for mounting a fs with sectorsize != pagesize, this will work automatically. Currently such filesystem fails to mount, as we know.
Quoted 8 lines
Whether it's 4k or 8k could be decided by the performance impact, but I have no numbers to vote for either.
david
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
I did some research on max_inline when I was playing with an experimental patch to allow inline extents to be greater than sectorsize (but still < leafsize and inodesize).
I concluded that that max_inline doesn't matter as long as it is > PAGE_CACHE_SIZE.
My guess is that at some point early in BTRFS history, they wanted to allow for inlining of really large extents (essentially inlining almost the entire filesystem except for really large files).
When Chris incorporated compression (Btrfs: Add zlib compression support, 2008-10-29, commit c8b978188c9a0fd3d535c13debd19d522b726f1f, and a follow-up patch: Btrfs: Compression corner fixes, 2008-10-31, 70b99e6959a4c28ae1b314985eca731f3db72f1d), the new cow_file_range_inline() function limited inline extent size to PAGE_CACHE_SIZE.
if (start > 0 ||
actual_end >= PAGE_CACHE_SIZE || data_len >= BTRFS_MAX_INLINE_DATA_SIZE(root) || (!compressed_size &&
(actual_end & (root->sectorsize - 1)) == 0) || end + 1 < isize ||
data_len > root->fs_info->max_inline) {
return 1;
}

Quoted 6 lines

So, this is why we could have a really large max_inline setting for all these years that didn't make anything go BOOM.
As Miao Xie suggested, if you use fs_info->max_inline = PAGE_CACHE_SIZE, the code will be ready if the issue with 64k page size is ever addressed.

If a default value of PAGE_CACHE_SIZE is the most preferred, I'm fine with it too.
Thanks for the feedback and information.

Quoted 3 lines

You could probably also refactor fs_info->max_inline out completely with some additional work.

Signature · 5 lines

Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."