From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sjc00mx2.hgst.com ([199.255.44.37]:21972 "EHLO sjc00mx2.hgst.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754679AbcEZTKs (ORCPT ); Thu, 26 May 2016 15:10:48 -0400 Date: Thu, 26 May 2016 15:09:56 -0400 From: Scott Talbert CC: "linux-btrfs@vger.kernel.org" Subject: Re: Device replace issues and disabling it until they are solved In-Reply-To: Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII"; format=flowed To: unlisted-recipients:; (no To-header on input) Sender: linux-btrfs-owner@vger.kernel.org List-ID: On Wed, 11 May 2016, Filipe Manana wrote: >> I've noticed some time ago that our device replace implementation is >> unreliable. Basically under several situations it ends up not copying >> extents (both data and metadata) from the old device to the new device >> (I briefly talked about some of the problems with Josef at LSF). >> >> Essentially it operates by iterating all the extents of the old device >> using the device tree's commit root, and for each device extent found, >> does a direct copy of it into the new device (it does not use the >> regular write paths). While it's doing this no extents can be >> allocated from the new device, this is only possible once it finishes >> copying all device extents and replaces the old device with the new >> device in the list of devices available for allocations. >> >> There are 4 main flaws with the implementation: >> >> 1) It processes each device extent only once. Clearly after it >> processes (copies) a device extent it's possible for future data and >> metadata writes to allocate extents from the corresponding block >> group, so any new extents are not copied into the new device. >> >> 2) Before it copies a device extent, it sets the corresponding block >> group to RO mode (as of commit [1], which sadly does not really fix an >> issue nor does it explain how the problem it claims to fix happens) to >> prevent new allocations from allocating extents from the block group >> while we are processing it, and once we finish processing it, it sets >> the block group back to RW mode. This is flawed too, as when it sets >> the block group to RO mode, some concurrent task might have just >> allocated an extent from the block group and ends up writing to it >> while or after we process the device extent. >> >> 3) Since it iterates over the device tree in an ascending fashion, it >> never considers the possibility for new device extents with an offset >> lower then the current search offset from being allocated for new >> block groups (just take a look at the while loop at >> scrub_enumerate_chunks() to see how flawed it is). So it totally >> ignores that device holes can be used while we are doing the replace >> and never looks back to check for any and copy the respective device >> extent into the new device. This is much more likely to happen >> nowadays since empty block groups are automatically removed by the >> cleaner kthread, while in the past this could only happen through >> balance operations triggered from userspace. >> >> 4) When we finish iterating "all" device extents, we call >> btrfs_dev_replace_finishing(), which flushes all delalloc, commits the >> current transaction, replaces the old device in all extent mappings >> with the new device, removes old device from the list of available >> devices for allocation and adds the new device to that list. So when >> flushing dellaloc more extents are allocated from existing block >> groups and new block groups might be allocated, with all the >> corresponding bios writing to the old device and not the new one, and >> after this flushing we surely don't copy all new or changed extents to >> the new device. >> >> These are the 4 major issues I see with device replace. Issue number 2 >> is trivial to solve but it's pointless to do so without addressing the >> other issues. >> >> Solving this isn't easy, not without some trade offs at least. We can >> keep in memory structures to track which new block groups are >> allocated while we are doing the replace and which new extents are >> allocated from existing block groups. Besides the needed >> synchronization between the allocator and the device replace code >> (which would bring yet more complexity), this can imply using a lot of >> memory if there's a significant number of writes happening all the >> time and ending up in an endless copy loop. Maybe some sort of write >> throttling would work out. There's also a bit of a chicken and the egg >> problem, as while the replace operation is ongoing we update a >> progress item in the devices tree (btrfs_run_dev_replace()) which in >> turn results in allocating new metadata extents. >> >> Anyway, I'm just hoping someone tells me that I'm completely wrong and >> misunderstood the current implementation we have. If not, and given >> how serious this is (affecting both data and metadata, and defeating >> the main purpose of raid), shouldn't we disable this feature, like we >> did for snapshot aware defrag a couple years ago, until we have these >> issues solved? > > On IRC, Stefan Behrens just pointed me out that for locations of the > old device behind the current left cursor, we make all writes go both > into the old and new devices (volumes.c:__btrfs_map_block()). This > solves problems 1 and 3, which are really the hard problems. The > remaining ones are easy to fix and I'll address them soon. Hi Filipe, Does your recent patch set (from May 20) address all of these issues? Scott