On Wed, Jun 27, 2018 at 04:38:22PM +0300, Nikolay Borisov wrote:
> The purpose of the function is to free all the pages comprising an
> extent buffer. This can be achieved with a simple for loop rather than
> the slitghly more involved 'do {} while' construct. So rewrite the
> loop using a 'for' construct. Additionally we can never have an
> extent_buffer that is 0 pages so remove the check for index == 0. No
> functional changes.
The reversed loop makes sense, the first page is special and used for
locking the whole extent buffer's pages, as can be seen eg.
897ca6e9b4fef86d5dfb6b31 or 4f2de97acee6532b36dd6e99 . Also see current
alloc_extent_buffer for the locking and managing the private bit.
So you can still rewrite it as a for loop but would have to preserve the
logic or provide the reason that it's correct to iterate from 0 to
num_pages. There are some subltle races regarding pages and extents and
their presence in various trees, so I'd rather be careful here.
> Signed-off-by: Nikolay Borisov <nborisov@xxxxxxxx>
> ---
> fs/btrfs/extent_io.c | 13 ++++---------
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index cce6087d6880..4180a3b7e725 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4641,19 +4641,14 @@ int extent_buffer_under_io(struct extent_buffer *eb)
> */
> static void btrfs_release_extent_buffer_page(struct extent_buffer *eb)
> {
> - unsigned long index;
> - struct page *page;
> + int i;
> int mapped = !test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags);
>
> BUG_ON(extent_buffer_under_io(eb));
>
> - index = num_extent_pages(eb->start, eb->len);
> - if (index == 0)
> - return;
This check does seem to be redundant.
> + for (i = 0; i < num_extent_pages(eb->start, eb->len); i++) {
I think that the num_extent_pages(...) would be better put to a
temporary variable so it's not evaluated each time the loop termination
condition is checked.
> + struct page *page = eb->pages[i];
>
> - do {
> - index--;
> - page = eb->pages[index];
> if (!page)
> continue;
> if (mapped)
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
