[PATCH] btrfs: zero out delayed node upon allocation

[PATCH] btrfs: zero out delayed node upon allocation

[Alexandru Moise]

This allows us to trim out half of btrfs_init_delayed_node()
which is now reduntant.

Signed-off-by: Alexandru Moise <00moses.alexander00@gmail.com>
---
 fs/btrfs/delayed-inode.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index a2ae427..7aa84be 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -53,17 +53,11 @@ static inline void btrfs_init_delayed_node(
 {
 	delayed_node->root = root;
 	delayed_node->inode_id = inode_id;
-	atomic_set(&delayed_node->refs, 0);
-	delayed_node->count = 0;
-	delayed_node->flags = 0;
 	delayed_node->ins_root = RB_ROOT;
 	delayed_node->del_root = RB_ROOT;
 	mutex_init(&delayed_node->mutex);
-	delayed_node->index_cnt = 0;
 	INIT_LIST_HEAD(&delayed_node->n_list);
 	INIT_LIST_HEAD(&delayed_node->p_list);
-	delayed_node->bytes_reserved = 0;
-	memset(&delayed_node->inode_item, 0, sizeof(delayed_node->inode_item));
 }
 
 static inline int btrfs_is_continuous_delayed_item(
@@ -132,7 +126,7 @@ again:
 	if (node)
 		return node;
 
-	node = kmem_cache_alloc(delayed_node_cache, GFP_NOFS);
+	node = kmem_cache_zalloc(delayed_node_cache, GFP_NOFS);
 	if (!node)
 		return ERR_PTR(-ENOMEM);
 	btrfs_init_delayed_node(node, root, ino);
-- 
2.6.2

Re: [PATCH] btrfs: zero out delayed node upon allocation

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 10/25/15 1:48 PM, Alexandru Moise wrote:
> This allows us to trim out half of btrfs_init_delayed_node() which
> is now reduntant.

It's redundant if kmem_cache_zalloc is used, but you haven't
documented that doing so is now required.  For all of these changes
you've posted, if they're to be accepted, I'd really prefer to set up
the slab with a constructor instead.  Then we don't need to worry
about such guarantees.  The object returned via kmem_cache_alloc will
always be properly initialized.

> Signed-off-by: Alexandru Moise <00moses.alexander00@gmail.com> --- 
> fs/btrfs/delayed-inode.c | 8 +------- 1 file changed, 1
> insertion(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c 
> index a2ae427..7aa84be 100644 --- a/fs/btrfs/delayed-inode.c +++
> b/fs/btrfs/delayed-inode.c @@ -53,17 +53,11 @@ static inline void
> btrfs_init_delayed_node( { delayed_node->root = root; 
> delayed_node->inode_id = inode_id; -
> atomic_set(&delayed_node->refs, 0);

This makes assumptions about atomic_t and what atomic_set does that
aren't guaranteed to be true.  When accessors/mutators are part of the
API they should be used.

- -Jeff

> -	delayed_node->count = 0; -	delayed_node->flags = 0; 
> delayed_node->ins_root = RB_ROOT; delayed_node->del_root =
> RB_ROOT; mutex_init(&delayed_node->mutex); -
> delayed_node->index_cnt = 0; 
> INIT_LIST_HEAD(&delayed_node->n_list); 
> INIT_LIST_HEAD(&delayed_node->p_list); -
> delayed_node->bytes_reserved = 0; -
> memset(&delayed_node->inode_item, 0,
> sizeof(delayed_node->inode_item)); }
> 
> static inline int btrfs_is_continuous_delayed_item( @@ -132,7
> +126,7 @@ again: if (node) return node;
> 
> -	node = kmem_cache_alloc(delayed_node_cache, GFP_NOFS); +	node =
> kmem_cache_zalloc(delayed_node_cache, GFP_NOFS); if (!node) return
> ERR_PTR(-ENOMEM); btrfs_init_delayed_node(node, root, ino);
> 


- -- 
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2

iQIcBAEBCAAGBQJWLP5RAAoJEB57S2MheeWyQMoQAMUNGQFl14cwbJWtSsNjwZJy
eRaSckKcRinc4zoFxATSn4R/UQLadFNg+tAtSnNgoJngpg/nn6Fc19vMS5QVpU1c
OhPgYUVkBTjLxhBaY6Iivinbx4+pjFaEP+e6LoqXefelB6Wq17e8DavQcDU5gH2e
3vzXLt8kKa1hlC5SbtjW59cLgHzrclR6h4qeO00R7vWPA4YyIzOKOnYznrgZ69wS
V97YBmcyucrl+EENYFe5BYBJmP5LojHjsxfPtF2zzZnvCpSrWPZ5Num/2yCofFir
7HNs86wLEBjsEUhkSjZ/7u3XATjVOXNHVUsknOOMB/B6zk9ngnK51mp7a2Ib41PM
nMbNOVHhauumY9sf++RWjN53HTGH8aAlp7B0JIRTceGrvKdT2YGjW8iWwIrtlCE0
bSoEXRNcMYFvR+0we/WKVi4sX0ysRfhDbCuQeWPEJ6R4DbzmtqjCmmHQnZyVpwex
1ZxT8yvFVJvQHEl2E+mHOneRgdVuj89j8AZJ99idIHKa9b60tHAlkq+zXoW0odIt
qV09bYoc7J87W5JvTgKVks13ovwKmM7Ij7zHFMBLVO/BGrd+zFFW1qq8r6GJBKG9
PaXt+HUHBCt4TpvAza5JZ0Yn7tYvALHizj+IZStTw2Kz1vQC5KEnjZIMa3LcKrsx
r2oQLSfGkcsf8EcIzDX1
=qINk
-----END PGP SIGNATURE-----

Re: [PATCH] btrfs: zero out delayed node upon allocation

[Alexandru Moise]

> > This allows us to trim out half of btrfs_init_delayed_node() which
> > is now reduntant.
> 
> It's redundant if kmem_cache_zalloc is used, but you haven't
> documented that doing so is now required.  For all of these changes
> you've posted, if they're to be accepted, I'd really prefer to set up
> the slab with a constructor instead.  Then we don't need to worry
> about such guarantees.  The object returned via kmem_cache_alloc will
> always be properly initialized.

Well I wouldn't say it's *required* just makes this particular piece
of code neater, since we memset-zero the node's inode_item _anyways_.
I like the constructor idea though, do you suggest I should invest in
that idea?

> 
> This makes assumptions about atomic_t and what atomic_set does that
> aren't guaranteed to be true.  When accessors/mutators are part of the
> API they should be used.
> 
> - -Jeff

You're right, taking out that atomic_set was really stupid. I'll
resent the patch with a proper explanation in the commit message and
put the atomic_set back.

Unless you feel that the change is rather pointless, I'll gladly back
off :-).

Regards,
	Alex

Re: [PATCH] btrfs: zero out delayed node upon allocation

[Jeff Mahoney]

On Oct 25, 2015, at 3:50 PM, Alexandru Moise <00moses.alexander00@gmail.com> wrote:

>>> This allows us to trim out half of btrfs_init_delayed_node() which
>>> is now reduntant.
>> 
>> It's redundant if kmem_cache_zalloc is used, but you haven't
>> documented that doing so is now required.  For all of these changes
>> you've posted, if they're to be accepted, I'd really prefer to set up
>> the slab with a constructor instead.  Then we don't need to worry
>> about such guarantees.  The object returned via kmem_cache_alloc will
>> always be properly initialized.
> 
> Well I wouldn't say it's *required* just makes this particular piece
> of code neater, since we memset-zero the node's inode_item _anyways_.

But the rest of the delayed node still needs to be initialized. That's happening implicitly with your patch via zalloc but if anyone ever adds a new allocation from that cache, they'll need to know that zalloc is required for proper initializatiom. Documenting that is one approach. Constructors are another.

> I like the constructor idea though, do you suggest I should invest in
> that idea?

Well, see below.

>> 
>> This makes assumptions about atomic_t and what atomic_set does that
>> aren't guaranteed to be true.  When accessors/mutators are part of the
>> API they should be used.
>> 
>> - -Jeff
> 
> You're right, taking out that atomic_set was really stupid. I'll
> resent the patch with a proper explanation in the commit message and
> put the atomic_set back.
> 
> Unless you feel that the change is rather pointless, I'll gladly back
> off :-).

I don't want to dissuade new contributors but there's plenty of work to be done before we start worrying about cleaning initializers. These aren't hot paths (but if they were, the implicit memset of the whole object might have negative effects.) The immediate impact of cleanup patches like these is code churn so that developers with outstanding patch sets need to rebase on top of new context. It's a normal part of the development cycle but it's more work for not a lot of benefit.

So, if your goal is to contribute to btrfs, I'd suggest digging into the code. Try out changes. Break it and figure out how you broke it. It's like learning how to navigate in a new city: by getting lost you learn more. :)

-Jeff

Re: [PATCH] btrfs: zero out delayed node upon allocation

[Alexandru Moise]

On Sun, Oct 25, 2015 at 01:33:45PM -0400, Jeff Mahoney wrote:
> 
> On Oct 25, 2015, at 3:50 PM, Alexandru Moise <00moses.alexander00@gmail.com> wrote:
> 
> >>> This allows us to trim out half of btrfs_init_delayed_node() which
> >>> is now reduntant.
> >> 
> >> It's redundant if kmem_cache_zalloc is used, but you haven't
> >> documented that doing so is now required.  For all of these changes
> >> you've posted, if they're to be accepted, I'd really prefer to set up
> >> the slab with a constructor instead.  Then we don't need to worry
> >> about such guarantees.  The object returned via kmem_cache_alloc will
> >> always be properly initialized.
> > 
> > Well I wouldn't say it's *required* just makes this particular piece
> > of code neater, since we memset-zero the node's inode_item _anyways_.
> 
> But the rest of the delayed node still needs to be initialized. That's happening implicitly with your patch via zalloc but if anyone ever adds a new allocation from that cache, they'll need to know that zalloc is required for proper initializatiom. Documenting that is one approach. Constructors are another.
> 
> > I like the constructor idea though, do you suggest I should invest in
> > that idea?
> 
> Well, see below.
> 
> >> 
> >> This makes assumptions about atomic_t and what atomic_set does that
> >> aren't guaranteed to be true.  When accessors/mutators are part of the
> >> API they should be used.
> >> 
> >> - -Jeff
> > 
> > You're right, taking out that atomic_set was really stupid. I'll
> > resent the patch with a proper explanation in the commit message and
> > put the atomic_set back.
> > 
> > Unless you feel that the change is rather pointless, I'll gladly back
> > off :-).
> 
> I don't want to dissuade new contributors but there's plenty of work to be done before we start worrying about cleaning initializers. These aren't hot paths (but if they were, the implicit memset of the whole object might have negative effects.) The immediate impact of cleanup patches like these is code churn so that developers with outstanding patch sets need to rebase on top of new context. It's a normal part of the development cycle but it's more work for not a lot of benefit.
> 
> So, if your goal is to contribute to btrfs, I'd suggest digging into the code. Try out changes. Break it and figure out how you broke it. It's like learning how to navigate in a new city: by getting lost you learn more. :)
> 
> -Jeff

Yeah I see your point, I take back version 2 of this patch and will
follow your advice. Much appreciated!

	Alex