Re: [PATCH] btrfs: zero out delayed node upon allocation
On Sun, Oct 25, 2015 at 01:33:45PM -0400, Jeff Mahoney wrote: > > On Oct 25, 2015, at 3:50 PM, Alexandru Moise <00moses.alexande...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] btrfs: zero out delayed node upon allocation
On Oct 25, 2015, at 3:50 PM, Alexandru Moise <00moses.alexande...@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-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] btrfs: zero out delayed node upon allocation
> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] btrfs: zero out delayed node upon allocation
-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.alexande...@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-
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] btrfs: zero out delayed node upon allocation
This allows us to trim out half of btrfs_init_delayed_node()
which is now reduntant.
Signed-off-by: Alexandru Moise <00moses.alexande...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
