On Thu, Apr 30, 2015 at 05:47:05PM +0100, Filipe Manana wrote:
> If the call to btrfs_truncate_inode_items() failed and we don't have a block
> group, we were unlocking the cache_write_mutex without having locked it (we
> do it only if we have a block group).
>
> Fixes: 1bbc621ef284 ("Btrfs: allow block group cache writeout
> outside critical section in commit")
>
> Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx>
Patch is ok, I have one comment to the transaction abort calls. The
pattern is to put them right after the error is detected so we can
trace it back to the sources based on the reports.
> @@ -231,6 +231,7 @@ int btrfs_truncate_free_space_cache(struct btrfs_root *root,
> {
> int ret = 0;
> struct btrfs_path *path = btrfs_alloc_path();
> + bool locked = false;
>
> if (!path) {
> ret = -ENOMEM;
the context:
goto fail;
> @@ -269,18 +271,14 @@ int btrfs_truncate_free_space_cache(struct btrfs_root *root,
> */
> ret = btrfs_truncate_inode_items(trans, root, inode,
> 0, BTRFS_EXTENT_DATA_KEY);
> - if (ret) {
> - mutex_unlock(&trans->transaction->cache_write_mutex);
> - btrfs_abort_transaction(trans, root, ret);
> - return ret;
> - }
> + if (ret)
> + goto fail;
>
> ret = btrfs_update_inode(trans, root, inode);
>
> - if (block_group)
> - mutex_unlock(&trans->transaction->cache_write_mutex);
> -
> fail:
> + if (locked)
> + mutex_unlock(&trans->transaction->cache_write_mutex);
> if (ret)
> btrfs_abort_transaction(trans, root, ret);
Now two code paths lead to one abort. We can get ENOMEM from both,
however this kind of error is not really a bug we want to analyze deeper
so I'm fine with it.
Reviewed-by: David Sterba <dsterba@xxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
