Re: [PATCH] Btrfs: check return value of btrfs_set_extent_delalloc()

[Josef Bacik <jbacik@fusionio.com>]

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