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 <jbacik@xxxxxxxxxxxx>
> ---
> 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,
Reviewed-by: Jan Schmidt <list.xfs@xxxxxxxxxxxxx>
Thanks,
-Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
