On Thu, Aug 08, 2013 at 09:36:52AM +0200, Jan Schmidt wrote: > On Wed, August 07, 2013 at 23:03 (+0200), Josef Bacik wrote: > > We can get ENOMEM trying to allocate dummy bufs for the rewind operation of > > the > > tree mod log. Instead of BUG_ON()'ing in this case pass up ENOMEM. I > > looked > > back through the callers and I'm pretty sure I got everybody who did > > BUG_ON(ret) > > in this path. Thanks, > > > > Signed-off-by: Josef Bacik <jba...@fusionio.com> > > --- > > V2->V3: > > -unlock and free the original buffer on error > > -return NULL instead of ERR_PTR(-ENOMEM) > > V1->V2: missed a BUG_ON() for alloc_dummy_extent_buffer. > > > > fs/btrfs/ctree.c | 16 +++++- > > fs/btrfs/extent_io.c | 145 > > +++++++++++++++++++++++++------------------------ > > 2 files changed, 88 insertions(+), 73 deletions(-) > > > > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c > > index 0d5c686..1dd8a71 100644 > > --- a/fs/btrfs/ctree.c > > +++ b/fs/btrfs/ctree.c > > @@ -1211,7 +1211,11 @@ tree_mod_log_rewind(struct btrfs_fs_info *fs_info, > > struct extent_buffer *eb, > > BUG_ON(tm->slot != 0); > > eb_rewin = alloc_dummy_extent_buffer(eb->start, > > fs_info->tree_root->nodesize); > > - BUG_ON(!eb_rewin); > > + if (!eb_rewin) { > > + btrfs_tree_read_unlock(eb); > > + free_extent_buffer(eb); > > + return NULL; > > + } > > btrfs_set_header_bytenr(eb_rewin, eb->start); > > btrfs_set_header_backref_rev(eb_rewin, > > btrfs_header_backref_rev(eb)); > > @@ -1219,7 +1223,11 @@ tree_mod_log_rewind(struct btrfs_fs_info *fs_info, > > struct extent_buffer *eb, > > btrfs_set_header_level(eb_rewin, btrfs_header_level(eb)); > > } else { > > eb_rewin = btrfs_clone_extent_buffer(eb); > > - BUG_ON(!eb_rewin); > > + if (!eb_rewin) { > > + btrfs_tree_read_unlock(eb); > > + free_extent_buffer(eb); > > + return NULL; > > + } > > } > > > > btrfs_tree_read_unlock(eb); > > @@ -2772,6 +2780,10 @@ again: > > BTRFS_READ_LOCK); > > } > > b = tree_mod_log_rewind(root->fs_info, b, time_seq); > > + if (!b) { > > + ret = -ENOMEM; > > + goto done; > > + } > > p->locks[level] = BTRFS_READ_LOCK; > > p->nodes[level] = b; > > } else { > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > > index deaea9c..b422cba 100644 > > --- a/fs/btrfs/extent_io.c > > +++ b/fs/btrfs/extent_io.c > > @@ -4222,6 +4222,76 @@ static void __free_extent_buffer(struct > > extent_buffer *eb) > > kmem_cache_free(extent_buffer_cache, eb); > > } > > > > +static int extent_buffer_under_io(struct extent_buffer *eb) > > +{ > > + return (atomic_read(&eb->io_pages) || > > + test_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags) || > > + test_bit(EXTENT_BUFFER_DIRTY, &eb->bflags)); > > +} > > + > > +/* > > + * Helper for releasing extent buffer page. > > + */ > > +static void btrfs_release_extent_buffer_page(struct extent_buffer *eb, > > + unsigned long start_idx) > > +{ > > + unsigned long index; > > + unsigned long num_pages; > > + struct page *page; > > + int mapped = !test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags); > > + > > + BUG_ON(extent_buffer_under_io(eb)); > > + > > + num_pages = num_extent_pages(eb->start, eb->len); > > + index = start_idx + num_pages; > > + if (start_idx >= index) > > + return; > > + > > + do { > > + index--; > > + page = extent_buffer_page(eb, index); > > + if (page && mapped) { > > + spin_lock(&page->mapping->private_lock); > > + /* > > + * We do this since we'll remove the pages after we've > > + * removed the eb from the radix tree, so we could race > > + * and have this page now attached to the new eb. So > > + * only clear page_private if it's still connected to > > + * this eb. > > + */ > > + if (PagePrivate(page) && > > + page->private == (unsigned long)eb) { > > + BUG_ON(test_bit(EXTENT_BUFFER_DIRTY, > > &eb->bflags)); > > + BUG_ON(PageDirty(page)); > > + BUG_ON(PageWriteback(page)); > > + /* > > + * We need to make sure we haven't be attached > > + * to a new eb. > > + */ > > + ClearPagePrivate(page); > > + set_page_private(page, 0); > > + /* One for the page private */ > > + page_cache_release(page); > > + } > > + spin_unlock(&page->mapping->private_lock); > > + > > + } > > + if (page) { > > + /* One for when we alloced the page */ > > + page_cache_release(page); > > + } > > + } while (index != start_idx); > > +} > > + > > +/* > > + * Helper for releasing the extent buffer. > > + */ > > +static inline void btrfs_release_extent_buffer(struct extent_buffer *eb) > > +{ > > + btrfs_release_extent_buffer_page(eb, 0); > > + __free_extent_buffer(eb); > > +} > > + > > static struct extent_buffer *__alloc_extent_buffer(struct extent_io_tree > > *tree, > > u64 start, > > unsigned long len, > > @@ -4276,7 +4346,10 @@ struct extent_buffer > > *btrfs_clone_extent_buffer(struct extent_buffer *src) > > > > for (i = 0; i < num_pages; i++) { > > p = alloc_page(GFP_ATOMIC); > > - BUG_ON(!p); > > + if (!p) { > > + btrfs_release_extent_buffer(new); > > + return NULL; > > + } > > attach_extent_buffer_page(new, p); > > WARN_ON(PageDirty(p)); > > SetPageUptodate(p); > > @@ -4317,76 +4390,6 @@ err: > > return NULL; > > } > > > > -static int extent_buffer_under_io(struct extent_buffer *eb) > > -{ > > - return (atomic_read(&eb->io_pages) || > > - test_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags) || > > - test_bit(EXTENT_BUFFER_DIRTY, &eb->bflags)); > > -} > > - > > -/* > > - * Helper for releasing extent buffer page. > > - */ > > -static void btrfs_release_extent_buffer_page(struct extent_buffer *eb, > > - unsigned long start_idx) > > -{ > > - unsigned long index; > > - unsigned long num_pages; > > - struct page *page; > > - int mapped = !test_bit(EXTENT_BUFFER_DUMMY, &eb->bflags); > > - > > - BUG_ON(extent_buffer_under_io(eb)); > > - > > - num_pages = num_extent_pages(eb->start, eb->len); > > - index = start_idx + num_pages; > > - if (start_idx >= index) > > - return; > > - > > - do { > > - index--; > > - page = extent_buffer_page(eb, index); > > - if (page && mapped) { > > - spin_lock(&page->mapping->private_lock); > > - /* > > - * We do this since we'll remove the pages after we've > > - * removed the eb from the radix tree, so we could race > > - * and have this page now attached to the new eb. So > > - * only clear page_private if it's still connected to > > - * this eb. > > - */ > > - if (PagePrivate(page) && > > - page->private == (unsigned long)eb) { > > - BUG_ON(test_bit(EXTENT_BUFFER_DIRTY, > > &eb->bflags)); > > - BUG_ON(PageDirty(page)); > > - BUG_ON(PageWriteback(page)); > > - /* > > - * We need to make sure we haven't be attached > > - * to a new eb. > > - */ > > - ClearPagePrivate(page); > > - set_page_private(page, 0); > > - /* One for the page private */ > > - page_cache_release(page); > > - } > > - spin_unlock(&page->mapping->private_lock); > > - > > - } > > - if (page) { > > - /* One for when we alloced the page */ > > - page_cache_release(page); > > - } > > - } while (index != start_idx); > > -} > > - > > -/* > > - * Helper for releasing the extent buffer. > > - */ > > -static inline void btrfs_release_extent_buffer(struct extent_buffer *eb) > > -{ > > - btrfs_release_extent_buffer_page(eb, 0); > > - __free_extent_buffer(eb); > > -} > > - > > static void check_buffer_tree_ref(struct extent_buffer *eb) > > { > > int refs; > > > > Weird patch formatting concerning extent_io.c, I assume there are no changes > in > extent_buffer_under_io and btrfs_release_extent_buffer_page, you just moved > btrfs_clone_extent_buffer, right? Perhaps --patience or --minimal could do > better? Otherwise, >
Yeah I just moved them up so I could call them, I do hate how it makes the diff look massive when there's basically no change. Thanks, Josef -- 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