2011-08-17 08:13:45

by Kazuya Mio

[permalink] [raw]
Subject: [PATCH v2 05/12] e4defrag: Add force option for e4defrag

Currently, e4defrag calls EXT4_IOC_MOVE_EXT ioctl if the fragmentation score
of a donor file is zero. However, it is difficult sometimes to create the file
that has the average of 4096 blocks per extent. We can use e4defrag -F to defrag
in this case.

Signed-off-by: Kazuya Mio <[email protected]>
---
misc/e4defrag.8.in | 5 ++++-
misc/e4defrag.c | 14 ++++++++++----
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/misc/e4defrag.8.in b/misc/e4defrag.8.in
index 81adc29..ede7455 100644
--- a/misc/e4defrag.8.in
+++ b/misc/e4defrag.8.in
@@ -4,7 +4,7 @@ e4defrag \- online defragmenter for ext4 filesystem
.SH SYNOPSIS
.B e4defrag
[
-.B \-v
+.B \-Fv
]
.I target
\&...
@@ -31,6 +31,9 @@ gets the mount point of it and reduces fragmentation of all files in this mount
point.
.SH OPTIONS
.TP
+.B \-F
+Force defrag if the fragmentation gets better.
+.TP
.B \-v
Print error messages and the fragmentation count before and after defrag for
each file.
diff --git a/misc/e4defrag.c b/misc/e4defrag.c
index 44d54f3..64b1474 100644
--- a/misc/e4defrag.c
+++ b/misc/e4defrag.c
@@ -80,6 +80,7 @@

/* The mode of defrag */
#define DETAIL 0x01
+#define FORCE 0x02

#define DEVNAME 0
#define DIRNAME 1
@@ -107,7 +108,7 @@

/* The following macros are error message */
#define MSG_USAGE \
-"Usage : e4defrag [-v] file...| directory...| device...\n"
+"Usage : e4defrag [-Fv] file...| directory...| device...\n"

#define NGMSG_EXT4 "Filesystem is not ext4 filesystem"
#define NGMSG_FILE_EXTENT "Failed to get file extents"
@@ -1153,7 +1154,8 @@ check_improvement:
extents_before_defrag += file_frags_start;
}

- if (!orig_score || donor_score) {
+ if (!orig_score || (donor_score && !(mode_flag & FORCE)) ||
+ (orig_score <= donor_score && (mode_flag & FORCE))) {
printf("\033[79;0H\033[K[%u/%u]%s:\t%3d%%",
defraged_file_count, total_count, file, 100);
if (mode_flag & DETAIL)
@@ -1231,8 +1233,12 @@ int main(int argc, char *argv[])
if (argc == 1)
goto out;

- while ((opt = getopt(argc, argv, "v")) != EOF) {
+ while ((opt = getopt(argc, argv, "Fv")) != EOF) {
switch (opt) {
+ case 'F':
+ /* Force defrag if the fragmentation gets better */
+ mode_flag |= FORCE;
+ break;
case 'v':
mode_flag |= DETAIL;
break;
@@ -1245,7 +1251,7 @@ int main(int argc, char *argv[])
goto out;

current_uid = getuid();
- threshold = DEFAULT_THRESHOLD;
+ threshold = (mode_flag & FORCE) ? ~0U : DEFAULT_THRESHOLD;

/* Main process */
for (i = optind; i < argc; i++) {


2011-08-17 16:30:39

by Andreas Dilger

[permalink] [raw]
Subject: Re: [PATCH v2 05/12] e4defrag: Add force option for e4defrag

On 2011-08-17, at 1:47 AM, Kazuya Mio <[email protected]> wrote:
> Currently, e4defrag calls EXT4_IOC_MOVE_EXT ioctl if the fragmentation score
> of a donor file is zero. However, it is difficult sometimes to create the file
> that has the average of 4096 blocks per extent. We can use e4defrag -F to defrag
> in this case.
>
> Signed-off-by: Kazuya Mio <[email protected]>
> ---
> misc/e4defrag.8.in | 5 ++++-
> misc/e4defrag.c | 14 ++++++++++----
> 2 files changed, 14 insertions(+), 5 deletions(-)
> diff --git a/misc/e4defrag.8.in b/misc/e4defrag.8.in
> index 81adc29..ede7455 100644
> --- a/misc/e4defrag.8.in
> +++ b/misc/e4defrag.8.in
> @@ -4,7 +4,7 @@ e4defrag \- online defragmenter for ext4 filesystem
> .SH SYNOPSIS
> .B e4defrag
> [
> -.B \-v
> +.B \-Fv
> ]
> .I target
> \&...
> @@ -31,6 +31,9 @@ gets the mount point of it and reduces fragmentation of all files in this mount
> point.
> .SH OPTIONS
> .TP
> +.B \-F
> +Force defrag if the fragmentation gets better.
> +.TP
> .B \-v
> Print error messages and the fragmentation count before and after defrag for
> each file.
> diff --git a/misc/e4defrag.c b/misc/e4defrag.c
> index 44d54f3..64b1474 100644
> --- a/misc/e4defrag.c
> +++ b/misc/e4defrag.c
> @@ -80,6 +80,7 @@
>
> /* The mode of defrag */
> #define DETAIL 0x01
> +#define FORCE 0x02
>
> #define DEVNAME 0
> #define DIRNAME 1
> @@ -107,7 +108,7 @@
>
> /* The following macros are error message */
> #define MSG_USAGE \
> -"Usage : e4defrag [-v] file...| directory...| device...\n"
> +"Usage : e4defrag [-Fv] file...| directory...| device...\n"
>
> #define NGMSG_EXT4 "Filesystem is not ext4 filesystem"
> #define NGMSG_FILE_EXTENT "Failed to get file extents"
> @@ -1153,7 +1154,8 @@ check_improvement:
> extents_before_defrag += file_frags_start;
> }
>
> - if (!orig_score || donor_score) {
> + if (!orig_score || (donor_score && !(mode_flag & FORCE)) ||
> + (orig_score <= donor_score && (mode_flag & FORCE))) {

It is confusing in conditionals like this when integer variables are treated as boolean values. It would be more clear to compare the orig_score and donor_score to zero.

if (orig_score == 0 || (donor_score > 0 && !(mode_flag & FORCE)) ||
(orig_score <= donor_score && (mode_flag & FORCE))) {

To be honest, I still can't understand the above logic. I would think it is enough to check:

if (orig_score < donor_score && !(mode_flag & FORCE))) {

so that the file is not defragged if the score would get worse, but "force" means it is always moved.

> printf("\033[79;0H\033[K[%u/%u]%s:\t%3d%%",

It would be nice if the color terminal output was optional (maybe only on by default for tty output). This is not typical for other e2fsprogs utilities, and makes a mess of logs being kept of the output.

> defraged_file_count, total_count, file, 100);
> if (mode_flag & DETAIL)
> @@ -1231,8 +1233,12 @@ int main(int argc, char *argv[])
> if (argc == 1)
> goto out;
>
> - while ((opt = getopt(argc, argv, "v")) != EOF) {
> + while ((opt = getopt(argc, argv, "Fv")) != EOF) {
> switch (opt) {
> + case 'F':
> + /* Force defrag if the fragmentation gets better */
> + mode_flag |= FORCE;

Should this comment be "Force defrag if the fragmentation gets _worse_"?

> + break;
> case 'v':
> mode_flag |= DETAIL;
> break;
> @@ -1245,7 +1251,7 @@ int main(int argc, char *argv[])
> goto out;
>
> current_uid = getuid();
> - threshold = DEFAULT_THRESHOLD;
> + threshold = (mode_flag & FORCE) ? ~0U : DEFAULT_THRESHOLD;
>
> /* Main process */
> for (i = optind; i < argc; i++) {
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2011-08-18 08:58:08

by Kazuya Mio

[permalink] [raw]
Subject: Re: [PATCH v2 05/12] e4defrag: Add force option for e4defrag

2011/08/18 1:31, Andreas Dilger wrote:
> It is confusing in conditionals like this when integer variables are treated
> as boolean values. It would be more clear to compare the orig_score and
> donor_score to zero.
>
> if (orig_score == 0 || (donor_score > 0 && !(mode_flag & FORCE)) ||
> (orig_score <= donor_score && (mode_flag & FORCE))) {
>
> To be honest, I still can't understand the above logic. I would think it is
> enough to check:
>
> if (orig_score < donor_score && !(mode_flag & FORCE))) {
>
> so that the file is not defragged if the score would get worse, but "force"
> means it is always moved.

e4defrag has two conditions to call EXT4_IOC_MOVE_EXT ioctl:
(1) the original file is fragmented file (orig_score > 0)
(2) the donor file is not fragmented file (donor_score == 0)

It could be that the donor file is a little fragmented file in case of few disk
space available, so sometimes we cannot defrag a file due to the condition (2).
However, it makes no sense that the fragmentation gets worse due to
e4defrag. Hence, I added a new condition (orig_score > donor_score) instead of
the condition (2) to check whether the fragmentation gets better.

The condition (1) is necessary for e4defarg -F. Because if e2p_get_fragscore()
returns zero in case of maximum threshold (2^32-1), it means the number of
the extents doesn't decrease any more even if the file gets the best extent
mapping.

>> printf("\033[79;0H\033[K[%u/%u]%s:\t%3d%%",
> It would be nice if the color terminal output was optional (maybe only on
> by default for tty output). This is not typical for other e2fsprogs utilities,
> and makes a mess of logs being kept of the output.

I see. I'll fix the output based on mke2fs.

Regards,
Kazuya Mio