On Thu, Jan 28, 2021 at 11:29 AM Qu Wenruo <w...@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.carpen...@oracle.com>
> Signed-off-by: Qu Wenruo <w...@suse.com>
Reviewed-by: Filipe Manana <fdman...@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.”
