Re: [PATCH] btrfs: cleanup a straight free-after-malloc branch for free-space-cache

[Gui Hecheng <guihc.fnst@cn.fujitsu.com>]

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