On 25 Jun 2018, at 7:10, David Sterba wrote:
On Fri, Jun 22, 2018 at 05:25:54PM -0400, Chris Mason wrote:
The bug came here:
commit a528a24150870c5c16cbbbec69dba7e992b08456
Author: Souptick Joarder <jrdr.li...@gmail.com>
Date: Wed Jun 6 19:54:44 2018 +0530
btrfs: change return type of btrfs_page_mkwrite to vm_fault_t
When page->mapping != mapping, we improperly return success because
ret2
is zero on goto out_unlock.
I'm running a fix through a full xfstests and I'll post soon.
The ret/ret2 pattern is IMO used in the wrong way, the ret2 is some
temporary value and it should be set and tested next to each other,
not
holding the state accross the function.
The fix I'd propose is to avoid relying on it and add a separate exit
block, similar to out_noreserve:
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8981,7 +8981,6 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault
*vmf)
ret = VM_FAULT_SIGBUS;
goto out_unlock;
}
- ret2 = 0;
/* page is wholly or partially inside EOF */
if (page_start + PAGE_SIZE > size)
@@ -9004,14 +9003,6 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault
*vmf)
BTRFS_I(inode)->last_log_commit =
BTRFS_I(inode)->root->last_log_commit;
unlock_extent_cached(io_tree, page_start, page_end,
&cached_state);
-
-out_unlock:
- if (!ret2) {
- btrfs_delalloc_release_extents(BTRFS_I(inode),
PAGE_SIZE, true);
- sb_end_pagefault(inode->i_sb);
- extent_changeset_free(data_reserved);
- return VM_FAULT_LOCKED;
- }
unlock_page(page);
VM_FAULT_LOCKED is where we return success. The out_unlock goto is
confusing because it's actually only used for failure, but the goto
lands right above the if (everything actually worked) {} test.
On top of the patch I sent today, moving out_unlock: after the if would
make it more clear.
-chris
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
