All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] btrfs: make max inline data can be equal to sectorsize
@ 2016-10-11  6:47 Wang Xiaoguang
  2016-10-11 15:49 ` Chris Murphy
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Wang Xiaoguang @ 2016-10-11  6:47 UTC (permalink / raw)
  To: linux-btrfs

If we use mount option "-o max_inline=sectorsize", say 4096, indeed
even for a fresh fs, say nodesize is 16k, we can not make the first
4k data completely inline, I found this conditon causing this issue:
  !compressed_size && (actual_end & (root->sectorsize - 1)) == 0

If it retuns true, we'll not make data inline. For 4k sectorsize,
0~4094 dara range, we can make it inline, but 0~4095, it can not.
I don't think this limition is useful, so here remove it which will
make max inline data can be equal to sectorsize.

Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
---
 fs/btrfs/inode.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index ea15520..c0db393 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -267,8 +267,6 @@ static noinline int cow_file_range_inline(struct btrfs_root *root,
 	if (start > 0 ||
 	    actual_end > root->sectorsize ||
 	    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;
-- 
2.9.0




^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [RFC] btrfs: make max inline data can be equal to sectorsize
  2016-10-11  6:47 [RFC] btrfs: make max inline data can be equal to sectorsize Wang Xiaoguang
@ 2016-10-11 15:49 ` Chris Murphy
  2016-10-12  3:35   ` Wang Xiaoguang
  2016-10-26  7:32 ` Wang Xiaoguang
  2016-11-11 20:22 ` Liu Bo
  2 siblings, 1 reply; 12+ messages in thread
From: Chris Murphy @ 2016-10-11 15:49 UTC (permalink / raw)
  To: Wang Xiaoguang; +Cc: Btrfs BTRFS

On Tue, Oct 11, 2016 at 12:47 AM, Wang Xiaoguang
<wangxg.fnst@cn.fujitsu.com> wrote:
> If we use mount option "-o max_inline=sectorsize", say 4096, indeed
> even for a fresh fs, say nodesize is 16k, we can not make the first
> 4k data completely inline, I found this conditon causing this issue:
>   !compressed_size && (actual_end & (root->sectorsize - 1)) == 0
>
> If it retuns true, we'll not make data inline. For 4k sectorsize,
> 0~4094 dara range, we can make it inline, but 0~4095, it can not.
> I don't think this limition is useful, so here remove it which will
> make max inline data can be equal to sectorsize.
>
> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
> ---
>  fs/btrfs/inode.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index ea15520..c0db393 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -267,8 +267,6 @@ static noinline int cow_file_range_inline(struct btrfs_root *root,
>         if (start > 0 ||
>             actual_end > root->sectorsize ||
>             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;
> --
> 2.9.0


Before making any further changes to inline data, does it make sense
to find the source of corruption Zygo has been experiencing? That's in
the "btrfs rare silent data corruption with kernel data leak" thread.


-- 
Chris Murphy

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] btrfs: make max inline data can be equal to sectorsize
  2016-10-11 15:49 ` Chris Murphy
@ 2016-10-12  3:35   ` Wang Xiaoguang
  2016-10-15 22:28     ` Zygo Blaxell
  0 siblings, 1 reply; 12+ messages in thread
From: Wang Xiaoguang @ 2016-10-12  3:35 UTC (permalink / raw)
  To: Chris Murphy; +Cc: Btrfs BTRFS

hi,

On 10/11/2016 11:49 PM, Chris Murphy wrote:
> On Tue, Oct 11, 2016 at 12:47 AM, Wang Xiaoguang
> <wangxg.fnst@cn.fujitsu.com> wrote:
>> If we use mount option "-o max_inline=sectorsize", say 4096, indeed
>> even for a fresh fs, say nodesize is 16k, we can not make the first
>> 4k data completely inline, I found this conditon causing this issue:
>>    !compressed_size && (actual_end & (root->sectorsize - 1)) == 0
>>
>> If it retuns true, we'll not make data inline. For 4k sectorsize,
>> 0~4094 dara range, we can make it inline, but 0~4095, it can not.
>> I don't think this limition is useful, so here remove it which will
>> make max inline data can be equal to sectorsize.
>>
>> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
>> ---
>>   fs/btrfs/inode.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index ea15520..c0db393 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -267,8 +267,6 @@ static noinline int cow_file_range_inline(struct btrfs_root *root,
>>          if (start > 0 ||
>>              actual_end > root->sectorsize ||
>>              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;
>> --
>> 2.9.0
>
> Before making any further changes to inline data, does it make sense
> to find the source of corruption Zygo has been experiencing? That's in
> the "btrfs rare silent data corruption with kernel data leak" thread.
Yes, agree.
Also Zygo has sent a patch to fix that bug this morning :)

Regards,
XIaoguang Wang

>
>




^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] btrfs: make max inline data can be equal to sectorsize
  2016-10-12  3:35   ` Wang Xiaoguang
@ 2016-10-15 22:28     ` Zygo Blaxell
  0 siblings, 0 replies; 12+ messages in thread
From: Zygo Blaxell @ 2016-10-15 22:28 UTC (permalink / raw)
  To: Wang Xiaoguang; +Cc: Chris Murphy, Btrfs BTRFS

[-- Attachment #1: Type: text/plain, Size: 2348 bytes --]

On Wed, Oct 12, 2016 at 11:35:46AM +0800, Wang Xiaoguang wrote:
> hi,
> 
> On 10/11/2016 11:49 PM, Chris Murphy wrote:
> >On Tue, Oct 11, 2016 at 12:47 AM, Wang Xiaoguang
> ><wangxg.fnst@cn.fujitsu.com> wrote:
> >>If we use mount option "-o max_inline=sectorsize", say 4096, indeed
> >>even for a fresh fs, say nodesize is 16k, we can not make the first
> >>4k data completely inline, I found this conditon causing this issue:
> >>   !compressed_size && (actual_end & (root->sectorsize - 1)) == 0
> >>
> >>If it retuns true, we'll not make data inline. For 4k sectorsize,
> >>0~4094 dara range, we can make it inline, but 0~4095, it can not.
> >>I don't think this limition is useful, so here remove it which will
> >>make max inline data can be equal to sectorsize.
> >>
> >>Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
> >>---
> >>  fs/btrfs/inode.c | 2 --
> >>  1 file changed, 2 deletions(-)
> >>
> >>diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> >>index ea15520..c0db393 100644
> >>--- a/fs/btrfs/inode.c
> >>+++ b/fs/btrfs/inode.c
> >>@@ -267,8 +267,6 @@ static noinline int cow_file_range_inline(struct btrfs_root *root,
> >>         if (start > 0 ||
> >>             actual_end > root->sectorsize ||
> >>             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;
> >>--
> >>2.9.0
> >
> >Before making any further changes to inline data, does it make sense
> >to find the source of corruption Zygo has been experiencing? That's in
> >the "btrfs rare silent data corruption with kernel data leak" thread.
> Yes, agree.
> Also Zygo has sent a patch to fix that bug this morning :)

FWIW I don't see any connection between this and the problem I found.
A page-sized inline extent wouldn't have any room for uninitialized
bytes.  If anthing, it's the one rare case that already worked.  ;)

> Regards,
> XIaoguang Wang
> 
> >
> >
> 
> 
> 
> --
> 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: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] btrfs: make max inline data can be equal to sectorsize
  2016-10-11  6:47 [RFC] btrfs: make max inline data can be equal to sectorsize Wang Xiaoguang
  2016-10-11 15:49 ` Chris Murphy
@ 2016-10-26  7:32 ` Wang Xiaoguang
  2016-11-11 20:22 ` Liu Bo
  2 siblings, 0 replies; 12+ messages in thread
From: Wang Xiaoguang @ 2016-10-26  7:32 UTC (permalink / raw)
  To: linux-btrfs

hi,

On 10/11/2016 02:47 PM, Wang Xiaoguang wrote:
> If we use mount option "-o max_inline=sectorsize", say 4096, indeed
> even for a fresh fs, say nodesize is 16k, we can not make the first
> 4k data completely inline, I found this conditon causing this issue:
>    !compressed_size && (actual_end & (root->sectorsize - 1)) == 0
>
> If it retuns true, we'll not make data inline. For 4k sectorsize,
> 0~4094 dara range, we can make it inline, but 0~4095, it can not.
> I don't think this limition is useful, so here remove it which will
> make max inline data can be equal to sectorsize.
>
> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
> ---
>   fs/btrfs/inode.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index ea15520..c0db393 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -267,8 +267,6 @@ static noinline int cow_file_range_inline(struct btrfs_root *root,
>   	if (start > 0 ||
>   	    actual_end > root->sectorsize ||
>   	    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;
Any comments about this patch?

Regards,
Xiaoguang Wang





^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] btrfs: make max inline data can be equal to sectorsize
  2016-10-11  6:47 [RFC] btrfs: make max inline data can be equal to sectorsize Wang Xiaoguang
  2016-10-11 15:49 ` Chris Murphy
  2016-10-26  7:32 ` Wang Xiaoguang
@ 2016-11-11 20:22 ` Liu Bo
  2016-11-14  1:55   ` Qu Wenruo
  2 siblings, 1 reply; 12+ messages in thread
From: Liu Bo @ 2016-11-11 20:22 UTC (permalink / raw)
  To: Wang Xiaoguang; +Cc: linux-btrfs

On Tue, Oct 11, 2016 at 02:47:42PM +0800, Wang Xiaoguang wrote:
> If we use mount option "-o max_inline=sectorsize", say 4096, indeed
> even for a fresh fs, say nodesize is 16k, we can not make the first
> 4k data completely inline, I found this conditon causing this issue:
>   !compressed_size && (actual_end & (root->sectorsize - 1)) == 0
> 
> If it retuns true, we'll not make data inline. For 4k sectorsize,
> 0~4094 dara range, we can make it inline, but 0~4095, it can not.
> I don't think this limition is useful, so here remove it which will
> make max inline data can be equal to sectorsize.

It's difficult to tell whether we need this, I'm not a big fan of using
max_inline size more than the default size 2048, given that most reports
about ENOSPC is due to metadata and inline may make it worse.

Thanks,

-liubo

> 
> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
> ---
>  fs/btrfs/inode.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index ea15520..c0db393 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -267,8 +267,6 @@ static noinline int cow_file_range_inline(struct btrfs_root *root,
>  	if (start > 0 ||
>  	    actual_end > root->sectorsize ||
>  	    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;
> -- 
> 2.9.0
> 
> 
> 
> --
> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] btrfs: make max inline data can be equal to sectorsize
  2016-11-11 20:22 ` Liu Bo
@ 2016-11-14  1:55   ` Qu Wenruo
  2016-11-16 16:10     ` David Sterba
  0 siblings, 1 reply; 12+ messages in thread
From: Qu Wenruo @ 2016-11-14  1:55 UTC (permalink / raw)
  To: bo.li.liu, Wang Xiaoguang; +Cc: linux-btrfs



At 11/12/2016 04:22 AM, Liu Bo wrote:
> On Tue, Oct 11, 2016 at 02:47:42PM +0800, Wang Xiaoguang wrote:
>> If we use mount option "-o max_inline=sectorsize", say 4096, indeed
>> even for a fresh fs, say nodesize is 16k, we can not make the first
>> 4k data completely inline, I found this conditon causing this issue:
>>   !compressed_size && (actual_end & (root->sectorsize - 1)) == 0
>>
>> If it retuns true, we'll not make data inline. For 4k sectorsize,
>> 0~4094 dara range, we can make it inline, but 0~4095, it can not.
>> I don't think this limition is useful, so here remove it which will
>> make max inline data can be equal to sectorsize.
>
> It's difficult to tell whether we need this, I'm not a big fan of using
> max_inline size more than the default size 2048, given that most reports
> about ENOSPC is due to metadata and inline may make it worse.

IMHO if we can use inline data extents to trigger ENOSPC more easily, 
then we should allow it to dig the problem further.

Just ignoring it because it may cause more bug will not solve the real 
problem anyway.

Thanks,
Qu

>
> Thanks,
>
> -liubo
>
>>
>> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
>> ---
>>  fs/btrfs/inode.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index ea15520..c0db393 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -267,8 +267,6 @@ static noinline int cow_file_range_inline(struct btrfs_root *root,
>>  	if (start > 0 ||
>>  	    actual_end > root->sectorsize ||
>>  	    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;
>> --
>> 2.9.0
>>
>>
>>
>> --
>> 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
>
>



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] btrfs: make max inline data can be equal to sectorsize
  2016-11-14  1:55   ` Qu Wenruo
@ 2016-11-16 16:10     ` David Sterba
  2016-11-18 20:58       ` Chris Mason
  0 siblings, 1 reply; 12+ messages in thread
From: David Sterba @ 2016-11-16 16:10 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: bo.li.liu, Wang Xiaoguang, linux-btrfs

On Mon, Nov 14, 2016 at 09:55:34AM +0800, Qu Wenruo wrote:
> At 11/12/2016 04:22 AM, Liu Bo wrote:
> > On Tue, Oct 11, 2016 at 02:47:42PM +0800, Wang Xiaoguang wrote:
> >> If we use mount option "-o max_inline=sectorsize", say 4096, indeed
> >> even for a fresh fs, say nodesize is 16k, we can not make the first
> >> 4k data completely inline, I found this conditon causing this issue:
> >>   !compressed_size && (actual_end & (root->sectorsize - 1)) == 0
> >>
> >> If it retuns true, we'll not make data inline. For 4k sectorsize,
> >> 0~4094 dara range, we can make it inline, but 0~4095, it can not.
> >> I don't think this limition is useful, so here remove it which will
> >> make max inline data can be equal to sectorsize.
> >
> > It's difficult to tell whether we need this, I'm not a big fan of using
> > max_inline size more than the default size 2048, given that most reports
> > about ENOSPC is due to metadata and inline may make it worse.
> 
> IMHO if we can use inline data extents to trigger ENOSPC more easily, 
> then we should allow it to dig the problem further.
> 
> Just ignoring it because it may cause more bug will not solve the real 
> problem anyway.

Not allowing the full 4k value as max_inline looks artificial to me.
We've removed other similar limitation in the past so I'd tend to agree
to do the same here. There's no significant use for it as far as I can
tell, if you want to exhaust metadata, the difference to max_inline=4095
would be really tiny in the end. So, I'm okay with merging it. If
anybody feels like adding his reviewed-by, please do so.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] btrfs: make max inline data can be equal to sectorsize
  2016-11-16 16:10     ` David Sterba
@ 2016-11-18 20:58       ` Chris Mason
  2016-11-19  8:27         ` Zygo Blaxell
  2016-11-22  7:54         ` Wang Xiaoguang
  0 siblings, 2 replies; 12+ messages in thread
From: Chris Mason @ 2016-11-18 20:58 UTC (permalink / raw)
  To: dsterba, Qu Wenruo, bo.li.liu, Wang Xiaoguang, linux-btrfs



On 11/16/2016 11:10 AM, David Sterba wrote:
> On Mon, Nov 14, 2016 at 09:55:34AM +0800, Qu Wenruo wrote:
>> At 11/12/2016 04:22 AM, Liu Bo wrote:
>>> On Tue, Oct 11, 2016 at 02:47:42PM +0800, Wang Xiaoguang wrote:
>>>> If we use mount option "-o max_inline=sectorsize", say 4096, indeed
>>>> even for a fresh fs, say nodesize is 16k, we can not make the first
>>>> 4k data completely inline, I found this conditon causing this issue:
>>>>   !compressed_size && (actual_end & (root->sectorsize - 1)) == 0
>>>>
>>>> If it retuns true, we'll not make data inline. For 4k sectorsize,
>>>> 0~4094 dara range, we can make it inline, but 0~4095, it can not.
>>>> I don't think this limition is useful, so here remove it which will
>>>> make max inline data can be equal to sectorsize.
>>>
>>> It's difficult to tell whether we need this, I'm not a big fan of using
>>> max_inline size more than the default size 2048, given that most reports
>>> about ENOSPC is due to metadata and inline may make it worse.
>>
>> IMHO if we can use inline data extents to trigger ENOSPC more easily,
>> then we should allow it to dig the problem further.
>>
>> Just ignoring it because it may cause more bug will not solve the real
>> problem anyway.
>
> Not allowing the full 4k value as max_inline looks artificial to me.
> We've removed other similar limitation in the past so I'd tend to agree
> to do the same here. There's no significant use for it as far as I can
> tell, if you want to exhaust metadata, the difference to max_inline=4095
> would be really tiny in the end. So, I'm okay with merging it. If
> anybody feels like adding his reviewed-by, please do so.

The check is there because in practice it doesn't make sense to inline 
an extent if it fits perfectly in a data block.  You could argue its 
saving seeks, but we're also adding seeks by spreading out the metadata 
in general.  So, I'd want to see benchmarks before deciding.

If we're using it for debugging, I'd rather stick with max_inline=4095.

-chris

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] btrfs: make max inline data can be equal to sectorsize
  2016-11-18 20:58       ` Chris Mason
@ 2016-11-19  8:27         ` Zygo Blaxell
  2017-01-02 17:21           ` David Sterba
  2016-11-22  7:54         ` Wang Xiaoguang
  1 sibling, 1 reply; 12+ messages in thread
From: Zygo Blaxell @ 2016-11-19  8:27 UTC (permalink / raw)
  To: Chris Mason; +Cc: dsterba, Qu Wenruo, bo.li.liu, Wang Xiaoguang, linux-btrfs

[-- Attachment #1: Type: text/plain, Size: 2624 bytes --]

On Fri, Nov 18, 2016 at 03:58:06PM -0500, Chris Mason wrote:
> 
> 
> On 11/16/2016 11:10 AM, David Sterba wrote:
> >On Mon, Nov 14, 2016 at 09:55:34AM +0800, Qu Wenruo wrote:
> >>At 11/12/2016 04:22 AM, Liu Bo wrote:
> >>>On Tue, Oct 11, 2016 at 02:47:42PM +0800, Wang Xiaoguang wrote:
> >>>>If we use mount option "-o max_inline=sectorsize", say 4096, indeed
> >>>>even for a fresh fs, say nodesize is 16k, we can not make the first
> >>>>4k data completely inline, I found this conditon causing this issue:
> >>>>  !compressed_size && (actual_end & (root->sectorsize - 1)) == 0
> >>>>
> >>>>If it retuns true, we'll not make data inline. For 4k sectorsize,
> >>>>0~4094 dara range, we can make it inline, but 0~4095, it can not.
> >>>>I don't think this limition is useful, so here remove it which will
> >>>>make max inline data can be equal to sectorsize.
> >>>
> >>>It's difficult to tell whether we need this, I'm not a big fan of using
> >>>max_inline size more than the default size 2048, given that most reports
> >>>about ENOSPC is due to metadata and inline may make it worse.
> >>
> >>IMHO if we can use inline data extents to trigger ENOSPC more easily,
> >>then we should allow it to dig the problem further.
> >>
> >>Just ignoring it because it may cause more bug will not solve the real
> >>problem anyway.
> >
> >Not allowing the full 4k value as max_inline looks artificial to me.
> >We've removed other similar limitation in the past so I'd tend to agree
> >to do the same here. There's no significant use for it as far as I can
> >tell, if you want to exhaust metadata, the difference to max_inline=4095
> >would be really tiny in the end. So, I'm okay with merging it. If
> >anybody feels like adding his reviewed-by, please do so.
> 
> The check is there because in practice it doesn't make sense to inline an
> extent if it fits perfectly in a data block.  You could argue its saving
> seeks, but we're also adding seeks by spreading out the metadata in general.
> So, I'd want to see benchmarks before deciding.

Does that limit kick in before or after compression?  A compressed extent
could easily have 4096 bytes of data in 200 bytes.  If a filesystem
contained a whole lot of exactly-4096-byte compressible files that extra
byte might be worth something.

> If we're using it for debugging, I'd rather stick with max_inline=4095.
> 
> -chris
> --
> 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: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] btrfs: make max inline data can be equal to sectorsize
  2016-11-18 20:58       ` Chris Mason
  2016-11-19  8:27         ` Zygo Blaxell
@ 2016-11-22  7:54         ` Wang Xiaoguang
  1 sibling, 0 replies; 12+ messages in thread
From: Wang Xiaoguang @ 2016-11-22  7:54 UTC (permalink / raw)
  To: linux-btrfs

hello,

On 11/19/2016 04:58 AM, Chris Mason wrote:
>
>
> On 11/16/2016 11:10 AM, David Sterba wrote:
>> On Mon, Nov 14, 2016 at 09:55:34AM +0800, Qu Wenruo wrote:
>>> At 11/12/2016 04:22 AM, Liu Bo wrote:
>>>> On Tue, Oct 11, 2016 at 02:47:42PM +0800, Wang Xiaoguang wrote:
>>>>> If we use mount option "-o max_inline=sectorsize", say 4096, indeed
>>>>> even for a fresh fs, say nodesize is 16k, we can not make the first
>>>>> 4k data completely inline, I found this conditon causing this issue:
>>>>>   !compressed_size && (actual_end & (root->sectorsize - 1)) == 0
>>>>>
>>>>> If it retuns true, we'll not make data inline. For 4k sectorsize,
>>>>> 0~4094 dara range, we can make it inline, but 0~4095, it can not.
>>>>> I don't think this limition is useful, so here remove it which will
>>>>> make max inline data can be equal to sectorsize.
>>>>
>>>> It's difficult to tell whether we need this, I'm not a big fan of 
>>>> using
>>>> max_inline size more than the default size 2048, given that most 
>>>> reports
>>>> about ENOSPC is due to metadata and inline may make it worse.
>>>
>>> IMHO if we can use inline data extents to trigger ENOSPC more easily,
>>> then we should allow it to dig the problem further.
>>>
>>> Just ignoring it because it may cause more bug will not solve the real
>>> problem anyway.
>>
>> Not allowing the full 4k value as max_inline looks artificial to me.
>> We've removed other similar limitation in the past so I'd tend to agree
>> to do the same here. There's no significant use for it as far as I can
>> tell, if you want to exhaust metadata, the difference to max_inline=4095
>> would be really tiny in the end. So, I'm okay with merging it. If
>> anybody feels like adding his reviewed-by, please do so.
>
> The check is there because in practice it doesn't make sense to inline 
> an extent if it fits perfectly in a data block. 
I see, thanks for this clarification.

> You could argue its saving seeks, but we're also adding seeks by 
> spreading out the metadata in general.  So, I'd want to see benchmarks 
> before deciding.
I had tried to construct some benchmark tests, such as create and read 
plenty of
small files, copy linux source codes, I don't see any obvious 
difference, it maybe
reasonable, after all, this patch just results in one bytes difference.

>
> If we're using it for debugging, I'd rather stick with max_inline=4095.
I was just curious that we could make inline for 4095, but not allow 
4096 before,
just one bytes :)

Regards,
Xiaoguang Wang

>
> -chris
>
>




^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] btrfs: make max inline data can be equal to sectorsize
  2016-11-19  8:27         ` Zygo Blaxell
@ 2017-01-02 17:21           ` David Sterba
  0 siblings, 0 replies; 12+ messages in thread
From: David Sterba @ 2017-01-02 17:21 UTC (permalink / raw)
  To: Zygo Blaxell
  Cc: Chris Mason, dsterba, Qu Wenruo, bo.li.liu, Wang Xiaoguang, linux-btrfs

On Sat, Nov 19, 2016 at 03:27:02AM -0500, Zygo Blaxell wrote:
> On Fri, Nov 18, 2016 at 03:58:06PM -0500, Chris Mason wrote:
> > 
> > 
> > On 11/16/2016 11:10 AM, David Sterba wrote:
> > >On Mon, Nov 14, 2016 at 09:55:34AM +0800, Qu Wenruo wrote:
> > >>At 11/12/2016 04:22 AM, Liu Bo wrote:
> > >>>On Tue, Oct 11, 2016 at 02:47:42PM +0800, Wang Xiaoguang wrote:
> > >>>>If we use mount option "-o max_inline=sectorsize", say 4096, indeed
> > >>>>even for a fresh fs, say nodesize is 16k, we can not make the first
> > >>>>4k data completely inline, I found this conditon causing this issue:
> > >>>>  !compressed_size && (actual_end & (root->sectorsize - 1)) == 0
> > >>>>
> > >>>>If it retuns true, we'll not make data inline. For 4k sectorsize,
> > >>>>0~4094 dara range, we can make it inline, but 0~4095, it can not.
> > >>>>I don't think this limition is useful, so here remove it which will
> > >>>>make max inline data can be equal to sectorsize.
> > >>>
> > >>>It's difficult to tell whether we need this, I'm not a big fan of using
> > >>>max_inline size more than the default size 2048, given that most reports
> > >>>about ENOSPC is due to metadata and inline may make it worse.
> > >>
> > >>IMHO if we can use inline data extents to trigger ENOSPC more easily,
> > >>then we should allow it to dig the problem further.
> > >>
> > >>Just ignoring it because it may cause more bug will not solve the real
> > >>problem anyway.
> > >
> > >Not allowing the full 4k value as max_inline looks artificial to me.
> > >We've removed other similar limitation in the past so I'd tend to agree
> > >to do the same here. There's no significant use for it as far as I can
> > >tell, if you want to exhaust metadata, the difference to max_inline=4095
> > >would be really tiny in the end. So, I'm okay with merging it. If
> > >anybody feels like adding his reviewed-by, please do so.
> > 
> > The check is there because in practice it doesn't make sense to inline an
> > extent if it fits perfectly in a data block.  You could argue its saving
> > seeks, but we're also adding seeks by spreading out the metadata in general.
> > So, I'd want to see benchmarks before deciding.
> 
> Does that limit kick in before or after compression?  A compressed extent
> could easily have 4096 bytes of data in 200 bytes.  If a filesystem
> contained a whole lot of exactly-4096-byte compressible files that extra
> byte might be worth something.

A 4k file that compresses to a small number of bytes will be inlined.
The exact constraints are:
 * file size < sectorsize,
 * compressed result <= inline limit (either in the leaf space or max_inline).

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-01-02 17:21 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-11  6:47 [RFC] btrfs: make max inline data can be equal to sectorsize Wang Xiaoguang
2016-10-11 15:49 ` Chris Murphy
2016-10-12  3:35   ` Wang Xiaoguang
2016-10-15 22:28     ` Zygo Blaxell
2016-10-26  7:32 ` Wang Xiaoguang
2016-11-11 20:22 ` Liu Bo
2016-11-14  1:55   ` Qu Wenruo
2016-11-16 16:10     ` David Sterba
2016-11-18 20:58       ` Chris Mason
2016-11-19  8:27         ` Zygo Blaxell
2017-01-02 17:21           ` David Sterba
2016-11-22  7:54         ` Wang Xiaoguang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.