Btrfs: fix defragmentation regression
PATCH 1/4 Btrfs: fix defragmentation regression
There's an off-by-one bug:
# create a file with lots of 4K file extents
# btrfs fi defrag /mnt/file
# sync
# filefrag -v /mnt/file
Filesystem type is: 9123683e
File size of /mnt/file is 1228800 (300 blocks, blocksize 4096)
ext logical physical expected length flags
0 0 3372 64
1 64 3136 3435 1
2 65 3436 3136 64
3 129 3201 3499 1
4 130 3500 3201 64
5 194 3266 3563 1
6 195 3564 3266 64
7 259 3331 3627 1
8 260 3628 3331 40 eofAfter this patch:
...
# filefrag -v /mnt/file
Filesystem type is: 9123683e
File size of /mnt/file is 1228800 (300 blocks, blocksize 4096)
ext logical physical expected length flags
0 0 3372 300 eof
/mnt/file: 1 extent found--- fs/btrfs/ioctl.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
fs/btrfs/ioctl.c +4 -2
@@ -1081,7 +1081,6 @@ int btrfs_defrag_file(struct inode *inode, struct file *file, defrag_count += ret;balance_dirty_pages_ratelimited_nr (inode->i_mapping, ret); - i += ret;if (newer_than) {if (newer_off == (u64)-1 )
@@ -1101,7 +1100,10 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,break ; } }else { - i++; +if (ret >0 ) + i += ret; +else + i++; } }
Signature · 1 line
1.7.3.1
PATCH 2/4 Btrfs: use i_size_read() in btrfs_defrag_file()
Don't use inode->i_size directly, since we're not holding i_mutex.
This also fixes another bug, that i_size can change after it's checked against 0 and then (i_size - 1) can be negative.
--- fs/btrfs/ioctl.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
fs/btrfs/ioctl.c +4 -3
@@ -972,6 +972,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,struct btrfs_super_block *disk_super;struct file_ra_state *ra =NULL ;unsigned long last_index; +u64 isize =i_size_read (inode);u64 features;u64 last_len =0 ;u64 skip =0 ;
@@ -997,7 +998,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file, compress_type = range->compress_type; } -if (inode->i_size ==0 ) +if (isize ==0 )return 0 ;/*
@@ -1022,10 +1023,10 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,/* find the last page to defrag */ if (range->start + range->len > range->start) { - last_index =min_t (u64, inode->i_size -1 , + last_index =min_t (u64, isize -1 , range->start + range->len -1 ) >>PAGE_CACHE_SHIFT ; }else { - last_index = (inode->i_size -1 ) >>PAGE_CACHE_SHIFT ; + last_index = (isize -1 ) >>PAGE_CACHE_SHIFT ; }if (newer_than) {
Signature · 1 line
1.7.3.1
Re: [PATCH 2/4] Btrfs: use i_size_read() in btrfs_defrag_file()
Quoted 6 lines · Li Zefan
Don't use
inode->i_sizedirectly, since we're not holdingi_mutex.This also fixes another bug, that
i_sizecan change after it's checked against 0 and then (i_size- 1) can be negative.Signed-off-by Li Zefan <[email protected]>
Quoted 44 lines
--- fs/btrfs/ioctl.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)fs/btrfs/ioctl.c +4
-3@@ -972,6 +972,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,struct btrfs_super_block *disk_super;struct file_ra_state *ra =NULL ;unsigned long last_index; +u64 isize =i_size_read (inode);u64 features;u64 last_len =0 ;u64 skip =0 ;@@ -997,7 +998,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file, compress_type = range->compress_type; } -if (inode->i_size ==0 ) +if (isize ==0 )return 0 ;/* @@ -1022,10 +1023,10 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,/* find the last page to defrag */ if (range->start + range->len > range->start) { - last_index =min_t (u64, inode->i_size -1 , + last_index =min_t (u64, isize -1 , range->start + range->len -1 ) >>PAGE_CACHE_SHIFT ; }else { - last_index = (inode->i_size -1 ) >>PAGE_CACHE_SHIFT ; + last_index = (isize -1 ) >>PAGE_CACHE_SHIFT ; }if (newer_than) {Signature · 5 lines
1.7.3.1 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 3/4 Btrfs: fix wrong max_to_defrag in btrfs_defrag_file()
In reply to Li ZefanIt's off-by-one, and thus we may skip the last page while defragmenting.
An example case:
# create /mnt/file with 2 4K file extents # btrfs fi defrag /mnt/file # sync # filefrag /mnt/file /mnt/file: 2 extents found
So it's not defragmented.
--- fs/btrfs/ioctl.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
fs/btrfs/ioctl.c +1 -1
@@ -1046,7 +1046,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file, i = range->start >>PAGE_CACHE_SHIFT ; }if (!max_to_defrag) - max_to_defrag = last_index -1 ; + max_to_defrag = last_index;while (i <= last_index && defrag_count < max_to_defrag) {/*
Signature · 1 line
1.7.3.1
Re: [PATCH 3/4] Btrfs: fix wrong max_to_defrag in btrfs_defrag_file()
Quoted 1 line · Li Zefan
It's off-by-one, and thus we may skip the last page while defragmenting.
Good catch.
Quoted 9 lines
An example case:
# create /mnt/file with 2 4K file extents # btrfs fi defrag /mnt/file # sync # filefrag /mnt/file /mnt/file: 2 extents found
Quoted 3 lines
So it's not defragmented.
Signed-off-by Li Zefan <[email protected]>
Quoted 23 lines
--- fs/btrfs/ioctl.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)fs/btrfs/ioctl.c +1
-1@@ -1046,7 +1046,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file, i = range->start >>PAGE_CACHE_SHIFT ; }if (!max_to_defrag) - max_to_defrag = last_index -1 ; + max_to_defrag = last_index;while (i <= last_index && defrag_count < max_to_defrag) {/* Signature · 5 lines
1.7.3.1 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 4/4 Btrfs: honor extent thresh during defragmentation
In reply to Li ZefanWe won't defrag an extent, if it's bigger than the threshold we specified and there's no small extent before it, but actually the code doesn't work this way.
There are three bugs:
- When should_defrag_range() decides we should keep on defragmenting an extent, last_len is not incremented. (old bug)
- The length that passes to should_defrag_range() is not the length we're going to defrag. (new bug)
- We always defrag 256K bytes data, and a big extent can be part of this range. (new bug)
For a file with 4 extents:
| 4K | 4K | 256K | 256K |
The result of defrag with (the default) 256K extent thresh should be:
| 264K | 256K |
but with those bugs, we'll get:
| 520K |
--- fs/btrfs/ioctl.c | 37 ++++++++++++++++++++++++++----------- 1 files changed, 26 insertions(+), 11 deletions(-)
fs/btrfs/ioctl.c +26 -11
@@ -760,7 +760,7 @@ static int should_defrag_range(struct inode *inode, u64 start, u64 len,int ret =1 ;/* -* make sure that once we start defragging and extent, we keep on +* make sure that once we start defragging an extent, we keep on * defragging it */ if (start < *defrag_end)
@@ -805,7 +805,6 @@ static int should_defrag_range(struct inode *inode, u64 start, u64 len,* extent will force at least part of that big extent to be defragged. */ if (ret) { - *last_len += len; *defrag_end =extent_map_end (em); }else { *last_len =0 ;
@@ -978,13 +977,14 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,u64 skip =0 ;u64 defrag_end =0 ;u64 newer_off = range->start; -int newer_left =0 ;unsigned long i; +unsigned long ra_index =0 ;int ret;int defrag_count =0 ;int compress_type =BTRFS_COMPRESS_ZLIB ;int extent_thresh = range->extent_thresh; -int newer_cluster = (256 *1024 ) >>PAGE_CACHE_SHIFT ; +int max_cluster = (256 *1024 ) >>PAGE_CACHE_SHIFT ; +int cluster = max_cluster;u64 new_align = ~((u64 )128 *1024 -1 );struct page **pages =NULL ;
@@ -1014,7 +1014,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file, ra = &file->f_ra; } - pages =kmalloc (sizeof (struct page *) * newer_cluster, + pages =kmalloc (sizeof (struct page *) * max_cluster,GFP_NOFS );if (!pages) { ret = -ENOMEM ;
@@ -1039,7 +1039,6 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,* the extents in the file evenly spaced */ i = (newer_off & new_align) >>PAGE_CACHE_SHIFT ; - newer_left = newer_cluster; }else goto out_ra; }else {
@@ -1071,12 +1070,26 @@ int btrfs_defrag_file(struct inode *inode, struct file *file, i =max (i +1 , next);continue ; } + +if (!newer_than) { + cluster = (PAGE_CACHE_ALIGN (defrag_end) >> +PAGE_CACHE_SHIFT ) - i; + cluster =min (cluster, max_cluster); + }else { + cluster = max_cluster; + } +if (range->flags &BTRFS_DEFRAG_RANGE_COMPRESS )BTRFS_I (inode)->force_compress = compress_type; -btrfs_force_ra (inode->i_mapping, ra, file, i, newer_cluster); +if (i + cluster > ra_index) { + ra_index =max (i, ra_index); +btrfs_force_ra (inode->i_mapping, ra, file, ra_index, + cluster); + ra_index += max_cluster; + } - ret =cluster_pages_for_defrag (inode, pages, i, newer_cluster); + ret =cluster_pages_for_defrag (inode, pages, i, cluster);if (ret <0 ) goto out_ra;
@@ -1096,15 +1109,17 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,if (!ret) { range->start = newer_off; i = (newer_off & new_align) >>PAGE_CACHE_SHIFT ; - newer_left = newer_cluster; }else {break ; } }else { -if (ret >0 ) +if (ret >0 ) { i += ret; -else + last_len += ret <<PAGE_CACHE_SHIFT ; + }else { i++; + last_len =0 ; + } } }
Signature · 1 line
1.7.3.1
Quoted 19 lines · Li Zefan
There's an off-by-one bug:
# create a file with lots of 4K file extents # btrfs fi defrag /mnt/file # sync # filefrag -v /mnt/file Filesystem type is: 9123683e File size of /mnt/file is 1228800 (300 blocks, blocksize 4096) ext logical physical expected length flags 0 0 3372 64 1 64 3136 3435 1 2 65 3436 3136 64 3 129 3201 3499 1 4 130 3500 3201 64 5 194 3266 3563 1 6 195 3564 3266 64 7 259 3331 3627 1 8 260 3628 3331 40 eof
Quoted 1 line
After this patch:
Can you please create an xfstests testcase for this?
On Fri, Sep 2, 2011 at 4:42 AM, Christoph Hellwig <[email protected]> w= rote:
Quoted 11 lines · Li Zefan
Quoted 10 lines
There's an off-by-one bug:
=A0 # create a file with lots of 4K file extents
=A0 # btrfs fi defrag/mnt/file
=A0 # sync
=A0 # filefrag -v/mnt/file
=A0 Filesystem type is:9123683e
=A0 File size of/mnt/fileis 1228800 (300 blocks, blocksize 4096) =A0 =A0ext logical physical expected length flags
=A0 =A0 =A00 =A0 =A0 =A0 0 =A0 =A0 3372 =A0 =A0 =A0 =A0 =A0 =A0 =A06=
4
Quoted 9 lines
Quoted 9 lines
=A0 =A0 =A01 =A0 =A0 =A064 =A0 =A0 3136 =A0 =A0 3435 =A0 =A0 =A01 =A0 =A0 =A02 =A0 =A0 =A065 =A0 =A0 3436 =A0 =A0 3136 =A0 =A0 64 =A0 =A0 =A03 =A0 =A0 129 =A0 =A0 3201 =A0 =A0 3499 =A0 =A0 =A01 =A0 =A0 =A04 =A0 =A0 130 =A0 =A0 3500 =A0 =A0 3201 =A0 =A0 64 =A0 =A0 =A05 =A0 =A0 194 =A0 =A0 3266 =A0 =A0 3563 =A0 =A0 =A01 =A0 =A0 =A06 =A0 =A0 195 =A0 =A0 3564 =A0 =A0 3266 =A0 =A0 64 =A0 =A0 =A07 =A0 =A0 259 =A0 =A0 3331 =A0 =A0 3627 =A0 =A0 =A01 =A0 =A0 =A08 =A0 =A0 260 =A0 =A0 3628 =A0 =A0 3331 =A0 =A0 40 eof
Quoted 3 lines
Quoted 1 line
After this patch:
Can you please create an xfstests testcase for this?
Did this fix get lost? I don't see it in git, and defragmenting a file still results in 10x as many fragments as it started with. (3.1-rc9)
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" = in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 21 lines · Dan Merillat
Quoted 20 lines · Christoph Hellwig
Quoted 19 lines · Li Zefan
There's an off-by-one bug:
# create a file with lots of 4K file extents # btrfs fi defrag /mnt/file # sync # filefrag -v /mnt/file Filesystem type is: 9123683e File size of /mnt/file is 1228800 (300 blocks, blocksize 4096) ext logical physical expected length flags 0 0 3372 64 1 64 3136 3435 1 2 65 3436 3136 64 3 129 3201 3499 1 4 130 3500 3201 64 5 194 3266 3563 1 6 195 3564 3266 64 7 259 3331 3627 1 8 260 3628 3331 40 eof
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
After this patch:
Can you please create an xfstests testcase for this?
Did this fix get lost? I don't see it in git, and defragmenting a file still results in 10x as many fragments as it started with. (3.1-rc9)
No, it's queued for 3.2, but I think it's a good candidate for 3.1.x.