Re: [PATCH] btrfs: add comment on why we can return 0 if we failed to atomically lock the page in read_extent_buffer_pages()

[Filipe Manana <fdmanana@gmail.com>]

On Thu, Jan 28, 2021 at 11:29 AM Qu Wenruo <wqu@suse.com> wrote:
>
> In read_extent_buffer_pages(), if we failed to lock the page atomically,
> we just exit with return value 0.
>
> This is pretty counter-intuitive, as normally if we can't lock what we
> need, we would return something like -EAGAIN.
>
> But the that return hides under (wait == WAIT_NONE) branch, which only
> get triggered for readahead.
>
> And for readahead, if we failed to lock the page, it means the extent
> buffer is either being read by other thread, or has been read and is
> under modification.
> Either way the eb will or has been cached, thus readahead has no need to
> wait for it.
>
> This patch will add extra comment on this counter-intuitive behavior.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

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

Looks good, thanks.

> ---
>  fs/btrfs/extent_io.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 7f689ad7709c..038adc423454 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5577,6 +5577,13 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
>         for (i = 0; i < num_pages; i++) {
>                 page = eb->pages[i];
>                 if (wait == WAIT_NONE) {
> +                       /*
> +                        * WAIT_NONE is only utilized by readahead. If we can't
> +                        * acquire the lock atomically it means either the eb
> +                        * is being read out or under modification.
> +                        * Either way the eb will be or has been cached,
> +                        * readahead can exit safely.
> +                        */
>                         if (!trylock_page(page))
>                                 goto unlock_exit;
>                 } else {
> --
> 2.30.0
>


-- 
Filipe David Manana,

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