Received-SPF: pass (google.com: domain of linux-kernel+bounces-87306-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 29 Feb 2024 13:43:15 -0500
From: Brian Foster <bfoster@redhat.com>
To: Kent Overstreet <kent.overstreet@linux.dev>
Cc: linux-bcachefs@vger.kernel.org, linux-kernel@vger.kernel.org,
	djwong@kernel.org
Subject: Re: [PATCH 01/21] bcachefs: KEY_TYPE_accounting
Message-ID: <ZeDQQ8cyMTgYaY6D@bfoster>
References: <20240225023826.2413565-1-kent.overstreet@linux.dev>
 <20240225023826.2413565-2-kent.overstreet@linux.dev>
 <Zd4Ef49kHX3g69VT@bfoster>
 <t6cc6gqgla2csyvrsv2znel5lg76vx7t2zna772qpw7zd6pnft@rxdakxza5cab>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <t6cc6gqgla2csyvrsv2znel5lg76vx7t2zna772qpw7zd6pnft@rxdakxza5cab>

On Wed, Feb 28, 2024 at 02:39:38PM -0500, Kent Overstreet wrote:
> On Tue, Feb 27, 2024 at 10:49:19AM -0500, Brian Foster wrote:
> > On Sat, Feb 24, 2024 at 09:38:03PM -0500, Kent Overstreet wrote:
> > > New key type for the disk space accounting rewrite.
> > > 
> > >  - Holds a variable sized array of u64s (may be more than one for
> > >    accounting e.g. compressed and uncompressed size, or buckets and
> > >    sectors for a given data type)
> > > 
> > >  - Updates are deltas, not new versions of the key: this means updates
> > >    to accounting can happen via the btree write buffer, which we'll be
> > >    teaching to accumulate deltas.
> > > 
> > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > > ---
> > >  fs/bcachefs/Makefile                 |   3 +-
> > >  fs/bcachefs/bcachefs.h               |   1 +
> > >  fs/bcachefs/bcachefs_format.h        |  80 +++------------
> > >  fs/bcachefs/bkey_methods.c           |   1 +
> > >  fs/bcachefs/disk_accounting.c        |  70 ++++++++++++++
> > >  fs/bcachefs/disk_accounting.h        |  52 ++++++++++
> > >  fs/bcachefs/disk_accounting_format.h | 139 +++++++++++++++++++++++++++
> > >  fs/bcachefs/replicas_format.h        |  21 ++++
> > >  fs/bcachefs/sb-downgrade.c           |  12 ++-
> > >  fs/bcachefs/sb-errors_types.h        |   3 +-
> > >  10 files changed, 311 insertions(+), 71 deletions(-)
> > >  create mode 100644 fs/bcachefs/disk_accounting.c
> > >  create mode 100644 fs/bcachefs/disk_accounting.h
> > >  create mode 100644 fs/bcachefs/disk_accounting_format.h
> > >  create mode 100644 fs/bcachefs/replicas_format.h
> > > 
> > ...
> > > diff --git a/fs/bcachefs/disk_accounting_format.h b/fs/bcachefs/disk_accounting_format.h
> > > new file mode 100644
> > > index 000000000000..e06a42f0d578
> > > --- /dev/null
> > > +++ b/fs/bcachefs/disk_accounting_format.h
> > > @@ -0,0 +1,139 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef _BCACHEFS_DISK_ACCOUNTING_FORMAT_H
> > > +#define _BCACHEFS_DISK_ACCOUNTING_FORMAT_H
> > > +
> > > +#include "replicas_format.h"
> > > +
> > > +/*
> > > + * Disk accounting - KEY_TYPE_accounting - on disk format:
> > > + *
> > > + * Here, the key has considerably more structure than a typical key (bpos); an
> > > + * accounting key is 'struct disk_accounting_key', which is a union of bpos.
> > > + *
> > 
> > First impression.. I'm a little confused why the key type is a union of
> > bpos. I'm possibly missing something fundamental/obvious, but could you
> > elaborate more on why that is here?
> 
> How's this?
> 
>  * More specifically: a key is just a muliword integer (where word endianness   
>  * matches native byte order), so we're treating bpos as an opaque 20 byte                                                                                  
>  * integer and mapping bch_accounting_key to that.
> 

Hmm.. I think the connection I missed on first look is basically
disk_accounting_key_to_bpos(). I think what is confusing is that calling
this a key makes me think of bkey, which I understand to contain a bpos,
so then overlaying it with a bpos didn't really make a lot of sense to
me conceptually.

So when I look at disk_accounting_key_to_bpos(), I see we are actually
using the bpos _pad field, and this structure basically _is_ the bpos
for a disk accounting btree bkey. So that kind of makes me wonder why
this isn't called something like disk_accounting_pos instead of _key,
but maybe that is wrong for other reasons.

Either way, what I'm trying to get at is that I think this documentation
would be better if it explained conceptually how disk_accounting_key
relates to bkey/bpos, and why it exists separately from bkey vs. other
key types, rather than (or at least before) getting into the lower level
side effects of a union with bpos.

Brian