| Message ID | 20180625111031.GA27958@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.linux@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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); out: btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, (ret != 0)); @@ -9021,6 +9012,12 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf) sb_end_pagefault(inode->i_sb); extent_changeset_free(data_reserved); return ret; + +out_unlock: + 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; } static int btrfs_truncate(struct inode *inode, bool skip_writeback)