On Sun, June 30, 2013 at 15:55 (+0200), Josef Bacik wrote: > On Sun, Jun 30, 2013 at 10:25:05AM +0200, Jan Schmidt wrote: >> On 30.06.2013 05:17, Josef Bacik wrote: >>> We need to hold the tree mod log lock in __tree_mod_log_rewind since we walk >>> forward in the tree mod entries, otherwise we'll end up with random entries >>> and >>> trip the BUG_ON() at the front of __tree_mod_log_rewind. This fixes the >>> panics >>> people were seeing when running >>> >>> find /whatever -type f -exec btrfs fi defrag {} \; >> >> This patch cannot help to solve the problem, as far as I've understood >> what is going on. It does change timing, though, which presumably makes >> it pass the current reproducer we're having. >> >> On rewinding, iteration through the tree mod log rb-tree goes backwards >> in time, which means that once we've found our staring point we cannot >> be trapped by later additions. The old items we're rewinding towards >> cannot be freed, because we've allocated a blocker element within the >> tree and rewinding never goes beyond the allocated blocker. The blocker >> element is allocated by btrfs_get_tree_mod_seq and mostly referred to as >> time_seq within the other tree mod log functions in ctree.c. To sum up, >> the added lock is not required. >> >> The debug output I've analyzed so far shows that after we've rewinded >> all REMOVE_WHILE_FREEING operations on a buffer, ordered consecutively >> as expected, there comes another REMOVE_WHILE_FREEING with a sequence >> number much further in the past for the same buffer (but that sequence >> number is still higher than out time_seq rewind barrier at that point). >> This must be a logical problem I've not completely understood so far, >> but locking doesn't seem to be the right track. >> > > Finally reproduced it, this is my output > > btrfs-endio-wri-23110 [000] ...2 9556.882103: __tree_mod_log_rewind: > rewinding 15450537984 > btrfs-endio-wri-23110 [000] ...2 9556.882104: __tree_mod_log_rewind: > 15450537984: processing ffff880246590a40, op 3, seq 68719476829, slot 0 > btrfs-endio-wri-23110 [000] ...2 9556.882106: __tree_mod_log_rewind: > 15450537984: processing ffff880246590ac0, op 3, seq 68719476828, slot 1 > btrfs-endio-wri-23110 [000] ...2 9556.882108: __tree_mod_log_rewind: > 15450537984: processing ffff880246590a40, op 3, seq 68719476829, slot 0 > btrfs-endio-wri-23110 [000] ...2 9556.882110: __tree_mod_log_rewind: > 15450537984: this tm is failing, ffff880246590a40, seq 68719476829, slot 0 > > so I'm inclined to beleive I've got it right. Thanks,
Looking at the code I agree we should have a read lock around rb_next, protecting it against reorganization during insertions. Fits to that kind of debug output. How about just getting the lock for the rb_next call? There can be quite a lot of operations to rewind and I'd rather not have every other fs tree modification block on that. Thanks, -Jan > 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 > -- 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