Commit 660d3f6c authored by Josef Bacik's avatar Josef Bacik

Btrfs: fix how we do delalloc reservations and how we free reservations on error

Running xfstests 269 with some tracing my scripts kept spitting out errors about
releasing bytes that we didn't actually have reserved.  This took me down a huge
rabbit hole and it turns out the way we deal with reserved_extents is wrong,
we need to only be setting it if the reservation succeeds, otherwise the free()
method will come in and unreserve space that isn't actually reserved yet, which
can lead to other warnings and such.  The math was all working out right in the
end, but it caused all sorts of other issues in addition to making my scripts
yell and scream and generally make it impossible for me to track down the
original issue I was looking for.  The other problem is with our error handling
in the reservation code.  There are two cases that we need to deal with

1) We raced with free.  In this case free won't free anything because csum_bytes
is modified before we dro the lock in our reservation path, so free rightly
doesn't release any space because the reservation code may be depending on that
reservation.  However if we fail, we need the reservation side to do the free at
that point since that space is no longer in use.  So as it stands the code was
doing this fine and it worked out, except in case #2

2) We don't race with free.  Nobody comes in and changes anything, and our
reservation fails.  In this case we didn't reserve anything anyway and we just
need to clean up csum_bytes but not free anything.  So we keep track of
csum_bytes before we drop the lock and if it hasn't changed we know we can just
decrement csum_bytes and carry on.

Because of the case where we can race with free()'s since we have to drop our
spin_lock to do the reservation, I'm going to serialize all reservations with
the i_mutex.  We already get this for free in the heavy use paths, truncate and
file write all hold the i_mutex, just needed to add it to page_mkwrite and
various ioctl/balance things.  With this patch my space leak scripts no longer
scream bloody murder.  Thanks,
Signed-off-by: default avatarJosef Bacik <josef@redhat.com>
parent 22c44fe6
...@@ -4203,12 +4203,17 @@ int btrfs_delalloc_reserve_metadata(struct inode *inode, u64 num_bytes) ...@@ -4203,12 +4203,17 @@ int btrfs_delalloc_reserve_metadata(struct inode *inode, u64 num_bytes)
struct btrfs_root *root = BTRFS_I(inode)->root; struct btrfs_root *root = BTRFS_I(inode)->root;
struct btrfs_block_rsv *block_rsv = &root->fs_info->delalloc_block_rsv; struct btrfs_block_rsv *block_rsv = &root->fs_info->delalloc_block_rsv;
u64 to_reserve = 0; u64 to_reserve = 0;
u64 csum_bytes;
unsigned nr_extents = 0; unsigned nr_extents = 0;
int extra_reserve = 0;
int flush = 1; int flush = 1;
int ret; int ret;
/* Need to be holding the i_mutex here if we aren't free space cache */
if (btrfs_is_free_space_inode(root, inode)) if (btrfs_is_free_space_inode(root, inode))
flush = 0; flush = 0;
else
WARN_ON(!mutex_is_locked(&inode->i_mutex));
if (flush && btrfs_transaction_in_commit(root->fs_info)) if (flush && btrfs_transaction_in_commit(root->fs_info))
schedule_timeout(1); schedule_timeout(1);
...@@ -4219,11 +4224,9 @@ int btrfs_delalloc_reserve_metadata(struct inode *inode, u64 num_bytes) ...@@ -4219,11 +4224,9 @@ int btrfs_delalloc_reserve_metadata(struct inode *inode, u64 num_bytes)
BTRFS_I(inode)->outstanding_extents++; BTRFS_I(inode)->outstanding_extents++;
if (BTRFS_I(inode)->outstanding_extents > if (BTRFS_I(inode)->outstanding_extents >
BTRFS_I(inode)->reserved_extents) { BTRFS_I(inode)->reserved_extents)
nr_extents = BTRFS_I(inode)->outstanding_extents - nr_extents = BTRFS_I(inode)->outstanding_extents -
BTRFS_I(inode)->reserved_extents; BTRFS_I(inode)->reserved_extents;
BTRFS_I(inode)->reserved_extents += nr_extents;
}
/* /*
* Add an item to reserve for updating the inode when we complete the * Add an item to reserve for updating the inode when we complete the
...@@ -4231,11 +4234,12 @@ int btrfs_delalloc_reserve_metadata(struct inode *inode, u64 num_bytes) ...@@ -4231,11 +4234,12 @@ int btrfs_delalloc_reserve_metadata(struct inode *inode, u64 num_bytes)
*/ */
if (!BTRFS_I(inode)->delalloc_meta_reserved) { if (!BTRFS_I(inode)->delalloc_meta_reserved) {
nr_extents++; nr_extents++;
BTRFS_I(inode)->delalloc_meta_reserved = 1; extra_reserve = 1;
} }
to_reserve = btrfs_calc_trans_metadata_size(root, nr_extents); to_reserve = btrfs_calc_trans_metadata_size(root, nr_extents);
to_reserve += calc_csum_metadata_size(inode, num_bytes, 1); to_reserve += calc_csum_metadata_size(inode, num_bytes, 1);
csum_bytes = BTRFS_I(inode)->csum_bytes;
spin_unlock(&BTRFS_I(inode)->lock); spin_unlock(&BTRFS_I(inode)->lock);
ret = reserve_metadata_bytes(root, block_rsv, to_reserve, flush); ret = reserve_metadata_bytes(root, block_rsv, to_reserve, flush);
...@@ -4245,22 +4249,35 @@ int btrfs_delalloc_reserve_metadata(struct inode *inode, u64 num_bytes) ...@@ -4245,22 +4249,35 @@ int btrfs_delalloc_reserve_metadata(struct inode *inode, u64 num_bytes)
spin_lock(&BTRFS_I(inode)->lock); spin_lock(&BTRFS_I(inode)->lock);
dropped = drop_outstanding_extent(inode); dropped = drop_outstanding_extent(inode);
to_free = calc_csum_metadata_size(inode, num_bytes, 0);
spin_unlock(&BTRFS_I(inode)->lock);
to_free += btrfs_calc_trans_metadata_size(root, dropped);
/* /*
* Somebody could have come in and twiddled with the * If the inodes csum_bytes is the same as the original
* reservation, so if we have to free more than we would have * csum_bytes then we know we haven't raced with any free()ers
* reserved from this reservation go ahead and release those * so we can just reduce our inodes csum bytes and carry on.
* bytes. * Otherwise we have to do the normal free thing to account for
* the case that the free side didn't free up its reserve
* because of this outstanding reservation.
*/ */
to_free -= to_reserve; if (BTRFS_I(inode)->csum_bytes == csum_bytes)
calc_csum_metadata_size(inode, num_bytes, 0);
else
to_free = calc_csum_metadata_size(inode, num_bytes, 0);
spin_unlock(&BTRFS_I(inode)->lock);
if (dropped)
to_free += btrfs_calc_trans_metadata_size(root, dropped);
if (to_free) if (to_free)
btrfs_block_rsv_release(root, block_rsv, to_free); btrfs_block_rsv_release(root, block_rsv, to_free);
return ret; return ret;
} }
spin_lock(&BTRFS_I(inode)->lock);
if (extra_reserve) {
BTRFS_I(inode)->delalloc_meta_reserved = 1;
nr_extents--;
}
BTRFS_I(inode)->reserved_extents += nr_extents;
spin_unlock(&BTRFS_I(inode)->lock);
block_rsv_add_bytes(block_rsv, to_reserve, 1); block_rsv_add_bytes(block_rsv, to_reserve, 1);
return 0; return 0;
......
...@@ -2192,7 +2192,14 @@ int btrfs_orphan_cleanup(struct btrfs_root *root) ...@@ -2192,7 +2192,14 @@ int btrfs_orphan_cleanup(struct btrfs_root *root)
continue; continue;
} }
nr_truncate++; nr_truncate++;
/*
* Need to hold the imutex for reservation purposes, not
* a huge deal here but I have a WARN_ON in
* btrfs_delalloc_reserve_space to catch offenders.
*/
mutex_lock(&inode->i_mutex);
ret = btrfs_truncate(inode); ret = btrfs_truncate(inode);
mutex_unlock(&inode->i_mutex);
} else { } else {
nr_unlink++; nr_unlink++;
} }
...@@ -6342,7 +6349,10 @@ int btrfs_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf) ...@@ -6342,7 +6349,10 @@ int btrfs_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
u64 page_start; u64 page_start;
u64 page_end; u64 page_end;
/* Need this to keep space reservations serialized */
mutex_lock(&inode->i_mutex);
ret = btrfs_delalloc_reserve_space(inode, PAGE_CACHE_SIZE); ret = btrfs_delalloc_reserve_space(inode, PAGE_CACHE_SIZE);
mutex_unlock(&inode->i_mutex);
if (!ret) if (!ret)
ret = btrfs_update_time(vma->vm_file); ret = btrfs_update_time(vma->vm_file);
if (ret) { if (ret) {
......
...@@ -858,8 +858,10 @@ static int cluster_pages_for_defrag(struct inode *inode, ...@@ -858,8 +858,10 @@ static int cluster_pages_for_defrag(struct inode *inode,
return 0; return 0;
file_end = (isize - 1) >> PAGE_CACHE_SHIFT; file_end = (isize - 1) >> PAGE_CACHE_SHIFT;
mutex_lock(&inode->i_mutex);
ret = btrfs_delalloc_reserve_space(inode, ret = btrfs_delalloc_reserve_space(inode,
num_pages << PAGE_CACHE_SHIFT); num_pages << PAGE_CACHE_SHIFT);
mutex_unlock(&inode->i_mutex);
if (ret) if (ret)
return ret; return ret;
again: again:
......
...@@ -2947,7 +2947,9 @@ static int relocate_file_extent_cluster(struct inode *inode, ...@@ -2947,7 +2947,9 @@ static int relocate_file_extent_cluster(struct inode *inode,
index = (cluster->start - offset) >> PAGE_CACHE_SHIFT; index = (cluster->start - offset) >> PAGE_CACHE_SHIFT;
last_index = (cluster->end - offset) >> PAGE_CACHE_SHIFT; last_index = (cluster->end - offset) >> PAGE_CACHE_SHIFT;
while (index <= last_index) { while (index <= last_index) {
mutex_lock(&inode->i_mutex);
ret = btrfs_delalloc_reserve_metadata(inode, PAGE_CACHE_SIZE); ret = btrfs_delalloc_reserve_metadata(inode, PAGE_CACHE_SIZE);
mutex_unlock(&inode->i_mutex);
if (ret) if (ret)
goto out; goto out;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment