* [PATCH] btrfs: change the set_page_extent_mapped() call into an ASSERT()
@ 2021-07-27 1:39 Qu Wenruo
2021-07-27 10:29 ` Qu Wenruo
0 siblings, 1 reply; 3+ messages in thread
From: Qu Wenruo @ 2021-07-27 1:39 UTC (permalink / raw)
To: linux-btrfs
Btrfs uses set_page_extent_mapped() to properly setup a page.
That function would set PagePrivate, and populate needed structure for
subpage.
The timing of calling set_page_extent_mapped() happens before reading a
page or dirtying a page.
Thus when we got a page to write back, if it doesn't have PagePrivate,
it is a big problem in code logic.
Calling set_page_extent_mapped() for such page would just mask the
problem.
Furthermore, for subpage case, we call subpage error helper to clear the
page error bit before calling set_page_extent_mapped().
If we really got a page without Private bit, it can call kernel NULL
pointer dereference.
So change the set_page_extent_mapped() call to an ASSERT(), and move the
check before any page status update call.
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
fs/btrfs/extent_io.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 62f0ed2de2b9..219add264acf 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4099,6 +4099,12 @@ static int __extent_writepage(struct page *page, struct writeback_control *wbc,
WARN_ON(!PageLocked(page));
+ /*
+ * All dirty page to be written should have PagePrivate set by
+ * set_page_extent_mapped() when creating the page.
+ */
+ ASSERT(PagePrivate(page));
+
btrfs_page_clear_error(btrfs_sb(inode->i_sb), page,
page_offset(page), PAGE_SIZE);
@@ -4115,12 +4121,6 @@ static int __extent_writepage(struct page *page, struct writeback_control *wbc,
flush_dcache_page(page);
}
- ret = set_page_extent_mapped(page);
- if (ret < 0) {
- SetPageError(page);
- goto done;
- }
-
if (!epd->extent_locked) {
ret = writepage_delalloc(BTRFS_I(inode), page, wbc,
&nr_written);
--
2.32.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] btrfs: change the set_page_extent_mapped() call into an ASSERT()
2021-07-27 1:39 [PATCH] btrfs: change the set_page_extent_mapped() call into an ASSERT() Qu Wenruo
@ 2021-07-27 10:29 ` Qu Wenruo
2021-07-27 14:23 ` David Sterba
0 siblings, 1 reply; 3+ messages in thread
From: Qu Wenruo @ 2021-07-27 10:29 UTC (permalink / raw)
To: linux-btrfs
On 2021/7/27 上午9:39, Qu Wenruo wrote:
> Btrfs uses set_page_extent_mapped() to properly setup a page.
>
> That function would set PagePrivate, and populate needed structure for
> subpage.
> The timing of calling set_page_extent_mapped() happens before reading a
> page or dirtying a page.
> Thus when we got a page to write back, if it doesn't have PagePrivate,
> it is a big problem in code logic.
>
> Calling set_page_extent_mapped() for such page would just mask the
> problem.
> Furthermore, for subpage case, we call subpage error helper to clear the
> page error bit before calling set_page_extent_mapped().
> If we really got a page without Private bit, it can call kernel NULL
> pointer dereference.
>
> So change the set_page_extent_mapped() call to an ASSERT(), and move the
> check before any page status update call.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
Please discard the patch.
Although I haven't hit any problem testing the patch, it's still
possible that we have a special page that would need cow fixup.
Such page will be:
- Dirty
- Not Private
Thus no page->private, this could still cause problem for subpage case
though
- No EXTENT_DELALLOC flags set for any range inside the page
Thus writepage_delalloc() will not find a delalloc range inside the
page.
Such page will be caught by btrfs_writepage_cow_fixup(), but it will
trigger the ASSERT() added by this patch.
Thanks,
Qu
> ---
> fs/btrfs/extent_io.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 62f0ed2de2b9..219add264acf 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4099,6 +4099,12 @@ static int __extent_writepage(struct page *page, struct writeback_control *wbc,
>
> WARN_ON(!PageLocked(page));
>
> + /*
> + * All dirty page to be written should have PagePrivate set by
> + * set_page_extent_mapped() when creating the page.
> + */
> + ASSERT(PagePrivate(page));
> +
> btrfs_page_clear_error(btrfs_sb(inode->i_sb), page,
> page_offset(page), PAGE_SIZE);
>
> @@ -4115,12 +4121,6 @@ static int __extent_writepage(struct page *page, struct writeback_control *wbc,
> flush_dcache_page(page);
> }
>
> - ret = set_page_extent_mapped(page);
> - if (ret < 0) {
> - SetPageError(page);
> - goto done;
> - }
> -
> if (!epd->extent_locked) {
> ret = writepage_delalloc(BTRFS_I(inode), page, wbc,
> &nr_written);
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] btrfs: change the set_page_extent_mapped() call into an ASSERT()
2021-07-27 10:29 ` Qu Wenruo
@ 2021-07-27 14:23 ` David Sterba
0 siblings, 0 replies; 3+ messages in thread
From: David Sterba @ 2021-07-27 14:23 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
On Tue, Jul 27, 2021 at 06:29:08PM +0800, Qu Wenruo wrote:
>
>
> On 2021/7/27 上午9:39, Qu Wenruo wrote:
> > Btrfs uses set_page_extent_mapped() to properly setup a page.
> >
> > That function would set PagePrivate, and populate needed structure for
> > subpage.
> > The timing of calling set_page_extent_mapped() happens before reading a
> > page or dirtying a page.
> > Thus when we got a page to write back, if it doesn't have PagePrivate,
> > it is a big problem in code logic.
> >
> > Calling set_page_extent_mapped() for such page would just mask the
> > problem.
> > Furthermore, for subpage case, we call subpage error helper to clear the
> > page error bit before calling set_page_extent_mapped().
> > If we really got a page without Private bit, it can call kernel NULL
> > pointer dereference.
> >
> > So change the set_page_extent_mapped() call to an ASSERT(), and move the
> > check before any page status update call.
> >
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> Please discard the patch.
>
> Although I haven't hit any problem testing the patch, it's still
> possible that we have a special page that would need cow fixup.
>
> Such page will be:
>
> - Dirty
>
> - Not Private
> Thus no page->private, this could still cause problem for subpage case
> though
>
> - No EXTENT_DELALLOC flags set for any range inside the page
> Thus writepage_delalloc() will not find a delalloc range inside the
> page.
>
> Such page will be caught by btrfs_writepage_cow_fixup(), but it will
> trigger the ASSERT() added by this patch.
Hm yeah the mismatch between dirty/private/delalloc sounds exactly like
work for the cow fixup.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-07-27 14:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-27 1:39 [PATCH] btrfs: change the set_page_extent_mapped() call into an ASSERT() Qu Wenruo
2021-07-27 10:29 ` Qu Wenruo
2021-07-27 14:23 ` David Sterba
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.