All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Btrfs: skip set_page_dirty if eb is dirty
@ 2018-09-11 22:06 Liu Bo
  2018-09-12  6:37 ` Nikolay Borisov
  2018-09-13 17:44 ` [PATCH v2] " Liu Bo
  0 siblings, 2 replies; 8+ messages in thread
From: Liu Bo @ 2018-09-11 22:06 UTC (permalink / raw)
  To: linux-btrfs

As long as @eb is marked with EXTENT_BUFFER_DIRTY, all of its pages
are dirty, so no need to set pages dirty again.

Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
 fs/btrfs/extent_io.c | 11 +++++++----
 fs/btrfs/extent_io.h |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 628f1aef34b0..fb2bf50134a1 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5165,11 +5165,11 @@ void clear_extent_buffer_dirty(struct extent_buffer *eb)
 	WARN_ON(atomic_read(&eb->refs) == 0);
 }
 
-int set_extent_buffer_dirty(struct extent_buffer *eb)
+bool set_extent_buffer_dirty(struct extent_buffer *eb)
 {
 	int i;
 	int num_pages;
-	int was_dirty = 0;
+	bool was_dirty;
 
 	check_buffer_tree_ref(eb);
 
@@ -5179,8 +5179,11 @@ int set_extent_buffer_dirty(struct extent_buffer *eb)
 	WARN_ON(atomic_read(&eb->refs) == 0);
 	WARN_ON(!test_bit(EXTENT_BUFFER_TREE_REF, &eb->bflags));
 
-	for (i = 0; i < num_pages; i++)
-		set_page_dirty(eb->pages[i]);
+	if (!was_dirty) {
+		for (i = 0; i < num_pages; i++)
+			set_page_dirty(eb->pages[i]);
+	}
+
 	return was_dirty;
 }
 
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index b4d03e677e1d..f2ab42d57f02 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -479,7 +479,7 @@ void extent_buffer_bitmap_set(struct extent_buffer *eb, unsigned long start,
 void extent_buffer_bitmap_clear(struct extent_buffer *eb, unsigned long start,
 				unsigned long pos, unsigned long len);
 void clear_extent_buffer_dirty(struct extent_buffer *eb);
-int set_extent_buffer_dirty(struct extent_buffer *eb);
+bool set_extent_buffer_dirty(struct extent_buffer *eb);
 void set_extent_buffer_uptodate(struct extent_buffer *eb);
 void clear_extent_buffer_uptodate(struct extent_buffer *eb);
 int extent_buffer_under_io(struct extent_buffer *eb);
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH] Btrfs: skip set_page_dirty if eb is dirty
  2018-09-11 22:06 [PATCH] Btrfs: skip set_page_dirty if eb is dirty Liu Bo
@ 2018-09-12  6:37 ` Nikolay Borisov
  2018-09-12 19:27   ` Liu Bo
  2018-09-13 17:44 ` [PATCH v2] " Liu Bo
  1 sibling, 1 reply; 8+ messages in thread
From: Nikolay Borisov @ 2018-09-12  6:37 UTC (permalink / raw)
  To: Liu Bo, linux-btrfs



On 12.09.2018 01:06, Liu Bo wrote:
> As long as @eb is marked with EXTENT_BUFFER_DIRTY, all of its pages
> are dirty, so no need to set pages dirty again.
> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>

Does make it any performance difference, numbers?

Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/extent_io.c | 11 +++++++----
>  fs/btrfs/extent_io.h |  2 +-
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 628f1aef34b0..fb2bf50134a1 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5165,11 +5165,11 @@ void clear_extent_buffer_dirty(struct extent_buffer *eb)
>  	WARN_ON(atomic_read(&eb->refs) == 0);
>  }
>  
> -int set_extent_buffer_dirty(struct extent_buffer *eb)
> +bool set_extent_buffer_dirty(struct extent_buffer *eb)
>  {
>  	int i;
>  	int num_pages;
> -	int was_dirty = 0;
> +	bool was_dirty;
>  
>  	check_buffer_tree_ref(eb);
>  
> @@ -5179,8 +5179,11 @@ int set_extent_buffer_dirty(struct extent_buffer *eb)
>  	WARN_ON(atomic_read(&eb->refs) == 0);
>  	WARN_ON(!test_bit(EXTENT_BUFFER_TREE_REF, &eb->bflags));
>  
> -	for (i = 0; i < num_pages; i++)
> -		set_page_dirty(eb->pages[i]);
> +	if (!was_dirty) {
> +		for (i = 0; i < num_pages; i++)
> +			set_page_dirty(eb->pages[i]);
> +	}
> +
>  	return was_dirty;
>  }
>  
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index b4d03e677e1d..f2ab42d57f02 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -479,7 +479,7 @@ void extent_buffer_bitmap_set(struct extent_buffer *eb, unsigned long start,
>  void extent_buffer_bitmap_clear(struct extent_buffer *eb, unsigned long start,
>  				unsigned long pos, unsigned long len);
>  void clear_extent_buffer_dirty(struct extent_buffer *eb);
> -int set_extent_buffer_dirty(struct extent_buffer *eb);
> +bool set_extent_buffer_dirty(struct extent_buffer *eb);
>  void set_extent_buffer_uptodate(struct extent_buffer *eb);
>  void clear_extent_buffer_uptodate(struct extent_buffer *eb);
>  int extent_buffer_under_io(struct extent_buffer *eb);
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] Btrfs: skip set_page_dirty if eb is dirty
  2018-09-12  6:37 ` Nikolay Borisov
@ 2018-09-12 19:27   ` Liu Bo
  2018-09-13 11:29     ` David Sterba
  0 siblings, 1 reply; 8+ messages in thread
From: Liu Bo @ 2018-09-12 19:27 UTC (permalink / raw)
  To: Nikolay Borisov; +Cc: linux-btrfs

On Wed, Sep 12, 2018 at 09:37:20AM +0300, Nikolay Borisov wrote:
> 
> 
> On 12.09.2018 01:06, Liu Bo wrote:
> > As long as @eb is marked with EXTENT_BUFFER_DIRTY, all of its pages
> > are dirty, so no need to set pages dirty again.
> > 
> > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> 
> Does make it any performance difference, numbers?
>

To be honest, the performance difference would be trivial in a normal
big test round.  But I just looked into the difference from my ftrace,
removing the loop can reduce the time spent by 10us in my box.

> Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Thanks a lot for reviewing.

thanks,
-liubo
> > ---
> >  fs/btrfs/extent_io.c | 11 +++++++----
> >  fs/btrfs/extent_io.h |  2 +-
> >  2 files changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index 628f1aef34b0..fb2bf50134a1 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -5165,11 +5165,11 @@ void clear_extent_buffer_dirty(struct extent_buffer *eb)
> >  	WARN_ON(atomic_read(&eb->refs) == 0);
> >  }
> >  
> > -int set_extent_buffer_dirty(struct extent_buffer *eb)
> > +bool set_extent_buffer_dirty(struct extent_buffer *eb)
> >  {
> >  	int i;
> >  	int num_pages;
> > -	int was_dirty = 0;
> > +	bool was_dirty;
> >  
> >  	check_buffer_tree_ref(eb);
> >  
> > @@ -5179,8 +5179,11 @@ int set_extent_buffer_dirty(struct extent_buffer *eb)
> >  	WARN_ON(atomic_read(&eb->refs) == 0);
> >  	WARN_ON(!test_bit(EXTENT_BUFFER_TREE_REF, &eb->bflags));
> >  
> > -	for (i = 0; i < num_pages; i++)
> > -		set_page_dirty(eb->pages[i]);
> > +	if (!was_dirty) {
> > +		for (i = 0; i < num_pages; i++)
> > +			set_page_dirty(eb->pages[i]);
> > +	}
> > +
> >  	return was_dirty;
> >  }
> >  
> > diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> > index b4d03e677e1d..f2ab42d57f02 100644
> > --- a/fs/btrfs/extent_io.h
> > +++ b/fs/btrfs/extent_io.h
> > @@ -479,7 +479,7 @@ void extent_buffer_bitmap_set(struct extent_buffer *eb, unsigned long start,
> >  void extent_buffer_bitmap_clear(struct extent_buffer *eb, unsigned long start,
> >  				unsigned long pos, unsigned long len);
> >  void clear_extent_buffer_dirty(struct extent_buffer *eb);
> > -int set_extent_buffer_dirty(struct extent_buffer *eb);
> > +bool set_extent_buffer_dirty(struct extent_buffer *eb);
> >  void set_extent_buffer_uptodate(struct extent_buffer *eb);
> >  void clear_extent_buffer_uptodate(struct extent_buffer *eb);
> >  int extent_buffer_under_io(struct extent_buffer *eb);
> > 

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] Btrfs: skip set_page_dirty if eb is dirty
  2018-09-12 19:27   ` Liu Bo
@ 2018-09-13 11:29     ` David Sterba
  2018-09-13 16:54       ` Liu Bo
  0 siblings, 1 reply; 8+ messages in thread
From: David Sterba @ 2018-09-13 11:29 UTC (permalink / raw)
  To: Liu Bo; +Cc: Nikolay Borisov, linux-btrfs

On Wed, Sep 12, 2018 at 12:27:46PM -0700, Liu Bo wrote:
> On Wed, Sep 12, 2018 at 09:37:20AM +0300, Nikolay Borisov wrote:
> > 
> > 
> > On 12.09.2018 01:06, Liu Bo wrote:
> > > As long as @eb is marked with EXTENT_BUFFER_DIRTY, all of its pages
> > > are dirty, so no need to set pages dirty again.
> > > 
> > > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> > 
> > Does make it any performance difference, numbers?
> >
> 
> To be honest, the performance difference would be trivial in a normal
> big test round.  But I just looked into the difference from my ftrace,
> removing the loop can reduce the time spent by 10us in my box.

10us was for the case where the pages were dirty already and the for
cycle was then skipped?

set_page_dirty is not lightweight, calls down to various functions and
holds locks. I can't tell if this is still fast enough on your machine
so that it really takes 10us.

The conditional dirtying is definetelly worth though,

Reviewed-by: David Sterba <dsterba@suse.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] Btrfs: skip set_page_dirty if eb is dirty
  2018-09-13 11:29     ` David Sterba
@ 2018-09-13 16:54       ` Liu Bo
  0 siblings, 0 replies; 8+ messages in thread
From: Liu Bo @ 2018-09-13 16:54 UTC (permalink / raw)
  To: dsterba, Nikolay Borisov, linux-btrfs

On Thu, Sep 13, 2018 at 01:29:59PM +0200, David Sterba wrote:
> On Wed, Sep 12, 2018 at 12:27:46PM -0700, Liu Bo wrote:
> > On Wed, Sep 12, 2018 at 09:37:20AM +0300, Nikolay Borisov wrote:
> > > 
> > > 
> > > On 12.09.2018 01:06, Liu Bo wrote:
> > > > As long as @eb is marked with EXTENT_BUFFER_DIRTY, all of its pages
> > > > are dirty, so no need to set pages dirty again.
> > > > 
> > > > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> > > 
> > > Does make it any performance difference, numbers?
> > >
> > 
> > To be honest, the performance difference would be trivial in a normal
> > big test round.  But I just looked into the difference from my ftrace,
> > removing the loop can reduce the time spent by 10us in my box.
> 
> 10us was for the case where the pages were dirty already and the for
> cycle was then skipped?
> 
> set_page_dirty is not lightweight, calls down to various functions and
> holds locks. I can't tell if this is still fast enough on your machine
> so that it really takes 10us.
>

Sorry for not making it clear, on my box 10us was spent in the 'for'
loop, which iterates 4 pages and 4 calls for set_page_dirty().

thanks,
-liubo

> The conditional dirtying is definetelly worth though,
> 
> Reviewed-by: David Sterba <dsterba@suse.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH v2] Btrfs: skip set_page_dirty if eb is dirty
  2018-09-11 22:06 [PATCH] Btrfs: skip set_page_dirty if eb is dirty Liu Bo
  2018-09-12  6:37 ` Nikolay Borisov
@ 2018-09-13 17:44 ` Liu Bo
  2018-09-14  6:30   ` Nikolay Borisov
  2018-09-14 12:59   ` David Sterba
  1 sibling, 2 replies; 8+ messages in thread
From: Liu Bo @ 2018-09-13 17:44 UTC (permalink / raw)
  To: linux-btrfs

As long as @eb is marked with EXTENT_BUFFER_DIRTY, all of its pages
are dirty, so no need to set pages dirty again.

Ftrace showed that the loop took 10us on my dev box, so removing this
can save us at least 10us if eb is already dirty.

Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
v2: Update the patch's description.

 fs/btrfs/extent_io.c | 11 +++++++----
 fs/btrfs/extent_io.h |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 628f1aef34b0..fb2bf50134a1 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5165,11 +5165,11 @@ void clear_extent_buffer_dirty(struct extent_buffer *eb)
 	WARN_ON(atomic_read(&eb->refs) == 0);
 }
 
-int set_extent_buffer_dirty(struct extent_buffer *eb)
+bool set_extent_buffer_dirty(struct extent_buffer *eb)
 {
 	int i;
 	int num_pages;
-	int was_dirty = 0;
+	bool was_dirty;
 
 	check_buffer_tree_ref(eb);
 
@@ -5179,8 +5179,11 @@ int set_extent_buffer_dirty(struct extent_buffer *eb)
 	WARN_ON(atomic_read(&eb->refs) == 0);
 	WARN_ON(!test_bit(EXTENT_BUFFER_TREE_REF, &eb->bflags));
 
-	for (i = 0; i < num_pages; i++)
-		set_page_dirty(eb->pages[i]);
+	if (!was_dirty) {
+		for (i = 0; i < num_pages; i++)
+			set_page_dirty(eb->pages[i]);
+	}
+
 	return was_dirty;
 }
 
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index b4d03e677e1d..f2ab42d57f02 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -479,7 +479,7 @@ void extent_buffer_bitmap_set(struct extent_buffer *eb, unsigned long start,
 void extent_buffer_bitmap_clear(struct extent_buffer *eb, unsigned long start,
 				unsigned long pos, unsigned long len);
 void clear_extent_buffer_dirty(struct extent_buffer *eb);
-int set_extent_buffer_dirty(struct extent_buffer *eb);
+bool set_extent_buffer_dirty(struct extent_buffer *eb);
 void set_extent_buffer_uptodate(struct extent_buffer *eb);
 void clear_extent_buffer_uptodate(struct extent_buffer *eb);
 int extent_buffer_under_io(struct extent_buffer *eb);
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH v2] Btrfs: skip set_page_dirty if eb is dirty
  2018-09-13 17:44 ` [PATCH v2] " Liu Bo
@ 2018-09-14  6:30   ` Nikolay Borisov
  2018-09-14 12:59   ` David Sterba
  1 sibling, 0 replies; 8+ messages in thread
From: Nikolay Borisov @ 2018-09-14  6:30 UTC (permalink / raw)
  To: Liu Bo, linux-btrfs



On 13.09.2018 20:44, Liu Bo wrote:
> As long as @eb is marked with EXTENT_BUFFER_DIRTY, all of its pages
> are dirty, so no need to set pages dirty again.
> 
> Ftrace showed that the loop took 10us on my dev box, so removing this
> can save us at least 10us if eb is already dirty.
> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
> v2: Update the patch's description.
> 
>  fs/btrfs/extent_io.c | 11 +++++++----
>  fs/btrfs/extent_io.h |  2 +-
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 628f1aef34b0..fb2bf50134a1 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5165,11 +5165,11 @@ void clear_extent_buffer_dirty(struct extent_buffer *eb)
>  	WARN_ON(atomic_read(&eb->refs) == 0);
>  }
>  
> -int set_extent_buffer_dirty(struct extent_buffer *eb)
> +bool set_extent_buffer_dirty(struct extent_buffer *eb)
>  {
>  	int i;
>  	int num_pages;
> -	int was_dirty = 0;
> +	bool was_dirty;
>  
>  	check_buffer_tree_ref(eb);
>  
> @@ -5179,8 +5179,11 @@ int set_extent_buffer_dirty(struct extent_buffer *eb)
>  	WARN_ON(atomic_read(&eb->refs) == 0);
>  	WARN_ON(!test_bit(EXTENT_BUFFER_TREE_REF, &eb->bflags));
>  
> -	for (i = 0; i < num_pages; i++)
> -		set_page_dirty(eb->pages[i]);
> +	if (!was_dirty) {
> +		for (i = 0; i < num_pages; i++)
> +			set_page_dirty(eb->pages[i]);
> +	}
> +
>  	return was_dirty;
>  }
>  
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index b4d03e677e1d..f2ab42d57f02 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -479,7 +479,7 @@ void extent_buffer_bitmap_set(struct extent_buffer *eb, unsigned long start,
>  void extent_buffer_bitmap_clear(struct extent_buffer *eb, unsigned long start,
>  				unsigned long pos, unsigned long len);
>  void clear_extent_buffer_dirty(struct extent_buffer *eb);
> -int set_extent_buffer_dirty(struct extent_buffer *eb);
> +bool set_extent_buffer_dirty(struct extent_buffer *eb);
>  void set_extent_buffer_uptodate(struct extent_buffer *eb);
>  void clear_extent_buffer_uptodate(struct extent_buffer *eb);
>  int extent_buffer_under_io(struct extent_buffer *eb);
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v2] Btrfs: skip set_page_dirty if eb is dirty
  2018-09-13 17:44 ` [PATCH v2] " Liu Bo
  2018-09-14  6:30   ` Nikolay Borisov
@ 2018-09-14 12:59   ` David Sterba
  1 sibling, 0 replies; 8+ messages in thread
From: David Sterba @ 2018-09-14 12:59 UTC (permalink / raw)
  To: Liu Bo; +Cc: linux-btrfs

On Fri, Sep 14, 2018 at 01:44:42AM +0800, Liu Bo wrote:
> As long as @eb is marked with EXTENT_BUFFER_DIRTY, all of its pages
> are dirty, so no need to set pages dirty again.
> 
> Ftrace showed that the loop took 10us on my dev box, so removing this
> can save us at least 10us if eb is already dirty.
> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>

Reviewed-by: David Sterba <dsterba@suse.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-09-14 18:14 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-11 22:06 [PATCH] Btrfs: skip set_page_dirty if eb is dirty Liu Bo
2018-09-12  6:37 ` Nikolay Borisov
2018-09-12 19:27   ` Liu Bo
2018-09-13 11:29     ` David Sterba
2018-09-13 16:54       ` Liu Bo
2018-09-13 17:44 ` [PATCH v2] " Liu Bo
2018-09-14  6:30   ` Nikolay Borisov
2018-09-14 12:59   ` David Sterba

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.