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

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

[Kazuya Mio]

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 <k-mio@sx.jp.nec.com>
---
 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++) {

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

[Andreas Dilger]

On 2011-08-17, at 1:47 AM, Kazuya Mio <k-mio@sx.jp.nec.com> 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 <k-mio@sx.jp.nec.com>
> ---
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Kazuya Mio]

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