From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7E0F5C433F5 for ; Tue, 14 Dec 2021 20:04:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237554AbhLNUEd (ORCPT ); Tue, 14 Dec 2021 15:04:33 -0500 Received: from drax.kayaks.hungrycats.org ([174.142.148.226]:43422 "EHLO drax.kayaks.hungrycats.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230188AbhLNUEd (ORCPT ); Tue, 14 Dec 2021 15:04:33 -0500 Received: by drax.kayaks.hungrycats.org (Postfix, from userid 1002) id 9FBDAE1181; Tue, 14 Dec 2021 15:04:32 -0500 (EST) Date: Tue, 14 Dec 2021 15:04:32 -0500 From: Zygo Blaxell To: kreijack@inwind.it Cc: Josef Bacik , David Sterba , Sinnamohideen Shafeeq , Paul Jones , "linux-btrfs@vger.kernel.org" Subject: Re: [RFC][V8][PATCH 0/5] btrfs: allocation_hint mode Message-ID: References: <1d725df7-b435-4acf-4d17-26c2bd171c1a@libero.it> <71e523dc-2854-ca9b-9eee-e36b0bd5c2cb@libero.it> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <71e523dc-2854-ca9b-9eee-e36b0bd5c2cb@libero.it> Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org On Tue, Dec 14, 2021 at 08:03:45PM +0100, Goffredo Baroncelli wrote: > On 12/13/21 23:49, Zygo Blaxell wrote: > > On Mon, Dec 13, 2021 at 04:15:14PM -0500, Josef Bacik wrote: > > > On Mon, Dec 13, 2021 at 08:54:24PM +0100, Goffredo Baroncelli wrote: > > > > Gentle ping :-) > > > > > > > > Are there anyone of the mains developer interested in supporting this patch ? > > > > > > > > I am open to improve it if required. > > > > > > > > > > Sorry I missed this go by. I like the interface, we don't have a use for > > > device->type yet, so this fits nicely. > > > > > > I don't see the btrfs-progs patches in my inbox, and these don't apply, so > > > you'll definitely need to refresh for a proper review, but looking at these > > > patches they seem sane enough, and I like the interface. I'd like to hear > > > Zygo's opinion as well. > > > > I've been running earlier versions with modifications since summer 2020, > > and this version mostly unmodified (rebase changes only) since it was > > posted. It seems to work, even in corner cases like converting balances, > > replacing drives, and running out of space. The "running out of space" > > experience is on btrfs is weird at the best of times, and these patches > > add some more new special cases, but it doesn't behave in ways that > > would surprise a sysadmin familiar with how btrfs chunk allocation works. > > > > One major piece that's missing is adjusting the statvfs (aka df) > > available blocks field so that it doesn't include unallocated space on > > any metadata-only devices. Right now all the unallocated space on > > metadata-only devices is counted as free even though it's impossible to > > put a data block there, so anything that is triggered automatically > > on "f_bavail < some_threshold" will be confused. > > > > I don't think that piece has to block the rest of the patch series--if > > you're not using the feature, df gives the right number (or at least the > > same number it gave before), and if you are using the feature, you can > > subtract the unavailable data space until a later patch comes along to > > fix it. > > > > I like > > > > echo data_only > /sys/fs/btrfs/$uuid/devinfo/3/type > > Only to be clear, for now you can pass a numeric value to "type". Not a text > like your example. > > However I want to put on the table another option: to not expose all the > "type" field, but only the "allocation policy"; we can add a new sysfs field > called "allocation policy" that internally change the dev_item->type field. > > It is not only a "cosmetic" change. If we want to change the allocation > policy, now the correct way is: > - read the type field > - change the "allocation policy" bits > - write the type field > > Which is race 'prone' > For now it is not a problem, because type contains only the allocation bits. > But in future when the type field will contains further properties this could > be a problem. Yeah, keep the interface very narrow, don't hand out access to random bits. If the kernel supports additional bits, it should support additional sysfs filenames to go with them. Or it could put all the supported options in the sysfs field, like block IO schedulers do, so you could find this in the file by reading it: [prefer_data] prefer_metadata metadata_only data_only > > more than patching btrfs-progs so I can use > > > > btrfs prop set /dev/... allocation_hint data_only > > > > but I admit that might be because I'm weird. > > I prefer the echo approach too; however it is not very ergonomics in conjunction > to sudo.... For /proc/sys/* we have the 'sysctl' tool, so you can write 'sysctl vm.drop_caches=1' or 'sudo sysctl vm.drop_caches=1'. For some reason we don't have this for sysfs (or maybe it's just Debian...?) so we have to write things like 'echo foo | sudo tee /sys/fs/...'. Of course btrfs-progs could always open the /sys/fs/btrfs/.../allocation_policy file and write to it. But if we're modifying btrfs-progs then we could use the ioctl interface anyway. I don't have a strong preference for either sysfs or ioctl, nor am I opposed to simply implementing both. I'll let someone who does have such a preference make their case. > > > If we're going to use device->type for this, and since we don't have a user of > > > device->type, I'd also like you to go ahead and re-name ->type to > > > ->allocation_policy, that way it's clear what we're using it for now. > > > > > > I'd also like some xfstests to validate the behavior so we're sure we're testing > > > this. I'd want 1 test to just test the mechanics, like mkfs with different > > > policies and validate they're set right, change policies, add/remove disks with > > > different policies. > > > > > > Then a second test to do something like fsstress with each set of allocation > > > policies to validate that we did actually allocate from the correct disks. For > > > this please also test with compression on to make sure the test validation works > > > for both normal allocation and compression (ie it doesn't assume writing 5gib of > > > data == 5 gib of data usage, as compression chould give you a different value). > > > > > > With that in place I think this is the correct way to implement this feature. > > > Thanks, > > > > > > Josef > > > -- > gpg @keyserver.linux.it: Goffredo Baroncelli > Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5