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@fusionio.com>
---
V1->V2: missed a BUG_ON() for alloc_dummy_extent_buffer.
fs/btrfs/ctree.c | 10 +++-
fs/btrfs/extent_io.c | 145 +++++++++++++++++++++++++------------------------
2 files changed, 82 insertions(+), 73 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 0d5c686..e56b3bb 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1211,7 +1211,8 @@ 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)
+ return ERR_PTR(-ENOMEM);
btrfs_set_header_bytenr(eb_rewin, eb->start);
btrfs_set_header_backref_rev(eb_rewin,
btrfs_header_backref_rev(eb));
@@ -1219,7 +1220,8 @@ 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)
+ return ERR_PTR(-ENOMEM);
}
btrfs_tree_read_unlock(eb);
@@ -2772,6 +2774,10 @@ again:
BTRFS_READ_LOCK);
}
b = tree_mod_log_rewind(root->fs_info, b, time_seq);
+ if (IS_ERR(b)) {
+ ret = PTR_ERR(b);
+ 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;
--
1.7.7.6
--
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
Zach Brown
2013-Aug-07 20:02 UTC
Re: [PATCH] Btrfs: deal with enomem in the rewind path V2
> --- a/fs/btrfs/ctree.c > +++ b/fs/btrfs/ctree.c > @@ -1211,7 +1211,8 @@ 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) > + return ERR_PTR(-ENOMEM);Don''t these error paths need to unlock and free eb...> @@ -2772,6 +2774,10 @@ again: > BTRFS_READ_LOCK); > } > b = tree_mod_log_rewind(root->fs_info, b, time_seq); > + if (IS_ERR(b)) { > + ret = PTR_ERR(b); > + goto done; > + }... because this b = doit(, b, ) will leak the b input eb if the b return is an error pointer? - z -- 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
Josef Bacik
2013-Aug-07 20:18 UTC
Re: [PATCH] Btrfs: deal with enomem in the rewind path V2
On Wed, Aug 07, 2013 at 01:02:30PM -0700, Zach Brown wrote:> > --- a/fs/btrfs/ctree.c > > +++ b/fs/btrfs/ctree.c > > @@ -1211,7 +1211,8 @@ 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) > > + return ERR_PTR(-ENOMEM); > > Don''t these error paths need to unlock and free eb... > > > @@ -2772,6 +2774,10 @@ again: > > BTRFS_READ_LOCK); > > } > > b = tree_mod_log_rewind(root->fs_info, b, time_seq); > > + if (IS_ERR(b)) { > > + ret = PTR_ERR(b); > > + goto done; > > + } > > ... because this b = doit(, b, ) will leak the b input eb if the b > return is an error pointer? >Ah yeah I completely missed that, thanks. Josef -- 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