[PATCH] Tidy while loop in end_compressed_writeback

[PATCH] Tidy while loop in end_compressed_writeback

[Sahil Kang]

Hi,

This is my first patch so it's a minor code change.
I think removing the early continue from the loop makes the function a 
little easier to follow.
Please have a look and I'd appreciate any feedback.

Thanks,
Sahil


 From 98afe83a570180e841fefe3fd48d450accc42ea3 Mon Sep 17 00:00:00 2001
From: Sahil Kang <sahil.kang@asilaycomputing.com>
Date: Wed, 19 Apr 2017 04:47:00 -0700
Subject: [PATCH] Tidy while loop in end_compressed_writeback

Instead of continuing early in the loop when ret is 0,
we can decrement/increment nr_pages/index by 1 at the ending.
The for loop will not execute when ret is 0 anyway.

Signed-off-by: Sahil Kang <sahil.kang@asilaycomputing.com>
---
  fs/btrfs/compression.c | 7 ++-----
  1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index c473c42..c653297 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -238,17 +238,14 @@ static noinline void 
end_compressed_writeback(struct inode *inode,
          ret = find_get_pages_contig(inode->i_mapping, index,
                       min_t(unsigned long,
                       nr_pages, ARRAY_SIZE(pages)), pages);
-        if (ret == 0) {
-            nr_pages -= 1;
-            index += 1;
-            continue;
-        }
          for (i = 0; i < ret; i++) {
              if (cb->errors)
                  SetPageError(pages[i]);
              end_page_writeback(pages[i]);
              page_cache_release(pages[i]);
          }
+
+        ret = ret ? ret : 1; /* set ret to 1 if it's 0 */
          nr_pages -= ret;
          index += ret;
      }
-- 
2.1.4

Re: [PATCH] Tidy while loop in end_compressed_writeback

[Sahil Kang]

Hi David,

I think my previous email may have failed to send.
Can you merge my patch below?
It's a minor update that I think improves readability.

Thanks,
Sahil

On 04/19/2017 05:43 AM, Sahil Kang wrote:
> Hi,
>
> This is my first patch so it's a minor code change.
> I think removing the early continue from the loop makes the function a 
> little easier to follow.
> Please have a look and I'd appreciate any feedback.
>
> Thanks,
> Sahil
>
>
> From 98afe83a570180e841fefe3fd48d450accc42ea3 Mon Sep 17 00:00:00 2001
> From: Sahil Kang <sahil.kang@asilaycomputing.com>
> Date: Wed, 19 Apr 2017 04:47:00 -0700
> Subject: [PATCH] Tidy while loop in end_compressed_writeback
>
> Instead of continuing early in the loop when ret is 0,
> we can decrement/increment nr_pages/index by 1 at the ending.
> The for loop will not execute when ret is 0 anyway.
>
> Signed-off-by: Sahil Kang <sahil.kang@asilaycomputing.com>
> ---
>  fs/btrfs/compression.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index c473c42..c653297 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -238,17 +238,14 @@ static noinline void 
> end_compressed_writeback(struct inode *inode,
>          ret = find_get_pages_contig(inode->i_mapping, index,
>                       min_t(unsigned long,
>                       nr_pages, ARRAY_SIZE(pages)), pages);
> -        if (ret == 0) {
> -            nr_pages -= 1;
> -            index += 1;
> -            continue;
> -        }
>          for (i = 0; i < ret; i++) {
>              if (cb->errors)
>                  SetPageError(pages[i]);
>              end_page_writeback(pages[i]);
>              page_cache_release(pages[i]);
>          }
> +
> +        ret = ret ? ret : 1; /* set ret to 1 if it's 0 */
>          nr_pages -= ret;
>          index += ret;
>      }

Re: [PATCH] Tidy while loop in end_compressed_writeback

[Sahil Kang]

Hi Chris,

Would you merge my patch below?
It's a minor update to improve the loop's readability.

Thanks,
Sahil

On 04/19/2017 05:43 AM, Sahil Kang wrote:
> Hi,
>
> This is my first patch so it's a minor code change.
> I think removing the early continue from the loop makes the function a 
> little easier to follow.
> Please have a look and I'd appreciate any feedback.
>
> Thanks,
> Sahil
>
>
> From 98afe83a570180e841fefe3fd48d450accc42ea3 Mon Sep 17 00:00:00 2001
> From: Sahil Kang <sahil.kang@asilaycomputing.com>
> Date: Wed, 19 Apr 2017 04:47:00 -0700
> Subject: [PATCH] Tidy while loop in end_compressed_writeback
>
> Instead of continuing early in the loop when ret is 0,
> we can decrement/increment nr_pages/index by 1 at the ending.
> The for loop will not execute when ret is 0 anyway.
>
> Signed-off-by: Sahil Kang <sahil.kang@asilaycomputing.com>
> ---
>  fs/btrfs/compression.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index c473c42..c653297 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -238,17 +238,14 @@ static noinline void 
> end_compressed_writeback(struct inode *inode,
>          ret = find_get_pages_contig(inode->i_mapping, index,
>                       min_t(unsigned long,
>                       nr_pages, ARRAY_SIZE(pages)), pages);
> -        if (ret == 0) {
> -            nr_pages -= 1;
> -            index += 1;
> -            continue;
> -        }
>          for (i = 0; i < ret; i++) {
>              if (cb->errors)
>                  SetPageError(pages[i]);
>              end_page_writeback(pages[i]);
>              page_cache_release(pages[i]);
>          }
> +
> +        ret = ret ? ret : 1; /* set ret to 1 if it's 0 */
>          nr_pages -= ret;
>          index += ret;
>      }

Re: [PATCH] Tidy while loop in end_compressed_writeback

[David Sterba]

On Wed, Apr 19, 2017 at 05:43:19AM -0700, Sahil Kang wrote:
> This is my first patch so it's a minor code change.
> I think removing the early continue from the loop makes the function a 
> little easier to follow.

I think the opposite. In the current code, it's clear that if ret is 0,
then there's a shortcut. While your code change is functionally
equivalent, it takes a bit more time to see that the for loop does
nothing if ret == 0 and overall it looks like we don't handle the return
value coming from find_get_pages_contig, in exchange for some ?:
trickery that requires a comment.

So I don't think this patch makes code better and I'm not going to apply
it, sorry.

Re: [PATCH] Tidy while loop in end_compressed_writeback

[Sahil Kang]

I didn't consider it before, but I agree with your point on explicitly handling the return value from find_get_pages_contig.

Thanks for looking over it,
Sahil


-------- Original Message --------
From: David Sterba <dsterba@suse.cz>
Sent: May 2, 2017 9:01:34 AM PDT
To: Sahil Kang <sahil.kang@asilaycomputing.com>
Cc: linux-btrfs@vger.kernel.org
Subject: Re: [PATCH] Tidy while loop in end_compressed_writeback

On Wed, Apr 19, 2017 at 05:43:19AM -0700, Sahil Kang wrote:
> This is my first patch so it's a minor code change.
> I think removing the early continue from the loop makes the function a 
> little easier to follow.

I think the opposite. In the current code, it's clear that if ret is 0,
then there's a shortcut. While your code change is functionally
equivalent, it takes a bit more time to see that the for loop does
nothing if ret == 0 and overall it looks like we don't handle the return
value coming from find_get_pages_contig, in exchange for some ?:
trickery that requires a comment.

So I don't think this patch makes code better and I'm not going to apply
it, sorry.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html