public inbox for linux-btrfs@vger.kernel.org
 help / color / mirror / Atom feed
* [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