| Message ID | 20180625134532.3889459-1-clm@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 25, 2018 at 06:45:32AM -0700, Chris Mason wrote: > The vm_fault_t conversion commit introduced a ret2 variable for > tracking the integer return values from internal btrfs functions. It > was sometimes returning VM_FAULT_LOCKED for pages that were actually > invalid and had been removed from the radix. Something like this: > > ret2 = btrfs_delalloc_reserve_space() // returns zero on success > > lock_page(page) > if (page->mapping != inode->i_mapping) > goto out_unlock; > > ... > > out_unlock: > if (!ret2) { > ... > return VM_FAULT_LOCKED; > } > > This ends up triggering this WARNING in btrfs_destroy_inode() > WARN_ON(BTRFS_I(inode)->block_rsv.size); > > The fix used here is to use ret instead of ret2 in our check for success. > > Fixes: a528a2415087 (btrfs: change return type of btrfs_page_mkwrite to vm_fault_t) > Signed-off-by: Chris Mason <clm@fb.com> > --- > fs/btrfs/inode.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 193f933..38403aa 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -9013,6 +9013,7 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf) > goto out_unlock; > } > ret2 = 0; > + ret = 0; That's something I'd rather avoid, now there are two error values that need to be tracked. Shouldn't the 'ret2 = 0' be removed completely? > > /* page is wholly or partially inside EOF */ > if (page_start + PAGE_SIZE > size) > @@ -9037,7 +9038,7 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf) > unlock_extent_cached(io_tree, page_start, page_end, &cached_state); > > out_unlock: > - if (!ret2) { > + if (!ret) { I'm not sure is safe, the new ret is one of the VM_FAULT_ values and it's not 0 by default and in fact I don't see anything that would set it to 0 directly or indirectly. Which means that the code in the out: label also needs to be revisited: 9016 out: 9017 btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, (ret != 0)); 9018 btrfs_delalloc_release_space(inode, data_reserved, page_start, 9019 reserved_space, (ret != 0)); > btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, true); > sb_end_pagefault(inode->i_sb); > extent_changeset_free(data_reserved); -- 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
On 25 Jun 2018, at 9:54, David Sterba wrote: > On Mon, Jun 25, 2018 at 06:45:32AM -0700, Chris Mason wrote: >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c >> index 193f933..38403aa 100644 >> --- a/fs/btrfs/inode.c >> +++ b/fs/btrfs/inode.c >> @@ -9013,6 +9013,7 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf) >> goto out_unlock; >> } >> ret2 = 0; >> + ret = 0; > > That's something I'd rather avoid, now there are two error values that > need to be tracked. Shouldn't the 'ret2 = 0' be removed completely? Yeah, the whole thing hurts my eyes. Fixing. > >> >> /* page is wholly or partially inside EOF */ >> if (page_start + PAGE_SIZE > size) >> @@ -9037,7 +9038,7 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf) >> unlock_extent_cached(io_tree, page_start, page_end, &cached_state); >> >> out_unlock: >> - if (!ret2) { >> + if (!ret) { > > I'm not sure is safe, the new ret is one of the VM_FAULT_ values and > it's not 0 by default and in fact I don't see anything that would set it > to 0 directly or indirectly. Which means that the code in the out: label > also needs to be revisited: Good point, ok. -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
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 193f933..38403aa 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -9013,6 +9013,7 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf) goto out_unlock; } ret2 = 0; + ret = 0; /* page is wholly or partially inside EOF */ if (page_start + PAGE_SIZE > size) @@ -9037,7 +9038,7 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf) unlock_extent_cached(io_tree, page_start, page_end, &cached_state); out_unlock: - if (!ret2) { + if (!ret) { btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, true); sb_end_pagefault(inode->i_sb); extent_changeset_free(data_reserved);
The vm_fault_t conversion commit introduced a ret2 variable for tracking the integer return values from internal btrfs functions. It was sometimes returning VM_FAULT_LOCKED for pages that were actually invalid and had been removed from the radix. Something like this: ret2 = btrfs_delalloc_reserve_space() // returns zero on success lock_page(page) if (page->mapping != inode->i_mapping) goto out_unlock; ... out_unlock: if (!ret2) { ... return VM_FAULT_LOCKED; } This ends up triggering this WARNING in btrfs_destroy_inode() WARN_ON(BTRFS_I(inode)->block_rsv.size); The fix used here is to use ret instead of ret2 in our check for success. Fixes: a528a2415087 (btrfs: change return type of btrfs_page_mkwrite to vm_fault_t) Signed-off-by: Chris Mason <clm@fb.com> --- fs/btrfs/inode.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)