diff mbox

filefrag: count optimize non-verbose mode; count 0 extents properly when verbose

Message ID 4CEC4C86.1030806@redhat.com
State Superseded, archived
Headers show

Commit Message

Eric Sandeen Nov. 23, 2010, 11:21 p.m. UTC
# rm -f a; touch a; filefrag -v a

yields 1 extent when it should be 0.  Without -v, 0 is returned.

Fix this up by special-casing no extents returned in verbose
mode; skip printing the header for the columns too, since there
are no columns to print.

Also, in nonverbose mode we can set fm_extent_count to 0
so that FIEMAP will just query the extent count without gathering
details; as it is today I think a non-verbose query may under-report
the extent count once "count" extents have been filled in.

Addresses-redhat-bugzilla: 653234
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

--
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

Comments

Eric Sandeen Dec. 9, 2010, 5:19 p.m. UTC | #1
On 11/23/10 5:21 PM, Eric Sandeen wrote:
> # rm -f a; touch a; filefrag -v a
> 
> yields 1 extent when it should be 0.  Without -v, 0 is returned.
> 
> Fix this up by special-casing no extents returned in verbose
> mode; skip printing the header for the columns too, since there
> are no columns to print.
> 
> Also, in nonverbose mode we can set fm_extent_count to 0
> so that FIEMAP will just query the extent count without gathering
> details; as it is today I think a non-verbose query may under-report
> the extent count once "count" extents have been filled in.
> 
> Addresses-redhat-bugzilla: 653234
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Ted, wondering if you can push this one in as well, if it looks ok;
I'd like to get it upstream before adding it to our package for the
above bug.

Thanks,
-Eric

> ---
> 
> diff --git a/misc/filefrag.c b/misc/filefrag.c
> index bd4486d..a48b9b0 100644
> --- a/misc/filefrag.c
> +++ b/misc/filefrag.c
> @@ -194,7 +194,14 @@ static int filefrag_fiemap(int fd, int blk_shift, int *num_extents)
>  	do {
>  		fiemap->fm_length = ~0ULL;
>  		fiemap->fm_flags = flags;
> -		fiemap->fm_extent_count = count;
> +		/*
> +		 * If fm_extent_count == 0, FIEMAP returns count of
> +		 * extents found without filling in details.
> +		 */
> +		if (!verbose)
> +			fiemap->fm_extent_count = 0;
> +		else
> +			fiemap->fm_extent_count = count;
>  		rc = ioctl(fd, FS_IOC_FIEMAP, (unsigned long) fiemap);
>  		if (rc < 0) {
>  			if (errno == EBADR && fiemap_incompat_printed == 0) {
> @@ -206,6 +213,14 @@ static int filefrag_fiemap(int fd, int blk_shift, int *num_extents)
>  		}
>  
>  		if (verbose && !fiemap_header_printed) {
> +			/*
> +			 * No extents on first call?
> +			 * Skip header and show 0 extents.
> +			 */
> +			if (fiemap->fm_mapped_extents == 0) {
> +				*num_extents = 0;
> +				goto out;
> +			}
>  			printf(" ext %*s %*s %*s length flags\n", logical_width,
>  			       "logical", physical_width, "physical",
>  			       physical_width, "expected");
> --
> 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
Eric Sandeen March 11, 2011, 3:01 p.m. UTC | #2
On 11/23/10 5:21 PM, Eric Sandeen wrote:
> # rm -f a; touch a; filefrag -v a
> 
> yields 1 extent when it should be 0.  Without -v, 0 is returned.
> 
> Fix this up by special-casing no extents returned in verbose
> mode; skip printing the header for the columns too, since there
> are no columns to print.
> 
> Also, in nonverbose mode we can set fm_extent_count to 0
> so that FIEMAP will just query the extent count without gathering
> details; as it is today I think a non-verbose query may under-report
> the extent count once "count" extents have been filled in.
> 
> Addresses-redhat-bugzilla: 653234
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Ted, ping on this one?  Looks like it got lost.  I noticed this when
Pádraig Brady filed another red hat bug for the same issue,
saying that he had also sent you a patch...

-Eric

> ---
> 
> diff --git a/misc/filefrag.c b/misc/filefrag.c
> index bd4486d..a48b9b0 100644
> --- a/misc/filefrag.c
> +++ b/misc/filefrag.c
> @@ -194,7 +194,14 @@ static int filefrag_fiemap(int fd, int blk_shift, int *num_extents)
>  	do {
>  		fiemap->fm_length = ~0ULL;
>  		fiemap->fm_flags = flags;
> -		fiemap->fm_extent_count = count;
> +		/*
> +		 * If fm_extent_count == 0, FIEMAP returns count of
> +		 * extents found without filling in details.
> +		 */
> +		if (!verbose)
> +			fiemap->fm_extent_count = 0;
> +		else
> +			fiemap->fm_extent_count = count;
>  		rc = ioctl(fd, FS_IOC_FIEMAP, (unsigned long) fiemap);
>  		if (rc < 0) {
>  			if (errno == EBADR && fiemap_incompat_printed == 0) {
> @@ -206,6 +213,14 @@ static int filefrag_fiemap(int fd, int blk_shift, int *num_extents)
>  		}
>  
>  		if (verbose && !fiemap_header_printed) {
> +			/*
> +			 * No extents on first call?
> +			 * Skip header and show 0 extents.
> +			 */
> +			if (fiemap->fm_mapped_extents == 0) {
> +				*num_extents = 0;
> +				goto out;
> +			}
>  			printf(" ext %*s %*s %*s length flags\n", logical_width,
>  			       "logical", physical_width, "physical",
>  			       physical_width, "expected");
> --
> 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
Eric Sandeen April 8, 2011, 4:44 p.m. UTC | #3
On 11/23/10 3:21 PM, Eric Sandeen wrote:
> # rm -f a; touch a; filefrag -v a
> 
> yields 1 extent when it should be 0.  Without -v, 0 is returned.
> 
> Fix this up by special-casing no extents returned in verbose
> mode; skip printing the header for the columns too, since there
> are no columns to print.
> 
> Also, in nonverbose mode we can set fm_extent_count to 0
> so that FIEMAP will just query the extent count without gathering
> details; as it is today I think a non-verbose query may under-report
> the extent count once "count" extents have been filled in.
> 
> Addresses-redhat-bugzilla: 653234
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

ping^3

> ---
> 
> diff --git a/misc/filefrag.c b/misc/filefrag.c
> index bd4486d..a48b9b0 100644
> --- a/misc/filefrag.c
> +++ b/misc/filefrag.c
> @@ -194,7 +194,14 @@ static int filefrag_fiemap(int fd, int blk_shift, int *num_extents)
>  	do {
>  		fiemap->fm_length = ~0ULL;
>  		fiemap->fm_flags = flags;
> -		fiemap->fm_extent_count = count;
> +		/*
> +		 * If fm_extent_count == 0, FIEMAP returns count of
> +		 * extents found without filling in details.
> +		 */
> +		if (!verbose)
> +			fiemap->fm_extent_count = 0;
> +		else
> +			fiemap->fm_extent_count = count;
>  		rc = ioctl(fd, FS_IOC_FIEMAP, (unsigned long) fiemap);
>  		if (rc < 0) {
>  			if (errno == EBADR && fiemap_incompat_printed == 0) {
> @@ -206,6 +213,14 @@ static int filefrag_fiemap(int fd, int blk_shift, int *num_extents)
>  		}
>  
>  		if (verbose && !fiemap_header_printed) {
> +			/*
> +			 * No extents on first call?
> +			 * Skip header and show 0 extents.
> +			 */
> +			if (fiemap->fm_mapped_extents == 0) {
> +				*num_extents = 0;
> +				goto out;
> +			}
>  			printf(" ext %*s %*s %*s length flags\n", logical_width,
>  			       "logical", physical_width, "physical",
>  			       physical_width, "expected");
> --
> 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
Andreas Dilger April 8, 2011, 11:40 p.m. UTC | #4
On 2011-04-08, at 10:44 AM, Eric Sandeen wrote:
> On 11/23/10 3:21 PM, Eric Sandeen wrote:
>> # rm -f a; touch a; filefrag -v a
>> 
>> yields 1 extent when it should be 0.  Without -v, 0 is returned.
>> 
>> Fix this up by special-casing no extents returned in verbose
>> mode; skip printing the header for the columns too, since there
>> are no columns to print.
>> 
>> Also, in nonverbose mode we can set fm_extent_count to 0
>> so that FIEMAP will just query the extent count without gathering
>> details; as it is today I think a non-verbose query may under-report
>> the extent count once "count" extents have been filled in.
>> 
>> Addresses-redhat-bugzilla: 653234
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> ping^3
> 
>> ---
>> 
>> diff --git a/misc/filefrag.c b/misc/filefrag.c
>> index bd4486d..a48b9b0 100644
>> --- a/misc/filefrag.c
>> +++ b/misc/filefrag.c
>> @@ -194,7 +194,14 @@ static int filefrag_fiemap(int fd, int blk_shift, int *num_extents)
>> 	do {
>> 		fiemap->fm_length = ~0ULL;
>> 		fiemap->fm_flags = flags;
>> -		fiemap->fm_extent_count = count;
>> +		/*
>> +		 * If fm_extent_count == 0, FIEMAP returns count of
>> +		 * extents found without filling in details.
>> +		 */
>> +		if (!verbose)
>> +			fiemap->fm_extent_count = 0;
>> +		else
>> +			fiemap->fm_extent_count = count;

This is already checked before the start of the do {} while() loop, just above the context of this patch:

	if (!verbose)
		count = 0;

No point in checking (!verbose) inside the loop.

>> @@ -206,6 +213,14 @@ static int filefrag_fiemap(int fd, int blk_shift, int *num_extents)
>> 		}
>> 
>> 		if (verbose && !fiemap_header_printed) {
>> +			/*
>> +			 * No extents on first call?
>> +			 * Skip header and show 0 extents.
>> +			 */
>> +			if (fiemap->fm_mapped_extents == 0) {
>> +				*num_extents = 0;
>> +				goto out;
>> +			}
>> 			printf(" ext %*s %*s %*s length flags\n", logical_width,
>> 			       "logical", physical_width, "physical",
>> 			       physical_width, "expected");

This part definitely makes sense.

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 mbox

Patch

diff --git a/misc/filefrag.c b/misc/filefrag.c
index bd4486d..a48b9b0 100644
--- a/misc/filefrag.c
+++ b/misc/filefrag.c
@@ -194,7 +194,14 @@  static int filefrag_fiemap(int fd, int blk_shift, int *num_extents)
 	do {
 		fiemap->fm_length = ~0ULL;
 		fiemap->fm_flags = flags;
-		fiemap->fm_extent_count = count;
+		/*
+		 * If fm_extent_count == 0, FIEMAP returns count of
+		 * extents found without filling in details.
+		 */
+		if (!verbose)
+			fiemap->fm_extent_count = 0;
+		else
+			fiemap->fm_extent_count = count;
 		rc = ioctl(fd, FS_IOC_FIEMAP, (unsigned long) fiemap);
 		if (rc < 0) {
 			if (errno == EBADR && fiemap_incompat_printed == 0) {
@@ -206,6 +213,14 @@  static int filefrag_fiemap(int fd, int blk_shift, int *num_extents)
 		}
 
 		if (verbose && !fiemap_header_printed) {
+			/*
+			 * No extents on first call?
+			 * Skip header and show 0 extents.
+			 */
+			if (fiemap->fm_mapped_extents == 0) {
+				*num_extents = 0;
+				goto out;
+			}
 			printf(" ext %*s %*s %*s length flags\n", logical_width,
 			       "logical", physical_width, "physical",
 			       physical_width, "expected");