| Message ID | 20140716192236.GA8628@birch.djwong.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi, On 07/17/2014 03:22 AM, Darrick J. Wong wrote: > On Wed, Jul 16, 2014 at 12:16:37PM +0800, Xiaoguang Wang wrote: >> Hi, >> >> When I run xfstests/tests/generic/018 for ext4 file system in RHEL7.0GA, >> sometimes it fails and sometime it succeeds. After looking into this case, I >> think it's not a kernel ext4 bug, it maybe an e4dfrag bug. I compiled the >> newest e2fsprogs to have test, it seems this issue still exits, so I still >> send a mail to this list to look for some help, thanks. >> >> The issue is that sometimes e4defrag does not defrag file correctly. >> Steps to reproduce this issue: >> 1. cd mntpoint >> 2. rm -f lege >> 3. for I in `seq 9 -1 0`; >> do dd if=/dev/zero of=lege bs=4k count=1 conv=notrunc seek=$I oflag=sync &>/dev/null; >> done; >> 4. e4defrag -c -v lege >> >> Repeatedly execute the 2, 3, 4 steps until you get a file which have the similar extent layout like below: >> ################################################################ >> [root@localhost test_e2fsprogs]# e4defrag -c -v lege >> <File> >> [ext 1]: start 49365571: logical 0: len 1 >> [ext 2]: start 49365570: logical 1: len 1 >> [ext 3]: start 49365569: logical 2: len 1 >> [ext 4]: start 49365568: logical 3: len 1 >> [ext 5]: start 49365567: logical 4: len 1 >> [ext 6]: start 49365566: logical 5: len 1 >> [ext 7]: start 49365565: logical 6: len 1 >> [ext 8]: start 49365564: logical 7: len 1 >> [ext 9]: start 49365563: logical 8: len 1 >> [ext 10]: start 49365562: logical 9: len 1 >> >> Total/best extents 10/1 >> Average size per extent 4 KB >> Fragmentation score 98 >> [0-30 no problem: 31-55 a little bit fragmented: 56- needs defrag] >> This file (lege) needs defragmentation. >> Done. >> ################################################################ >> The physical blocks are continuous but reversed. >> >> If we call e4defrag against this file, the output would be: >> ################################################################ >> [root@localhost test_e2fsprogs]# /tmp/e4defrag -v lege >> ext4 defragmentation for lege >> [1/1]lege: 100% extents: 10 -> 10 [ OK ] >> Success: [1/1] >> [root@localhost test_e2fsprogs]# /tmp/e4defrag -v -c lege >> <File> >> [ext 1]: start 49365571: logical 0: len 1 >> [ext 2]: start 49365570: logical 1: len 1 >> [ext 3]: start 49365569: logical 2: len 1 >> [ext 4]: start 49365568: logical 3: len 1 >> [ext 5]: start 49365567: logical 4: len 1 >> [ext 6]: start 49365566: logical 5: len 1 >> [ext 7]: start 49365565: logical 6: len 1 >> [ext 8]: start 49365564: logical 7: len 1 >> [ext 9]: start 49365563: logical 8: len 1 >> [ext 10]: start 49365562: logical 9: len 1 >> >> Total/best extents 10/1 >> Average size per extent 4 KB >> Fragmentation score 98 >> [0-30 no problem: 31-55 a little bit fragmented: 56- needs defrag] >> This file (lege) needs defragmentation. >> Done. >> ################################################################ >> According to my understanding, this file is not defraged correctly and should >> be convert into one extent. Or because if the physical blocks are continuous >> though reversed, we do not need to do defragment? > > Oh, I think we /do/ need to defragment. Granted, file readahead might paper > over the symptoms, but since the user explicitly ran e4defrag we can try > to do better. Yeah, agree. > >> I have checked the e4defrag source code, whether to do real defragment >> depends on some conditions, please >> see this code(e4defrag.c). >> --main >> --file_defrag >> >> In file_defrag(), there is such a judgement： >> "if (file_frags_start <= best || orig_physical_cnt <= donor_physical_cnt)", If this returns true, the e4defrag will >> not call call_defrag() to do real defragment work. >> >> Here file_frags_start: number of file fragments before defrag >> orig_physical_cnt: number of original file's continuous physical region >> donor_physical_cnt: number of donor file's continuous physical region >> >> In this "lege" file, the orig_physical_cnt is 1, and donor_physical_cnt is also 1, so the "if" is satisfied and >> call_defrag() won't be called. > > This is a curious corner case of e4defrag -- if you look in get_file_extents(), > the list of extents is insertion-sorted by physical block, which means that > get_physical_count() (stupidly) looks only for gaps in the runs of physical > blocks. Therefore, e4defrag thinks that this "lege" file has one physical > extent. Ignoring logical block ordering, this is true, but as you point out, > this leaves the "file written backwards" case in a fragmented state. So let's > not ignore the logical block ordering: > > What I think we really need to do here is make get_physical_count() smarter -- > if there's a gap either in the physical or logical offsets of extents, then we > need to increment *_physical_cnt so that we later decide to defragment the > file. > > (Please keep reading) I checked the code again, you are right, thanks for your explanation > >> Here I'd like to know the comparison "orig_physical_cnt <= >> donor_physical_cnt" is useful? According to my understanding, what should we >> have comparison are number of extents or average extent size. >> >> When I have this change: >> diff --git a/misc/e4defrag.c b/misc/e4defrag.c >> index a204793..cd95698 100644 >> --- a/misc/e4defrag.c >> +++ b/misc/e4defrag.c >> @@ -1598,8 +1598,7 @@ check_improvement: >> extents_before_defrag += file_frags_start; >> } >> >> - if (file_frags_start <= best || >> - orig_physical_cnt <= donor_physical_cnt) { >> + if (file_frags_start <= best) { > > This is incorrect, since the point of the "orig_physical_cnt <= > donor_physical_cnt" check is to ensure that we don't increase the fragmentation > of a file by swapping it with pieces from a donor file whose contents are > spread out over a larger number of runs of physical blocks. Ah, I see. I hadn't realized that, thanks. > > (It does, however, force defragmentation for all files, so you get the results > you wanted.) > > Please try the patch at the end of this message on for size. It fixes things > on my test VM; does it fix yours? Yeah, it works, thanks. Would you send a new version patch to fix this issue, or should I do it? Regards, Xiaoguang Wang > > --D > >> printf("\033[79;0H\033[K[%u/%u]%s:\t%3d%%", >> defraged_file_count, total_count, file, 100); >> if (mode_flag & DETAIL) >> >> Then the "lege" file could be defraged correctly. >> ################################################################## >> [root@localhost test_e2fsprogs]# /tmp/e4defrag -v lege >> ext4 defragmentation for lege >> [1/1]lege: 100% extents: 10 -> 1 [ OK ] >> Success: [1/1] >> [root@localhost test_e2fsprogs]# /tmp/e4defrag -v -c lege >> <File> >> [ext 1]: start 49366583: logical 0: len 10 >> >> Total/best extents 1/1 >> Average size per extent 40 KB >> Fragmentation score 0 >> [0-30 no problem: 31-55 a little bit fragmented: 56- needs defrag] >> This file (lege) does not need defragmentation. >> Done. >> ################################################################## >> >> Any opinion or suggestions will be appreciated! >> If I'm wrong, please correct me, thanks! >> >> Regards, >> Xiaoguang Wang > > diff --git a/misc/e4defrag.c b/misc/e4defrag.c > index a204793..d0eac60 100644 > --- a/misc/e4defrag.c > +++ b/misc/e4defrag.c > @@ -888,7 +888,9 @@ static int get_physical_count(struct fiemap_extent_list *physical_list_head) > > do { > if ((ext_list_tmp->data.physical + ext_list_tmp->data.len) > - != ext_list_tmp->next->data.physical) { > + != ext_list_tmp->next->data.physical || > + (ext_list_tmp->data.logical + ext_list_tmp->data.len) > + != ext_list_tmp->next->data.logical) { > /* This extent and next extent are not continuous. */ > ret++; > } > . > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 17, 2014 at 11:29:26AM +0800, Xiaoguang Wang wrote: > Hi, > > On 07/17/2014 03:22 AM, Darrick J. Wong wrote: > > On Wed, Jul 16, 2014 at 12:16:37PM +0800, Xiaoguang Wang wrote: > >> Hi, > >> > >> When I run xfstests/tests/generic/018 for ext4 file system in RHEL7.0GA, > >> sometimes it fails and sometime it succeeds. After looking into this case, I > >> think it's not a kernel ext4 bug, it maybe an e4dfrag bug. I compiled the > >> newest e2fsprogs to have test, it seems this issue still exits, so I still > >> send a mail to this list to look for some help, thanks. > >> > >> The issue is that sometimes e4defrag does not defrag file correctly. > >> Steps to reproduce this issue: > >> 1. cd mntpoint > >> 2. rm -f lege > >> 3. for I in `seq 9 -1 0`; > >> do dd if=/dev/zero of=lege bs=4k count=1 conv=notrunc seek=$I oflag=sync &>/dev/null; > >> done; > >> 4. e4defrag -c -v lege > >> > >> Repeatedly execute the 2, 3, 4 steps until you get a file which have the similar extent layout like below: > >> ################################################################ > >> [root@localhost test_e2fsprogs]# e4defrag -c -v lege > >> <File> > >> [ext 1]: start 49365571: logical 0: len 1 > >> [ext 2]: start 49365570: logical 1: len 1 > >> [ext 3]: start 49365569: logical 2: len 1 > >> [ext 4]: start 49365568: logical 3: len 1 > >> [ext 5]: start 49365567: logical 4: len 1 > >> [ext 6]: start 49365566: logical 5: len 1 > >> [ext 7]: start 49365565: logical 6: len 1 > >> [ext 8]: start 49365564: logical 7: len 1 > >> [ext 9]: start 49365563: logical 8: len 1 > >> [ext 10]: start 49365562: logical 9: len 1 > >> > >> Total/best extents 10/1 > >> Average size per extent 4 KB > >> Fragmentation score 98 > >> [0-30 no problem: 31-55 a little bit fragmented: 56- needs defrag] > >> This file (lege) needs defragmentation. > >> Done. > >> ################################################################ > >> The physical blocks are continuous but reversed. > >> > >> If we call e4defrag against this file, the output would be: > >> ################################################################ > >> [root@localhost test_e2fsprogs]# /tmp/e4defrag -v lege > >> ext4 defragmentation for lege > >> [1/1]lege: 100% extents: 10 -> 10 [ OK ] > >> Success: [1/1] > >> [root@localhost test_e2fsprogs]# /tmp/e4defrag -v -c lege > >> <File> > >> [ext 1]: start 49365571: logical 0: len 1 > >> [ext 2]: start 49365570: logical 1: len 1 > >> [ext 3]: start 49365569: logical 2: len 1 > >> [ext 4]: start 49365568: logical 3: len 1 > >> [ext 5]: start 49365567: logical 4: len 1 > >> [ext 6]: start 49365566: logical 5: len 1 > >> [ext 7]: start 49365565: logical 6: len 1 > >> [ext 8]: start 49365564: logical 7: len 1 > >> [ext 9]: start 49365563: logical 8: len 1 > >> [ext 10]: start 49365562: logical 9: len 1 > >> > >> Total/best extents 10/1 > >> Average size per extent 4 KB > >> Fragmentation score 98 > >> [0-30 no problem: 31-55 a little bit fragmented: 56- needs defrag] > >> This file (lege) needs defragmentation. > >> Done. > >> ################################################################ > >> According to my understanding, this file is not defraged correctly and should > >> be convert into one extent. Or because if the physical blocks are continuous > >> though reversed, we do not need to do defragment? > > > > Oh, I think we /do/ need to defragment. Granted, file readahead might paper > > over the symptoms, but since the user explicitly ran e4defrag we can try > > to do better. > > Yeah, agree. > > > >> I have checked the e4defrag source code, whether to do real defragment > >> depends on some conditions, please > >> see this code(e4defrag.c). > >> --main > >> --file_defrag > >> > >> In file_defrag(), there is such a judgement： > >> "if (file_frags_start <= best || orig_physical_cnt <= donor_physical_cnt)", If this returns true, the e4defrag will > >> not call call_defrag() to do real defragment work. > >> > >> Here file_frags_start: number of file fragments before defrag > >> orig_physical_cnt: number of original file's continuous physical region > >> donor_physical_cnt: number of donor file's continuous physical region > >> > >> In this "lege" file, the orig_physical_cnt is 1, and donor_physical_cnt is also 1, so the "if" is satisfied and > >> call_defrag() won't be called. > > > > This is a curious corner case of e4defrag -- if you look in get_file_extents(), > > the list of extents is insertion-sorted by physical block, which means that > > get_physical_count() (stupidly) looks only for gaps in the runs of physical > > blocks. Therefore, e4defrag thinks that this "lege" file has one physical > > extent. Ignoring logical block ordering, this is true, but as you point out, > > this leaves the "file written backwards" case in a fragmented state. So let's > > not ignore the logical block ordering: > > > > What I think we really need to do here is make get_physical_count() smarter -- > > if there's a gap either in the physical or logical offsets of extents, then we > > need to increment *_physical_cnt so that we later decide to defragment the > > file. > > > > (Please keep reading) > I checked the code again, you are right, thanks for your explanation > > > > >> Here I'd like to know the comparison "orig_physical_cnt <= > >> donor_physical_cnt" is useful? According to my understanding, what should we > >> have comparison are number of extents or average extent size. > >> > >> When I have this change: > >> diff --git a/misc/e4defrag.c b/misc/e4defrag.c > >> index a204793..cd95698 100644 > >> --- a/misc/e4defrag.c > >> +++ b/misc/e4defrag.c > >> @@ -1598,8 +1598,7 @@ check_improvement: > >> extents_before_defrag += file_frags_start; > >> } > >> > >> - if (file_frags_start <= best || > >> - orig_physical_cnt <= donor_physical_cnt) { > >> + if (file_frags_start <= best) { > > > > This is incorrect, since the point of the "orig_physical_cnt <= > > donor_physical_cnt" check is to ensure that we don't increase the fragmentation > > of a file by swapping it with pieces from a donor file whose contents are > > spread out over a larger number of runs of physical blocks. > > Ah, I see. I hadn't realized that, thanks. > > > > (It does, however, force defragmentation for all files, so you get the results > > you wanted.) > > > > Please try the patch at the end of this message on for size. It fixes things > > on my test VM; does it fix yours? > > Yeah, it works, thanks. > Would you send a new version patch to fix this issue, or should I do it? I'll send the patch (with a proper changelog) along in my -maint fixes rollup in a few days. --D > > Regards, > Xiaoguang Wang > > > > --D > > > >> printf("\033[79;0H\033[K[%u/%u]%s:\t%3d%%", > >> defraged_file_count, total_count, file, 100); > >> if (mode_flag & DETAIL) > >> > >> Then the "lege" file could be defraged correctly. > >> ################################################################## > >> [root@localhost test_e2fsprogs]# /tmp/e4defrag -v lege > >> ext4 defragmentation for lege > >> [1/1]lege: 100% extents: 10 -> 1 [ OK ] > >> Success: [1/1] > >> [root@localhost test_e2fsprogs]# /tmp/e4defrag -v -c lege > >> <File> > >> [ext 1]: start 49366583: logical 0: len 10 > >> > >> Total/best extents 1/1 > >> Average size per extent 40 KB > >> Fragmentation score 0 > >> [0-30 no problem: 31-55 a little bit fragmented: 56- needs defrag] > >> This file (lege) does not need defragmentation. > >> Done. > >> ################################################################## > >> > >> Any opinion or suggestions will be appreciated! > >> If I'm wrong, please correct me, thanks! > >> > >> Regards, > >> Xiaoguang Wang > > > > diff --git a/misc/e4defrag.c b/misc/e4defrag.c > > index a204793..d0eac60 100644 > > --- a/misc/e4defrag.c > > +++ b/misc/e4defrag.c > > @@ -888,7 +888,9 @@ static int get_physical_count(struct fiemap_extent_list *physical_list_head) > > > > do { > > if ((ext_list_tmp->data.physical + ext_list_tmp->data.len) > > - != ext_list_tmp->next->data.physical) { > > + != ext_list_tmp->next->data.physical || > > + (ext_list_tmp->data.logical + ext_list_tmp->data.len) > > + != ext_list_tmp->next->data.logical) { > > /* This extent and next extent are not continuous. */ > > ret++; > > } > > . > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" 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-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
This thread for me to thinking that it would be useful if filefrag also printed the "fragmentation score" that e4defrag prints that would make It more clear whether a file should be considered "fragmented" or not, and it helps the user to decide if e4defrag should be run on it. I'm not sure whether this is always printed or only with an option. Cheers, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 07/18/2014 10:05 AM, Andreas Dilger wrote: > This thread for me to thinking that it would be useful if filefrag also > printed the "fragmentation score" that e4defrag prints that would make > It more clear whether a file should be considered "fragmented" or not, > and it helps the user to decide if e4defrag should be run on it. Yeah, it would be much useful, later I'll try to do some work about it. > > I'm not sure whether this is always printed or only with an option. Only e4defrag's "-c" option will make "fragmentation score" printed. Regards, Xiaoguang Wang > > Cheers, Andreas > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/misc/e4defrag.c b/misc/e4defrag.c index a204793..d0eac60 100644 --- a/misc/e4defrag.c +++ b/misc/e4defrag.c @@ -888,7 +888,9 @@ static int get_physical_count(struct fiemap_extent_list *physical_list_head) do { if ((ext_list_tmp->data.physical + ext_list_tmp->data.len) - != ext_list_tmp->next->data.physical) { + != ext_list_tmp->next->data.physical || + (ext_list_tmp->data.logical + ext_list_tmp->data.len) + != ext_list_tmp->next->data.logical) { /* This extent and next extent are not continuous. */ ret++; }