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

2018-03-06 Thread Qu Wenruo 


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  wrote:
 On 2018年03月02日 18:46, Filipe Manana wrote:
> On Fri, Mar 2, 2018 at 5:22 AM, Qu Wenruo  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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 



signature.asc
Description: OpenPGP digital signature

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

2018-03-06 Thread 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  wrote:
> >> On 2018年03月02日 18:46, Filipe Manana wrote:
> >>> On Fri, Mar 2, 2018 at 5:22 AM, Qu Wenruo  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@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

2018-03-02 Thread Qu Wenruo 


On 2018年03月02日 19:00, Filipe Manana wrote:
> On Fri, Mar 2, 2018 at 10:54 AM, Qu Wenruo  wrote:
>>
>>
>> On 2018年03月02日 18:46, Filipe Manana wrote:
>>> On Fri, Mar 2, 2018 at 5:22 AM, Qu Wenruo  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 
 ---
  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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>>
>>
> 
> 
> 



signature.asc
Description: OpenPGP digital signature

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

2018-03-02 Thread Filipe Manana 
On Fri, Mar 2, 2018 at 10:54 AM, Qu Wenruo  wrote:
>
>
> On 2018年03月02日 18:46, Filipe Manana wrote:
>> On Fri, Mar 2, 2018 at 5:22 AM, Qu Wenruo  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 
>>> ---
>>>  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 majord...@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.”
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@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

2018-03-02 Thread Qu Wenruo 


On 2018年03月02日 18:46, Filipe Manana wrote:
> On Fri, Mar 2, 2018 at 5:22 AM, Qu Wenruo  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 
>> ---
>>  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 majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 



signature.asc
Description: OpenPGP digital signature

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

2018-03-02 Thread Filipe Manana 
On Fri, Mar 2, 2018 at 5:22 AM, Qu Wenruo  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 
> ---
>  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 majord...@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.”
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2018-03-01 Thread 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 
---
 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html