[3/6] iomap: Check iblocksize before transforming page->private
diff mbox series

Message ID 20190621192828.28900-4-rgoldwyn@suse.de
State New
Headers show
Series
  • Btrfs iomap
Related show

Commit Message

Goldwyn Rodrigues June 21, 2019, 7:28 p.m. UTC
From: Goldwyn Rodrigues <rgoldwyn@suse.com>

btrfs uses page->private as well to store extent_buffer. Make
the check stricter to make sure we are using page->private for iop by
comparing iblocksize < PAGE_SIZE.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 include/linux/iomap.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Darrick J. Wong June 22, 2019, 12:21 a.m. UTC | #1
On Fri, Jun 21, 2019 at 02:28:25PM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> btrfs uses page->private as well to store extent_buffer. Make
> the check stricter to make sure we are using page->private for iop by
> comparing iblocksize < PAGE_SIZE.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

/me wonders what will happen when btrfs decides to support blocksize !=
pagesize... will we have to add a pointer to struct iomap_page so that
btrfs can continue to associate an extent_buffer with a page?

--D

> ---
>  include/linux/iomap.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index f49767c7fd83..6511124e58b6 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -128,7 +128,8 @@ struct iomap_page {
>  
>  static inline struct iomap_page *to_iomap_page(struct page *page)
>  {
> -	if (page_has_private(page))
> +	if (i_blocksize(page->mapping->host) < PAGE_SIZE &&
> +			page_has_private(page))
>  		return (struct iomap_page *)page_private(page);
>  	return NULL;
>  }
> -- 
> 2.16.4
>
Christoph Hellwig June 24, 2019, 7:05 a.m. UTC | #2
On Fri, Jun 21, 2019 at 02:28:25PM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> btrfs uses page->private as well to store extent_buffer. Make
> the check stricter to make sure we are using page->private for iop by
> comparing iblocksize < PAGE_SIZE.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

If btrfs uses page->private itself and also uses functions that call
to_iomap_page we have a major problem, as we now have a usage conflict.

How do you end up here?
Goldwyn Rodrigues June 25, 2019, 6:56 p.m. UTC | #3
On  9:05 24/06, Christoph Hellwig wrote:
> On Fri, Jun 21, 2019 at 02:28:25PM -0500, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > 
> > btrfs uses page->private as well to store extent_buffer. Make
> > the check stricter to make sure we are using page->private for iop by
> > comparing iblocksize < PAGE_SIZE.
> > 
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> If btrfs uses page->private itself and also uses functions that call
> to_iomap_page we have a major problem, as we now have a usage conflict.
> 
> How do you end up here?  
> 

Btrfs uses page->private to identify which extent_buffer it belongs to.
So, if you read, it fills the page->private. Then you try to write to
it, iomap will assume it to be iomap_page pointer.

I don't think we can move extent_buffer out of page->private for btrfs.
Any other ideas?
Goldwyn Rodrigues June 25, 2019, 7:22 p.m. UTC | #4
On 17:21 21/06, Darrick J. Wong wrote:
> On Fri, Jun 21, 2019 at 02:28:25PM -0500, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > 
> > btrfs uses page->private as well to store extent_buffer. Make
> > the check stricter to make sure we are using page->private for iop by
> > comparing iblocksize < PAGE_SIZE.
> > 
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> /me wonders what will happen when btrfs decides to support blocksize !=
> pagesize... will we have to add a pointer to struct iomap_page so that
> btrfs can continue to associate an extent_buffer with a page?
> 

Yes, I did not think about that. It would be nasty to put wrappers
to get to the right pointer.
Filipe Manana June 25, 2019, 8:04 p.m. UTC | #5
On Tue, Jun 25, 2019 at 8:58 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>
> On  9:05 24/06, Christoph Hellwig wrote:
> > On Fri, Jun 21, 2019 at 02:28:25PM -0500, Goldwyn Rodrigues wrote:
> > > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > >
> > > btrfs uses page->private as well to store extent_buffer. Make
> > > the check stricter to make sure we are using page->private for iop by
> > > comparing iblocksize < PAGE_SIZE.
> > >
> > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >
> > If btrfs uses page->private itself and also uses functions that call
> > to_iomap_page we have a major problem, as we now have a usage conflict.
> >
> > How do you end up here?
> >
>
> Btrfs uses page->private to identify which extent_buffer it belongs to.
> So, if you read, it fills the page->private. Then you try to write to
> it, iomap will assume it to be iomap_page pointer.
>
> I don't think we can move extent_buffer out of page->private for btrfs.
> Any other ideas?

The extent buffer is only for pages belonging to the btree inode (i.e.
pages that correspond to a btree node/lead).
Haven't looked in detail to this patchset, but you can't do buffered
writes or direct IO against the btree inode, can you?
So for file inodes, this problem doesn't exist.

Thanks.

>
> --
> Goldwyn
Goldwyn Rodrigues June 26, 2019, 3:03 a.m. UTC | #6
On 21:04 25/06, Filipe Manana wrote:
> On Tue, Jun 25, 2019 at 8:58 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> >
> > On  9:05 24/06, Christoph Hellwig wrote:
> > > On Fri, Jun 21, 2019 at 02:28:25PM -0500, Goldwyn Rodrigues wrote:
> > > > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > > >
> > > > btrfs uses page->private as well to store extent_buffer. Make
> > > > the check stricter to make sure we are using page->private for iop by
> > > > comparing iblocksize < PAGE_SIZE.
> > > >
> > > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > >
> > > If btrfs uses page->private itself and also uses functions that call
> > > to_iomap_page we have a major problem, as we now have a usage conflict.
> > >
> > > How do you end up here?
> > >
> >
> > Btrfs uses page->private to identify which extent_buffer it belongs to.
> > So, if you read, it fills the page->private. Then you try to write to
> > it, iomap will assume it to be iomap_page pointer.
> >
> > I don't think we can move extent_buffer out of page->private for btrfs.
> > Any other ideas?
> 
> The extent buffer is only for pages belonging to the btree inode (i.e.
> pages that correspond to a btree node/lead).
> Haven't looked in detail to this patchset, but you can't do buffered
> writes or direct IO against the btree inode, can you?
> So for file inodes, this problem doesn't exist.

Why do we call set_page_extent_mapped(page) in lock_and_cleanup_extent_if_needed() or __do_readpage()?

I must admit, the backtrace crashes that I saw had the page->private set to
EXTENT_PAGE_PRIVATE rather than the extent_buffer pointer. Does that mean
calling this function is not necessary in these codepaths?
Christoph Hellwig June 26, 2019, 6:16 a.m. UTC | #7
On Tue, Jun 25, 2019 at 01:56:59PM -0500, Goldwyn Rodrigues wrote:
> Btrfs uses page->private to identify which extent_buffer it belongs to.
> So, if you read, it fills the page->private. Then you try to write to
> it, iomap will assume it to be iomap_page pointer.

Yes, and that is going to run into problems sooner or later, that is
if you want to support sub-page size block sizes in btrfs, which I
though is work in progress, or if you ever want to write through iomap.

> I don't think we can move extent_buffer out of page->private for btrfs.
> Any other ideas?

I think you'll have to.  That being said I don't see why you'd need
data in page->private for pages potentially being read in a setup
where blocksize == PAGESIZE anyway.
Nikolay Borisov June 26, 2019, 6:42 a.m. UTC | #8
On 26.06.19 г. 6:03 ч., Goldwyn Rodrigues wrote:
> On 21:04 25/06, Filipe Manana wrote:
>> On Tue, Jun 25, 2019 at 8:58 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>>>
>>> On  9:05 24/06, Christoph Hellwig wrote:
>>>> On Fri, Jun 21, 2019 at 02:28:25PM -0500, Goldwyn Rodrigues wrote:
>>>>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>>>
>>>>> btrfs uses page->private as well to store extent_buffer. Make
>>>>> the check stricter to make sure we are using page->private for iop by
>>>>> comparing iblocksize < PAGE_SIZE.
>>>>>
>>>>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>>
>>>> If btrfs uses page->private itself and also uses functions that call
>>>> to_iomap_page we have a major problem, as we now have a usage conflict.
>>>>
>>>> How do you end up here?
>>>>
>>>
>>> Btrfs uses page->private to identify which extent_buffer it belongs to.
>>> So, if you read, it fills the page->private. Then you try to write to
>>> it, iomap will assume it to be iomap_page pointer.
>>>
>>> I don't think we can move extent_buffer out of page->private for btrfs.
>>> Any other ideas?
>>
>> The extent buffer is only for pages belonging to the btree inode (i.e.
>> pages that correspond to a btree node/lead).
>> Haven't looked in detail to this patchset, but you can't do buffered
>> writes or direct IO against the btree inode, can you?
>> So for file inodes, this problem doesn't exist.
> 
> Why do we call set_page_extent_mapped(page) in lock_and_cleanup_extent_if_needed() or __do_readpage()?
> 
> I must admit, the backtrace crashes that I saw had the page->private set to
> EXTENT_PAGE_PRIVATE rather than the extent_buffer pointer. Does that mean
> calling this function is not necessary in these codepaths?

On a quick look it seems PagePrivate is not used for ordinary data
pages. E.g. set_Page_extent_mapped seems to be a leftover from old code.
No one is actually checking the EXTENT_PAGE_PRIVATE flag. For data pages
what seems to be important is PagePrivate2 for the fixup worker. IMO
set_page_extent_mapped should be removed as well as references to
pageprivate in the context of ordinary data pages.

Chris, any ideas what set_page_extent_mapped was used for?

>

Patch
diff mbox series

diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index f49767c7fd83..6511124e58b6 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -128,7 +128,8 @@  struct iomap_page {
 
 static inline struct iomap_page *to_iomap_page(struct page *page)
 {
-	if (page_has_private(page))
+	if (i_blocksize(page->mapping->host) < PAGE_SIZE &&
+			page_has_private(page))
 		return (struct iomap_page *)page_private(page);
 	return NULL;
 }