Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
From: Filipe Manana <fdmanana@gmail.com>
To: Matthew Wilcox <willy@infradead.org>
Cc: linux-btrfs <linux-btrfs@vger.kernel.org>
Subject: Re: Missing unread page handling in readpages
Date: Sat, 15 Feb 2020 11:16:46 +0000
Message-ID: <CAL3q7H7Z182t+qKk4UcN0_10FEe9dEsVehUvtFb7YsLpMzibHw@mail.gmail.com> (raw)
In-Reply-To: <20200215051554.GF7778@bombadil.infradead.org>

On Sat, Feb 15, 2020 at 5:22 AM Matthew Wilcox <willy@infradead.org> wrote:
>
>
> As part of my rewrite of the readahead code, I think I spotted a hole
> in btrfs' handling of errors in the current readpages code.
>
> btrfs_readpages() calls extent_readpages() calls (a number of things)
> then finishes up by calling submit_one_bio().  If submit_one_bio()
> returns an error, I believe btrfs never unlocks the pages which were in
> that bio.

So, the pages are unlocked at end_bio_extent_readpage().

The bio created by extent_readpages() (more specifically at
__do_readpage()) has its ->bi_end_io set to end_bio_extent_readpage().
Then submit_one_bio() calls btrfs_submit_bio_hook() or
btree_submit_bio_hook(). When these functions return an error, they
set the bio's ->bi_status to an error and then call bio_endio(), which
results in end_bio_extent_readpage() being called, and that will
unlock the pages, call SetPageError(), and do all necessary error
handling.

thanks

>  Certainly the VFS does not; the filesystem is presumed to
> unlock those pages when IO finishes.  But AFAICT, returning an error
> there means btrfs will never start IO on those pages submit_one_bio()
> is a btrfs function.  It calls tree->ops->submit_bio_hook() so that's
> either btree_submit_bio_hook() or btrfs_submit_bio_hook(), which certainly
> seem like they might be able to return errors.
>
> So do we need to do something to complete the bio with an error in
> order to unlock those pages?  Or have I failed to notice that already
> happening somewhere?
>


-- 
Filipe David Manana,

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

  reply index

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-15  5:15 Matthew Wilcox
2020-02-15 11:16 ` Filipe Manana [this message]
2020-02-15 16:48   ` Matthew Wilcox

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAL3q7H7Z182t+qKk4UcN0_10FEe9dEsVehUvtFb7YsLpMzibHw@mail.gmail.com \
    --to=fdmanana@gmail.com \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=willy@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org
	public-inbox-index linux-btrfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git