* [PATCH 1/3] btrfs: Fix some folio-related comments
@ 2025-01-21 5:40 Matthew Wilcox (Oracle)
2025-01-21 5:40 ` [PATCH 2/3] btrfs: Fix two misuses of folio_shift() Matthew Wilcox (Oracle)
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Matthew Wilcox (Oracle) @ 2025-01-21 5:40 UTC (permalink / raw)
To: Chris Mason, Josef Bacik, David Sterba
Cc: Matthew Wilcox (Oracle), linux-btrfs
Remove references to the page lock and page->mapping. Also
btrfs folios can never be swizzled into swap.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
fs/btrfs/extent_io.c | 8 +++-----
fs/btrfs/inode.c | 2 +-
fs/btrfs/subpage.c | 2 +-
3 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index d9f856358704..289ecb8ce217 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2189,10 +2189,8 @@ static int extent_write_cache_pages(struct address_space *mapping,
done_index = folio_next_index(folio);
/*
* At this point we hold neither the i_pages lock nor
- * the page lock: the page may be truncated or
- * invalidated (changing page->mapping to NULL),
- * or even swizzled back from swapper_space to
- * tmpfs file mapping
+ * the folio lock: the folio may be truncated or
+ * invalidated (changing folio->mapping to NULL)
*/
if (!folio_trylock(folio)) {
submit_write_bio(bio_ctrl, 0);
@@ -3222,7 +3220,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
}
/*
* Now all pages of that extent buffer is unmapped, set UNMAPPED flag,
- * so it can be cleaned up without utilizing page->mapping.
+ * so it can be cleaned up without utilizing folio->mapping.
*/
set_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index fe2c810335ff..e6ea4f172b50 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2891,7 +2891,7 @@ int btrfs_writepage_cow_fixup(struct folio *folio)
* We are already holding a reference to this inode from
* write_cache_pages. We need to hold it because the space reservation
* takes place outside of the folio lock, and we can't trust
- * page->mapping outside of the folio lock.
+ * folio->mapping outside of the folio lock.
*/
ihold(inode);
btrfs_folio_set_checked(fs_info, folio, folio_pos(folio), folio_size(folio));
diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c
index 722acf768396..6a8636c0ffed 100644
--- a/fs/btrfs/subpage.c
+++ b/fs/btrfs/subpage.c
@@ -72,7 +72,7 @@ bool btrfs_is_subpage(const struct btrfs_fs_info *fs_info, struct address_space
/*
* Only data pages (either through DIO or compression) can have no
- * mapping. And if page->mapping->host is data inode, it's subpage.
+ * mapping. And if mapping->host is data inode, it's subpage.
* As we have ruled our sectorsize >= PAGE_SIZE case already.
*/
if (!mapping || !mapping->host || is_data_inode(BTRFS_I(mapping->host)))
--
2.45.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] btrfs: Fix two misuses of folio_shift()
2025-01-21 5:40 [PATCH 1/3] btrfs: Fix some folio-related comments Matthew Wilcox (Oracle)
@ 2025-01-21 5:40 ` Matthew Wilcox (Oracle)
2025-01-21 7:21 ` Qu Wenruo
2025-01-21 5:40 ` [PATCH 3/3] btrfs: Convert io_ctl_prepare_pages() to work on folios Matthew Wilcox (Oracle)
2025-01-21 7:14 ` [PATCH 1/3] btrfs: Fix some folio-related comments Qu Wenruo
2 siblings, 1 reply; 11+ messages in thread
From: Matthew Wilcox (Oracle) @ 2025-01-21 5:40 UTC (permalink / raw)
To: Chris Mason, Josef Bacik, David Sterba
Cc: Matthew Wilcox (Oracle), linux-btrfs
It is meaningless to shift a byte count by folio_shift(). The folio index
is in units of PAGE_SIZE, not folio_size(). We can use folio_contains()
to make this work for arbitrary-order folios, so remove the assertion
that the folios are of order 0.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
fs/btrfs/extent_io.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 289ecb8ce217..c9b0ee841501 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -523,8 +523,6 @@ static void end_bbio_data_read(struct btrfs_bio *bbio)
u64 end;
u32 len;
- /* For now only order 0 folios are supported for data. */
- ASSERT(folio_order(folio) == 0);
btrfs_debug(fs_info,
"%s: bi_sector=%llu, err=%d, mirror=%u",
__func__, bio->bi_iter.bi_sector, bio->bi_status,
@@ -552,7 +550,6 @@ static void end_bbio_data_read(struct btrfs_bio *bbio)
if (likely(uptodate)) {
loff_t i_size = i_size_read(inode);
- pgoff_t end_index = i_size >> folio_shift(folio);
/*
* Zero out the remaining part if this range straddles
@@ -563,7 +560,8 @@ static void end_bbio_data_read(struct btrfs_bio *bbio)
*
* NOTE: i_size is exclusive while end is inclusive.
*/
- if (folio_index(folio) == end_index && i_size <= end) {
+ if (folio_contains(folio, i_size >> PAGE_SHIFT) &&
+ i_size <= end) {
u32 zero_start = max(offset_in_folio(folio, i_size),
offset_in_folio(folio, start));
u32 zero_len = offset_in_folio(folio, end) + 1 -
@@ -956,7 +954,7 @@ static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached,
return ret;
}
- if (folio->index == last_byte >> folio_shift(folio)) {
+ if (folio_contains(folio, last_byte >> PAGE_SHIFT)) {
size_t zero_offset = offset_in_folio(folio, last_byte);
if (zero_offset) {
--
2.45.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] btrfs: Convert io_ctl_prepare_pages() to work on folios
2025-01-21 5:40 [PATCH 1/3] btrfs: Fix some folio-related comments Matthew Wilcox (Oracle)
2025-01-21 5:40 ` [PATCH 2/3] btrfs: Fix two misuses of folio_shift() Matthew Wilcox (Oracle)
@ 2025-01-21 5:40 ` Matthew Wilcox (Oracle)
2025-01-21 7:22 ` Qu Wenruo
2025-01-21 7:14 ` [PATCH 1/3] btrfs: Fix some folio-related comments Qu Wenruo
2 siblings, 1 reply; 11+ messages in thread
From: Matthew Wilcox (Oracle) @ 2025-01-21 5:40 UTC (permalink / raw)
To: Chris Mason, Josef Bacik, David Sterba
Cc: Matthew Wilcox (Oracle), linux-btrfs
Retrieve folios instead of pages and work on them throughout. Removes
a few calls to compound_head() and a reference to page->mapping.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
fs/btrfs/free-space-cache.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index d42b6f882f57..93b3b7c23d9b 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -447,7 +447,7 @@ static void io_ctl_drop_pages(struct btrfs_io_ctl *io_ctl)
static int io_ctl_prepare_pages(struct btrfs_io_ctl *io_ctl, bool uptodate)
{
- struct page *page;
+ struct folio *folio;
struct inode *inode = io_ctl->inode;
gfp_t mask = btrfs_alloc_write_mask(inode->i_mapping);
int i;
@@ -455,31 +455,32 @@ static int io_ctl_prepare_pages(struct btrfs_io_ctl *io_ctl, bool uptodate)
for (i = 0; i < io_ctl->num_pages; i++) {
int ret;
- page = find_or_create_page(inode->i_mapping, i, mask);
- if (!page) {
+ folio = __filemap_get_folio(inode->i_mapping, i,
+ FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);
+ if (IS_ERR(folio)) {
io_ctl_drop_pages(io_ctl);
return -ENOMEM;
}
- ret = set_folio_extent_mapped(page_folio(page));
+ ret = set_folio_extent_mapped(folio);
if (ret < 0) {
- unlock_page(page);
- put_page(page);
+ folio_unlock(folio);
+ folio_put(folio);
io_ctl_drop_pages(io_ctl);
return ret;
}
- io_ctl->pages[i] = page;
- if (uptodate && !PageUptodate(page)) {
- btrfs_read_folio(NULL, page_folio(page));
- lock_page(page);
- if (page->mapping != inode->i_mapping) {
+ io_ctl->pages[i] = &folio->page;
+ if (uptodate && !folio_test_uptodate(folio)) {
+ btrfs_read_folio(NULL, folio);
+ folio_lock(folio);
+ if (folio->mapping != inode->i_mapping) {
btrfs_err(BTRFS_I(inode)->root->fs_info,
"free space cache page truncated");
io_ctl_drop_pages(io_ctl);
return -EIO;
}
- if (!PageUptodate(page)) {
+ if (!folio_test_uptodate(folio)) {
btrfs_err(BTRFS_I(inode)->root->fs_info,
"error reading free space cache");
io_ctl_drop_pages(io_ctl);
--
2.45.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] btrfs: Fix some folio-related comments
2025-01-21 5:40 [PATCH 1/3] btrfs: Fix some folio-related comments Matthew Wilcox (Oracle)
2025-01-21 5:40 ` [PATCH 2/3] btrfs: Fix two misuses of folio_shift() Matthew Wilcox (Oracle)
2025-01-21 5:40 ` [PATCH 3/3] btrfs: Convert io_ctl_prepare_pages() to work on folios Matthew Wilcox (Oracle)
@ 2025-01-21 7:14 ` Qu Wenruo
2 siblings, 0 replies; 11+ messages in thread
From: Qu Wenruo @ 2025-01-21 7:14 UTC (permalink / raw)
To: Matthew Wilcox (Oracle), Chris Mason, Josef Bacik, David Sterba
Cc: linux-btrfs
在 2025/1/21 16:10, Matthew Wilcox (Oracle) 写道:
> Remove references to the page lock and page->mapping. Also
> btrfs folios can never be swizzled into swap.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Qu Wenruo <wqu@suse.com>
Thanks,
Qu
> ---
> fs/btrfs/extent_io.c | 8 +++-----
> fs/btrfs/inode.c | 2 +-
> fs/btrfs/subpage.c | 2 +-
> 3 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index d9f856358704..289ecb8ce217 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2189,10 +2189,8 @@ static int extent_write_cache_pages(struct address_space *mapping,
> done_index = folio_next_index(folio);
> /*
> * At this point we hold neither the i_pages lock nor
> - * the page lock: the page may be truncated or
> - * invalidated (changing page->mapping to NULL),
> - * or even swizzled back from swapper_space to
> - * tmpfs file mapping
> + * the folio lock: the folio may be truncated or
> + * invalidated (changing folio->mapping to NULL)
> */
> if (!folio_trylock(folio)) {
> submit_write_bio(bio_ctrl, 0);
> @@ -3222,7 +3220,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
> }
> /*
> * Now all pages of that extent buffer is unmapped, set UNMAPPED flag,
> - * so it can be cleaned up without utilizing page->mapping.
> + * so it can be cleaned up without utilizing folio->mapping.
> */
> set_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index fe2c810335ff..e6ea4f172b50 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2891,7 +2891,7 @@ int btrfs_writepage_cow_fixup(struct folio *folio)
> * We are already holding a reference to this inode from
> * write_cache_pages. We need to hold it because the space reservation
> * takes place outside of the folio lock, and we can't trust
> - * page->mapping outside of the folio lock.
> + * folio->mapping outside of the folio lock.
> */
> ihold(inode);
> btrfs_folio_set_checked(fs_info, folio, folio_pos(folio), folio_size(folio));
> diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c
> index 722acf768396..6a8636c0ffed 100644
> --- a/fs/btrfs/subpage.c
> +++ b/fs/btrfs/subpage.c
> @@ -72,7 +72,7 @@ bool btrfs_is_subpage(const struct btrfs_fs_info *fs_info, struct address_space
>
> /*
> * Only data pages (either through DIO or compression) can have no
> - * mapping. And if page->mapping->host is data inode, it's subpage.
> + * mapping. And if mapping->host is data inode, it's subpage.
> * As we have ruled our sectorsize >= PAGE_SIZE case already.
> */
> if (!mapping || !mapping->host || is_data_inode(BTRFS_I(mapping->host)))
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] btrfs: Fix two misuses of folio_shift()
2025-01-21 5:40 ` [PATCH 2/3] btrfs: Fix two misuses of folio_shift() Matthew Wilcox (Oracle)
@ 2025-01-21 7:21 ` Qu Wenruo
2025-01-21 16:10 ` David Sterba
0 siblings, 1 reply; 11+ messages in thread
From: Qu Wenruo @ 2025-01-21 7:21 UTC (permalink / raw)
To: Matthew Wilcox (Oracle), Chris Mason, Josef Bacik, David Sterba
Cc: linux-btrfs
在 2025/1/21 16:10, Matthew Wilcox (Oracle) 写道:
> It is meaningless to shift a byte count by folio_shift(). The folio index
> is in units of PAGE_SIZE, not folio_size(). We can use folio_contains()
> to make this work for arbitrary-order folios, so remove the assertion
> that the folios are of order 0.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> fs/btrfs/extent_io.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 289ecb8ce217..c9b0ee841501 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -523,8 +523,6 @@ static void end_bbio_data_read(struct btrfs_bio *bbio)
> u64 end;
> u32 len;
>
> - /* For now only order 0 folios are supported for data. */
> - ASSERT(folio_order(folio) == 0);
I'd prefer to keep this ASSERT(), as all the btrfs_folio_*() helpers can
only handle page sized folio for now.
> btrfs_debug(fs_info,
> "%s: bi_sector=%llu, err=%d, mirror=%u",
> __func__, bio->bi_iter.bi_sector, bio->bi_status,
> @@ -552,7 +550,6 @@ static void end_bbio_data_read(struct btrfs_bio *bbio)
>
> if (likely(uptodate)) {
> loff_t i_size = i_size_read(inode);
> - pgoff_t end_index = i_size >> folio_shift(folio);
>
> /*
> * Zero out the remaining part if this range straddles
> @@ -563,7 +560,8 @@ static void end_bbio_data_read(struct btrfs_bio *bbio)
> *
> * NOTE: i_size is exclusive while end is inclusive.
> */
> - if (folio_index(folio) == end_index && i_size <= end) {
> + if (folio_contains(folio, i_size >> PAGE_SHIFT) &&
Although folio_contains() is already a pretty good improvement, can we
have a bytenr/i_sized based solution for fs usages?
Thanks,
Qu
> + i_size <= end) {
> u32 zero_start = max(offset_in_folio(folio, i_size),
> offset_in_folio(folio, start));
> u32 zero_len = offset_in_folio(folio, end) + 1 -
> @@ -956,7 +954,7 @@ static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached,
> return ret;
> }
>
> - if (folio->index == last_byte >> folio_shift(folio)) {
> + if (folio_contains(folio, last_byte >> PAGE_SHIFT)) {
> size_t zero_offset = offset_in_folio(folio, last_byte);
>
> if (zero_offset) {
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] btrfs: Convert io_ctl_prepare_pages() to work on folios
2025-01-21 5:40 ` [PATCH 3/3] btrfs: Convert io_ctl_prepare_pages() to work on folios Matthew Wilcox (Oracle)
@ 2025-01-21 7:22 ` Qu Wenruo
0 siblings, 0 replies; 11+ messages in thread
From: Qu Wenruo @ 2025-01-21 7:22 UTC (permalink / raw)
To: Matthew Wilcox (Oracle), Chris Mason, Josef Bacik, David Sterba
Cc: linux-btrfs
在 2025/1/21 16:10, Matthew Wilcox (Oracle) 写道:
> Retrieve folios instead of pages and work on them throughout. Removes
> a few calls to compound_head() and a reference to page->mapping.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Qu Wenruo <wqu@suse.com>
Thanks,
Qu
> ---
> fs/btrfs/free-space-cache.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
> index d42b6f882f57..93b3b7c23d9b 100644
> --- a/fs/btrfs/free-space-cache.c
> +++ b/fs/btrfs/free-space-cache.c
> @@ -447,7 +447,7 @@ static void io_ctl_drop_pages(struct btrfs_io_ctl *io_ctl)
>
> static int io_ctl_prepare_pages(struct btrfs_io_ctl *io_ctl, bool uptodate)
> {
> - struct page *page;
> + struct folio *folio;
> struct inode *inode = io_ctl->inode;
> gfp_t mask = btrfs_alloc_write_mask(inode->i_mapping);
> int i;
> @@ -455,31 +455,32 @@ static int io_ctl_prepare_pages(struct btrfs_io_ctl *io_ctl, bool uptodate)
> for (i = 0; i < io_ctl->num_pages; i++) {
> int ret;
>
> - page = find_or_create_page(inode->i_mapping, i, mask);
> - if (!page) {
> + folio = __filemap_get_folio(inode->i_mapping, i,
> + FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);
> + if (IS_ERR(folio)) {
> io_ctl_drop_pages(io_ctl);
> return -ENOMEM;
> }
>
> - ret = set_folio_extent_mapped(page_folio(page));
> + ret = set_folio_extent_mapped(folio);
> if (ret < 0) {
> - unlock_page(page);
> - put_page(page);
> + folio_unlock(folio);
> + folio_put(folio);
> io_ctl_drop_pages(io_ctl);
> return ret;
> }
>
> - io_ctl->pages[i] = page;
> - if (uptodate && !PageUptodate(page)) {
> - btrfs_read_folio(NULL, page_folio(page));
> - lock_page(page);
> - if (page->mapping != inode->i_mapping) {
> + io_ctl->pages[i] = &folio->page;
> + if (uptodate && !folio_test_uptodate(folio)) {
> + btrfs_read_folio(NULL, folio);
> + folio_lock(folio);
> + if (folio->mapping != inode->i_mapping) {
> btrfs_err(BTRFS_I(inode)->root->fs_info,
> "free space cache page truncated");
> io_ctl_drop_pages(io_ctl);
> return -EIO;
> }
> - if (!PageUptodate(page)) {
> + if (!folio_test_uptodate(folio)) {
> btrfs_err(BTRFS_I(inode)->root->fs_info,
> "error reading free space cache");
> io_ctl_drop_pages(io_ctl);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] btrfs: Fix two misuses of folio_shift()
2025-01-21 7:21 ` Qu Wenruo
@ 2025-01-21 16:10 ` David Sterba
2025-01-21 17:59 ` Matthew Wilcox
0 siblings, 1 reply; 11+ messages in thread
From: David Sterba @ 2025-01-21 16:10 UTC (permalink / raw)
To: Qu Wenruo
Cc: Matthew Wilcox (Oracle), Chris Mason, Josef Bacik, David Sterba,
linux-btrfs
On Tue, Jan 21, 2025 at 05:51:40PM +1030, Qu Wenruo wrote:
>
>
> 在 2025/1/21 16:10, Matthew Wilcox (Oracle) 写道:
> > It is meaningless to shift a byte count by folio_shift(). The folio index
> > is in units of PAGE_SIZE, not folio_size(). We can use folio_contains()
> > to make this work for arbitrary-order folios, so remove the assertion
> > that the folios are of order 0.
> >
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > ---
> > fs/btrfs/extent_io.c | 8 +++-----
> > 1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index 289ecb8ce217..c9b0ee841501 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -523,8 +523,6 @@ static void end_bbio_data_read(struct btrfs_bio *bbio)
> > u64 end;
> > u32 len;
> >
> > - /* For now only order 0 folios are supported for data. */
> > - ASSERT(folio_order(folio) == 0);
>
> I'd prefer to keep this ASSERT(), as all the btrfs_folio_*() helpers can
> only handle page sized folio for now.
>
> > btrfs_debug(fs_info,
> > "%s: bi_sector=%llu, err=%d, mirror=%u",
> > __func__, bio->bi_iter.bi_sector, bio->bi_status,
> > @@ -552,7 +550,6 @@ static void end_bbio_data_read(struct btrfs_bio *bbio)
> >
> > if (likely(uptodate)) {
> > loff_t i_size = i_size_read(inode);
> > - pgoff_t end_index = i_size >> folio_shift(folio);
> >
> > /*
> > * Zero out the remaining part if this range straddles
> > @@ -563,7 +560,8 @@ static void end_bbio_data_read(struct btrfs_bio *bbio)
> > *
> > * NOTE: i_size is exclusive while end is inclusive.
> > */
> > - if (folio_index(folio) == end_index && i_size <= end) {
> > + if (folio_contains(folio, i_size >> PAGE_SHIFT) &&
>
> Although folio_contains() is already a pretty good improvement, can we
> have a bytenr/i_sized based solution for fs usages?
Good point, something like folio_contains_offset() that does the correct
shifts.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] btrfs: Fix two misuses of folio_shift()
2025-01-21 16:10 ` David Sterba
@ 2025-01-21 17:59 ` Matthew Wilcox
2025-01-22 15:24 ` David Sterba
0 siblings, 1 reply; 11+ messages in thread
From: Matthew Wilcox @ 2025-01-21 17:59 UTC (permalink / raw)
To: David Sterba
Cc: Qu Wenruo, Chris Mason, Josef Bacik, David Sterba, linux-btrfs
On Tue, Jan 21, 2025 at 05:10:11PM +0100, David Sterba wrote:
> On Tue, Jan 21, 2025 at 05:51:40PM +1030, Qu Wenruo wrote:
> > 在 2025/1/21 16:10, Matthew Wilcox (Oracle) 写道:
> > > - /* For now only order 0 folios are supported for data. */
> > > - ASSERT(folio_order(folio) == 0);
> >
> > I'd prefer to keep this ASSERT(), as all the btrfs_folio_*() helpers can
> > only handle page sized folio for now.
I'd rather have the assertions in the places that actually don't work.
And the better assertion is !folio_test_large(folio) as it only has to
test one bit rather than testing a bit and then also testing a field.
> > > - if (folio_index(folio) == end_index && i_size <= end) {
> > > + if (folio_contains(folio, i_size >> PAGE_SHIFT) &&
> >
> > Although folio_contains() is already a pretty good improvement, can we
> > have a bytenr/i_sized based solution for fs usages?
>
> Good point, something like folio_contains_offset() that does the correct
> shifts.
I'd call it folio_contains_pos(), but I'm not sure there's a lot of
users. We've got a lot of filesystems converted now, and btrfs is the
first one to want something like that. I've had a look through iomap
and some other filesystems, and I can't see anywhere else that would use
folio_contains_pos().
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] btrfs: Fix two misuses of folio_shift()
2025-01-21 17:59 ` Matthew Wilcox
@ 2025-01-22 15:24 ` David Sterba
2025-01-24 6:11 ` Qu Wenruo
0 siblings, 1 reply; 11+ messages in thread
From: David Sterba @ 2025-01-22 15:24 UTC (permalink / raw)
To: Matthew Wilcox
Cc: David Sterba, Qu Wenruo, Chris Mason, Josef Bacik, David Sterba,
linux-btrfs
On Tue, Jan 21, 2025 at 05:59:53PM +0000, Matthew Wilcox wrote:
> > > > + if (folio_contains(folio, i_size >> PAGE_SHIFT) &&
> > >
> > > Although folio_contains() is already a pretty good improvement, can we
> > > have a bytenr/i_sized based solution for fs usages?
> >
> > Good point, something like folio_contains_offset() that does the correct
> > shifts.
>
> I'd call it folio_contains_pos(), but I'm not sure there's a lot of
> users. We've got a lot of filesystems converted now, and btrfs is the
> first one to want something like that. I've had a look through iomap
> and some other filesystems, and I can't see anywhere else that would use
> folio_contains_pos().
Ok then, we can live with that, but please keep it in mind for future
folio API updates. Thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] btrfs: Fix two misuses of folio_shift()
2025-01-22 15:24 ` David Sterba
@ 2025-01-24 6:11 ` Qu Wenruo
2025-01-24 21:49 ` David Sterba
0 siblings, 1 reply; 11+ messages in thread
From: Qu Wenruo @ 2025-01-24 6:11 UTC (permalink / raw)
To: dsterba, Matthew Wilcox
Cc: Qu Wenruo, Chris Mason, Josef Bacik, David Sterba, linux-btrfs
在 2025/1/23 01:54, David Sterba 写道:
> On Tue, Jan 21, 2025 at 05:59:53PM +0000, Matthew Wilcox wrote:
>>>>> + if (folio_contains(folio, i_size >> PAGE_SHIFT) &&
>>>>
>>>> Although folio_contains() is already a pretty good improvement, can we
>>>> have a bytenr/i_sized based solution for fs usages?
>>>
>>> Good point, something like folio_contains_offset() that does the correct
>>> shifts.
>>
>> I'd call it folio_contains_pos(), but I'm not sure there's a lot of
>> users. We've got a lot of filesystems converted now, and btrfs is the
>> first one to want something like that. I've had a look through iomap
>> and some other filesystems, and I can't see anywhere else that would use
>> folio_contains_pos().
>
> Ok then, we can live with that, but please keep it in mind for future
> folio API updates. Thanks.
>
Then the whole series looks good to me.
Reviewed-by: Qu Wenruo <wqu@suse.com>
And since the whole series is reviewed, should I merge this series into
for-next now?
Thanks,
Qu
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] btrfs: Fix two misuses of folio_shift()
2025-01-24 6:11 ` Qu Wenruo
@ 2025-01-24 21:49 ` David Sterba
0 siblings, 0 replies; 11+ messages in thread
From: David Sterba @ 2025-01-24 21:49 UTC (permalink / raw)
To: Qu Wenruo
Cc: Matthew Wilcox, Qu Wenruo, Chris Mason, Josef Bacik, David Sterba,
linux-btrfs
On Fri, Jan 24, 2025 at 04:41:46PM +1030, Qu Wenruo wrote:
>
>
> 在 2025/1/23 01:54, David Sterba 写道:
> > On Tue, Jan 21, 2025 at 05:59:53PM +0000, Matthew Wilcox wrote:
> >>>>> + if (folio_contains(folio, i_size >> PAGE_SHIFT) &&
> >>>>
> >>>> Although folio_contains() is already a pretty good improvement, can we
> >>>> have a bytenr/i_sized based solution for fs usages?
> >>>
> >>> Good point, something like folio_contains_offset() that does the correct
> >>> shifts.
> >>
> >> I'd call it folio_contains_pos(), but I'm not sure there's a lot of
> >> users. We've got a lot of filesystems converted now, and btrfs is the
> >> first one to want something like that. I've had a look through iomap
> >> and some other filesystems, and I can't see anywhere else that would use
> >> folio_contains_pos().
> >
> > Ok then, we can live with that, but please keep it in mind for future
> > folio API updates. Thanks.
> >
> Then the whole series looks good to me.
>
> Reviewed-by: Qu Wenruo <wqu@suse.com>
>
> And since the whole series is reviewed, should I merge this series into
> for-next now?
Yes please.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-01-24 21:49 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-21 5:40 [PATCH 1/3] btrfs: Fix some folio-related comments Matthew Wilcox (Oracle)
2025-01-21 5:40 ` [PATCH 2/3] btrfs: Fix two misuses of folio_shift() Matthew Wilcox (Oracle)
2025-01-21 7:21 ` Qu Wenruo
2025-01-21 16:10 ` David Sterba
2025-01-21 17:59 ` Matthew Wilcox
2025-01-22 15:24 ` David Sterba
2025-01-24 6:11 ` Qu Wenruo
2025-01-24 21:49 ` David Sterba
2025-01-21 5:40 ` [PATCH 3/3] btrfs: Convert io_ctl_prepare_pages() to work on folios Matthew Wilcox (Oracle)
2025-01-21 7:22 ` Qu Wenruo
2025-01-21 7:14 ` [PATCH 1/3] btrfs: Fix some folio-related comments Qu Wenruo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox