Minor cleanups and code simplifications

GIT PULL In review David Sterba 5 participants 20 messages Dec 3, 2015 – Dec 7, 2015 Btrfs file system

Share & tools

David Sterba

author maintainer

2015-12-03 16:58 UTC #0 lore raw

A random set of changes that popped up during code reading. Separated for easier review. For 4.5, please pull.

----------------------------------------------------------------
The following changes since commit 1ec218373b8ebda821aec00bb156a9c94fad9cd4:

Linux 4.4-rc2 (2015-11-22 16:45:59 -0800)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git cleanup/misc-simplify

for you to fetch changes up to bb45445ab63fcd83950180c8d17833ca0216da64:

btrfs: make set_range_writeback return void (2015-12-03 15:02:24 +0100)

----------------------------------------------------------------
David Sterba (12):
btrfs: make btrfs_close_one_device static
btrfs: sink parameter wait to btrfs_alloc_delalloc_work
btrfs: remove wait from struct btrfs_delalloc_work
btrfs: change how delay_iput is tracked in btrfs_delalloc_work
btrfs: remove a trivial helper btrfs_set_buffer_uptodate
btrfs: make set_extent_buffer_uptodate return void
btrfs: make clear_extent_buffer_uptodate return void
btrfs: make extent_clear_unlock_delalloc return void
btrfs: make end_extent_writepage return void
btrfs: make extent_range_clear_dirty_for_io return void
btrfs: make extent_range_redirty_for_io return void
btrfs: make set_range_writeback return void

 fs/btrfs/ctree.h       |  8 +++++---
 fs/btrfs/disk-io.c     |  5 -----
 fs/btrfs/disk-io.h     |  1 -
 fs/btrfs/extent-tree.c |  2 +-
 fs/btrfs/extent_io.c   | 27 +++++++++------------------
 fs/btrfs/extent_io.h   | 12 ++++++------
 fs/btrfs/inode.c       | 26 ++++++++++++++------------
 fs/btrfs/volumes.c     |  7 ++++---
 fs/btrfs/volumes.h     |  1 -
 9 files changed, 39 insertions(+), 50 deletions(-)

David Sterba

author maintainer

2015-12-03 16:58 UTC #1 lore raw

PATCH 1/12 btrfs: make btrfs_close_one_device static

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/volumes.c | 3 ++-
 fs/btrfs/volumes.h | 1 -
 2 files changed, 2 insertions(+), 2 deletions(-)

fs/btrfs/volumes.c +2 -1

index a6df8fdc1312..e335938825e1 100644
@@ -125,6 +125,7 @@ static int btrfs_relocate_sys_chunks(struct btrfs_root *root);
 static void __btrfs_reset_dev_stats(struct btrfs_device *dev);
 static void btrfs_dev_stat_print_on_error(struct btrfs_device *dev);
 static void btrfs_dev_stat_print_on_load(struct btrfs_device *device);
+static void btrfs_close_one_device(struct btrfs_device *device);
 
 DEFINE_MUTEX(uuid_mutex);
 static LIST_HEAD(fs_uuids);

@@ -6951,7 +6952,7 @@ void btrfs_reset_fs_info_ptr(struct btrfs_fs_info *fs_info)
 	}
 }
 
-void btrfs_close_one_device(struct btrfs_device *device)
+static void btrfs_close_one_device(struct btrfs_device *device)
 {
 	struct btrfs_fs_devices *fs_devices = device->fs_devices;
 	struct btrfs_device *new_device;

fs/btrfs/volumes.h +0 -1

index ec5712372732..2dc59eba188d 100644
@@ -566,6 +566,5 @@ static inline void unlock_chunks(struct btrfs_root *root)
 struct list_head *btrfs_get_fs_uuids(void);
 void btrfs_set_fs_info_ptr(struct btrfs_fs_info *fs_info);
 void btrfs_reset_fs_info_ptr(struct btrfs_fs_info *fs_info);
-void btrfs_close_one_device(struct btrfs_device *device);
 
 #endif

Signature · 1 line

2.6.2

David Sterba

author maintainer

2015-12-03 16:58 UTC #2 lore raw

PATCH 2/12 btrfs: sink parameter wait to btrfs_alloc_delalloc_work

In reply to David Sterba

There's only one caller and single value, we can propagate it down to the callee and remove the unused parameter.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/ctree.h | 2 +-
 fs/btrfs/inode.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

fs/btrfs/ctree.h +1 -1

index 8c58191249cc..cb540251f666 100644
@@ -3910,7 +3910,7 @@ struct btrfs_delalloc_work {
 };
 
 struct btrfs_delalloc_work *btrfs_alloc_delalloc_work(struct inode *inode,
-						    int wait, int delay_iput);
+						    int delay_iput);
 void btrfs_wait_and_free_delalloc_work(struct btrfs_delalloc_work *work);
 
 struct extent_map *btrfs_get_extent_fiemap(struct inode *inode, struct page *page,

fs/btrfs/inode.c +3 -3

index 994490d5fa64..9898b0d79c5a 100644
@@ -9457,7 +9457,7 @@ static void btrfs_run_delalloc_work(struct btrfs_work *work)
 }
 
 struct btrfs_delalloc_work *btrfs_alloc_delalloc_work(struct inode *inode,
-						    int wait, int delay_iput)
+						    int delay_iput)
 {
 	struct btrfs_delalloc_work *work;

@@ -9468,7 +9468,7 @@ struct btrfs_delalloc_work *btrfs_alloc_delalloc_work(struct inode *inode,
 	init_completion(&work->completion);
 	INIT_LIST_HEAD(&work->list);
 	work->inode = inode;
-	work->wait = wait;
+	work->wait = 0;
 	work->delay_iput = delay_iput;
 	WARN_ON_ONCE(!inode);
 	btrfs_init_work(&work->work, btrfs_flush_delalloc_helper,

@@ -9516,7 +9516,7 @@ static int __start_delalloc_inodes(struct btrfs_root *root, int delay_iput,
 		}
 		spin_unlock(&root->delalloc_lock);
 
-		work = btrfs_alloc_delalloc_work(inode, 0, delay_iput);
+		work = btrfs_alloc_delalloc_work(inode, delay_iput);
 		if (!work) {
 			if (delay_iput)
 				btrfs_add_delayed_iput(inode);

Signature · 1 line

2.6.2

David Sterba

author maintainer

2015-12-03 16:58 UTC #3 lore raw

PATCH 3/12 btrfs: remove wait from struct btrfs_delalloc_work

In reply to David Sterba

The value is 0 and never changes, we can propagate the value, remove wait and the implied dead code.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/ctree.h |  1 -
 fs/btrfs/inode.c | 11 +++--------
 2 files changed, 3 insertions(+), 9 deletions(-)

fs/btrfs/ctree.h +0 -1

index cb540251f666..a61c53bce162 100644
@@ -3902,7 +3902,6 @@ void btrfs_extent_item_to_extent_map(struct inode *inode,
 /* inode.c */
 struct btrfs_delalloc_work {
 	struct inode *inode;
-	int wait;
 	int delay_iput;
 	struct completion completion;
 	struct list_head list;

fs/btrfs/inode.c +3 -8

index 9898b0d79c5a..15b29e879ffc 100644
@@ -9440,14 +9440,10 @@ static void btrfs_run_delalloc_work(struct btrfs_work *work)
 	delalloc_work = container_of(work, struct btrfs_delalloc_work,
 				     work);
 	inode = delalloc_work->inode;
-	if (delalloc_work->wait) {
-		btrfs_wait_ordered_range(inode, 0, (u64)-1);
-	} else {
+	filemap_flush(inode->i_mapping);
+	if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
+				&BTRFS_I(inode)->runtime_flags))
 		filemap_flush(inode->i_mapping);
-		if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
-			     &BTRFS_I(inode)->runtime_flags))
-			filemap_flush(inode->i_mapping);
-	}
 
 	if (delalloc_work->delay_iput)
 		btrfs_add_delayed_iput(inode);

@@ -9468,7 +9464,6 @@ struct btrfs_delalloc_work *btrfs_alloc_delalloc_work(struct inode *inode,
 	init_completion(&work->completion);
 	INIT_LIST_HEAD(&work->list);
 	work->inode = inode;
-	work->wait = 0;
 	work->delay_iput = delay_iput;
 	WARN_ON_ONCE(!inode);
 	btrfs_init_work(&work->work, btrfs_flush_delalloc_helper,

Signature · 1 line

2.6.2

David Sterba

author maintainer

2015-12-03 16:58 UTC #4 lore raw

PATCH 4/12 btrfs: change how delay_iput is tracked in btrfs_delalloc_work

In reply to David Sterba

The dellaloc work is not frequently used, the delayed status is once set and read so it looks quite safe to drop the member and store the status in the lowest bit of the inode pointer.

Combined with the removal of 'wait' we got +2 objects per 4k slab.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/ctree.h |  5 ++++-
 fs/btrfs/inode.c | 11 +++++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

fs/btrfs/ctree.h +4 -1

index a61c53bce162..d5e250a65725 100644
@@ -3901,8 +3901,11 @@ void btrfs_extent_item_to_extent_map(struct inode *inode,
 
 /* inode.c */
 struct btrfs_delalloc_work {
+	/*
+	 * Note: lowest bit of inode tracks if the iput is delayed,
+	 * do not access the pointer directly.
+	 */
 	struct inode *inode;
-	int delay_iput;
 	struct completion completion;
 	struct list_head list;
 	struct btrfs_work work;

fs/btrfs/inode.c +9 -2

index 15b29e879ffc..529a53b80ca0 100644
@@ -9436,16 +9436,21 @@ static void btrfs_run_delalloc_work(struct btrfs_work *work)
 {
 	struct btrfs_delalloc_work *delalloc_work;
 	struct inode *inode;
+	int delay_iput;
 
 	delalloc_work = container_of(work, struct btrfs_delalloc_work,
 				     work);
 	inode = delalloc_work->inode;
+	/* Lowest bit of inode pointer tracks the delayed status */
+	delay_iput = ((unsigned long)inode & 1UL);
+	inode = (struct inode *)((unsigned long)inode & ~1UL);
+
 	filemap_flush(inode->i_mapping);
 	if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
 				&BTRFS_I(inode)->runtime_flags))
 		filemap_flush(inode->i_mapping);
 
-	if (delalloc_work->delay_iput)
+	if (delay_iput)
 		btrfs_add_delayed_iput(inode);
 	else
 		iput(inode);

@@ -9464,7 +9469,9 @@ struct btrfs_delalloc_work *btrfs_alloc_delalloc_work(struct inode *inode,
 	init_completion(&work->completion);
 	INIT_LIST_HEAD(&work->list);
 	work->inode = inode;
-	work->delay_iput = delay_iput;
+	/* Lowest bit of inode pointer tracks the delayed status */
+	if (delay_iput)
+		*((unsigned long *)work->inode) |= 1UL;
 	WARN_ON_ONCE(!inode);
 	btrfs_init_work(&work->work, btrfs_flush_delalloc_helper,
 			btrfs_run_delalloc_work, NULL, NULL);

Signature · 1 line

2.6.2

David Sterba

author maintainer

2015-12-03 16:58 UTC #11 lore raw

PATCH 5/12 btrfs: remove a trivial helper btrfs_set_buffer_uptodate

In reply to David Sterba

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/disk-io.c     | 5 -----
 fs/btrfs/disk-io.h     | 1 -
 fs/btrfs/extent-tree.c | 2 +-
 fs/btrfs/volumes.c     | 4 ++--
 4 files changed, 3 insertions(+), 9 deletions(-)

fs/btrfs/disk-io.c +0 -5

index 974be09e7556..166ad0821ec2 100644
@@ -3902,11 +3902,6 @@ int btrfs_buffer_uptodate(struct extent_buffer *buf, u64 parent_transid,
 	return !ret;
 }
 
-int btrfs_set_buffer_uptodate(struct extent_buffer *buf)
-{
-	return set_extent_buffer_uptodate(buf);
-}
-
 void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
 {
 	struct btrfs_root *root;

fs/btrfs/disk-io.h +0 -1

index adeb31830b9c..7c52e29fdb0d 100644
@@ -116,7 +116,6 @@ static inline void btrfs_put_fs_root(struct btrfs_root *root)
 void btrfs_mark_buffer_dirty(struct extent_buffer *buf);
 int btrfs_buffer_uptodate(struct extent_buffer *buf, u64 parent_transid,
 			  int atomic);
-int btrfs_set_buffer_uptodate(struct extent_buffer *buf);
 int btrfs_read_buffer(struct extent_buffer *buf, u64 parent_transid);
 u32 btrfs_csum_data(char *data, u32 seed, size_t len);
 void btrfs_csum_final(u32 crc, char *result);

fs/btrfs/extent-tree.c +1 -1

index acf3ed11cfb6..9dfd60aad1b5 100644
@@ -7831,7 +7831,7 @@ btrfs_init_new_buffer(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 	clear_bit(EXTENT_BUFFER_STALE, &buf->bflags);
 
 	btrfs_set_lock_blocking(buf);
-	btrfs_set_buffer_uptodate(buf);
+	set_extent_buffer_uptodate(buf);
 
 	if (root->root_key.objectid == BTRFS_TREE_LOG_OBJECTID) {
 		buf->log_index = root->log_transid % 2;

fs/btrfs/volumes.c +2 -2

index e335938825e1..83bbca7a3924 100644
@@ -6468,11 +6468,11 @@ int btrfs_read_sys_array(struct btrfs_root *root)
 	sb = btrfs_find_create_tree_block(root, BTRFS_SUPER_INFO_OFFSET);
 	if (!sb)
 		return -ENOMEM;
-	btrfs_set_buffer_uptodate(sb);
+	set_extent_buffer_uptodate(sb);
 	btrfs_set_buffer_lockdep_class(root->root_key.objectid, sb, 0);
 	/*
 	 * The sb extent buffer is artifical and just used to read the system array.
-	 * btrfs_set_buffer_uptodate() call does not properly mark all it's
+	 * set_extent_buffer_uptodate() call does not properly mark all it's
 	 * pages up-to-date when the page is larger: extent does not cover the
 	 * whole page and consequently check_page_uptodate does not find all
 	 * the page's extents up-to-date (the hole beyond sb),

Signature · 1 line

2.6.2

David Sterba

author maintainer

2015-12-03 16:58 UTC #12 lore raw

PATCH 6/12 btrfs: make set_extent_buffer_uptodate return void

In reply to David Sterba

Does not return any errors, nor anything from the callgraph.

Signed-off-by David Sterba <[email protected]>

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

fs/btrfs/extent_io.c +1 -2

index 9abe18763a7f..47db9069bf05 100644
@@ -5243,7 +5243,7 @@ int clear_extent_buffer_uptodate(struct extent_buffer *eb)
 	return 0;
 }
 
-int set_extent_buffer_uptodate(struct extent_buffer *eb)
+void set_extent_buffer_uptodate(struct extent_buffer *eb)
 {
 	unsigned long i;
 	struct page *page;

@@ -5255,7 +5255,6 @@ int set_extent_buffer_uptodate(struct extent_buffer *eb)
 		page = eb->pages[i];
 		SetPageUptodate(page);
 	}
-	return 0;
 }
 
 int extent_buffer_uptodate(struct extent_buffer *eb)

fs/btrfs/extent_io.h +1 -1

index f4c1ae11855f..013af0512f1a 100644
@@ -330,7 +330,7 @@ void memset_extent_buffer(struct extent_buffer *eb, char c,
 			  unsigned long start, unsigned long len);
 void clear_extent_buffer_dirty(struct extent_buffer *eb);
 int set_extent_buffer_dirty(struct extent_buffer *eb);
-int set_extent_buffer_uptodate(struct extent_buffer *eb);
+void set_extent_buffer_uptodate(struct extent_buffer *eb);
 int clear_extent_buffer_uptodate(struct extent_buffer *eb);
 int extent_buffer_uptodate(struct extent_buffer *eb);
 int extent_buffer_under_io(struct extent_buffer *eb);

Signature · 1 line

2.6.2

David Sterba

author maintainer

2015-12-03 16:58 UTC #13 lore raw

PATCH 7/12 btrfs: make clear_extent_buffer_uptodate return void

In reply to David Sterba

Does not return any errors, nor anything from the callgraph.

Signed-off-by David Sterba <[email protected]>

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

fs/btrfs/extent_io.c +1 -2

index 47db9069bf05..d5778e821d60 100644
@@ -5227,7 +5227,7 @@ int set_extent_buffer_dirty(struct extent_buffer *eb)
 	return was_dirty;
 }
 
-int clear_extent_buffer_uptodate(struct extent_buffer *eb)
+void clear_extent_buffer_uptodate(struct extent_buffer *eb)
 {
 	unsigned long i;
 	struct page *page;

@@ -5240,7 +5240,6 @@ int clear_extent_buffer_uptodate(struct extent_buffer *eb)
 		if (page)
 			ClearPageUptodate(page);
 	}
-	return 0;
 }
 
 void set_extent_buffer_uptodate(struct extent_buffer *eb)

fs/btrfs/extent_io.h +1 -1

index 013af0512f1a..ad1d28c1cfd4 100644
@@ -331,7 +331,7 @@ void memset_extent_buffer(struct extent_buffer *eb, char c,
 void clear_extent_buffer_dirty(struct extent_buffer *eb);
 int set_extent_buffer_dirty(struct extent_buffer *eb);
 void set_extent_buffer_uptodate(struct extent_buffer *eb);
-int clear_extent_buffer_uptodate(struct extent_buffer *eb);
+void clear_extent_buffer_uptodate(struct extent_buffer *eb);
 int extent_buffer_uptodate(struct extent_buffer *eb);
 int extent_buffer_under_io(struct extent_buffer *eb);
 int map_private_extent_buffer(struct extent_buffer *eb, unsigned long offset,

Signature · 1 line

2.6.2

David Sterba

author maintainer

2015-12-03 16:58 UTC #14 lore raw

PATCH 8/12 btrfs: make extent_clear_unlock_delalloc return void

In reply to David Sterba

Does not return any errors, nor anything from the callgraph.

Signed-off-by David Sterba <[email protected]>

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

fs/btrfs/extent_io.c +2 -3

index d5778e821d60..b7261ea08a87 100644
@@ -1820,7 +1820,7 @@ STATIC u64 find_lock_delalloc_range(struct inode *inode,
 	return found;
 }
 
-int extent_clear_unlock_delalloc(struct inode *inode, u64 start, u64 end,
+void extent_clear_unlock_delalloc(struct inode *inode, u64 start, u64 end,
 				 struct page *locked_page,
 				 unsigned clear_bits,
 				 unsigned long page_ops)

@@ -1835,7 +1835,7 @@ int extent_clear_unlock_delalloc(struct inode *inode, u64 start, u64 end,
 
 	clear_extent_bit(tree, start, end, clear_bits, 1, 0, NULL, GFP_NOFS);
 	if (page_ops == 0)
-		return 0;
+		return;
 
 	if ((page_ops & PAGE_SET_ERROR) && nr_pages > 0)
 		mapping_set_error(inode->i_mapping, -EIO);

@@ -1869,7 +1869,6 @@ int extent_clear_unlock_delalloc(struct inode *inode, u64 start, u64 end,
 		index += ret;
 		cond_resched();
 	}
-	return 0;
 }
 
 /*

fs/btrfs/extent_io.h +1 -1

index ad1d28c1cfd4..2d57166e20d0 100644
@@ -340,7 +340,7 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long offset,
 		      unsigned long *map_len);
 int extent_range_clear_dirty_for_io(struct inode *inode, u64 start, u64 end);
 int extent_range_redirty_for_io(struct inode *inode, u64 start, u64 end);
-int extent_clear_unlock_delalloc(struct inode *inode, u64 start, u64 end,
+void extent_clear_unlock_delalloc(struct inode *inode, u64 start, u64 end,
 				 struct page *locked_page,
 				 unsigned bits_to_clear,
 				 unsigned long page_ops);

Signature · 1 line

2.6.2

David Sterba

author maintainer

2015-12-03 16:58 UTC #15 lore raw

PATCH 9/12 btrfs: make end_extent_writepage return void

In reply to David Sterba

Does not return any errors, nor anything from the callgraph. The branch in end_bio_extent_writepage has been skipped since 5fd02043553b ("Btrfs: finish ordered extents in their own thread").

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/extent_io.c | 7 ++-----
 fs/btrfs/extent_io.h | 2 +-
 2 files changed, 3 insertions(+), 6 deletions(-)

fs/btrfs/extent_io.c +2 -5

index b7261ea08a87..0ea3e99501e4 100644
@@ -2515,7 +2515,7 @@ static int bio_readpage_error(struct bio *failed_bio, u64 phy_offset,
 
 /* lots and lots of room for performance fixes in the end_bio funcs */
 
-int end_extent_writepage(struct page *page, int err, u64 start, u64 end)
+void end_extent_writepage(struct page *page, int err, u64 start, u64 end)
 {
 	int uptodate = (err == 0);
 	struct extent_io_tree *tree;

@@ -2536,7 +2536,6 @@ int end_extent_writepage(struct page *page, int err, u64 start, u64 end)
 		ret = ret < 0 ? ret : -EIO;
 		mapping_set_error(page->mapping, ret);
 	}
-	return 0;
 }
 
 /*

@@ -2578,9 +2577,7 @@ static void end_bio_extent_writepage(struct bio *bio)
 		start = page_offset(page);
 		end = start + bvec->bv_offset + bvec->bv_len - 1;
 
-		if (end_extent_writepage(page, bio->bi_error, start, end))
-			continue;
-
+		end_extent_writepage(page, bio->bi_error, start, end);
 		end_page_writeback(page);
 	}

fs/btrfs/extent_io.h +1 -1

index 2d57166e20d0..81f84f1f488b 100644
@@ -357,7 +357,7 @@ int repair_io_failure(struct inode *inode, u64 start, u64 length, u64 logical,
 		      int mirror_num);
 int clean_io_failure(struct inode *inode, u64 start, struct page *page,
 		     unsigned int pg_offset);
-int end_extent_writepage(struct page *page, int err, u64 start, u64 end);
+void end_extent_writepage(struct page *page, int err, u64 start, u64 end);
 int repair_eb_io_failure(struct btrfs_root *root, struct extent_buffer *eb,
 			 int mirror_num);

Signature · 1 line

2.6.2

David Sterba

author maintainer

2015-12-03 16:58 UTC #16 lore raw

PATCH 10/12 btrfs: make extent_range_clear_dirty_for_io return void

In reply to David Sterba

Does not return any errors, nor anything from the callgraph. There's a BUG_ON but it's a sanity check and not an error condition we could recover from.

Signed-off-by David Sterba <[email protected]>

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

fs/btrfs/extent_io.c +1 -2

index 0ea3e99501e4..e55a728408bb 100644
@@ -1451,7 +1451,7 @@ int unlock_extent(struct extent_io_tree *tree, u64 start, u64 end)
 				GFP_NOFS);
 }
 
-int extent_range_clear_dirty_for_io(struct inode *inode, u64 start, u64 end)
+void extent_range_clear_dirty_for_io(struct inode *inode, u64 start, u64 end)
 {
 	unsigned long index = start >> PAGE_CACHE_SHIFT;
 	unsigned long end_index = end >> PAGE_CACHE_SHIFT;

@@ -1464,7 +1464,6 @@ int extent_range_clear_dirty_for_io(struct inode *inode, u64 start, u64 end)
 		page_cache_release(page);
 		index++;
 	}
-	return 0;
 }
 
 int extent_range_redirty_for_io(struct inode *inode, u64 start, u64 end)

fs/btrfs/extent_io.h +1 -1

index 81f84f1f488b..b0b2d20ffd3c 100644
@@ -338,7 +338,7 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long offset,
 		      unsigned long min_len, char **map,
 		      unsigned long *map_start,
 		      unsigned long *map_len);
-int extent_range_clear_dirty_for_io(struct inode *inode, u64 start, u64 end);
+void extent_range_clear_dirty_for_io(struct inode *inode, u64 start, u64 end);
 int extent_range_redirty_for_io(struct inode *inode, u64 start, u64 end);
 void extent_clear_unlock_delalloc(struct inode *inode, u64 start, u64 end,
 				 struct page *locked_page,

Signature · 1 line

2.6.2

David Sterba

author maintainer

2015-12-03 16:58 UTC #17 lore raw

PATCH 11/12 btrfs: make extent_range_redirty_for_io return void

In reply to David Sterba

Does not return any errors, nor anything from the callgraph. There's a BUG_ON but it's a sanity check and not an error condition we could recover from.

Signed-off-by David Sterba <[email protected]>

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

fs/btrfs/extent_io.c +1 -2

index e55a728408bb..f06800e48f97 100644
@@ -1466,7 +1466,7 @@ void extent_range_clear_dirty_for_io(struct inode *inode, u64 start, u64 end)
 	}
 }
 
-int extent_range_redirty_for_io(struct inode *inode, u64 start, u64 end)
+void extent_range_redirty_for_io(struct inode *inode, u64 start, u64 end)
 {
 	unsigned long index = start >> PAGE_CACHE_SHIFT;
 	unsigned long end_index = end >> PAGE_CACHE_SHIFT;

@@ -1480,7 +1480,6 @@ int extent_range_redirty_for_io(struct inode *inode, u64 start, u64 end)
 		page_cache_release(page);
 		index++;
 	}
-	return 0;
 }
 
 /*

fs/btrfs/extent_io.h +1 -1

index b0b2d20ffd3c..fbc6448e70e4 100644
@@ -339,7 +339,7 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long offset,
 		      unsigned long *map_start,
 		      unsigned long *map_len);
 void extent_range_clear_dirty_for_io(struct inode *inode, u64 start, u64 end);
-int extent_range_redirty_for_io(struct inode *inode, u64 start, u64 end);
+void extent_range_redirty_for_io(struct inode *inode, u64 start, u64 end);
 void extent_clear_unlock_delalloc(struct inode *inode, u64 start, u64 end,
 				 struct page *locked_page,
 				 unsigned bits_to_clear,

Signature · 1 line

2.6.2

David Sterba

author maintainer

2015-12-03 16:58 UTC #18 lore raw

PATCH 12/12 btrfs: make set_range_writeback return void

In reply to David Sterba

Does not return any errors, nor anything from the callgraph. There's a BUG_ON but it's a sanity check and not an error condition we could recover from.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/extent_io.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

fs/btrfs/extent_io.c +1 -2

index f06800e48f97..efe6ac627be9 100644
@@ -1485,7 +1485,7 @@ void extent_range_redirty_for_io(struct inode *inode, u64 start, u64 end)
 /*
  * helper function to set both pages and extents in the tree writeback
  */
-static int set_range_writeback(struct extent_io_tree *tree, u64 start, u64 end)
+static void set_range_writeback(struct extent_io_tree *tree, u64 start, u64 end)
 {
 	unsigned long index = start >> PAGE_CACHE_SHIFT;
 	unsigned long end_index = end >> PAGE_CACHE_SHIFT;

@@ -1498,7 +1498,6 @@ static int set_range_writeback(struct extent_io_tree *tree, u64 start, u64 end)
 		page_cache_release(page);
 		index++;
 	}
-	return 0;
 }
 
 /* find the first state struct with 'bits' set after 'start', and

Signature · 1 line

2.6.2

Liu Bo

2015-12-04 02:25 UTC #5 lore raw

Re: [PATCH 04/12] btrfs: change how delay_iput is tracked in btrfs_delalloc_work

In reply to David Sterba

Quoted 36 lines · David Sterba

The dellaloc work is not frequently used, the delayed status is once set and read so it looks quite safe to drop the member and store the status in the lowest bit of the inode pointer.

Combined with the removal of 'wait' we got +2 objects per 4k slab.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/ctree.h |  5 ++++-
 fs/btrfs/inode.c | 11 +++++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

fs/btrfs/ctree.h +4 -1

index a61c53bce162..d5e250a65725 100644
@@ -3901,8 +3901,11 @@ void btrfs_extent_item_to_extent_map(struct inode *inode,
 
 /* inode.c */
 struct btrfs_delalloc_work {
+	/*
+	 * Note: lowest bit of inode tracks if the iput is delayed,
+	 * do not access the pointer directly.
+	 */
 	struct inode *inode;
-	int delay_iput;
 	struct completion completion;
 	struct list_head list;
 	struct btrfs_work work;

fs/btrfs/inode.c +0 -0

index 15b29e879ffc..529a53b80ca0 100644
@@ -9436,16 +9436,21 @@ static void btrfs_run_delalloc_work(struct btrfs_work *work)
 {
 	struct btrfs_delalloc_work *delalloc_work;

fs/btrfs/inode.c static void btrfs_run_delalloc_work(struct btrfs_work *work) · line 9447

Quoted 10 lines

fs/btrfs/inode.c +5 -0

 	struct inode *inode;
+	int delay_iput;
 
 	delalloc_work = container_of(work, struct btrfs_delalloc_work,
 				     work);
 	inode = delalloc_work->inode;
+	/* Lowest bit of inode pointer tracks the delayed status */
+	delay_iput = ((unsigned long)inode & 1UL);
+	inode = (struct inode *)((unsigned long)inode & ~1UL);
+

To be quite frankly, I don't like this, it's a pointer anyway, error-prone in a debugging view, instead would 'u8 delayed_iput' help?

Thanks,

-liubo

Quoted 28 lines

fs/btrfs/inode.c +4 -2

 	filemap_flush(inode->i_mapping);
 	if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
 				&BTRFS_I(inode)->runtime_flags))
 		filemap_flush(inode->i_mapping);
 
-	if (delalloc_work->delay_iput)
+	if (delay_iput)
 		btrfs_add_delayed_iput(inode);
 	else
 		iput(inode);

@@ -9464,7 +9469,9 @@ struct btrfs_delalloc_work *btrfs_alloc_delalloc_work(struct inode *inode,
 	init_completion(&work->completion);
 	INIT_LIST_HEAD(&work->list);
 	work->inode = inode;
-	work->delay_iput = delay_iput;
+	/* Lowest bit of inode pointer tracks the delayed status */
+	if (delay_iput)
+		*((unsigned long *)work->inode) |= 1UL;
 	WARN_ON_ONCE(!inode);
 	btrfs_init_work(&work->work, btrfs_flush_delalloc_helper,
 			btrfs_run_delalloc_work, NULL, NULL);

Signature · 6 lines

2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Sterba

2015-12-04 12:38 UTC #6 lore raw

Re: [PATCH 04/12] btrfs: change how delay_iput is tracked in btrfs_delalloc_work

Quoted 15 lines · Liu Bo

Quoted 15 lines

fs/btrfs/ctree.h +0 -1

 	struct inode *inode;
-	int delay_iput;
 	struct completion completion;
 	struct list_head list;
 	struct btrfs_work work;

fs/btrfs/inode.c +1 -0

index 15b29e879ffc..529a53b80ca0 100644
@@ -9436,16 +9436,21 @@ static void btrfs_run_delalloc_work(struct btrfs_work *work)
 {
 	struct btrfs_delalloc_work *delalloc_work;
 	struct inode *inode;
+	int delay_iput;

Quoted 10 lines

fs/btrfs/inode.c static void btrfs_run_delalloc_work(struct btrfs_work *work) · line 9447
Quoted 7 lines
fs/btrfs/inode.c +4 -0
 	delalloc_work = container_of(work, struct btrfs_delalloc_work,
 				     work);
 	inode = delalloc_work->inode;
+	/* Lowest bit of inode pointer tracks the delayed status */
+	delay_iput = ((unsigned long)inode & 1UL);
+	inode = (struct inode *)((unsigned long)inode & ~1UL);
+
To be quite frankly, I don't like this, it's a pointer anyway, error-prone in a debugging view, instead would 'u8 delayed_iput' help?

The point was to shrink the structure. Adding the u8 will grow it by another 8 bytes, besides the slab objects are aligned to 8 bytes by default so the overall cost of storing the delayed information is 8 bytes:

struct btrfs_delalloc_work {

        struct inode *             inode;                /*     0     8 */
        struct completion          completion;           /*     8    32 */
        struct list_head           list;                 /*    40    16 */
        struct btrfs_work          work;                 /*    56    88 */
        /* --- cacheline 2 boundary (128 bytes) was 16 bytes ago --- */
        u8                         delay;                /*   144     1 */

        /* size: 152, cachelines: 3, members: 5 */
        /* padding: 7 */
        /* last cacheline: 24 bytes */
};

As the use of the inode pointer is limited, I don't think this would cause surprises. And it's commented where used which should help during debugging.

Abusing the low bits of pointers is nothing new, the page cache tags are implemented that way. This kind of low-level optimizations is IMO acceptable.

Holger Hoffstätte

2015-12-04 12:50 UTC #7 lore raw

Re: [PATCH 04/12] btrfs: change how delay_iput is tracked in btrfs_delalloc_work

On 12/04/15 13:36, David Sterba wrote:
[snip]

Quoted 3 lines

As the use of the inode pointer is limited, I don't think this would cause surprises. And it's commented where used which should help during debugging.

When I read through those bits (mostly pondering portability) I was wondering whether it might make sense to provide thin wrap/unwrap functions for the tag bit instead of relying on open code and comments only. Just an idea, not sure if it's worth the trouble. The code itself is functional and works fine as it is, I'm running it right now.

-h

Filipe Manana

2015-12-04 13:08 UTC #9 lore raw

Re: [PATCH 04/12] btrfs: change how delay_iput is tracked in btrfs_delalloc_work

In reply to David Sterba

Quoted 45 lines · David Sterba

Quoted 15 lines · Liu Bo

Quoted 15 lines

    struct inode *inode;

fs/btrfs/ctree.h +0 -1

-   int delay_iput;
    struct completion completion;
    struct list_head list;
    struct btrfs_work work;

fs/btrfs/inode.c +1 -0

index 15b29e879ffc..529a53b80ca0 100644
@@ -9436,16 +9436,21 @@ static void btrfs_run_delalloc_work(struct btrfs_work *work)
 {
    struct btrfs_delalloc_work *delalloc_work;
    struct inode *inode;
+   int delay_iput;

Quoted 10 lines

fs/btrfs/inode.c static void btrfs_run_delalloc_work(struct btrfs_work *work) · line 9447
Quoted 7 lines
fs/btrfs/inode.c +4 -0
    delalloc_work = container_of(work, struct btrfs_delalloc_work,
                                 work);
    inode = delalloc_work->inode;
+   /* Lowest bit of inode pointer tracks the delayed status */
+   delay_iput = ((unsigned long)inode & 1UL);
+   inode = (struct inode *)((unsigned long)inode & ~1UL);
+
To be quite frankly, I don't like this, it's a pointer anyway, error-prone in a debugging view, instead would 'u8 delayed_iput' help?

struct btrfs_delalloc_work {

        struct inode *             inode;                /*     0     8 */
        struct completion          completion;           /*     8    32 */
        struct list_head           list;                 /*    40    16 */
        struct btrfs_work          work;                 /*    56    88 */
        /* --- cacheline 2 boundary (128 bytes) was 16 bytes ago --- */
        u8                         delay;                /*   144     1 */

        /* size: 152, cachelines: 3, members: 5 */
        /* padding: 7 */
        /* last cacheline: 24 bytes */
};

Quoted 6 lines

As the use of the inode pointer is limited, I don't think this would cause surprises. And it's commented where used which should help during debugging.
Abusing the low bits of pointers is nothing new, the page cache tags are implemented that way. This kind of low-level optimizations is IMO acceptable.

Acceptable, but, is it needed? I mean, are we using so much memory with this structure or does the better packing causes any significant performance improvement to justify this sort of obscure code? IMO it's worth only when we know (measured) that it actually positively impacts workloads (either real ones or even artificial ones from benchmark tools).

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 5 lines

Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

David Sterba

2015-12-07 13:54 UTC #10 lore raw

Re: [PATCH 04/12] btrfs: change how delay_iput is tracked in btrfs_delalloc_work

Quoted 53 lines · Filipe Manana

Quoted 45 lines · David Sterba
Quoted 15 lines · Liu Bo
Quoted 15 lines
    struct inode *inode;
fs/btrfs/ctree.h +0 -1
-   int delay_iput;
    struct completion completion;
    struct list_head list;
    struct btrfs_work work;
fs/btrfs/inode.c +1 -0
index 15b29e879ffc..529a53b80ca0 100644
@@ -9436,16 +9436,21 @@ static void btrfs_run_delalloc_work(struct btrfs_work *work)
 {
    struct btrfs_delalloc_work *delalloc_work;
    struct inode *inode;
+   int delay_iput;
 
Quoted 10 lines
fs/btrfs/inode.c static void btrfs_run_delalloc_work(struct btrfs_work *work) · line 9447
Quoted 7 lines
fs/btrfs/inode.c +4 -0
    delalloc_work = container_of(work, struct btrfs_delalloc_work,
                                 work);
    inode = delalloc_work->inode;
+   /* Lowest bit of inode pointer tracks the delayed status */
+   delay_iput = ((unsigned long)inode & 1UL);
+   inode = (struct inode *)((unsigned long)inode & ~1UL);
+
To be quite frankly, I don't like this, it's a pointer anyway, error-prone in a debugging view, instead would 'u8 delayed_iput' help?
The point was to shrink the structure. Adding the u8 will grow it by another 8 bytes, besides the slab objects are aligned to 8 bytes by default so the overall cost of storing the delayed information is 8 bytes:
struct btrfs_delalloc_work {
        struct inode *             inode;                /*     0     8 */
        struct completion          completion;           /*     8    32 */
        struct list_head           list;                 /*    40    16 */
        struct btrfs_work          work;                 /*    56    88 */
        /* --- cacheline 2 boundary (128 bytes) was 16 bytes ago --- */
        u8                         delay;                /*   144     1 */

        /* size: 152, cachelines: 3, members: 5 */
        /* padding: 7 */
        /* last cacheline: 24 bytes */
};
Quoted 6 lines
As the use of the inode pointer is limited, I don't think this would cause surprises. And it's commented where used which should help during debugging.
Abusing the low bits of pointers is nothing new, the page cache tags are implemented that way. This kind of low-level optimizations is IMO acceptable.

Quoted 6 lines

Acceptable, but, is it needed? I mean, are we using so much memory with this structure or does the better packing causes any significant performance improvement to justify this sort of obscure code? IMO it's worth only when we know (measured) that it actually positively impacts workloads (either real ones or even artificial ones from benchmark tools).

And it turns out this structure is not critical, seldomly used so the space savings and bit tricks are not justified. Patch dropped.

David Sterba

2015-12-07 14:18 UTC #19 lore raw

In reply to David Sterba

Quoted 8 lines · David Sterba

----------------------------------------------------------------
The following changes since commit 1ec218373b8ebda821aec00bb156a9c94fad9cd4:
David Sterba (12):
btrfs: make btrfs_close_one_device static
btrfs: sink parameter wait to btrfs_alloc_delalloc_work
btrfs: remove wait from struct btrfs_delalloc_work
btrfs: change how delay_iput is tracked in btrfs_delalloc_work

FYI, this patch was dropped, branch force-pushed and the top commit is 35de6db28f260e88e9acc47305f7b3f272d76cbf

David Sterba

2015-12-07 14:25 UTC #8 lore raw

Re: [PATCH 04/12] btrfs: change how delay_iput is tracked in btrfs_delalloc_work

In reply to Holger Hoffstätte

Quoted 9 lines · Holger Hoffstätte

On 12/04/15 13:36, David Sterba wrote:
[snip]
Quoted 3 lines
As the use of the inode pointer is limited, I don't think this would cause surprises. And it's commented where used which should help during debugging.
When I read through those bits (mostly pondering portability) I was wondering whether it might make sense to provide thin wrap/unwrap functions for the tag bit instead of relying on open code and comments only.

I'm not sure if a helper makes sense for a single occurence of each operation, a comment seems enough to me. Anyway, the patch will be dropped.