On Wed, Feb 25, 2015 at 1:18 AM, Zhao Lei <zhaolei@xxxxxxxxxxxxxx> wrote: > Hi, Filipe > >> From: Filipe David Manana [mailto:fdmanana@xxxxxxxxx] >> >> On Tue, Feb 24, 2015 at 3:37 PM, Zhaolei <zhaolei@xxxxxxxxxxxxxx> wrote: >> > From: Zhao Lei <zhaolei@xxxxxxxxxxxxxx> >> > >> > Above BUG_ON() was triggered only one time in my test, but hadn't >> > happened again in same env. >> > >> > The reason maybe: >> > A block group which include pinned space was removed before >> > unpin_extent_range(), and no other block_group_cache after "start" >> > pos, so the code entered into above BUG_ON(). >> > >> > To support auto-remove-bgs, we can remove above BUG_ON(), and bypass >> > removed bgs. >> >> I don't think it's a good idea to remove this BUG_ON(). >> You're just hiding (potentially dangerous) logical bugs doing that - we need to >> understand exactly why that happens and fix it. >> >> I fixed a scenario where this happens recently, and the fix is in 4.0-rc1: >> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d4b4 >> 50cd4b33ce7c572e7fdccf33b59c4cdf361c >> >> Were you testing with or without this fix? >> > Thanks for notice, it is better way of fix. > I'll drop this patch. Sorry I also missed to mention 2 things before: 1) Your patch can lead to space leaks too. The range we're passing to unpin_extent_range() can cover more than 1 block group - for example the whole block group that is about to be deleted plus part of an adjacent block group that isn't empty. In this case you would be leaking space forever, and that space corresponds to the part of the block group that isn't empty; 2) There's another race my fix deals with which I haven't mentioned in the commit message. If you look at the time sequence diagram from the commit message, it's possible that after CPU 2 deletes the block group and before CPU 1 calls unpin_extent_range(), a new block group using the same logical address (but different physical address of course) is created and space allocated from it. In this (hopefully very unlikely) scenario, CPU 1 would just mark that allocated space as freed when it isn't, as it has no way to know there's a new block group with the same logical address - this would be terrible. thanks > > Thanks > Zhaolei > >> Thanks >> >> > >> > Signed-off-by: Zhao Lei <zhaolei@xxxxxxxxxxxxxx> >> > --- >> > fs/btrfs/extent-tree.c | 3 ++- >> > 1 file changed, 2 insertions(+), 1 deletion(-) >> > >> > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index >> > 8b51eb5..ef0b40d 100644 >> > --- a/fs/btrfs/extent-tree.c >> > +++ b/fs/btrfs/extent-tree.c >> > @@ -5751,7 +5751,8 @@ static int unpin_extent_range(struct btrfs_root >> *root, u64 start, u64 end, >> > if (cache) >> > btrfs_put_block_group(cache); >> > cache = btrfs_lookup_block_group(fs_info, >> start); >> > - BUG_ON(!cache); /* Logic error */ >> > + if (!cache) >> > + break; >> > } >> > >> > len = cache->key.objectid + cache->key.offset - start; >> > -- >> > 1.8.5.1 >> > >> > -- >> > 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 >> >> >> >> -- >> Filipe David Manana, >> >> "Reasonable men adapt themselves to the world. >> Unreasonable men adapt the world to themselves. >> That's why all progress depends on unreasonable men." > > -- Filipe David Manana, "Reasonable men adapt themselves to the world. Unreasonable men adapt the world to themselves. That's why all progress depends on unreasonable men." -- 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
