| Message ID | 20190621192828.28900-4-rgoldwyn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Btrfs iomap | expand |
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 >
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?
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?
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.
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
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 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.
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? >
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; }