Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] btrfs: Do not check for PagePrivate twice
@ 2019-10-18 18:15 Goldwyn Rodrigues
  2019-10-18 18:15 ` [PATCH] btrfs: Do not check for writeback in btrfs_releasepage() Goldwyn Rodrigues
  2019-10-21  9:52 ` [PATCH] btrfs: Do not check for PagePrivate twice Filipe Manana
  0 siblings, 2 replies; 5+ messages in thread
From: Goldwyn Rodrigues @ 2019-10-18 18:15 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Goldwyn Rodrigues

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

We are checking PagePrivate twice, once with lock and once without.
Perform the check only once.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/extent_io.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index cceaf05aada2..425ba359178c 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3959,9 +3959,6 @@ int btree_write_cache_pages(struct address_space *mapping,
 		for (i = 0; i < nr_pages; i++) {
 			struct page *page = pvec.pages[i];
 
-			if (!PagePrivate(page))
-				continue;
-
 			spin_lock(&mapping->private_lock);
 			if (!PagePrivate(page)) {
 				spin_unlock(&mapping->private_lock);
-- 
2.16.4


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH] btrfs: Do not check for writeback in btrfs_releasepage()
  2019-10-18 18:15 [PATCH] btrfs: Do not check for PagePrivate twice Goldwyn Rodrigues
@ 2019-10-18 18:15 ` Goldwyn Rodrigues
  2019-10-18 23:35   ` Goldwyn Rodrigues
  2019-10-21  9:52 ` [PATCH] btrfs: Do not check for PagePrivate twice Filipe Manana
  1 sibling, 1 reply; 5+ messages in thread
From: Goldwyn Rodrigues @ 2019-10-18 18:15 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Goldwyn Rodrigues

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

We check for PageWriteback in try_to_release_page(), the
sole caller of address_space->releasepage(). We don't need
to check it again in btrfs_releasepage()

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 0f2754eaa05b..10303c2379a9 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8793,7 +8793,7 @@ static int __btrfs_releasepage(struct page *page, gfp_t gfp_flags)
 
 static int btrfs_releasepage(struct page *page, gfp_t gfp_flags)
 {
-	if (PageWriteback(page) || PageDirty(page))
+	if (PageDirty(page))
 		return 0;
 	return __btrfs_releasepage(page, gfp_flags);
 }
-- 
2.16.4


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] btrfs: Do not check for writeback in btrfs_releasepage()
  2019-10-18 18:15 ` [PATCH] btrfs: Do not check for writeback in btrfs_releasepage() Goldwyn Rodrigues
@ 2019-10-18 23:35   ` Goldwyn Rodrigues
  0 siblings, 0 replies; 5+ messages in thread
From: Goldwyn Rodrigues @ 2019-10-18 23:35 UTC (permalink / raw)
  To: linux-btrfs


Scratch this. btrfs_releasepage() is called from btrfs_invalidatepage()
as well. Sorry for the noise.


On 13:15 18/10, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> We check for PageWriteback in try_to_release_page(), the
> sole caller of address_space->releasepage(). We don't need
> to check it again in btrfs_releasepage()
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 0f2754eaa05b..10303c2379a9 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8793,7 +8793,7 @@ static int __btrfs_releasepage(struct page *page, gfp_t gfp_flags)
>  
>  static int btrfs_releasepage(struct page *page, gfp_t gfp_flags)
>  {
> -	if (PageWriteback(page) || PageDirty(page))
> +	if (PageDirty(page))
>  		return 0;
>  	return __btrfs_releasepage(page, gfp_flags);
>  }
> -- 
> 2.16.4
> 

-- 
Goldwyn

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] btrfs: Do not check for PagePrivate twice
  2019-10-18 18:15 [PATCH] btrfs: Do not check for PagePrivate twice Goldwyn Rodrigues
  2019-10-18 18:15 ` [PATCH] btrfs: Do not check for writeback in btrfs_releasepage() Goldwyn Rodrigues
@ 2019-10-21  9:52 ` Filipe Manana
  2019-11-28 11:44   ` David Sterba
  1 sibling, 1 reply; 5+ messages in thread
From: Filipe Manana @ 2019-10-21  9:52 UTC (permalink / raw)
  To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues

On Sat, Oct 19, 2019 at 10:05 AM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> We are checking PagePrivate twice, once with lock and once without.
> Perform the check only once.

Have you checked if there's some performance degradation after
removing the check?
My guess is it's there to avoid taking the lock, as the lock can be
heavily used on a system under heavy load (maybe even if it's not too
heavy, since we generate a lot of dirty metadata due to cow).
The page may have been released after locking the mapping, that's why
we check it twice, and after unlocking we are sure it can not be
released due to taking a reference on the extent buffer.

Thanks.

>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/extent_io.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index cceaf05aada2..425ba359178c 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3959,9 +3959,6 @@ int btree_write_cache_pages(struct address_space *mapping,
>                 for (i = 0; i < nr_pages; i++) {
>                         struct page *page = pvec.pages[i];
>
> -                       if (!PagePrivate(page))
> -                               continue;
> -
>                         spin_lock(&mapping->private_lock);
>                         if (!PagePrivate(page)) {
>                                 spin_unlock(&mapping->private_lock);
> --
> 2.16.4
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] btrfs: Do not check for PagePrivate twice
  2019-10-21  9:52 ` [PATCH] btrfs: Do not check for PagePrivate twice Filipe Manana
@ 2019-11-28 11:44   ` David Sterba
  0 siblings, 0 replies; 5+ messages in thread
From: David Sterba @ 2019-11-28 11:44 UTC (permalink / raw)
  To: Filipe Manana; +Cc: Goldwyn Rodrigues, linux-btrfs, Goldwyn Rodrigues

On Mon, Oct 21, 2019 at 10:52:06AM +0100, Filipe Manana wrote:
> On Sat, Oct 19, 2019 at 10:05 AM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> > We are checking PagePrivate twice, once with lock and once without.
> > Perform the check only once.
> 
> Have you checked if there's some performance degradation after
> removing the check?
> My guess is it's there to avoid taking the lock, as the lock can be
> heavily used on a system under heavy load (maybe even if it's not too
> heavy, since we generate a lot of dirty metadata due to cow).
> The page may have been released after locking the mapping, that's why
> we check it twice, and after unlocking we are sure it can not be
> released due to taking a reference on the extent buffer.

That's my understanding as well, so the duplicate unlocked check should
stay.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-18 18:15 [PATCH] btrfs: Do not check for PagePrivate twice Goldwyn Rodrigues
2019-10-18 18:15 ` [PATCH] btrfs: Do not check for writeback in btrfs_releasepage() Goldwyn Rodrigues
2019-10-18 23:35   ` Goldwyn Rodrigues
2019-10-21  9:52 ` [PATCH] btrfs: Do not check for PagePrivate twice Filipe Manana
2019-11-28 11:44   ` David Sterba

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org
	public-inbox-index linux-btrfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git