[PATCH 0/5] max_inline related enhancement

[PATCH 0/5] max_inline related enhancement

[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

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

-- 
2.16.2

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

[Qu Wenruo]

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

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index a8ecccfc36de..f7f985ed5af9 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -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
-- 
2.16.2

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

[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 <wqu@suse.com>
---
 fs/btrfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e1a7f3cb5be9..48472509239b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -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

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

[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 <wqu@suse.com>
---
 fs/btrfs/ctree.h | 5 +++--
 fs/btrfs/inode.c | 1 -
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 13c260b525a1..90948096c00f 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -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)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 48472509239b..fe2991eeb337 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -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 ||
-- 
2.16.2

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

[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 <wqu@suse.com>
---
 fs/btrfs/inode.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index fe2991eeb337..1e9f4ff46b25 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -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;
-- 
2.16.2

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

[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 <wqu@suse.com>
---
 fs/btrfs/super.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 3a4dce153645..6685016bc0ec 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -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);
-- 
2.16.2

Re: [PATCH 0/5] max_inline related enhancement

[Nikolay Borisov]

On  2.03.2018 07:22, Qu Wenruo wrote:
> 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(-)

The series look good so:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>



> 

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

[Misono, Tomohiro]

On 2018/03/02 14:22, Qu Wenruo wrote:
> 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,

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

[Nikolay Borisov]

On  2.03.2018 10:21, Misono, Tomohiro wrote:
> On 2018/03/02 14:22, Qu Wenruo wrote:
>> 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?

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.

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

> 
> Thanks,
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1418 bytes --]



On 2018年03月02日 16:21, Misono, Tomohiro wrote:
> On 2018/03/02 14:22, Qu Wenruo wrote:
>> 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?

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

> 
> Thanks,
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

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

[Nikolay Borisov]

On  2.03.2018 10:34, Qu Wenruo wrote:
> 
> 
> On 2018年03月02日 16:21, Misono, Tomohiro wrote:
>> On 2018/03/02 14:22, Qu Wenruo wrote:
>>> 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?
> 
> 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 ?

> 
> Thanks,
> Qu
> 
>>
>> Thanks,
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 

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

[Filipe Manana]

On Fri, Mar 2, 2018 at 5:22 AM, Qu Wenruo <wqu@suse.com> wrote:
> 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.

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

>
> 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 <wqu@suse.com>
> ---
>  fs/btrfs/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index e1a7f3cb5be9..48472509239b 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

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

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

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2333 bytes --]



On 2018年03月02日 18:46, Filipe Manana wrote:
> On Fri, Mar 2, 2018 at 5:22 AM, Qu Wenruo <wqu@suse.com> wrote:
>> 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

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


> 
>>
>> 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 <wqu@suse.com>
>> ---
>>  fs/btrfs/inode.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index e1a7f3cb5be9..48472509239b 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -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 majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

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

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2547 bytes --]



On 2018年03月02日 16:37, Nikolay Borisov wrote:
> 
> 
> On  2.03.2018 10:34, Qu Wenruo wrote:
>>
>>
>> On 2018年03月02日 16:21, Misono, Tomohiro wrote:
>>> On 2018/03/02 14:22, Qu Wenruo wrote:
>>>> 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?
>>
>> 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 ?

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

> 
>>
>> Thanks,
>> Qu
>>
>>>
>>> Thanks,
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

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

[Filipe Manana]

On Fri, Mar 2, 2018 at 10:54 AM, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
> On 2018年03月02日 18:46, Filipe Manana wrote:
>> On Fri, Mar 2, 2018 at 5:22 AM, Qu Wenruo <wqu@suse.com> wrote:
>>> 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?

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

> 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
>
>>
>>> (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.
>
>
>>
>>>
>>> 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 <wqu@suse.com>
>>> ---
>>>  fs/btrfs/inode.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index e1a7f3cb5be9..48472509239b 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -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 majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>



-- 
Filipe David Manana,

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

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

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 3031 bytes --]



On 2018年03月02日 19:00, Filipe Manana wrote:
> On Fri, Mar 2, 2018 at 10:54 AM, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>> On 2018年03月02日 18:46, Filipe Manana wrote:
>>> On Fri, Mar 2, 2018 at 5:22 AM, Qu Wenruo <wqu@suse.com> wrote:
>>>> 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?
> 
> 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

> 
>> 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
>>
>>>
>>>> (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.
>>
>>
>>>
>>>>
>>>> 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 <wqu@suse.com>
>>>> ---
>>>>  fs/btrfs/inode.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>>> index e1a7f3cb5be9..48472509239b 100644
>>>> --- a/fs/btrfs/inode.c
>>>> +++ b/fs/btrfs/inode.c
>>>> @@ -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 majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>>
>>
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

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

[David Sterba]

On Fri, Mar 02, 2018 at 07:40:14PM +0800, Qu Wenruo wrote:
> On 2018年03月02日 19:00, Filipe Manana wrote:
> > On Fri, Mar 2, 2018 at 10:54 AM, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
> >> On 2018年03月02日 18:46, Filipe Manana wrote:
> >>> On Fri, Mar 2, 2018 at 5:22 AM, Qu Wenruo <wqu@suse.com> wrote:
> >>>> 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?
> > 
> > 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.

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.

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

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2505 bytes --]



On 2018年03月06日 19:58, David Sterba wrote:
> On Fri, Mar 02, 2018 at 07:40:14PM +0800, Qu Wenruo wrote:
>> On 2018年03月02日 19:00, Filipe Manana wrote:
>>> On Fri, Mar 2, 2018 at 10:54 AM, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>>> On 2018年03月02日 18:46, Filipe Manana wrote:
>>>>> On Fri, Mar 2, 2018 at 5:22 AM, Qu Wenruo <wqu@suse.com> wrote:
>>>>>> 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?
>>>
>>> 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.
> 
> 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.

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

Thanks,
Qu

> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

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

[David Sterba]

On Fri, Mar 02, 2018 at 01:22:52PM +0800, Qu Wenruo wrote:
> 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.

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

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

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

[David Sterba]

On Fri, Mar 02, 2018 at 01:22:50PM +0800, Qu Wenruo wrote:
> 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.

> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/disk-io.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index a8ecccfc36de..f7f985ed5af9 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -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.

>  		~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
> -- 
> 2.16.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html