| Message ID | 1536703587-94565-1-git-send-email-bo.liu@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Btrfs: skip set_page_dirty if eb is dirty | expand |
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); >
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); > >
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>
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>
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);
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(-)