| Message ID | 1421223534-11799-1-git-send-email-guihc.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
On Wed, Jan 14, 2015 at 04:18:54PM +0800, Gui Hecheng wrote: > Move the branch that is unrelated to the result of io_ctl_init() before > the function call, so we can save a kmalloc() & kfree() pair in that > branch. > > Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com> > --- > fs/btrfs/free-space-cache.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c > index d6c03f7..88f6122 100644 > --- a/fs/btrfs/free-space-cache.c > +++ b/fs/btrfs/free-space-cache.c > @@ -1132,10 +1132,6 @@ static int __btrfs_write_out_cache(struct btrfs_root *root, struct inode *inode, > if (!i_size_read(inode)) > return -1; > > - ret = io_ctl_init(&io_ctl, inode, root, 1); > - if (ret) > - return -1; I'm not sure this preserves the original semantics. This can fail if there's no memory, fine, but also ENOSPC if the "crcs do not fit into the first page" as the comment in io_ctl_init() says. There's an additional condition that the inode is not FREE_INO, ie. it is the FREE_SPACE inode. So in some cases io_ctl_init may fail but would not after your patch. > - > if (block_group && (block_group->flags & BTRFS_BLOCK_GROUP_DATA)) { > down_write(&block_group->data_rwsem); > spin_lock(&block_group->lock); > @@ -1145,11 +1141,15 @@ static int __btrfs_write_out_cache(struct btrfs_root *root, struct inode *inode, > up_write(&block_group->data_rwsem); > BTRFS_I(inode)->generation = 0; > ret = 0; > - goto out; > + goto out_skip; > } > spin_unlock(&block_group->lock); > } > > + ret = io_ctl_init(&io_ctl, inode, root, 1); > + if (ret) > + return -1; This would leave block_group->data_rwsem locked, ie. another exit path would have to be added that would reflect the current state (no io_ctl initialized and the extent range not locked). We cannot use out_enospc here. I'm not sure if the kmalloc/kfree savings are significant here. > + > /* Lock all pages first so we can lock the extent safely. */ > io_ctl_prepare_pages(&io_ctl, inode, 0); > > @@ -1212,13 +1212,14 @@ static int __btrfs_write_out_cache(struct btrfs_root *root, struct inode *inode, > /* Flush the dirty pages in the cache file. */ > ret = flush_dirty_cache(inode); > if (ret) > - goto out; > + goto out_free; > > /* Update the cache item to tell everyone this cache file is valid. */ > ret = update_cache_item(trans, root, inode, path, offset, > entries, bitmaps); > -out: > +out_free: > io_ctl_free(&io_ctl); > +out_skip: > if (ret) { > invalidate_inode_pages2(inode->i_mapping); > BTRFS_I(inode)->generation = 0; > @@ -1232,7 +1233,7 @@ out_nospc: > if (block_group && (block_group->flags & BTRFS_BLOCK_GROUP_DATA)) > up_write(&block_group->data_rwsem); > > - goto out; > + goto out_free; > } -- 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
On Wed, 2015-01-14 at 16:22 +0100, David Sterba wrote: > On Wed, Jan 14, 2015 at 04:18:54PM +0800, Gui Hecheng wrote: > > Move the branch that is unrelated to the result of io_ctl_init() before > > the function call, so we can save a kmalloc() & kfree() pair in that > > branch. > > > > Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com> > > --- > > fs/btrfs/free-space-cache.c | 17 +++++++++-------- > > 1 file changed, 9 insertions(+), 8 deletions(-) > > > > diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c > > index d6c03f7..88f6122 100644 > > --- a/fs/btrfs/free-space-cache.c > > +++ b/fs/btrfs/free-space-cache.c > > @@ -1132,10 +1132,6 @@ static int __btrfs_write_out_cache(struct btrfs_root *root, struct inode *inode, > > if (!i_size_read(inode)) > > return -1; > > > > - ret = io_ctl_init(&io_ctl, inode, root, 1); > > - if (ret) > > - return -1; > > I'm not sure this preserves the original semantics. This can fail if > there's no memory, fine, but also ENOSPC if the "crcs do not fit into > the first page" as the comment in io_ctl_init() says. There's an > additional condition that the inode is not FREE_INO, ie. it is the > FREE_SPACE inode. > > So in some cases io_ctl_init may fail but would not after your patch. > > > - > > if (block_group && (block_group->flags & BTRFS_BLOCK_GROUP_DATA)) { > > down_write(&block_group->data_rwsem); > > spin_lock(&block_group->lock); > > @@ -1145,11 +1141,15 @@ static int __btrfs_write_out_cache(struct btrfs_root *root, struct inode *inode, > > up_write(&block_group->data_rwsem); > > BTRFS_I(inode)->generation = 0; > > ret = 0; > > - goto out; > > + goto out_skip; > > } > > spin_unlock(&block_group->lock); > > } > > > > + ret = io_ctl_init(&io_ctl, inode, root, 1); > > + if (ret) > > + return -1; > > This would leave block_group->data_rwsem locked, ie. another exit path > would have to be added that would reflect the current state (no io_ctl > initialized and the extent range not locked). We cannot use out_enospc > here. Yes, you're right, the ->data_rwsem shall not be left locked. > I'm not sure if the kmalloc/kfree savings are significant here. I'm not sure whether it brings much, please *ignore* this patch and I will do more checks. Thanks, Gui > > + > > /* Lock all pages first so we can lock the extent safely. */ > > io_ctl_prepare_pages(&io_ctl, inode, 0); > > > > @@ -1212,13 +1212,14 @@ static int __btrfs_write_out_cache(struct btrfs_root *root, struct inode *inode, > > /* Flush the dirty pages in the cache file. */ > > ret = flush_dirty_cache(inode); > > if (ret) > > - goto out; > > + goto out_free; > > > > /* Update the cache item to tell everyone this cache file is valid. */ > > ret = update_cache_item(trans, root, inode, path, offset, > > entries, bitmaps); > > -out: > > +out_free: > > io_ctl_free(&io_ctl); > > +out_skip: > > if (ret) { > > invalidate_inode_pages2(inode->i_mapping); > > BTRFS_I(inode)->generation = 0; > > @@ -1232,7 +1233,7 @@ out_nospc: > > if (block_group && (block_group->flags & BTRFS_BLOCK_GROUP_DATA)) > > up_write(&block_group->data_rwsem); > > > > - goto out; > > + goto out_free; > > } > -- > 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 -- 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
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c index d6c03f7..88f6122 100644 --- a/fs/btrfs/free-space-cache.c +++ b/fs/btrfs/free-space-cache.c @@ -1132,10 +1132,6 @@ static int __btrfs_write_out_cache(struct btrfs_root *root, struct inode *inode, if (!i_size_read(inode)) return -1; - ret = io_ctl_init(&io_ctl, inode, root, 1); - if (ret) - return -1; - if (block_group && (block_group->flags & BTRFS_BLOCK_GROUP_DATA)) { down_write(&block_group->data_rwsem); spin_lock(&block_group->lock); @@ -1145,11 +1141,15 @@ static int __btrfs_write_out_cache(struct btrfs_root *root, struct inode *inode, up_write(&block_group->data_rwsem); BTRFS_I(inode)->generation = 0; ret = 0; - goto out; + goto out_skip; } spin_unlock(&block_group->lock); } + ret = io_ctl_init(&io_ctl, inode, root, 1); + if (ret) + return -1; + /* Lock all pages first so we can lock the extent safely. */ io_ctl_prepare_pages(&io_ctl, inode, 0); @@ -1212,13 +1212,14 @@ static int __btrfs_write_out_cache(struct btrfs_root *root, struct inode *inode, /* Flush the dirty pages in the cache file. */ ret = flush_dirty_cache(inode); if (ret) - goto out; + goto out_free; /* Update the cache item to tell everyone this cache file is valid. */ ret = update_cache_item(trans, root, inode, path, offset, entries, bitmaps); -out: +out_free: io_ctl_free(&io_ctl); +out_skip: if (ret) { invalidate_inode_pages2(inode->i_mapping); BTRFS_I(inode)->generation = 0; @@ -1232,7 +1233,7 @@ out_nospc: if (block_group && (block_group->flags & BTRFS_BLOCK_GROUP_DATA)) up_write(&block_group->data_rwsem); - goto out; + goto out_free; } int btrfs_write_out_cache(struct btrfs_root *root,
Move the branch that is unrelated to the result of io_ctl_init() before the function call, so we can save a kmalloc() & kfree() pair in that branch. Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com> --- fs/btrfs/free-space-cache.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)