* [PATCH] Btrfs: fix what bits we clear when erroring out from delalloc
@ 2013-07-29 17:24 Josef Bacik
2013-07-30 6:27 ` Miao Xie
0 siblings, 1 reply; 4+ messages in thread
From: Josef Bacik @ 2013-07-29 17:24 UTC (permalink / raw)
To: linux-btrfs
First of all we no longer set EXTENT_DIRTY when we dirty an extent so this patch
removes the clearing of EXTENT_DIRTY since it confuses me. This patch also adds
clearing EXTENT_DEFRAG and also doing EXTENT_DO_ACCOUNTING when we have errors.
This is because if we are clearing delalloc without adding an ordered extent
then we need to make sure the enospc handling stuff is accounted for. Also if
this range was DEFRAG we need to make sure that bit is cleared so we dont leak
it. Thanks,
Signed-off-by: Josef Bacik <jbacik@fusionio.com>
---
fs/btrfs/inode.c | 46 ++++++++++++++++++++++++++--------------------
1 files changed, 26 insertions(+), 20 deletions(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index a60be02..686ba5c 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -490,8 +490,9 @@ cont:
* Unlock and free up our temp pages.
*/
extent_clear_unlock_delalloc(inode, start, end, NULL,
- EXTENT_DIRTY |
- EXTENT_DELALLOC,
+ EXTENT_DELALLOC |
+ EXTENT_DO_ACCOUNTING |
+ EXTENT_DEFRAG,
PAGE_UNLOCK |
PAGE_CLEAR_DIRTY |
PAGE_SET_WRITEBACK |
@@ -593,9 +594,10 @@ free_pages_out:
cleanup_and_out:
extent_clear_unlock_delalloc(inode, start, end, NULL,
- EXTENT_DIRTY | EXTENT_DELALLOC,
- PAGE_UNLOCK | PAGE_CLEAR_DIRTY |
- PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK);
+ EXTENT_DELALLOC | EXTENT_DO_ACCOUNTING |
+ EXTENT_DEFRAG, PAGE_UNLOCK |
+ PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK |
+ PAGE_END_WRITEBACK);
if (!trans || IS_ERR(trans))
btrfs_error(root->fs_info, ret, "Failed to join transaction");
else
@@ -770,8 +772,8 @@ retry:
extent_clear_unlock_delalloc(inode, async_extent->start,
async_extent->start +
async_extent->ram_size - 1,
- NULL, EXTENT_LOCKED | EXTENT_DELALLOC |
- EXTENT_DIRTY, PAGE_UNLOCK | PAGE_CLEAR_DIRTY |
+ NULL, EXTENT_LOCKED | EXTENT_DELALLOC,
+ PAGE_UNLOCK | PAGE_CLEAR_DIRTY |
PAGE_SET_WRITEBACK);
ret = btrfs_submit_compressed_write(inode,
async_extent->start,
@@ -795,9 +797,9 @@ out_free:
async_extent->start +
async_extent->ram_size - 1,
NULL, EXTENT_LOCKED | EXTENT_DELALLOC |
- EXTENT_DIRTY, PAGE_UNLOCK |
- PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK |
- PAGE_END_WRITEBACK);
+ EXTENT_DEFRAG | EXTENT_DO_ACCOUNTING,
+ PAGE_UNLOCK | PAGE_CLEAR_DIRTY |
+ PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK);
kfree(async_extent);
goto again;
}
@@ -884,7 +886,7 @@ static noinline int __cow_file_range(struct btrfs_trans_handle *trans,
if (ret == 0) {
extent_clear_unlock_delalloc(inode, start, end, NULL,
EXTENT_LOCKED | EXTENT_DELALLOC |
- EXTENT_DIRTY, PAGE_UNLOCK |
+ EXTENT_DEFRAG, PAGE_UNLOCK |
PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK |
PAGE_END_WRITEBACK);
@@ -995,10 +997,10 @@ out_reserve:
btrfs_free_reserved_extent(root, ins.objectid, ins.offset);
out_unlock:
extent_clear_unlock_delalloc(inode, start, end, locked_page,
- EXTENT_LOCKED | EXTENT_DIRTY |
- EXTENT_DELALLOC, PAGE_UNLOCK |
- PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK |
- PAGE_END_WRITEBACK);
+ EXTENT_LOCKED | EXTENT_DO_ACCOUNTING |
+ EXTENT_DELALLOC | EXTENT_DEFRAG,
+ PAGE_UNLOCK | PAGE_CLEAR_DIRTY |
+ PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK);
goto out;
}
@@ -1016,7 +1018,8 @@ static noinline int cow_file_range(struct inode *inode,
if (IS_ERR(trans)) {
extent_clear_unlock_delalloc(inode, start, end, locked_page,
EXTENT_LOCKED | EXTENT_DELALLOC |
- EXTENT_DIRTY, PAGE_UNLOCK |
+ EXTENT_DO_ACCOUNTING |
+ EXTENT_DEFRAG, PAGE_UNLOCK |
PAGE_CLEAR_DIRTY |
PAGE_SET_WRITEBACK |
PAGE_END_WRITEBACK);
@@ -1201,7 +1204,8 @@ static noinline int run_delalloc_nocow(struct inode *inode,
if (!path) {
extent_clear_unlock_delalloc(inode, start, end, locked_page,
EXTENT_LOCKED | EXTENT_DELALLOC |
- EXTENT_DIRTY, PAGE_UNLOCK |
+ EXTENT_DO_ACCOUNTING |
+ EXTENT_DEFRAG, PAGE_UNLOCK |
PAGE_CLEAR_DIRTY |
PAGE_SET_WRITEBACK |
PAGE_END_WRITEBACK);
@@ -1218,7 +1222,8 @@ static noinline int run_delalloc_nocow(struct inode *inode,
if (IS_ERR(trans)) {
extent_clear_unlock_delalloc(inode, start, end, locked_page,
EXTENT_LOCKED | EXTENT_DELALLOC |
- EXTENT_DIRTY, PAGE_UNLOCK |
+ EXTENT_DO_ACCOUNTING |
+ EXTENT_DEFRAG, PAGE_UNLOCK |
PAGE_CLEAR_DIRTY |
PAGE_SET_WRITEBACK |
PAGE_END_WRITEBACK);
@@ -1434,8 +1439,9 @@ error:
if (ret && cur_offset < end)
extent_clear_unlock_delalloc(inode, cur_offset, end,
locked_page, EXTENT_LOCKED |
- EXTENT_DELALLOC | EXTENT_DIRTY,
- PAGE_UNLOCK | PAGE_CLEAR_DIRTY |
+ EXTENT_DELALLOC | EXTENT_DEFRAG |
+ EXTENT_DO_ACCOUNTING, PAGE_UNLOCK |
+ PAGE_CLEAR_DIRTY |
PAGE_SET_WRITEBACK |
PAGE_END_WRITEBACK);
btrfs_free_path(path);
--
1.7.7.6
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Btrfs: fix what bits we clear when erroring out from delalloc
2013-07-29 17:24 [PATCH] Btrfs: fix what bits we clear when erroring out from delalloc Josef Bacik
@ 2013-07-30 6:27 ` Miao Xie
2013-08-02 10:08 ` Miao Xie
0 siblings, 1 reply; 4+ messages in thread
From: Miao Xie @ 2013-07-30 6:27 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs
On mon, 29 Jul 2013 13:24:22 -0400, Josef Bacik wrote:
> First of all we no longer set EXTENT_DIRTY when we dirty an extent so this patch
> removes the clearing of EXTENT_DIRTY since it confuses me. This patch also adds
> clearing EXTENT_DEFRAG and also doing EXTENT_DO_ACCOUNTING when we have errors.
> This is because if we are clearing delalloc without adding an ordered extent
> then we need to make sure the enospc handling stuff is accounted for. Also if
> this range was DEFRAG we need to make sure that bit is cleared so we dont leak
> it. Thanks,
>
> Signed-off-by: Josef Bacik <jbacik@fusionio.com>
> ---
> fs/btrfs/inode.c | 46 ++++++++++++++++++++++++++--------------------
> 1 files changed, 26 insertions(+), 20 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index a60be02..686ba5c 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -490,8 +490,9 @@ cont:
> * Unlock and free up our temp pages.
> */
> extent_clear_unlock_delalloc(inode, start, end, NULL,
> - EXTENT_DIRTY |
> - EXTENT_DELALLOC,
> + EXTENT_DELALLOC |
> + EXTENT_DO_ACCOUNTING |
> + EXTENT_DEFRAG,
> PAGE_UNLOCK |
> PAGE_CLEAR_DIRTY |
> PAGE_SET_WRITEBACK |
I found we released the reserved space in cow_file_range_inline(), but at that time,
we didn't drop the outstanding_extents counter by the number of the delalloc extents,
so it might leave some reserved space which was not released. So I think we should remove
the release function in cow_file_range_inline().
(This bug is not introduced by your patch. but if we don't fix the above problem before
applying your patch, the double release would happen because we have released the space
in cow_file_range_inline())
> @@ -593,9 +594,10 @@ free_pages_out:
>
> cleanup_and_out:
> extent_clear_unlock_delalloc(inode, start, end, NULL,
> - EXTENT_DIRTY | EXTENT_DELALLOC,
> - PAGE_UNLOCK | PAGE_CLEAR_DIRTY |
> - PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK);
> + EXTENT_DELALLOC | EXTENT_DO_ACCOUNTING |
> + EXTENT_DEFRAG, PAGE_UNLOCK |
> + PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK |
> + PAGE_END_WRITEBACK);
> if (!trans || IS_ERR(trans))
> btrfs_error(root->fs_info, ret, "Failed to join transaction");
> else
> @@ -770,8 +772,8 @@ retry:
> extent_clear_unlock_delalloc(inode, async_extent->start,
> async_extent->start +
> async_extent->ram_size - 1,
> - NULL, EXTENT_LOCKED | EXTENT_DELALLOC |
> - EXTENT_DIRTY, PAGE_UNLOCK | PAGE_CLEAR_DIRTY |
> + NULL, EXTENT_LOCKED | EXTENT_DELALLOC,
> + PAGE_UNLOCK | PAGE_CLEAR_DIRTY |
> PAGE_SET_WRITEBACK);
> ret = btrfs_submit_compressed_write(inode,
> async_extent->start,
> @@ -795,9 +797,9 @@ out_free:
> async_extent->start +
> async_extent->ram_size - 1,
> NULL, EXTENT_LOCKED | EXTENT_DELALLOC |
> - EXTENT_DIRTY, PAGE_UNLOCK |
> - PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK |
> - PAGE_END_WRITEBACK);
> + EXTENT_DEFRAG | EXTENT_DO_ACCOUNTING,
> + PAGE_UNLOCK | PAGE_CLEAR_DIRTY |
> + PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK);
> kfree(async_extent);
> goto again;
> }
> @@ -884,7 +886,7 @@ static noinline int __cow_file_range(struct btrfs_trans_handle *trans,
> if (ret == 0) {
> extent_clear_unlock_delalloc(inode, start, end, NULL,
> EXTENT_LOCKED | EXTENT_DELALLOC |
> - EXTENT_DIRTY, PAGE_UNLOCK |
> + EXTENT_DEFRAG, PAGE_UNLOCK |
> PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK |
> PAGE_END_WRITEBACK);
If we remove the reserved space release function in cow_file_range_inline() as I said above,
we should add EXTENT_DO_ACCOUNTING here.
I will send a patch to fix the problem I said above soon, please wait a moment
The other code is OK.
Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Btrfs: fix what bits we clear when erroring out from delalloc
2013-07-30 6:27 ` Miao Xie
@ 2013-08-02 10:08 ` Miao Xie
2013-08-02 23:29 ` Josef Bacik
0 siblings, 1 reply; 4+ messages in thread
From: Miao Xie @ 2013-08-02 10:08 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs
Hi, Josef
On Tue, 30 Jul 2013 14:27:40 +0800, Miao Xie wrote:
>> extent_clear_unlock_delalloc(inode, start, end, NULL,
>> - EXTENT_DIRTY |
>> - EXTENT_DELALLOC,
>> + EXTENT_DELALLOC |
>> + EXTENT_DO_ACCOUNTING |
>> + EXTENT_DEFRAG,
>> PAGE_UNLOCK |
>> PAGE_CLEAR_DIRTY |
>> PAGE_SET_WRITEBACK |
>
> I found we released the reserved space in cow_file_range_inline(), but at that time,
> we didn't drop the outstanding_extents counter by the number of the delalloc extents,
> so it might leave some reserved space which was not released. So I think we should remove
> the release function in cow_file_range_inline().
>
> (This bug is not introduced by your patch. but if we don't fix the above problem before
> applying your patch, the double release would happen because we have released the space
> in cow_file_range_inline())
I'm sorry that I made a mistake, the problem I said above doesn't exist because the block
size is equal to the page size, and we can only inline the file extent which is <= the page size,
so it is unlikely that one inline extent has two extent state objects.
But the double release of the reserved space exists actually. We can fix by removing the release
function in cow_file_range_inline() and passing EXTENT_DO_ACCOUNTING to the clear function.
Thanks
Miao
>> @@ -593,9 +594,10 @@ free_pages_out:
>>
>> cleanup_and_out:
>> extent_clear_unlock_delalloc(inode, start, end, NULL,
>> - EXTENT_DIRTY | EXTENT_DELALLOC,
>> - PAGE_UNLOCK | PAGE_CLEAR_DIRTY |
>> - PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK);
>> + EXTENT_DELALLOC | EXTENT_DO_ACCOUNTING |
>> + EXTENT_DEFRAG, PAGE_UNLOCK |
>> + PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK |
>> + PAGE_END_WRITEBACK);
>> if (!trans || IS_ERR(trans))
>> btrfs_error(root->fs_info, ret, "Failed to join transaction");
>> else
>> @@ -770,8 +772,8 @@ retry:
>> extent_clear_unlock_delalloc(inode, async_extent->start,
>> async_extent->start +
>> async_extent->ram_size - 1,
>> - NULL, EXTENT_LOCKED | EXTENT_DELALLOC |
>> - EXTENT_DIRTY, PAGE_UNLOCK | PAGE_CLEAR_DIRTY |
>> + NULL, EXTENT_LOCKED | EXTENT_DELALLOC,
>> + PAGE_UNLOCK | PAGE_CLEAR_DIRTY |
>> PAGE_SET_WRITEBACK);
>> ret = btrfs_submit_compressed_write(inode,
>> async_extent->start,
>> @@ -795,9 +797,9 @@ out_free:
>> async_extent->start +
>> async_extent->ram_size - 1,
>> NULL, EXTENT_LOCKED | EXTENT_DELALLOC |
>> - EXTENT_DIRTY, PAGE_UNLOCK |
>> - PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK |
>> - PAGE_END_WRITEBACK);
>> + EXTENT_DEFRAG | EXTENT_DO_ACCOUNTING,
>> + PAGE_UNLOCK | PAGE_CLEAR_DIRTY |
>> + PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK);
>> kfree(async_extent);
>> goto again;
>> }
>> @@ -884,7 +886,7 @@ static noinline int __cow_file_range(struct btrfs_trans_handle *trans,
>> if (ret == 0) {
>> extent_clear_unlock_delalloc(inode, start, end, NULL,
>> EXTENT_LOCKED | EXTENT_DELALLOC |
>> - EXTENT_DIRTY, PAGE_UNLOCK |
>> + EXTENT_DEFRAG, PAGE_UNLOCK |
>> PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK |
>> PAGE_END_WRITEBACK);
>
> If we remove the reserved space release function in cow_file_range_inline() as I said above,
> we should add EXTENT_DO_ACCOUNTING here.
>
> I will send a patch to fix the problem I said above soon, please wait a moment
>
> The other code is OK.
>
> Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Btrfs: fix what bits we clear when erroring out from delalloc
2013-08-02 10:08 ` Miao Xie
@ 2013-08-02 23:29 ` Josef Bacik
0 siblings, 0 replies; 4+ messages in thread
From: Josef Bacik @ 2013-08-02 23:29 UTC (permalink / raw)
To: Miao Xie; +Cc: Josef Bacik, linux-btrfs
On Fri, Aug 02, 2013 at 06:08:01PM +0800, Miao Xie wrote:
> Hi, Josef
>
> On Tue, 30 Jul 2013 14:27:40 +0800, Miao Xie wrote:
> >> extent_clear_unlock_delalloc(inode, start, end, NULL,
> >> - EXTENT_DIRTY |
> >> - EXTENT_DELALLOC,
> >> + EXTENT_DELALLOC |
> >> + EXTENT_DO_ACCOUNTING |
> >> + EXTENT_DEFRAG,
> >> PAGE_UNLOCK |
> >> PAGE_CLEAR_DIRTY |
> >> PAGE_SET_WRITEBACK |
> >
> > I found we released the reserved space in cow_file_range_inline(), but at that time,
> > we didn't drop the outstanding_extents counter by the number of the delalloc extents,
> > so it might leave some reserved space which was not released. So I think we should remove
> > the release function in cow_file_range_inline().
> >
> > (This bug is not introduced by your patch. but if we don't fix the above problem before
> > applying your patch, the double release would happen because we have released the space
> > in cow_file_range_inline())
>
> I'm sorry that I made a mistake, the problem I said above doesn't exist because the block
> size is equal to the page size, and we can only inline the file extent which is <= the page size,
> so it is unlikely that one inline extent has two extent state objects.
>
> But the double release of the reserved space exists actually. We can fix by removing the release
> function in cow_file_range_inline() and passing EXTENT_DO_ACCOUNTING to the clear function.
>
Sorry I meant to send a v2, but what I've committed isn't actually this. We
need DO_ACCOUNTING if ret < 0 but not if ret == 0, so I've changed this to take
that into account since cow_file_range_inline does do the accounting as you say.
Thanks,
Josef
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-08-02 23:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-29 17:24 [PATCH] Btrfs: fix what bits we clear when erroring out from delalloc Josef Bacik
2013-07-30 6:27 ` Miao Xie
2013-08-02 10:08 ` Miao Xie
2013-08-02 23:29 ` Josef Bacik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).