* [PATCH] Btrfs: check return value of btrfs_set_extent_delalloc() @ 2012-06-26 3:22 Tsutomu Itoh 2012-06-26 13:26 ` David Sterba 0 siblings, 1 reply; 3+ messages in thread From: Tsutomu Itoh @ 2012-06-26 3:22 UTC (permalink / raw) To: chris.mason; +Cc: linux-btrfs btrfs_set_extent_delalloc() has the possibility of returning the error. So I add the code in which the return value of btrfs_set_extent_delalloc() is checked. Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com> --- fs/btrfs/ioctl.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-) diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 0e92e57..95e27d6 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -1016,13 +1016,16 @@ again: } - btrfs_set_extent_delalloc(inode, page_start, page_end - 1, - &cached_state); + ret = btrfs_set_extent_delalloc(inode, page_start, page_end - 1, + &cached_state); unlock_extent_cached(&BTRFS_I(inode)->io_tree, page_start, page_end - 1, &cached_state, GFP_NOFS); + if (ret) + goto out; + for (i = 0; i < i_done; i++) { clear_page_dirty_for_io(pages[i]); ClearPageChecked(pages[i]); ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] Btrfs: check return value of btrfs_set_extent_delalloc() 2012-06-26 3:22 [PATCH] Btrfs: check return value of btrfs_set_extent_delalloc() Tsutomu Itoh @ 2012-06-26 13:26 ` David Sterba 2012-06-26 13:42 ` Josef Bacik 0 siblings, 1 reply; 3+ messages in thread From: David Sterba @ 2012-06-26 13:26 UTC (permalink / raw) To: Tsutomu Itoh; +Cc: chris.mason, linux-btrfs, jbacik On Tue, Jun 26, 2012 at 12:22:10PM +0900, Tsutomu Itoh wrote: > btrfs_set_extent_delalloc() has the possibility of returning the error. > So I add the code in which the return value of btrfs_set_extent_delalloc() > is checked. The caller is cluster_pages_for_defrag, the only error I see returned via __set_extent_bit is -EEXIST, other errors BUG directly or via extent_io_tree_panic() . If the error happens, then the [page_start,page_end-1] is already set for delalloc, that's probably a bug and should be caught. There are two more unchecked calls to btrfs_set_extent_delalloc: inode.c:btrfs_writepage_fixup_worker 1729 btrfs_set_extent_delalloc(inode, page_start, page_end, &cached_state); 1730 ClearPageChecked(page); 1731 set_page_dirty(page); IIRC from the days full of fixup worker fun, the reason why this is safe to ignore is because the call chain leading here is exactly due to missing delalloc bits on the page. relocation.c:relocate_file_extent_cluster 3034 btrfs_set_extent_delalloc(inode, page_start, page_end, NULL); 3035 set_page_dirty(page); hmm relocation ... :) Anyway, I'd like to let Josef take another look at your patch. > > Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com> > --- > fs/btrfs/ioctl.c | 7 +++++-- > 1 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 0e92e57..95e27d6 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -1016,13 +1016,16 @@ again: > } > > > - btrfs_set_extent_delalloc(inode, page_start, page_end - 1, > - &cached_state); > + ret = btrfs_set_extent_delalloc(inode, page_start, page_end - 1, > + &cached_state); > > unlock_extent_cached(&BTRFS_I(inode)->io_tree, > page_start, page_end - 1, &cached_state, > GFP_NOFS); > > + if (ret) > + goto out; > + > for (i = 0; i < i_done; i++) { > clear_page_dirty_for_io(pages[i]); > ClearPageChecked(pages[i]); > > -- > 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 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Btrfs: check return value of btrfs_set_extent_delalloc() 2012-06-26 13:26 ` David Sterba @ 2012-06-26 13:42 ` Josef Bacik 0 siblings, 0 replies; 3+ messages in thread From: Josef Bacik @ 2012-06-26 13:42 UTC (permalink / raw) To: David Sterba Cc: Tsutomu Itoh, Chris L. Mason, linux-btrfs@vger.kernel.org, Josef Bacik On Tue, Jun 26, 2012 at 07:26:53AM -0600, David Sterba wrote: > On Tue, Jun 26, 2012 at 12:22:10PM +0900, Tsutomu Itoh wrote: > > btrfs_set_extent_delalloc() has the possibility of returning the error. > > So I add the code in which the return value of btrfs_set_extent_delalloc() > > is checked. > > The caller is cluster_pages_for_defrag, the only error I see returned > via __set_extent_bit is -EEXIST, other errors BUG directly or via > extent_io_tree_panic() . > > If the error happens, then the [page_start,page_end-1] is already set > for delalloc, that's probably a bug and should be caught. > > There are two more unchecked calls to btrfs_set_extent_delalloc: > > inode.c:btrfs_writepage_fixup_worker > > 1729 btrfs_set_extent_delalloc(inode, page_start, page_end, &cached_state); > 1730 ClearPageChecked(page); > 1731 set_page_dirty(page); > > IIRC from the days full of fixup worker fun, the reason why this is safe > to ignore is because the call chain leading here is exactly due to missing > delalloc bits on the page. > > relocation.c:relocate_file_extent_cluster > > 3034 btrfs_set_extent_delalloc(inode, page_start, page_end, NULL); > 3035 set_page_dirty(page); > > hmm relocation ... :) > > > Anyway, I'd like to let Josef take another look at your patch. > Even better, we clear delalloc right before doing the set and we have the pages locked, so theres no way we can get EEXIST here, so I'll just drop this patch. Thanks Dave, Josef ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-06-26 13:42 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-06-26 3:22 [PATCH] Btrfs: check return value of btrfs_set_extent_delalloc() Tsutomu Itoh 2012-06-26 13:26 ` David Sterba 2012-06-26 13:42 ` 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).