diff mbox

btrfs: use only memmove_extent_buffer and simplify the helpers

Message ID 1367242681-8835-1-git-send-email-dsterba@suse.cz (mailing list archive)
State New, archived
Headers show

Commit Message

David Sterba April 29, 2013, 1:38 p.m. UTC
After commit a65917156e34594 ("Btrfs: stop using highmem for
extent_buffers") we don't need to call kmap_atomic anymore and can
reduce the move_pages helper to a simple memmove.

There's only one caller of memcpy_extent_buffer, we can use the
memmove_ variant here.

Signed-off-by: David Sterba <dsterba@suse.cz>
---
 fs/btrfs/ctree.c     |    2 +-
 fs/btrfs/extent_io.c |   88 +------------------------------------------------
 2 files changed, 3 insertions(+), 87 deletions(-)

Comments

Josef Bacik May 3, 2013, 8:33 p.m. UTC | #1
On Mon, Apr 29, 2013 at 07:38:01AM -0600, David Sterba wrote:
> After commit a65917156e34594 ("Btrfs: stop using highmem for
> extent_buffers") we don't need to call kmap_atomic anymore and can
> reduce the move_pages helper to a simple memmove.
> 
> There's only one caller of memcpy_extent_buffer, we can use the
> memmove_ variant here.
> 

This makes -l 64k blow the hell up, just try generic/001.  I'm kicking this
patch out.  Thanks,

Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Mason May 3, 2013, 8:37 p.m. UTC | #2
Quoting Josef Bacik (2013-05-03 16:33:44)
> On Mon, Apr 29, 2013 at 07:38:01AM -0600, David Sterba wrote:
> > After commit a65917156e34594 ("Btrfs: stop using highmem for
> > extent_buffers") we don't need to call kmap_atomic anymore and can
> > reduce the move_pages helper to a simple memmove.
> > 
> > There's only one caller of memcpy_extent_buffer, we can use the
> > memmove_ variant here.
> > 
> 
> This makes -l 64k blow the hell up, just try generic/001.  I'm kicking this
> patch out.  Thanks,

Sorry Dave, I only now remember having this same problem the last time I
tried to get rid of memcpy.

-chris

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba May 7, 2013, 12:13 p.m. UTC | #3
On Fri, May 03, 2013 at 04:37:41PM -0400, Chris Mason wrote:
> Quoting Josef Bacik (2013-05-03 16:33:44)
> > On Mon, Apr 29, 2013 at 07:38:01AM -0600, David Sterba wrote:
> > > After commit a65917156e34594 ("Btrfs: stop using highmem for
> > > extent_buffers") we don't need to call kmap_atomic anymore and can
> > > reduce the move_pages helper to a simple memmove.
> > > 
> > > There's only one caller of memcpy_extent_buffer, we can use the
> > > memmove_ variant here.
> > > 
> > 
> > This makes -l 64k blow the hell up, just try generic/001.  I'm kicking this
> > patch out.  Thanks,
> 
> Sorry Dave, I only now remember having this same problem the last time I
> tried to get rid of memcpy.

The patch evolved from a minor cleanup inside copy_page into a
functional change that looked ok to me after checking that memmove
behaves like memcpy if it's safe (both generic and arch-specific
implementations). Well, apparently not, but I still see some space for
micro-optimizations around copy_pages/move_pages.

david
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 566d99b..154573e 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -4270,7 +4270,7 @@  int btrfs_duplicate_item(struct btrfs_trans_handle *trans,
 			       item_size, item_size +
 			       sizeof(struct btrfs_item), 1);
 	leaf = path->nodes[0];
-	memcpy_extent_buffer(leaf,
+	memmove_extent_buffer(leaf,
 			     btrfs_item_ptr_offset(leaf, path->slots[0]),
 			     btrfs_item_ptr_offset(leaf, path->slots[0] - 1),
 			     item_size);
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index dbb2bde..e1abebd 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4943,89 +4943,9 @@  static void move_pages(struct page *dst_page, struct page *src_page,
 		       unsigned long len)
 {
 	char *dst_kaddr = page_address(dst_page);
-	if (dst_page == src_page) {
-		memmove(dst_kaddr + dst_off, dst_kaddr + src_off, len);
-	} else {
-		char *src_kaddr = page_address(src_page);
-		char *p = dst_kaddr + dst_off + len;
-		char *s = src_kaddr + src_off + len;
+	char *src_kaddr = page_address(src_page);
 
-		while (len--)
-			*--p = *--s;
-	}
-}
-
-static inline bool areas_overlap(unsigned long src, unsigned long dst, unsigned long len)
-{
-	unsigned long distance = (src > dst) ? src - dst : dst - src;
-	return distance < len;
-}
-
-static void copy_pages(struct page *dst_page, struct page *src_page,
-		       unsigned long dst_off, unsigned long src_off,
-		       unsigned long len)
-{
-	char *dst_kaddr = page_address(dst_page);
-	char *src_kaddr;
-	int must_memmove = 0;
-
-	if (dst_page != src_page) {
-		src_kaddr = page_address(src_page);
-	} else {
-		src_kaddr = dst_kaddr;
-		if (areas_overlap(src_off, dst_off, len))
-			must_memmove = 1;
-	}
-
-	if (must_memmove)
-		memmove(dst_kaddr + dst_off, src_kaddr + src_off, len);
-	else
-		memcpy(dst_kaddr + dst_off, src_kaddr + src_off, len);
-}
-
-void memcpy_extent_buffer(struct extent_buffer *dst, unsigned long dst_offset,
-			   unsigned long src_offset, unsigned long len)
-{
-	size_t cur;
-	size_t dst_off_in_page;
-	size_t src_off_in_page;
-	size_t start_offset = dst->start & ((u64)PAGE_CACHE_SIZE - 1);
-	unsigned long dst_i;
-	unsigned long src_i;
-
-	if (src_offset + len > dst->len) {
-		printk(KERN_ERR "btrfs memmove bogus src_offset %lu move "
-		       "len %lu dst len %lu\n", src_offset, len, dst->len);
-		BUG_ON(1);
-	}
-	if (dst_offset + len > dst->len) {
-		printk(KERN_ERR "btrfs memmove bogus dst_offset %lu move "
-		       "len %lu dst len %lu\n", dst_offset, len, dst->len);
-		BUG_ON(1);
-	}
-
-	while (len > 0) {
-		dst_off_in_page = (start_offset + dst_offset) &
-			((unsigned long)PAGE_CACHE_SIZE - 1);
-		src_off_in_page = (start_offset + src_offset) &
-			((unsigned long)PAGE_CACHE_SIZE - 1);
-
-		dst_i = (start_offset + dst_offset) >> PAGE_CACHE_SHIFT;
-		src_i = (start_offset + src_offset) >> PAGE_CACHE_SHIFT;
-
-		cur = min(len, (unsigned long)(PAGE_CACHE_SIZE -
-					       src_off_in_page));
-		cur = min_t(unsigned long, cur,
-			(unsigned long)(PAGE_CACHE_SIZE - dst_off_in_page));
-
-		copy_pages(extent_buffer_page(dst, dst_i),
-			   extent_buffer_page(dst, src_i),
-			   dst_off_in_page, src_off_in_page, cur);
-
-		src_offset += cur;
-		dst_offset += cur;
-		len -= cur;
-	}
+	memmove(dst_kaddr + dst_off, src_kaddr + src_off, len);
 }
 
 void memmove_extent_buffer(struct extent_buffer *dst, unsigned long dst_offset,
@@ -5050,10 +4970,6 @@  void memmove_extent_buffer(struct extent_buffer *dst, unsigned long dst_offset,
 		       "len %lu len %lu\n", dst_offset, len, dst->len);
 		BUG_ON(1);
 	}
-	if (dst_offset < src_offset) {
-		memcpy_extent_buffer(dst, dst_offset, src_offset, len);
-		return;
-	}
 	while (len > 0) {
 		dst_i = (start_offset + dst_end) >> PAGE_CACHE_SHIFT;
 		src_i = (start_offset + src_end) >> PAGE_CACHE_SHIFT;