Re: [PATCH] btrfs: fix a bug that btrfs_invalidapge() can double account ordered extent for subpage

[Filipe Manana <fdmanana@gmail.com>]

On Wed, Jan 27, 2021 at 8:29 AM Qu Wenruo <wqu@suse.com> wrote:
>
> Commit dbfdb6d1b369 ("Btrfs: Search for all ordered extents that could
> span across a page") make btrfs_invalidapage() to search all ordered
> extents.
>
> The offending code looks like this:
>
> again:
>         start = page_start;
>         ordered = btrfs_lookup_ordered_range(inode, start, page_end - start + 1);
>         if (ordred) {
>                 end = min(page_end,
>                           ordered->file_offset + ordered->num_bytes - 1);
>
>                 /* Do the cleanup */
>
>                 start = end + 1;
>                 if (start < page_end)
>                         goto again;
>         }
>
> The behavior is indeed necessary for the incoming subpage support, but
> when it iterate through all the ordered extents, it also resets the
> search range @start.
>
> This means, for the following cases, we can double account the ordered
> extents, causing its bytes_left underflow:
>
>         Page offset
>         0               16K             32K
>         |<--- OE 1  --->|<--- OE 2 ---->|
>
> As the first iteration will find OE 1, which doesn't cover the full
> page, thus after cleanup code, we need to retry again.
> But again label will reset start to page_start, and we got OE 1 again,
> which causes double account on OE1, and cause OE1's byte_left to
> underflow.
>
> The only good news is, this problem can only happen for subpage case, as
> for regular sectorsize == PAGE_SIZE case, we will always find a OE ends
> at or after page end, thus no way to trigger the problem.
>
> This patch will move the again label after start = page_start, to fix
> the bug.
> This is just a quick fix, which is easy to backport.

Hum? Why does it need to be backported?
There are no kernel releases that support subpage sector size yet and
this does not affect the case where sector size is the same as the
page size.

>
> There will be more comprehensive rework to convert the open coded loop to
> a proper while loop.
>
> Fixes: dbfdb6d1b369 ("Btrfs: Search for all ordered extents that could span across a page")
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Anyway, it looks good to me, thanks.

Reviewed-by: Filipe Manana <fdmanana@suse.com>

> ---
>  fs/btrfs/inode.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index ef6cb7b620d0..2eea7d22405a 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8184,8 +8184,9 @@ static void btrfs_invalidatepage(struct page *page, unsigned int offset,
>
>         if (!inode_evicting)
>                 lock_extent_bits(tree, page_start, page_end, &cached_state);
> -again:
> +
>         start = page_start;
> +again:
>         ordered = btrfs_lookup_ordered_range(inode, start, page_end - start + 1);
>         if (ordered) {
>                 found_ordered = true;
> --
> 2.30.0
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”