| Message ID | 55137BE2.80603@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Mar 26, 2015 at 3:24 AM, Eric Sandeen <sandeen@redhat.com> wrote: > Looks like "btrfs: fix leak of path in btrfs_find_item" got sent > to stable trees, but in my testing, it causes deadlocks on mount: > > [23379.359246] mount D 0000000000000000 0 22541 22274 0x00000080 > [23379.366326] ffff8803ebadf6c8 0000000000000086 ffff88027ff10230 0000000000013680 > [23379.373770] 0000000000013680 ffff8803ebadffd8 ffff8803ebadc010 0000000000013680 > [23379.381208] ffff8803ebadffd8 0000000000013680 ffff880261c78b60 ffff8802140a0b60 > [23379.388648] Call Trace: > [23379.391106] [<ffffffff816182b9>] schedule+0x29/0x70 > [23379.396091] [<ffffffffa06b82f5>] btrfs_tree_lock+0xb5/0x290 [btrfs] > [23379.402444] [<ffffffff8109b470>] ? wake_up_bit+0x40/0x40 > [23379.407855] [<ffffffffa064f7a5>] ? generic_bin_search+0xf5/0x180 [btrfs] > [23379.414643] [<ffffffffa065041b>] btrfs_lock_root_node+0x3b/0x50 [btrfs] > [23379.421345] [<ffffffffa065936b>] btrfs_search_slot+0x63b/0x800 [btrfs] > [23379.427956] [<ffffffffa064fa49>] ? btrfs_set_path_blocking+0x39/0x80 [btrfs] > [23379.435088] [<ffffffffa0659ede>] btrfs_insert_empty_items+0x7e/0xe0 [btrfs] > [23379.442125] [<ffffffffa065051a>] ? btrfs_alloc_path+0x1a/0x20 [btrfs] > [23379.448655] [<ffffffffa06b8989>] btrfs_insert_orphan_item+0x69/0x90 [btrfs] > [23379.455696] [<ffffffffa06ba938>] insert_orphan_item+0x68/0x90 [btrfs] > [23379.462251] [<ffffffffa06bf772>] replay_one_buffer+0x372/0x380 [btrfs] > [23379.468878] [<ffffffffa069a4c1>] ? mark_extent_buffer_accessed+0x51/0x70 [btrfs] > [23379.476372] [<ffffffffa06ba54b>] walk_up_log_tree+0x1cb/0x250 [btrfs] > [23379.482910] [<ffffffffa06ba68f>] walk_log_tree+0xbf/0x1b0 [btrfs] > [23379.489098] [<ffffffffa06bd51c>] btrfs_recover_log_trees+0x1ec/0x4c0 [btrfs] > ... > > I could hit this by running ./check generic/015 generic/039 in fstests, > with a SCRATCH_DEV_POOL defined (not sure it matters, it's just what I > have...) > > This fixes it, though I'm not totally sure why. Refcounts? Hi Eric, Your patch seems correct to me. The problem is that btrfs_insert_orphan_item tries to get a write lock on the same node/leaf for which its caller (insert_orphan_item) is already holding a read lock. If you plan to submit a proper patch, feel free to add my Reviewed-by: Filipe Manana <fdmanana@suse.com> thanks > > diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c > index 906934e..d37e6d1 100644 > --- a/fs/btrfs/tree-log.c > +++ b/fs/btrfs/tree-log.c > @@ -1265,11 +1265,11 @@ static int insert_orphan_item(struct btrfs_trans_handle *trans, > > ret = btrfs_find_item(root, path, BTRFS_ORPHAN_OBJECTID, > offset, BTRFS_ORPHAN_ITEM_KEY, NULL); > + btrfs_free_path(path); > + > if (ret > 0) > ret = btrfs_insert_orphan_item(trans, root, offset); > > - btrfs_free_path(path); > - > return ret; > } > > > but it never likely showed up upstream, because > > 9c4f61f btrfs: simplify insert_orphan_item > > made the whole path alloc/free go away. > > -Eric > -- > 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
On 3/26/15 5:23 AM, Filipe David Manana wrote: > On Thu, Mar 26, 2015 at 3:24 AM, Eric Sandeen <sandeen@redhat.com> wrote: >> Looks like "btrfs: fix leak of path in btrfs_find_item" got sent >> to stable trees, but in my testing, it causes deadlocks on mount: >> >> [23379.359246] mount D 0000000000000000 0 22541 22274 0x00000080 >> [23379.366326] ffff8803ebadf6c8 0000000000000086 ffff88027ff10230 0000000000013680 >> [23379.373770] 0000000000013680 ffff8803ebadffd8 ffff8803ebadc010 0000000000013680 >> [23379.381208] ffff8803ebadffd8 0000000000013680 ffff880261c78b60 ffff8802140a0b60 >> [23379.388648] Call Trace: >> [23379.391106] [<ffffffff816182b9>] schedule+0x29/0x70 >> [23379.396091] [<ffffffffa06b82f5>] btrfs_tree_lock+0xb5/0x290 [btrfs] >> [23379.402444] [<ffffffff8109b470>] ? wake_up_bit+0x40/0x40 >> [23379.407855] [<ffffffffa064f7a5>] ? generic_bin_search+0xf5/0x180 [btrfs] >> [23379.414643] [<ffffffffa065041b>] btrfs_lock_root_node+0x3b/0x50 [btrfs] >> [23379.421345] [<ffffffffa065936b>] btrfs_search_slot+0x63b/0x800 [btrfs] >> [23379.427956] [<ffffffffa064fa49>] ? btrfs_set_path_blocking+0x39/0x80 [btrfs] >> [23379.435088] [<ffffffffa0659ede>] btrfs_insert_empty_items+0x7e/0xe0 [btrfs] >> [23379.442125] [<ffffffffa065051a>] ? btrfs_alloc_path+0x1a/0x20 [btrfs] >> [23379.448655] [<ffffffffa06b8989>] btrfs_insert_orphan_item+0x69/0x90 [btrfs] >> [23379.455696] [<ffffffffa06ba938>] insert_orphan_item+0x68/0x90 [btrfs] >> [23379.462251] [<ffffffffa06bf772>] replay_one_buffer+0x372/0x380 [btrfs] >> [23379.468878] [<ffffffffa069a4c1>] ? mark_extent_buffer_accessed+0x51/0x70 [btrfs] >> [23379.476372] [<ffffffffa06ba54b>] walk_up_log_tree+0x1cb/0x250 [btrfs] >> [23379.482910] [<ffffffffa06ba68f>] walk_log_tree+0xbf/0x1b0 [btrfs] >> [23379.489098] [<ffffffffa06bd51c>] btrfs_recover_log_trees+0x1ec/0x4c0 [btrfs] >> ... >> >> I could hit this by running ./check generic/015 generic/039 in fstests, >> with a SCRATCH_DEV_POOL defined (not sure it matters, it's just what I >> have...) >> >> This fixes it, though I'm not totally sure why. Refcounts? > > Hi Eric, > > Your patch seems correct to me. > The problem is that btrfs_insert_orphan_item tries to get a write lock > on the same node/leaf for which its caller (insert_orphan_item) is > already holding a read lock. > > If you plan to submit a proper patch, feel free to add my Reviewed-by: > Filipe Manana <fdmanana@suse.com> Thanks; well - >> but it never likely showed up upstream, because >> >> 9c4f61f btrfs: simplify insert_orphan_item >> >> made the whole path alloc/free go away. so I think there's no need for my patch; may as well just send the above to stable and fix it that way, as long as 9c4f61f is deemed safe & correct, I think. -Eric -- 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
On Thu, Mar 26, 2015 at 10:11 AM, Eric Sandeen <sandeen@redhat.com> wrote: > On 3/26/15 5:23 AM, Filipe David Manana wrote: >> On Thu, Mar 26, 2015 at 3:24 AM, Eric Sandeen <sandeen@redhat.com> >> wrote: >>> Looks like "btrfs: fix leak of path in btrfs_find_item" got sent >>> to stable trees, but in my testing, it causes deadlocks on mount: >>> >>> [23379.359246] mount D 0000000000000000 0 22541 >>> 22274 0x00000080 >>> [23379.366326] ffff8803ebadf6c8 0000000000000086 ffff88027ff10230 >>> 0000000000013680 >>> [23379.373770] 0000000000013680 ffff8803ebadffd8 ffff8803ebadc010 >>> 0000000000013680 >>> [23379.381208] ffff8803ebadffd8 0000000000013680 ffff880261c78b60 >>> ffff8802140a0b60 >>> [23379.388648] Call Trace: >>> [23379.391106] [<ffffffff816182b9>] schedule+0x29/0x70 >>> [23379.396091] [<ffffffffa06b82f5>] btrfs_tree_lock+0xb5/0x290 >>> [btrfs] >>> [23379.402444] [<ffffffff8109b470>] ? wake_up_bit+0x40/0x40 >>> [23379.407855] [<ffffffffa064f7a5>] ? >>> generic_bin_search+0xf5/0x180 [btrfs] >>> [23379.414643] [<ffffffffa065041b>] >>> btrfs_lock_root_node+0x3b/0x50 [btrfs] >>> [23379.421345] [<ffffffffa065936b>] btrfs_search_slot+0x63b/0x800 >>> [btrfs] >>> [23379.427956] [<ffffffffa064fa49>] ? >>> btrfs_set_path_blocking+0x39/0x80 [btrfs] >>> [23379.435088] [<ffffffffa0659ede>] >>> btrfs_insert_empty_items+0x7e/0xe0 [btrfs] >>> [23379.442125] [<ffffffffa065051a>] ? btrfs_alloc_path+0x1a/0x20 >>> [btrfs] >>> [23379.448655] [<ffffffffa06b8989>] >>> btrfs_insert_orphan_item+0x69/0x90 [btrfs] >>> [23379.455696] [<ffffffffa06ba938>] insert_orphan_item+0x68/0x90 >>> [btrfs] >>> [23379.462251] [<ffffffffa06bf772>] replay_one_buffer+0x372/0x380 >>> [btrfs] >>> [23379.468878] [<ffffffffa069a4c1>] ? >>> mark_extent_buffer_accessed+0x51/0x70 [btrfs] >>> [23379.476372] [<ffffffffa06ba54b>] walk_up_log_tree+0x1cb/0x250 >>> [btrfs] >>> [23379.482910] [<ffffffffa06ba68f>] walk_log_tree+0xbf/0x1b0 >>> [btrfs] >>> [23379.489098] [<ffffffffa06bd51c>] >>> btrfs_recover_log_trees+0x1ec/0x4c0 [btrfs] >>> ... >>> >>> I could hit this by running ./check generic/015 generic/039 in >>> fstests, >>> with a SCRATCH_DEV_POOL defined (not sure it matters, it's just >>> what I >>> have...) >>> >>> This fixes it, though I'm not totally sure why. Refcounts? >> >> Hi Eric, >> >> Your patch seems correct to me. >> The problem is that btrfs_insert_orphan_item tries to get a write >> lock >> on the same node/leaf for which its caller (insert_orphan_item) is >> already holding a read lock. >> >> If you plan to submit a proper patch, feel free to add my >> Reviewed-by: >> Filipe Manana <fdmanana@suse.com> > > Thanks; well - > >>> but it never likely showed up upstream, because >>> >>> 9c4f61f btrfs: simplify insert_orphan_item >>> >>> made the whole path alloc/free go away. > > so I think there's no need for my patch; may as well just send the > above to stable > and fix it that way, as long as 9c4f61f is deemed safe & correct, I > think. Nice catch, thanks Eric. 9c4f61f looks fine for stable to me, but since he's already testing on stable, I talked Eric into giving it a pass through xfstests before I send it up. -chris -- 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
On 3/26/15 9:48 AM, Chris Mason wrote: > On Thu, Mar 26, 2015 at 10:11 AM, Eric Sandeen <sandeen@redhat.com> wrote: ... >>>> 9c4f61f btrfs: simplify insert_orphan_item >>>> >>>> made the whole path alloc/free go away. >> >> so I think there's no need for my patch; may as well just send the above to stable >> and fix it that way, as long as 9c4f61f is deemed safe & correct, I think. > > Nice catch, thanks Eric. 9c4f61f looks fine for stable to me, but > since he's already testing on stable, I talked Eric into giving it a > pass through xfstests before I send it up. > > -chris ./check -g auto on 3.19-stable-ish seems fine-ish. Certainly no worse w/ the patch added :) Failures: btrfs/010 btrfs/017 btrfs/078 generic/015 generic/039 generic/040 generic/041 generic/065 generic/066 generic/071 generic/204 Failed 11 of 202 tests I'd say ship it! -Eric -- 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
On Thu, Mar 26, 2015 at 5:11 PM, Eric Sandeen <sandeen@redhat.com> wrote: > On 3/26/15 9:48 AM, Chris Mason wrote: >> On Thu, Mar 26, 2015 at 10:11 AM, Eric Sandeen <sandeen@redhat.com> wrote: > > ... > >>>>> 9c4f61f btrfs: simplify insert_orphan_item >>>>> >>>>> made the whole path alloc/free go away. >>> >>> so I think there's no need for my patch; may as well just send the above to stable >>> and fix it that way, as long as 9c4f61f is deemed safe & correct, I think. >> >> Nice catch, thanks Eric. 9c4f61f looks fine for stable to me, but >> since he's already testing on stable, I talked Eric into giving it a >> pass through xfstests before I send it up. >> >> -chris > > ./check -g auto on 3.19-stable-ish seems fine-ish. Certainly no worse w/ the patch added :) > > Failures: btrfs/010 btrfs/017 btrfs/078 generic/015 generic/039 generic/040 generic/041 generic/065 generic/066 generic/071 generic/204 > Failed 11 of 202 tests Just curious, how did btrfs/078 fail? It isn't supposed to fail on 3.19.x nor 3.18.x. > > I'd say ship it! > > -Eric >
On 3/26/15 12:25 PM, Filipe David Manana wrote: > On Thu, Mar 26, 2015 at 5:11 PM, Eric Sandeen <sandeen@redhat.com> wrote: >> On 3/26/15 9:48 AM, Chris Mason wrote: >>> On Thu, Mar 26, 2015 at 10:11 AM, Eric Sandeen <sandeen@redhat.com> wrote: >> >> ... >> >>>>>> 9c4f61f btrfs: simplify insert_orphan_item >>>>>> >>>>>> made the whole path alloc/free go away. >>>> >>>> so I think there's no need for my patch; may as well just send the above to stable >>>> and fix it that way, as long as 9c4f61f is deemed safe & correct, I think. >>> >>> Nice catch, thanks Eric. 9c4f61f looks fine for stable to me, but >>> since he's already testing on stable, I talked Eric into giving it a >>> pass through xfstests before I send it up. >>> >>> -chris >> >> ./check -g auto on 3.19-stable-ish seems fine-ish. Certainly no worse w/ the patch added :) >> >> Failures: btrfs/010 btrfs/017 btrfs/078 generic/015 generic/039 generic/040 generic/041 generic/065 generic/066 generic/071 generic/204 >> Failed 11 of 202 tests > > Just curious, how did btrfs/078 fail? It isn't supposed to fail on > 3.19.x nor 3.18.x. This was the use after free / segfault / valgrind splat from userspace that I reported yesterday. btrfs/078 104s ... 106s *** glibc detected *** btrfs check: double free or corruption (fasttop): 0x0000000000c12a40 *** ... ./common/rc: line 1932: 43334 Aborted btrfsck $device > $tmp.fsck 2>&1 _check_btrfs_filesystem: filesystem on /dev/sdc5 is inconsistent (see /mnt/test2/git/xfstests/results//btrfs/078.full) Ran: btrfs/078 Failures: btrfs/078 Failed 1 of 1 tests -Eric -- 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
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c index 906934e..d37e6d1 100644 --- a/fs/btrfs/tree-log.c +++ b/fs/btrfs/tree-log.c @@ -1265,11 +1265,11 @@ static int insert_orphan_item(struct btrfs_trans_handle *trans, ret = btrfs_find_item(root, path, BTRFS_ORPHAN_OBJECTID, offset, BTRFS_ORPHAN_ITEM_KEY, NULL); + btrfs_free_path(path); + if (ret > 0) ret = btrfs_insert_orphan_item(trans, root, offset); - btrfs_free_path(path); - return ret; }