Re: [PATCH 2/3] btrfs: volumes: Add btrfs_fs_devices::missing_list to collect missing devices

From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo.btrfs@gmx.com>
Cc: Anand Jain <anand.jain@oracle.com>, Qu Wenruo <wqu@suse.com>,
	linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 2/3] btrfs: volumes: Add btrfs_fs_devices::missing_list to collect missing devices
Date: Wed, 27 Nov 2019 20:36:31 +0100	[thread overview]
Message-ID: <20191127193631.GC2734@twin.jikos.cz> (raw)
In-Reply-To: <0d4d78eb-eede-98c0-109a-c731ddffb5d7@gmx.com>

On Tue, Nov 19, 2019 at 06:29:46PM +0800, Qu Wenruo wrote:
> On 2019/11/19 下午6:03, Anand Jain wrote:
> > On 11/7/19 2:27 PM, Qu Wenruo wrote:
> >> This enables btrfs to iterate missing devices separately, without
> >> iterating all fs_devices.
> > 
> >  IMO.
> >  We don't need another list to maintain the missing device. We already
> >  have good enough device lists.
> >  The way its been implemented is
> >  Allo_list is the only list from which we shall alloc the chunks.
> >  Missing is a state of the device.
> >  A device in the alloc list can be in the Missing state.
> 
> That would cause problem, especially when you only want to use missing
> device as last resort method.
> 
> IIRC it's you mentioned this is a problem in my original design (which
> put all missing deviecs into alloc_list). Or it's David?
> 
> > 
> >  If there is missing_list that means the device in the missing list
> >  is also possible candidate for the alloc that's messy.
> 
> But when you want to avoid missing device, alloc_list and missing_list
> makes sense.
> 
> E.g. 6 devices RAID5, with one missing device, we should *avoid* using
> missing devices as we have enough (5) devices to allocate from.

I tend to agree that adding more lists would make things messy. This
needs to keep the missing state bit and presence in the list in sync,
there's the counter of missing devices. And that there are typically
only very few missing devices is also something to consider.

The device selection in __btrfs_alloc_chunk can avoid that. There's an
array allocated, with some size related data then it's passed to qsort
so the first N drives will be used for the chunk.

In case the degraded allocation is allowed (as mentioned in the other
mail, only for the mirrored profiles)

* add the missing device to the array
* update the comparison function btrfs_cmp_device_info to order missing
  devices to the end

Then the same logic "first N" would work here.