| Message ID | 20190225190744.21664-1-rgoldwyn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] btrfs: Initialize btrfs_io_ctl instead of memsetting it | expand |
On Mon, Feb 25, 2019 at 01:07:42PM -0600, Goldwyn Rodrigues wrote: > From: Goldwyn Rodrigues <rgoldwyn@suse.com> > > io_ctl_init() memsets it to zero anyways. However, I presume the > memset was added to avoid the WARN_ON in io_ctl_init(). I don't see any WARN_ON in io_ctl_init, you probably mean __btrfs_write_out_cache. > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > --- > fs/btrfs/free-space-cache.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c > index 74aa552f4793..c813378ebf08 100644 > --- a/fs/btrfs/free-space-cache.c > +++ b/fs/btrfs/free-space-cache.c > @@ -3544,13 +3544,12 @@ int btrfs_write_out_ino_cache(struct btrfs_root *root, > struct btrfs_fs_info *fs_info = root->fs_info; > struct btrfs_free_space_ctl *ctl = root->free_ino_ctl; > int ret; > - struct btrfs_io_ctl io_ctl; > + struct btrfs_io_ctl io_ctl = {0}; Doesn't this zero the bytes unconditionally? The memset below happens only when the inode cache is on. > bool release_metadata = true; > > if (!btrfs_test_opt(fs_info, INODE_MAP_CACHE)) > return 0; > > - memset(&io_ctl, 0, sizeof(io_ctl)); > ret = __btrfs_write_out_cache(root, inode, ctl, NULL, &io_ctl, trans); > if (!ret) { > /* > -- > 2.16.4
On 16:47 27/02, David Sterba wrote: > On Mon, Feb 25, 2019 at 01:07:42PM -0600, Goldwyn Rodrigues wrote: > > From: Goldwyn Rodrigues <rgoldwyn@suse.com> > > > > io_ctl_init() memsets it to zero anyways. However, I presume the > > memset was added to avoid the WARN_ON in io_ctl_init(). > > I don't see any WARN_ON in io_ctl_init, you probably mean > __btrfs_write_out_cache. Yes, __btrfs_write_out_cache. > > > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > > --- > > fs/btrfs/free-space-cache.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c > > index 74aa552f4793..c813378ebf08 100644 > > --- a/fs/btrfs/free-space-cache.c > > +++ b/fs/btrfs/free-space-cache.c > > @@ -3544,13 +3544,12 @@ int btrfs_write_out_ino_cache(struct btrfs_root *root, > > struct btrfs_fs_info *fs_info = root->fs_info; > > struct btrfs_free_space_ctl *ctl = root->free_ino_ctl; > > int ret; > > - struct btrfs_io_ctl io_ctl; > > + struct btrfs_io_ctl io_ctl = {0}; > > Doesn't this zero the bytes unconditionally? The memset below happens > only when the inode cache is on. Yes, but does it matter. I assumed assignment is faster than memset, but a google search says gcc optimization is smart enough now. So, this patch seems irrelevant.
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c index 74aa552f4793..c813378ebf08 100644 --- a/fs/btrfs/free-space-cache.c +++ b/fs/btrfs/free-space-cache.c @@ -3544,13 +3544,12 @@ int btrfs_write_out_ino_cache(struct btrfs_root *root, struct btrfs_fs_info *fs_info = root->fs_info; struct btrfs_free_space_ctl *ctl = root->free_ino_ctl; int ret; - struct btrfs_io_ctl io_ctl; + struct btrfs_io_ctl io_ctl = {0}; bool release_metadata = true; if (!btrfs_test_opt(fs_info, INODE_MAP_CACHE)) return 0; - memset(&io_ctl, 0, sizeof(io_ctl)); ret = __btrfs_write_out_cache(root, inode, ctl, NULL, &io_ctl, trans); if (!ret) { /*