[PULL][PATCH 00/12] Minor cleanups and code simplifications

[PULL][PATCH 00/12] Minor cleanups and code simplifications

[David Sterba]

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(-)

[PATCH 01/12] btrfs: make btrfs_close_one_device static

[David Sterba]

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/volumes.c | 3 ++-
 fs/btrfs/volumes.h | 1 -
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a6df8fdc1312..e335938825e1 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -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;
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index ec5712372732..2dc59eba188d 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -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
-- 
2.6.2

[PATCH 02/12] btrfs: sink parameter wait to btrfs_alloc_delalloc_work

[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 <dsterba@suse.com>
---
 fs/btrfs/ctree.h | 2 +-
 fs/btrfs/inode.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 8c58191249cc..cb540251f666 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -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,
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 994490d5fa64..9898b0d79c5a 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -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);
-- 
2.6.2

[PATCH 03/12] btrfs: remove wait from struct btrfs_delalloc_work

[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 <dsterba@suse.com>
---
 fs/btrfs/ctree.h |  1 -
 fs/btrfs/inode.c | 11 +++--------
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index cb540251f666..a61c53bce162 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -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;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 9898b0d79c5a..15b29e879ffc 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -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,
-- 
2.6.2

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

[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 <dsterba@suse.com>
---
 fs/btrfs/ctree.h |  5 ++++-
 fs/btrfs/inode.c | 11 +++++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index a61c53bce162..d5e250a65725 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -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;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 15b29e879ffc..529a53b80ca0 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -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);
-- 
2.6.2

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

[Liu Bo]

On Thu, Dec 03, 2015 at 05:56:32PM +0100, David Sterba wrote:
> 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 <dsterba@suse.com>
> ---
>  fs/btrfs/ctree.h |  5 ++++-
>  fs/btrfs/inode.c | 11 +++++++++--
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index a61c53bce162..d5e250a65725 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -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;
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 15b29e879ffc..529a53b80ca0 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -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);
> +

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

>  	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);
> -- 
> 2.6.2
> 
> --
> 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

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

[David Sterba]

On Thu, Dec 03, 2015 at 06:25:37PM -0800, Liu Bo wrote:
> >  	struct inode *inode;
> > -	int delay_iput;
> >  	struct completion completion;
> >  	struct list_head list;
> >  	struct btrfs_work work;
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 15b29e879ffc..529a53b80ca0 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -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);
> > +
> 
> 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.

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

[Holger Hoffstätte]

On 12/04/15 13:36, David Sterba wrote:
[snip]
> 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

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

[David Sterba]

On Fri, Dec 04, 2015 at 01:50:39PM +0100, Holger Hoffstätte wrote:
> On 12/04/15 13:36, David Sterba wrote:
> [snip]
> > 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.

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

[Filipe Manana]

On Fri, Dec 4, 2015 at 12:36 PM, David Sterba <dsterba@suse.cz> wrote:
> On Thu, Dec 03, 2015 at 06:25:37PM -0800, Liu Bo wrote:
>> >     struct inode *inode;
>> > -   int delay_iput;
>> >     struct completion completion;
>> >     struct list_head list;
>> >     struct btrfs_work work;
>> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> > index 15b29e879ffc..529a53b80ca0 100644
>> > --- a/fs/btrfs/inode.c
>> > +++ b/fs/btrfs/inode.c
>> > @@ -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);
>> > +
>>
>> 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.

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).

> --
> 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



-- 
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."

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

[David Sterba]

On Fri, Dec 04, 2015 at 01:08:59PM +0000, Filipe Manana wrote:
> On Fri, Dec 4, 2015 at 12:36 PM, David Sterba <dsterba@suse.cz> wrote:
> > On Thu, Dec 03, 2015 at 06:25:37PM -0800, Liu Bo wrote:
> >> >     struct inode *inode;
> >> > -   int delay_iput;
> >> >     struct completion completion;
> >> >     struct list_head list;
> >> >     struct btrfs_work work;
> >> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> >> > index 15b29e879ffc..529a53b80ca0 100644
> >> > --- a/fs/btrfs/inode.c
> >> > +++ b/fs/btrfs/inode.c
> >> > @@ -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);
> >> > +
> >>
> >> 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.
> 
> 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.

[PATCH 05/12] btrfs: remove a trivial helper btrfs_set_buffer_uptodate

[David Sterba]

Signed-off-by: David Sterba <dsterba@suse.com>
---
 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(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 974be09e7556..166ad0821ec2 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -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;
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index adeb31830b9c..7c52e29fdb0d 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -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);
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index acf3ed11cfb6..9dfd60aad1b5 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -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;
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e335938825e1..83bbca7a3924 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -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),
-- 
2.6.2

[PATCH 06/12] btrfs: make set_extent_buffer_uptodate return void

[David Sterba]

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

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

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 9abe18763a7f..47db9069bf05 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -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)
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index f4c1ae11855f..013af0512f1a 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -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);
-- 
2.6.2

[PATCH 07/12] btrfs: make clear_extent_buffer_uptodate return void

[David Sterba]

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

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

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 47db9069bf05..d5778e821d60 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -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)
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 013af0512f1a..ad1d28c1cfd4 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -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,
-- 
2.6.2

[PATCH 08/12] btrfs: make extent_clear_unlock_delalloc return void

[David Sterba]

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

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/extent_io.c | 5 ++---
 fs/btrfs/extent_io.h | 2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index d5778e821d60..b7261ea08a87 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -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;
 }
 
 /*
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index ad1d28c1cfd4..2d57166e20d0 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -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);
-- 
2.6.2

[PATCH 09/12] btrfs: make end_extent_writepage return void

[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 <dsterba@suse.com>
---
 fs/btrfs/extent_io.c | 7 ++-----
 fs/btrfs/extent_io.h | 2 +-
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index b7261ea08a87..0ea3e99501e4 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -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);
 	}
 
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 2d57166e20d0..81f84f1f488b 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -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);
 
-- 
2.6.2

[PATCH 10/12] btrfs: make extent_range_clear_dirty_for_io return void

[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 <dsterba@suse.com>
---
 fs/btrfs/extent_io.c | 3 +--
 fs/btrfs/extent_io.h | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 0ea3e99501e4..e55a728408bb 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -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)
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 81f84f1f488b..b0b2d20ffd3c 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -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,
-- 
2.6.2

[PATCH 11/12] btrfs: make extent_range_redirty_for_io return void

[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 <dsterba@suse.com>
---
 fs/btrfs/extent_io.c | 3 +--
 fs/btrfs/extent_io.h | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index e55a728408bb..f06800e48f97 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -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;
 }
 
 /*
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index b0b2d20ffd3c..fbc6448e70e4 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -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,
-- 
2.6.2

[PATCH 12/12] btrfs: make set_range_writeback return void

[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 <dsterba@suse.com>
---
 fs/btrfs/extent_io.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index f06800e48f97..efe6ac627be9 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -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
-- 
2.6.2

Re: [PULL][PATCH 00/12] Minor cleanups and code simplifications

[David Sterba]

On Thu, Dec 03, 2015 at 05:56:19PM +0100, David Sterba wrote:
> ----------------------------------------------------------------
> 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