Re: [PATCH 3/9] btrfs: track original extent subvol in a new inline ref

From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <wqu@suse.com>
Cc: Boris Burkov <boris@bur.io>, Qu Wenruo <quwenruo.btrfs@gmx.com>,
	linux-btrfs@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH 3/9] btrfs: track original extent subvol in a new inline ref
Date: Wed, 10 May 2023 01:58:44 +0200	[thread overview]
Message-ID: <20230509235844.GO32559@twin.jikos.cz> (raw)
In-Reply-To: <e92837ae-0a14-21ee-1d2e-699165391ff2@suse.com>

On Fri, May 05, 2023 at 05:49:00AM +0800, Qu Wenruo wrote:
> On 2023/5/5 00:17, Boris Burkov wrote:
> > On Wed, May 03, 2023 at 11:17:12AM +0800, Qu Wenruo wrote:
> >> On 2023/5/3 08:59, Boris Burkov wrote:
> >>> In order to implement simple quota groups, we need to be able to
> >>> associate a data extent with the subvolume that created it. Once you
> >>> account for reflink, this information cannot be recovered without
> >>> explicitly storing it. Options for storing it are:
> >>> - a new key/item
> >>> - a new extent inline ref item
> >>>
> >>> The former is backwards compatible, but wastes space, the latter is
> >>> incompat, but is efficient in space and reuses the existing inline ref
> >>> machinery, while only abusing it a tiny amount -- specifically, the new
> >>> item is not a ref, per-se.
> >>
> >> Even we introduce new extent tree items, we can still mark the fs compat_ro.
> >>
> >> As long as we don't do any writes, we can still read the fs without any
> >> compatibility problem, and the enable/disable should be addressed by
> >> btrfstune/mkfs anyway.
> > 
> > Unfortunately, I don't believe compat_ro is possible with this design.
> > Because of how inline ref items are implemented, there is a lot of code
> > that makes assumptions about the extent item size, and the inline ref
> > item size based on their type. The best example that definitely breaks
> > things rather than maybe just warning is check_extent in tree-checker.c
> 
> IIRC if it's compat_ro, older kernel would reject the block group items 
> read.
> 
> If we expand that behavior to reject the whole extent tree, it can stay 
> compat_ro.
> Although you may need to do extra backports.
> 
> > 
> > With a new unparseable ref item inserted in the sequence of refs, that
> > code will either overflow or detect padding. The size calculation comes
> > up 0, etc. Perhaps there is a clever way to trick it, but I have not
> > seen it yet.
> > 
> > I was able to make it compat_ro by introducing an entirely new item for
> > the owner ref, but that comes with a per extent disk usage tradeoff that
> > is fairly steep for storing just a single u64.
> 
> If it's only to glue the original ref to an extent, I guess a new key 
> without an item would be enough.
> Although that's still quite expensive.

I consider allocating a new key as a high cost, it's worth for new
feature like verity or encryption where we require a fine grained
tracking of some new information. The number space is 255 values wide
and there are some ranges that are relatively ordered so the placement
in the logical b-tree space is good. We still have enough free values
but the gaps get smaller each time so I'd rather consider other options
first.

One drawback with features defined by keys is that it can't be easily
seen from superblock if the feature is present or not. Like extended
refs, no holes, lzo/zstd compressed extents. We always need to add the
compat bit. In case we would add a new key just to store little data and
still need to add the incompat bit it's time to think again if we could
get away with just the incompat bit. With some loss of backward
compatibility.

Right now I don't know what would be the best way forward but I'm
leaning more towards less backward compatibility and saving space in
structures. We get new incompat features "regularly" and people move to
newer kernels eventually after some period where we have time to iron
out bugs and explore the use case.

The simple quotas should fill the gap that qgroups can't so it makes
sense and people have been asking for something like that.