[PATCH] ext4: fix COLLAPSE RANGE test failure when bigalloc is enable

[PATCH] ext4: fix COLLAPSE RANGE test failure when bigalloc is enable

[Namjae Jeon]

Blocks in collapse range should be collapsed per cluster unit when bigalloc
is enable. If bigalloc is not enable, EXT4_CLUSTER_SIZE will be same with
EXT4_BLOCK_SIZE.

Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
Signed-off-by: Ashish Sangwan <a.sangwan@samsung.com>
---
 fs/ext4/extents.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 4da228a..2b9f5f3 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5403,16 +5403,13 @@ int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
 	int ret;
 
 	/* Collapse range works only on fs block size aligned offsets. */
-	if (offset & (EXT4_BLOCK_SIZE(sb) - 1) ||
-	    len & (EXT4_BLOCK_SIZE(sb) - 1))
+	if (offset & (EXT4_CLUSTER_SIZE(sb) - 1) ||
+	    len & (EXT4_CLUSTER_SIZE(sb) - 1))
 		return -EINVAL;
 
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 
-	if (EXT4_SB(inode->i_sb)->s_cluster_ratio > 1)
-		return -EOPNOTSUPP;

Re: [PATCH] ext4: fix COLLAPSE RANGE test failure when bigalloc is enable

[Lukáš Czerner]

On Wed, 4 Jun 2014, Namjae Jeon wrote:

> Date: Wed, 04 Jun 2014 17:08:45 +0900
> From: Namjae Jeon <namjae.jeon@samsung.com>
> To: Theodore Ts'o <tytso@mit.edu>
> Cc: linux-ext4 <linux-ext4@vger.kernel.org>,
>     Ashish Sangwan <a.sangwan@samsung.com>
> Subject: [PATCH] ext4: fix COLLAPSE RANGE test failure when bigalloc is enable
> 
> Blocks in collapse range should be collapsed per cluster unit when bigalloc
> is enable. If bigalloc is not enable, EXT4_CLUSTER_SIZE will be same with
> EXT4_BLOCK_SIZE.

I wonder why it is so ? Bigalloc only affects the way we allocate
and free blocks, it does not affect extent tree at all and so
freeing and allocating extents at the block boundary on bigalloc
file system should be just fine - underlying code should be able to
handle it.

It might be that there is some complication in shift_extent code
which is not obvious to me. Could you please describe the problem
and why this is needed little bit more ?

Have you done some testing with bigalloc enabled file system with
respect to collapse range ?

Thanks!
-Lukas

> 
> Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
> Signed-off-by: Ashish Sangwan <a.sangwan@samsung.com>
> ---
>  fs/ext4/extents.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 4da228a..2b9f5f3 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -5403,16 +5403,13 @@ int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
>  	int ret;
>  
>  	/* Collapse range works only on fs block size aligned offsets. */
> -	if (offset & (EXT4_BLOCK_SIZE(sb) - 1) ||
> -	    len & (EXT4_BLOCK_SIZE(sb) - 1))
> +	if (offset & (EXT4_CLUSTER_SIZE(sb) - 1) ||
> +	    len & (EXT4_CLUSTER_SIZE(sb) - 1))
>  		return -EINVAL;
>  
>  	if (!S_ISREG(inode->i_mode))
>  		return -EINVAL;
>  
> -	if (EXT4_SB(inode->i_sb)->s_cluster_ratio > 1)
> -		return -EOPNOTSUPP;
> -
>  	trace_ext4_collapse_range(inode, offset, len);
>  
>  	punch_start = offset >> EXT4_BLOCK_SIZE_BITS(sb);
> 

RE: [PATCH] ext4: fix COLLAPSE RANGE test failure when bigalloc is enable

[Namjae Jeon]

> On Wed, 4 Jun 2014, Namjae Jeon wrote:
> 
> > Date: Wed, 04 Jun 2014 17:08:45 +0900
> > From: Namjae Jeon <namjae.jeon@samsung.com>
> > To: Theodore Ts'o <tytso@mit.edu>
> > Cc: linux-ext4 <linux-ext4@vger.kernel.org>,
> >     Ashish Sangwan <a.sangwan@samsung.com>
> > Subject: [PATCH] ext4: fix COLLAPSE RANGE test failure when bigalloc is enable
> >
> > Blocks in collapse range should be collapsed per cluster unit when bigalloc
> > is enable. If bigalloc is not enable, EXT4_CLUSTER_SIZE will be same with
> > EXT4_BLOCK_SIZE.
> 
> I wonder why it is so ? Bigalloc only affects the way we allocate
> and free blocks, it does not affect extent tree at all and so
> freeing and allocating extents at the block boundary on bigalloc
> file system should be just fine - underlying code should be able to
> handle it.
> 
> It might be that there is some complication in shift_extent code
> which is not obvious to me. Could you please describe the problem
> and why this is needed little bit more ?
The reason we can not do intra cluster collapse is because the way
ext4 code works when bigalloc is enabled.
It does not expect the relative mapping between file's logical block
number and physical block numbers within a cluster to be changed.
The following example elaborates this point:
Logs on a ext4 partition with cluster size as 64k.

1. Create a 64k file and dump its extent tree =>
VDLinux#> dd if=/dev/zero of=abc bs=65536 count=1
1+0 records in
1+0 records out
65536 bytes (64.0KB) copied, 0.000699 seconds, 89.4MB/s

debugfs: ex abc
Level Entries Logical Physical Length Flags
0/ 0 1/ 1 0 - 15 557088 - 557103 16

2. Collapse the first block =>
debugfs: ex abc
Level Entries Logical Physical Length Flags
0/ 0 1/ 1 0 - 14 557089 - 557103 15

3. punch a hole at second block =>
debugfs: ex abc
Level Entries Logical Physical Length Flags
0/ 0 1/ 2 0 - 0 557089 - 557089 1
0/ 0 2/ 2 2 - 14 557091 - 557103 13

4. Again allocate block for the hole at block1.
This time already allocated block is allocated.
debugfs: ex abc
Level Entries Logical Physical Length Flags
0/ 0 1/ 3 0 - 0 557089 - 557089 1
0/ 0 2/ 3 1 - 1 557089 - 557089 1 Uninit
0/ 0 3/ 3 2 - 14 557091 - 557103 13

mballoc code thinks that at logical block 1, block number 557089 is
present but when we shift by 1 block using collapse range, 557089
is moved to block 0. But mballoc code does not expect this intra
cluster block movement, so when again try to allocate for block 1,
it allocates block 557089 again.
Also, we can exercise collapse range such that a single block could
be part of 2 clusters:

debugfs:  ex abc
Level Entries       Logical          Physical Length Flags
 0/ 0   1/  4     0 -    14  557088 -  557102     15
 0/ 0   2/  4    15 -    15  557104 -  557104      1
 0/ 0   3/  4    16 -    16  557104 -  557104      1 Uninit
 0/ 0   4/  4    17 -    30  557106 -  557119     14

block number 557104 is part of both cluster#0 and #1.
when we try to remove such a file, ext4 throws error.
[ 2488.440000] EXT4-fs error (device sdb2):
ext4_mb_free_metadata:4563: group 1, block 557104:Block already on
to-be-freed list
[ 2488.452000] JBD2: Spotted dirty metadata buffer (dev = sdb2,
blocknr = 0). There's a risk of filesystem corruption in case of
system crash.

> 
> Have you done some testing with bigalloc enabled file system with
> respect to collapse range ?
Yes, generic/075 and 091 in xfstests was tested. It was getting
failed and on checking we found the above issue.

Thanks!
> 
> Thanks!
> -Lukas
> 
> >
> > Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
> > Signed-off-by: Ashish Sangwan <a.sangwan@samsung.com>
> > ---
> >  fs/ext4/extents.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > index 4da228a..2b9f5f3 100644
> > --- a/fs/ext4/extents.c
> > +++ b/fs/ext4/extents.c
> > @@ -5403,16 +5403,13 @@ int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
> >  	int ret;
> >
> >  	/* Collapse range works only on fs block size aligned offsets. */
> > -	if (offset & (EXT4_BLOCK_SIZE(sb) - 1) ||
> > -	    len & (EXT4_BLOCK_SIZE(sb) - 1))
> > +	if (offset & (EXT4_CLUSTER_SIZE(sb) - 1) ||
> > +	    len & (EXT4_CLUSTER_SIZE(sb) - 1))
> >  		return -EINVAL;
> >
> >  	if (!S_ISREG(inode->i_mode))
> >  		return -EINVAL;
> >
> > -	if (EXT4_SB(inode->i_sb)->s_cluster_ratio > 1)
> > -		return -EOPNOTSUPP;
> > -
> >  	trace_ext4_collapse_range(inode, offset, len);
> >
> >  	punch_start = offset >> EXT4_BLOCK_SIZE_BITS(sb);
> >

Re: [PATCH] ext4: fix COLLAPSE RANGE test failure when bigalloc is enable

[Theodore Ts'o]

On Wed, Jun 04, 2014 at 05:08:45PM +0900, Namjae Jeon wrote:
> Blocks in collapse range should be collapsed per cluster unit when bigalloc
> is enable. If bigalloc is not enable, EXT4_CLUSTER_SIZE will be same with
> EXT4_BLOCK_SIZE.
> 
> Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
> Signed-off-by: Ashish Sangwan <a.sangwan@samsung.com>

Correct me if I'm wrong, but this patch enables COLLAPSE_RANGE when
bigalloc is enabled.  The commit description (and the one-line
summary) should emphasize this.

And I assume you've run a full xfstests auto run with bigalloc and
everything passed?

Thanks,

						- Ted

RE: [PATCH] ext4: fix COLLAPSE RANGE test failure when bigalloc is enable

[Namjae Jeon]

 On Wed, Jun 04, 2014 at 05:08:45PM +0900, Namjae Jeon wrote:
> > Blocks in collapse range should be collapsed per cluster unit when bigalloc
> > is enable. If bigalloc is not enable, EXT4_CLUSTER_SIZE will be same with
> > EXT4_BLOCK_SIZE.
> >
> > Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
> > Signed-off-by: Ashish Sangwan <a.sangwan@samsung.com>
> 
> Correct me if I'm wrong, but this patch enables COLLAPSE_RANGE when
> bigalloc is enabled.  The commit description (and the one-line
> summary) should emphasize this.
Yes, You're right. I will update description.
> 
> And I assume you've run a full xfstests auto run with bigalloc and
> everything passed?
Yes, kvm-test is passed with no issues.(kvm-xfstests -c bigalloc -g auto,
kvm-xfstests -c bigalloc_1k -g auto)
In order to run collapse range xfstests with bigalloc, I had to make some changes.
1) In case of collapse, fsx and fsstress use block aligned paramteres, but for
bigalloc it has to be cluster size aligned. I am not aware of any method by which
ext4 can tell cluster size to user space so I changed fsx and fsstress code to use
hard coded 64KB cluster size alignment.

2) xfstests which tests corner cases for collapse could not be executed with
bigalloc as they are written keeping in mind that blocksize >= 4096.
So, for testing purpose. I changed the multiple factor in function
_test_generic_punch to 64 from 4. Also, the corresponding .out files has to
be changed.

I am looking for a proper way to integrate these changes in xfstests.

Thanks!
> 
> Thanks,
> 
> 						- Ted

Re: [PATCH] ext4: fix COLLAPSE RANGE test failure when bigalloc is enable

[Theodore Ts'o]

On Thu, Jun 19, 2014 at 01:51:58PM +0900, Namjae Jeon wrote:
>  On Wed, Jun 04, 2014 at 05:08:45PM +0900, Namjae Jeon wrote:
> > > Blocks in collapse range should be collapsed per cluster unit when bigalloc
> > > is enable. If bigalloc is not enable, EXT4_CLUSTER_SIZE will be same with
> > > EXT4_BLOCK_SIZE.
> > >
> > > Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
> > > Signed-off-by: Ashish Sangwan <a.sangwan@samsung.com>
> > 
> > Correct me if I'm wrong, but this patch enables COLLAPSE_RANGE when
> > bigalloc is enabled.  The commit description (and the one-line
> > summary) should emphasize this.
> Yes, You're right. I will update description.

I don't think I saw an updated description, so I've taken care of
updating the description and applied the patch:

commit 3c17714e61314b101fb9bb671e7191ef1e9e63f3
Author: Namjae Jeon <namjae.jeon@samsung.com>
Date:   Tue Jul 29 10:45:31 2014 -0400

    ext4: fix COLLAPSE RANGE test for bigalloc file systems
    
    Blocks in collapse range should be collapsed per cluster unit when
    bigalloc is enable. If bigalloc is not enable, EXT4_CLUSTER_SIZE will
    be same with EXT4_BLOCK_SIZE.
    
    With this bug fixed, patch enables COLLAPSE_RANGE for bigalloc, which
    fixes a large number of xfstest failures which use fsx.
    
    Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
    Signed-off-by: Ashish Sangwan <a.sangwan@samsung.com>
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

							- Ted

RE: [PATCH] ext4: fix COLLAPSE RANGE test failure when bigalloc is enable

[Namjae Jeon]

> 
> On Thu, Jun 19, 2014 at 01:51:58PM +0900, Namjae Jeon wrote:
> >  On Wed, Jun 04, 2014 at 05:08:45PM +0900, Namjae Jeon wrote:
> > > > Blocks in collapse range should be collapsed per cluster unit when bigalloc
> > > > is enable. If bigalloc is not enable, EXT4_CLUSTER_SIZE will be same with
> > > > EXT4_BLOCK_SIZE.
> > > >
> > > > Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
> > > > Signed-off-by: Ashish Sangwan <a.sangwan@samsung.com>
> > >
> > > Correct me if I'm wrong, but this patch enables COLLAPSE_RANGE when
> > > bigalloc is enabled.  The commit description (and the one-line
> > > summary) should emphasize this.
> > Yes, You're right. I will update description.
> 
> I don't think I saw an updated description, so I've taken care of
> updating the description and applied the patch:
Sorry, I forgot to update it..
Thanks Ted!

> 
> commit 3c17714e61314b101fb9bb671e7191ef1e9e63f3
> Author: Namjae Jeon <namjae.jeon@samsung.com>
> Date:   Tue Jul 29 10:45:31 2014 -0400
> 
>     ext4: fix COLLAPSE RANGE test for bigalloc file systems
> 
>     Blocks in collapse range should be collapsed per cluster unit when
>     bigalloc is enable. If bigalloc is not enable, EXT4_CLUSTER_SIZE will
>     be same with EXT4_BLOCK_SIZE.
> 
>     With this bug fixed, patch enables COLLAPSE_RANGE for bigalloc, which
>     fixes a large number of xfstest failures which use fsx.
> 
>     Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
>     Signed-off-by: Ashish Sangwan <a.sangwan@samsung.com>
>     Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> 
> 							- Ted