max_inline related enhancement

PATCH In review Qu Wenruo 6 participants 20 messages Mar 2, 2018 – Mar 7, 2018 Btrfs file system

lore

Share & tools

lore

Qu Wenruo

author

2018-03-02 05:23 UTC #0 lore raw

cover max_inline related enhancement

This patchset intends to reduce confusion about "max_inline" mount option.

The max_inline mount option has the following problems:

1) Different behavior for plain and compressed data extent For plain data extent, it's limiting the extent data size, and will never reach sector size.
For compressed data extent, it's limiting the compressed data size, and compressed data size can reach sector size.

The compressed behavior is very confusing for normal user, as it's almost impossible for end user to know if their operation will end up inlined or no inlined.

2) Inaccurate max inline output
Passing max_inline=4096 and kernel will prompt max_inline is 4096, but we still don't allow inline plain data extent to reach 4096.

3) Symbol link can exceed sector size for its inlined data Since btrfs_symlink() is calling BTRFS_MAX_INLINE_DATA_SIZE() directly without extra truncation.

This patchset will fixes such problems by:

1) Limit both plain and compressed inline extent size by uncompressed data size
So user know exactly what will end up on-disk, just by checking the data size.

2) Output max inline size by limiting it to BTRFS_MAX_INLINE_DATA_SIZE() other than sector size.

3) Embed sector size check into BTRFS_MAX_INLINE_DATA_SIZE() So now btrfs_symlink() won't create any inline extent larger than page size.
(Only affects later operations, and can still read such existing
symbol link)

Qu Wenruo (5):
btrfs: Parse options after node/sector size initialized
btrfs: Always limit inline extent size by uncompressed size
btrfs: Embed sector size check into BTRFS_MAX_INLINE_DATA_SIZE()
btrfs: Unify inline extent creation condition for plain and compressed
data
btrfs: Show more accurate max_inline

 fs/btrfs/ctree.h   |  5 +++--
 fs/btrfs/disk-io.c | 13 +++++++------
 fs/btrfs/inode.c   |  5 +----
 fs/btrfs/super.c   |  4 ++--
 4 files changed, 13 insertions(+), 14 deletions(-)

Signature · 1 line

2.16.2

Qu Wenruo

author

2018-03-02 05:23 UTC #1 lore raw

PATCH 1/5 btrfs: Parse options after node/sector size initialized

This provides the basis for later max_inline enhancement, which needs to access fs_info->nodesize.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/disk-io.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

fs/btrfs/disk-io.c +7 -6

index a8ecccfc36de..f7f985ed5af9 100644
@@ -2644,12 +2644,6 @@ int open_ctree(struct super_block *sb,
 	 */
 	fs_info->compress_type = BTRFS_COMPRESS_ZLIB;
 
-	ret = btrfs_parse_options(fs_info, options, sb->s_flags);
-	if (ret) {
-		err = ret;
-		goto fail_alloc;
-	}
-
 	features = btrfs_super_incompat_flags(disk_super) &
 		~BTRFS_FEATURE_INCOMPAT_SUPP;
 	if (features) {

@@ -2692,6 +2686,13 @@ int open_ctree(struct super_block *sb,
 	fs_info->sectorsize = sectorsize;
 	fs_info->stripesize = stripesize;
 
+	/* Only parse options after node/sector size initialized */
+	ret = btrfs_parse_options(fs_info, options, sb->s_flags);
+	if (ret) {
+		err = ret;
+		goto fail_alloc;
+	}
+
 	/*
 	 * mixed block groups end up with duplicate but slightly offset
 	 * extent buffers for the same range.  It leads to corruptions

Signature · 1 line

2.16.2

David Sterba

2018-03-07 15:22 UTC #2 lore raw

Re: [PATCH 1/5] btrfs: Parse options after node/sector size initialized

Quoted 2 lines · Qu Wenruo

This provides the basis for later max_inline enhancement, which needs to access fs_info->nodesize.

I've checked if this patch can be applied independently, but no, see the comment below.

Quoted 10 lines

Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/disk-io.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
fs/btrfs/disk-io.c +0 -0
index a8ecccfc36de..f7f985ed5af9 100644

fs/btrfs/disk-io.c int open_ctree(struct super_block *sb, · line 2647

Quoted 10 lines

fs/btrfs/disk-io.c +0 -6

@@ -2644,12 +2644,6 @@ int open_ctree(struct super_block *sb,
 	 */
 	fs_info->compress_type = BTRFS_COMPRESS_ZLIB;
 
-	ret = btrfs_parse_options(fs_info, options, sb->s_flags);
-	if (ret) {
-		err = ret;
-		goto fail_alloc;
-	}
-
 	features = btrfs_super_incompat_flags(disk_super) &

We cannot swap the order of btrfs_parse_options and btrfs_super_incompat_flags as the incompat flags can be set by mount options. Currently it's for lzo or zstd, that are supported, but int the future this can be anything and such bug would be hard to catch. If the nodesize is requred, it would need to be obtained in another way.

Quoted 23 lines

fs/btrfs/disk-io.c +7 -0

 		~BTRFS_FEATURE_INCOMPAT_SUPP;
 	if (features) {

@@ -2692,6 +2686,13 @@ int open_ctree(struct super_block *sb,
 	fs_info->sectorsize = sectorsize;
 	fs_info->stripesize = stripesize;
 
+	/* Only parse options after node/sector size initialized */
+	ret = btrfs_parse_options(fs_info, options, sb->s_flags);
+	if (ret) {
+		err = ret;
+		goto fail_alloc;
+	}
+
 	/*
 	 * mixed block groups end up with duplicate but slightly offset
 	 * extent buffers for the same range.  It leads to corruptions

Signature · 6 lines

2.16.2

--
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

Qu Wenruo

author

2018-03-02 05:23 UTC #3 lore raw

PATCH 2/5 btrfs: Always limit inline extent size by uncompressed size

In reply to Qu Wenruo

Normally when specifying max_inline, we should normally limit it by uncompressed extent size, as it's the only thing user can control. (Control the algorithm and compressed data is almost impossible)

Since btrfs is providing *TRANSPARENT* compression, max_inline should behave the same for both plain and compress data.

So this patch will use @inline_len instead of @data_len in cow_file_range_inline() so user will know their max_inline mount option works exactly the same for both plain and compressed data extent.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/btrfs/inode.c +1 -1

index e1a7f3cb5be9..48472509239b 100644
@@ -303,7 +303,7 @@ static noinline int cow_file_range_inline(struct btrfs_root *root,
 	    (!compressed_size &&
 	    (actual_end & (fs_info->sectorsize - 1)) == 0) ||
 	    end + 1 < isize ||
-	    data_len > fs_info->max_inline) {
+	    inline_len > fs_info->max_inline) {
 		return 1;
 	}

Signature · 1 line

2.16.2

Filipe Manana

2018-03-02 10:46 UTC #4 lore raw

Re: [PATCH 2/5] btrfs: Always limit inline extent size by uncompressed size

Quoted 2 lines · Qu Wenruo

Normally when specifying max_inline, we should normally limit it by uncompressed extent size, as it's the only thing user can control.

Why does it matter that users can control it? Will they write less (or more) data to files because stuff won't get inlined?
Why do they care about stuff getting inlined or not? That's an implementation detail of btrfs to speed up access to file data and save some space.

Quoted 4 lines

(Control the algorithm and compressed data is almost impossible)
Since btrfs is providing *TRANSPARENT* compression, max_inline should behave the same for both plain and compress data.

Taking away the benefits of compression for. So now some cases that ended up getting the benefits of inlining won't get them anymore.

I don't agree with this change.

Quoted 30 lines

So this patch will use @inline_len instead of @data_len in cow_file_range_inline() so user will know their max_inline mount option works exactly the same for both plain and compressed data extent.
Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/btrfs/inode.c +1 -2
index e1a7f3cb5be9..48472509239b 100644
@@ -303,7 +303,7 @@ static noinline int cow_file_range_inline(struct btrfs_root *root,
            (!compressed_size &&
            (actual_end & (fs_info->sectorsize - 1)) == 0) ||
            end + 1 < isize ||
-           data_len > fs_info->max_inline) {
+           inline_len > fs_info->max_inline) {
                return 1;
        }
 
--
2.16.2
--
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

Signature · 3 lines

Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Qu Wenruo

2018-03-02 10:54 UTC #5 lore raw

Re: [PATCH 2/5] btrfs: Always limit inline extent size by uncompressed size

Quoted 9 lines · Filipe Manana

Quoted 2 lines · Qu Wenruo
Normally when specifying max_inline, we should normally limit it by uncompressed extent size, as it's the only thing user can control.
Why does it matter that users can control it? Will they write less (or more) data to files because stuff won't get inlined?
Why do they care about stuff getting inlined or not? That's an implementation detail of btrfs to speed up access to file data and save some space.

Then why we still have max_inline mount option?
Just do everything we *think* is best is good enough in that case.

If we provide that mount option to allow *user* to specify the behavior, then allow then to do the same control.

Thanks,
Qu

Quoted 46 lines

Quoted 5 lines
(Control the algorithm and compressed data is almost impossible)
Since btrfs is providing *TRANSPARENT* compression, max_inline should behave the same for both plain and compress data.
Taking away the benefits of compression for. So now some cases that ended up getting the benefits of inlining won't get them anymore.
I don't agree with this change.
Quoted 30 lines
So this patch will use @inline_len instead of @data_len in cow_file_range_inline() so user will know their max_inline mount option works exactly the same for both plain and compressed data extent.
Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/btrfs/inode.c +1 -2
index e1a7f3cb5be9..48472509239b 100644
@@ -303,7 +303,7 @@ static noinline int cow_file_range_inline(struct btrfs_root *root,
            (!compressed_size &&
            (actual_end & (fs_info->sectorsize - 1)) == 0) ||
            end + 1 < isize ||
-           data_len > fs_info->max_inline) {
+           inline_len > fs_info->max_inline) {
                return 1;
        }
 
--
2.16.2
--
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

Filipe Manana

2018-03-02 11:00 UTC #6 lore raw

Re: [PATCH 2/5] btrfs: Always limit inline extent size by uncompressed size

Quoted 7 lines · Qu Wenruo

Quoted 4 lines · Filipe Manana
Quoted 2 lines · Qu Wenruo
Normally when specifying max_inline, we should normally limit it by uncompressed extent size, as it's the only thing user can control.

Quoted 7 lines

Quoted 5 lines
Why does it matter that users can control it? Will they write less (or more) data to files because stuff won't get inlined?
Why do they care about stuff getting inlined or not? That's an implementation detail of btrfs to speed up access to file data and save some space.
Then why we still have max_inline mount option?

My comment was about deciding based on which size to make the decision (compressed vs uncompressed).

Quoted 55 lines

Just do everything we *think* is best is good enough in that case.
If we provide that mount option to allow *user* to specify the behavior, then allow then to do the same control.
Thanks,
Qu
Quoted 46 lines
Quoted 5 lines
(Control the algorithm and compressed data is almost impossible)
Since btrfs is providing *TRANSPARENT* compression, max_inline should behave the same for both plain and compress data.
Taking away the benefits of compression for. So now some cases that ended up getting the benefits of inlining won't get them anymore.
I don't agree with this change.
Quoted 30 lines
So this patch will use @inline_len instead of @data_len in cow_file_range_inline() so user will know their max_inline mount option works exactly the same for both plain and compressed data extent.
Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/btrfs/inode.c +1 -2
index e1a7f3cb5be9..48472509239b 100644
@@ -303,7 +303,7 @@ static noinline int cow_file_range_inline(struct btrfs_root *root,
            (!compressed_size &&
            (actual_end & (fs_info->sectorsize - 1)) == 0) ||
            end + 1 < isize ||
-           data_len > fs_info->max_inline) {
+           inline_len > fs_info->max_inline) {
                return 1;
        }
 
--
2.16.2
--
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

Signature · 3 lines

Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Qu Wenruo

2018-03-02 11:40 UTC #7 lore raw

Re: [PATCH 2/5] btrfs: Always limit inline extent size by uncompressed size

Quoted 14 lines · Filipe Manana

Quoted 13 lines · Qu Wenruo
Quoted 9 lines · Filipe Manana
Quoted 2 lines · Qu Wenruo
Normally when specifying max_inline, we should normally limit it by uncompressed extent size, as it's the only thing user can control.
Why does it matter that users can control it? Will they write less (or more) data to files because stuff won't get inlined?
Why do they care about stuff getting inlined or not? That's an implementation detail of btrfs to speed up access to file data and save some space.

Quoted 4 lines

Quoted 1 line
Then why we still have max_inline mount option?
My comment was about deciding based on which size to make the decision (compressed vs uncompressed).

The same thing, we have given user options to trigger the behavior, then we should give them *predictable* option to modify the behavior.

Not something confusing like current max_inline.

Either we give user max_inline and max_inline_compressed, or both follow max_inline.

Thanks,
Qu

Quoted 59 lines

Quoted 56 lines
Just do everything we *think* is best is good enough in that case.
If we provide that mount option to allow *user* to specify the behavior, then allow then to do the same control.
Thanks,
Qu
Quoted 46 lines
Quoted 5 lines
(Control the algorithm and compressed data is almost impossible)
Since btrfs is providing *TRANSPARENT* compression, max_inline should behave the same for both plain and compress data.
Taking away the benefits of compression for. So now some cases that ended up getting the benefits of inlining won't get them anymore.
I don't agree with this change.
Quoted 30 lines
So this patch will use @inline_len instead of @data_len in cow_file_range_inline() so user will know their max_inline mount option works exactly the same for both plain and compressed data extent.
Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/btrfs/inode.c +1 -2
index e1a7f3cb5be9..48472509239b 100644
@@ -303,7 +303,7 @@ static noinline int cow_file_range_inline(struct btrfs_root *root,
            (!compressed_size &&
            (actual_end & (fs_info->sectorsize - 1)) == 0) ||
            end + 1 < isize ||
-           data_len > fs_info->max_inline) {
+           inline_len > fs_info->max_inline) {
                return 1;
        }
 
--
2.16.2
--
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

David Sterba

2018-03-06 12:00 UTC #8 lore raw

Re: [PATCH 2/5] btrfs: Always limit inline extent size by uncompressed size

Quoted 18 lines · Qu Wenruo

Quoted 12 lines · Filipe Manana
Quoted 11 lines · Qu Wenruo
Quoted 9 lines · Filipe Manana
Quoted 2 lines · Qu Wenruo
Normally when specifying max_inline, we should normally limit it by uncompressed extent size, as it's the only thing user can control.
Why does it matter that users can control it? Will they write less (or more) data to files because stuff won't get inlined?
Why do they care about stuff getting inlined or not? That's an implementation detail of btrfs to speed up access to file data and save some space.
Quoted 4 lines
Quoted 1 line
Then why we still have max_inline mount option?
My comment was about deciding based on which size to make the decision (compressed vs uncompressed).

Quoted 7 lines

The same thing, we have given user options to trigger the behavior, then we should give them *predictable* option to modify the behavior.
Not something confusing like current max_inline.
Either we give user max_inline and max_inline_compressed, or both follow max_inline.

I agree with Filipe and don't see a reason to limit the inlining capabilities. Max inline exists to set the maximum file size that will be considered for inlining, the rest is implementation detail.

In a similar way the compression can decide if the data will be compressed, though the user has specified the mount option.

Adding another option (max_inline_compressed) does not necessarily improve the situation. This requires the user to understand the values it can have and how it interacts with other options.

I've been thinking about this patchset for a few days and still don't think there's a problem we need to fix. We can certainly improve the reporting, so the mount option value will be adjusted to the exact inline limit and then printed to syslog. Additionally we can export the value as sysfs file, and udate documentation.

Qu Wenruo

2018-03-06 12:08 UTC #9 lore raw

Re: [PATCH 2/5] btrfs: Always limit inline extent size by uncompressed size

Quoted 38 lines · David Sterba

Quoted 18 lines · Qu Wenruo
Quoted 12 lines · Filipe Manana
Quoted 11 lines · Qu Wenruo
Quoted 9 lines · Filipe Manana
Quoted 2 lines · Qu Wenruo
Normally when specifying max_inline, we should normally limit it by uncompressed extent size, as it's the only thing user can control.
Why does it matter that users can control it? Will they write less (or more) data to files because stuff won't get inlined?
Why do they care about stuff getting inlined or not? That's an implementation detail of btrfs to speed up access to file data and save some space.
Quoted 4 lines
Quoted 1 line
Then why we still have max_inline mount option?
My comment was about deciding based on which size to make the decision (compressed vs uncompressed).
Quoted 7 lines
The same thing, we have given user options to trigger the behavior, then we should give them *predictable* option to modify the behavior.
Not something confusing like current max_inline.
Either we give user max_inline and max_inline_compressed, or both follow max_inline.
I agree with Filipe and don't see a reason to limit the inlining capabilities. Max inline exists to set the maximum file size that will be considered for inlining, the rest is implementation detail.
In a similar way the compression can decide if the data will be compressed, though the user has specified the mount option.
Adding another option (max_inline_compressed) does not necessarily improve the situation. This requires the user to understand the values it can have and how it interacts with other options.

Quoted 5 lines

I've been thinking about this patchset for a few days and still don't think there's a problem we need to fix. We can certainly improve the reporting, so the mount option value will be adjusted to the exact inline limit and then printed to syslog. Additionally we can export the value as sysfs file, and udate documentation.

Fine, I will discard this patch and just enhance the prompt.

Thanks,
Qu

Quoted 5 lines

--
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

Qu Wenruo

author

2018-03-02 05:23 UTC #10 lore raw

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

In reply to 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.

Signed-off-by Qu Wenruo <[email protected]>

---
 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 48472509239b..fe2991eeb337 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

David Sterba

2018-03-06 12:36 UTC #11 lore raw

Re: [PATCH 3/5] btrfs: Embed sector size check into BTRFS_MAX_INLINE_DATA_SIZE()

Quoted 5 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.

I'm reading what the standard says about symlink and restrictions,
http://pubs.opengroup.org/onlinepubs/009695399/functions/symlink.html

the maximum length of the symlink contents should not exceed SYMLINK_MAX, 'getconf SYMLINK_MAX /path/to/btrfs' says undefined and I can't find any restrictions in manual pages. At least the target should not exceed PATH_MAX in case is's an absolute path, the rest is up to the path name resolution to decide if a relative symlink plus the base directory exceeds PATH_MAX.

IOW, the symlink callback could extend the condition to check against PATH_MAX, that is independent on the page size and sectorsize.

A larger symlink target is not a bug per se, just that it will never pass through VFS once it would be used for file access.

Quoted 5 lines

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.

I don't think it's wise to mix sectorsize and nodesize, here the macro BTRFS_MAX_INLINE_DATA_SIZE is related only to b-tree nodes.

Quoted 3 lines

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.

The page size limit will be hit so >4096 symlink targets can be created only on systems with big pages.

Qu Wenruo

author

2018-03-02 05:23 UTC #12 lore raw

PATCH 4/5 btrfs: Unify inline extent creation condition for plain and compressed data

In reply to Qu Wenruo

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.

However we limited BTRFS_MAX_INLINE_DATA_SIZE() to (sectorsize - 1), and unified the inline extent condition, there is no such difference any longer, just remove the extra check.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/inode.c | 2 --
 1 file changed, 2 deletions(-)

fs/btrfs/inode.c +0 -2

index fe2991eeb337..1e9f4ff46b25 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 ||
 	    inline_len > fs_info->max_inline) {
 		return 1;

Signature · 1 line

2.16.2

Qu Wenruo

author

2018-03-02 05:23 UTC #13 lore raw

PATCH 5/5 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.

Signed-off-by Qu Wenruo <[email protected]>

---
 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

Misono, Tomohiro

2018-03-02 08:22 UTC #14 lore raw

Re: [PATCH 5/5] btrfs: Show more accurate max_inline

Quoted 3 lines · 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.

Hello,
I have a question.

man mount(8) says:
max_inline=bytes
Specify the maximum amount of space, in bytes, that can be inlined in a metadata B-tree leaf. The value is specified in bytes, optionally with a K, M, or G suffix, case insensitive. In practice, this value is limited by the root sector size, with some space unavailable due to leaf headers. For a 4k sectorsize, max inline data is ~3900 bytes.

So, is the size of 4k-(size of leaf header) actually the maximum value of max_inline instead of 4095 for 4k sectorsize?

Thanks,

Nikolay Borisov

2018-03-02 08:33 UTC #15 lore raw

Re: [PATCH 5/5] btrfs: Show more accurate max_inline

Quoted 17 lines · Tomohiro

Quoted 3 lines · 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.
Hello,
I have a question.
man mount(8) says:
max_inline=bytes
Specify the maximum amount of space, in bytes, that can be inlined in a metadata B-tree leaf. The value is specified in bytes, optionally with a K, M, or G suffix, case insensitive. In practice, this value is limited by the root sector size, with some space unavailable due to leaf headers. For a 4k sectorsize, max inline data is ~3900 bytes.

Quoted 2 lines

So, is the size of 4k-(size of leaf header) actually the maximum value of max_inline instead of 4095 for 4k sectorsize?

I think the documentation is wrong. Without patch 3/5 we have the max inline data size as:

BTRFS_MAX_ITEM_SIZE(info) - BTRFS_FILE_EXTENT_INLINE_DATA_START

so MAX_ITEM_SIZE = nodesize - sizeof(btrfs_header) - sizeof(btrfs_item). So this gives us the data portion in the leaf.

So if we substitute the raw number we get:

16k - 101 - 25 = 16258 bytes.

Quoted 1 line

From this number we also subtract the offset of disk_bytenr in

btrfs_file_extent_item (which is 21). So we end up with MAX_INLINE_DATA_SIZE of 16258 - 21 = 16237

With Qu's patch the min_t will always be taking 4095 as the MAX_INLINE_DATA_SIZE.

Quoted 8 lines

Thanks,
--
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

Qu Wenruo

2018-03-02 08:34 UTC #16 lore raw

Re: [PATCH 5/5] btrfs: Show more accurate max_inline

In reply to Misono, Tomohiro

Quoted 17 lines · Tomohiro

Quoted 3 lines · 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.
Hello,
I have a question.
man mount(8) says:
max_inline=bytes
Specify the maximum amount of space, in bytes, that can be inlined in a metadata B-tree leaf. The value is specified in bytes, optionally with a K, M, or G suffix, case insensitive. In practice, this value is limited by the root sector size, with some space unavailable due to leaf headers. For a 4k sectorsize, max inline data is ~3900 bytes.

Quoted 2 lines

So, is the size of 4k-(size of leaf header) actually the maximum value of max_inline instead of 4095 for 4k sectorsize?

Not exactly.

For 4K nodesize, max_inline would be 3960 bytes. As leaf header and EXTENT_ITEM header takes extra bytes.

For 16K nodesize (default), we can go up to 4095 bytes then.

And that man page needs updated, as it should be 4K *nodesize*.

Thanks,
Qu

Quoted 8 lines

Thanks,
--
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

Nikolay Borisov

2018-03-02 08:37 UTC #17 lore raw

Re: [PATCH 5/5] btrfs: Show more accurate max_inline

Quoted 23 lines · Qu Wenruo

Quoted 17 lines · Tomohiro
Quoted 3 lines · 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.
Hello,
I have a question.
man mount(8) says:
max_inline=bytes
Specify the maximum amount of space, in bytes, that can be inlined in a metadata B-tree leaf. The value is specified in bytes, optionally with a K, M, or G suffix, case insensitive. In practice, this value is limited by the root sector size, with some space unavailable due to leaf headers. For a 4k sectorsize, max inline data is ~3900 bytes.
Quoted 2 lines
So, is the size of 4k-(size of leaf header) actually the maximum value of max_inline instead of 4095 for 4k sectorsize?

Quoted 8 lines

Not exactly.
For 4K nodesize, max_inline would be 3960 bytes. As leaf header and EXTENT_ITEM header takes extra bytes.
For 16K nodesize (default), we can go up to 4095 bytes then.
And that man page needs updated, as it should be 4K *nodesize*.

Actually Qu what is preventing the btrfs_drop_extents of dropping inline extents larger than pagesize? This why you are doing this patchset, right ?

Quoted 13 lines

Thanks,
Qu
Quoted 8 lines
Thanks,
--
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

Qu Wenruo

2018-03-02 10:58 UTC #18 lore raw

Re: [PATCH 5/5] btrfs: Show more accurate max_inline

Quoted 31 lines · Nikolay Borisov

Quoted 28 lines · Qu Wenruo
Quoted 17 lines · Tomohiro
Quoted 3 lines · 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.
Hello,
I have a question.
man mount(8) says:
max_inline=bytes
Specify the maximum amount of space, in bytes, that can be inlined in a metadata B-tree leaf. The value is specified in bytes, optionally with a K, M, or G suffix, case insensitive. In practice, this value is limited by the root sector size, with some space unavailable due to leaf headers. For a 4k sectorsize, max inline data is ~3900 bytes.
Quoted 2 lines
So, is the size of 4k-(size of leaf header) actually the maximum value of max_inline instead of 4095 for 4k sectorsize?
Not exactly.
For 4K nodesize, max_inline would be 3960 bytes. As leaf header and EXTENT_ITEM header takes extra bytes.

Quoted 6 lines

Quoted 3 lines
For 16K nodesize (default), we can go up to 4095 bytes then.
And that man page needs updated, as it should be 4K *nodesize*.
Actually Qu what is preventing the btrfs_drop_extents of dropping inline extents larger than pagesize? This why you are doing this patchset, right ?

In fact, current kernel code won't create any inline extent for real file to cross page boundary.

So kernel itself won't try to call __btrfs_drop_extents() inside inline extent.

(Although for symbol link, we can still create such large inline extent, but we won't use __btrfs_drop_extent() in this case).

What I'm doing in this patchset is to fix the confusing behavior related to inline extents.

From its different behavior between plain and compressed extent to larger than one page inlined extent used in symbol link.

Thanks,
Qu

Quoted 19 lines

Quoted 14 lines
Thanks,
Qu
Quoted 8 lines
Thanks,
--
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
--
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

Nikolay Borisov

Reviewed-by on the v1 series

2018-03-02 08:13 UTC #19 lore raw

In reply to Qu Wenruo

Quoted 47 lines · Qu Wenruo

This patchset intends to reduce confusion about "max_inline" mount option.
The max_inline mount option has the following problems:
1) Different behavior for plain and compressed data extent For plain data extent, it's limiting the extent data size, and will never reach sector size.
For compressed data extent, it's limiting the compressed data size, and compressed data size can reach sector size.
The compressed behavior is very confusing for normal user, as it's almost impossible for end user to know if their operation will end up inlined or no inlined.
2) Inaccurate max inline output
Passing max_inline=4096 and kernel will prompt max_inline is 4096, but we still don't allow inline plain data extent to reach 4096.
3) Symbol link can exceed sector size for its inlined data Since btrfs_symlink() is calling BTRFS_MAX_INLINE_DATA_SIZE() directly without extra truncation.
This patchset will fixes such problems by:
1) Limit both plain and compressed inline extent size by uncompressed data size
So user know exactly what will end up on-disk, just by checking the data size.
2) Output max inline size by limiting it to BTRFS_MAX_INLINE_DATA_SIZE() other than sector size.
3) Embed sector size check into BTRFS_MAX_INLINE_DATA_SIZE() So now btrfs_symlink() won't create any inline extent larger than page size.
(Only affects later operations, and can still read such existing
symbol link)
Qu Wenruo (5):
btrfs: Parse options after node/sector size initialized
btrfs: Always limit inline extent size by uncompressed size
btrfs: Embed sector size check into BTRFS_MAX_INLINE_DATA_SIZE()
btrfs: Unify inline extent creation condition for plain and compressed
data
btrfs: Show more accurate max_inline

Quoted 5 lines

 fs/btrfs/ctree.h   |  5 +++--
 fs/btrfs/disk-io.c | 13 +++++++------
 fs/btrfs/inode.c   |  5 +----
 fs/btrfs/super.c   |  4 ++--
 4 files changed, 13 insertions(+), 14 deletions(-)

The series look good so:

Reviewed-by Nikolay Borisov <[email protected]>

Quoted 1 line