Re: [PATCH v2 0/8] fuse: DAX device based extent maps (famfs)

Re: [PATCH v2 0/8] fuse: DAX device based extent maps (famfs)

[John Groves]

On 26/10/01 05:07PM, Miklos Szeredi wrote:
> This is the FUSIfication of the famfs-fuse patchset.

This series (v1) was sent on the day I left on a vacation from which I
returned at the end of last week.

I feel I should acknowledge this and assure everybody that I'm working
through it. This really runs my famfs/fuse patches through a 
chipper/shredder, and it is gonna take me some time to review it, 
understand it and test it.

A few comments below, and specific patch comments should start trickling out.

One very high level quip though: If you're comfortable with this, go ahead 
and merge it. We can fix bugs and performance problems, if any, in due 
course. I'm serious about that.

> 
>  - extent maps are assigned to a backing ID
> 
>  - 64 bit backing ID's managed by server

I thought we agreed you weren't gonna do these two, but if it does no harm,
whatever.

> 
>  - maps can be linear or cyclic (striped)

Please correct me if any of my analysis below is wrong...

Here what you've done is remove my explicitly interleaved extent lists,
which have a header containing a chunk size and a set of strip extents (which
are basically fully self-describing), and replace is with just simple extent 
lists that have an optional CYCLIC flag in the header. If the flag is set on 
an extent list, (and let's say the strip extents need to be 1GiB and the 
chunk size is 2MiB), each CYCLIC extent gets its size set to the chunk size 
and the kernel trusts that the actual size allocated per stirp is actually 
sufficient that i_size will not cycle past the end of the *actually* 
allocated size of each strip extent.

This resembles an idea I considered, to try to make the metadata *look* 
simpler while still carrying the info that needs to be conveyed. But this 
implementation feels pretty janky to me, and it should at least be 
meticulously documented. Better might be to put the chunk_size in the header,
and then you arguably don't need the CYCLIC flag, since non-zero chunk_size
means the same thing. But actually, you can accept multiple CYCLIC strip
lists if you set chunk_size and then set the CYCLIC flag on the first of
each strip set.

I see the kernel-side rejects these s/size/chunk_size/ strip extents if they
are not all the same size. Why not put the chunk size in the header and let
the extents honestly describe the offset and range that they cover? 
The current design can't test for sufficient strip extent sizes, although 
it replaces code that did verify this.

> 
>  - API supports nesting, but not implemented yet (i.e. multiple striped
>    extents per inode, a-la famfs)
> 
> libfuse/famfs trees for testing:
> 
>  https://github.com/szmi/libfuse.git#extent-map-v2
>  https://github.com/szmi/famfs.git#extent-map-v2
> 
> This patchset:
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#extent-map-v2
> 
> Passes famfs smoke test on emulated dax device.

Thank you for doing that. Prior to this there have been many strong opinions
but none of the holders of said opinions built or tested famfs.

However, the smoke tests passing isn't fully legit, because the patch to the
famfs fuse server passes the entire strip size on CYCLIC mappings, meaning
it just produces a concatenation of ranges - which does not achieve the 
interleaving goal at all. That case degenerates to simple extents.

If this is the new design for passing cycling maps into the kernel, a test
will be added to catch this - but it's brand new and didn't get a test added,
so it wasn't caught automatically.

> 
> Comments are welcome.

One more... This patch inserts an rbtree lookup into the fault path via the
very-performance-critical fuse_ext_map_iomap_begin() path. This is 
performance-critical because it handles TLB/PTE/PMD faults when the memory
is never sparse (no I/O to amortize over). The code you replaced did several
things to avoid this sort of chasing:

- With interleaved extent lists, strip and strip-offset can be calculated
  in order 1 (and then strip overflow can be checked and avoided)
- Simple extent lists were checked for constant extent sizes; if constant,
  we can just index into the list (also order 1)

Why do I want to avoid an rbtree lookup or the bpf nonsense in the fault path?
"Because this is memory and it must run at memory speeds" --yours truly

To be specific: each chase/dereference that results in an L3 cache miss adds
a stall of (1 * MEMORY_LATENCY) to the fault time. This is the good reason 
why I didn't do any chasing in the fault path of the famfs code that you're
replacing.

OK, one more. You're using the rbtree for indexing by file offset, which is
a zero-based non-sparse range in this case. If this could not be avoided 
(which I think it can) you should use an xarray or radix tree for that...
right?

> 
> Thanks,
> Miklos
> 
> ---
> Changes since v1:
> 
>  - moved 3 prep patches to fuse.git#for-next
>  - introduce FUSE_PASSTHROUGH_V2 (Amir)
>  - lots of small fixes and cleanups (Amir, Sashiko)
> 
> ---
> John Groves (1):
>   dax: replace exported dax_dev_get() with non-allocating dax_dev_find()

Golly, it seems like there's more than 1 patch worth of content from me here,
but the chipper/shredder might meet the letter of copyright law in some 
jurisdictions...

> 
> Miklos Szeredi (7):
>   fuse: add helpers for EIO return value with kernel message
>   fuse: support 64 bit, server allocated backing ID
>   fuse: support opening 64 bit backing ID
>   fuse: add support for opening dax device as backing
>   fuse: add extent map data structure
>   fuse: add extent map I/O support
>   fuse: add support for striped backing
> 
>  drivers/dax/super.c       |  38 ++++-
>  fs/fuse/Makefile          |   2 +-
>  fs/fuse/backing.c         | 292 ++++++++++++++++++++++++++++++-------
>  fs/fuse/dev.c             |  21 +++
>  fs/fuse/dev.h             |   3 +
>  fs/fuse/dir.c             |   3 +-
>  fs/fuse/ext_map.c         | 299 ++++++++++++++++++++++++++++++++++++++
>  fs/fuse/file.c            |  16 +-
>  fs/fuse/fuse_i.h          |  72 +++++++--
>  fs/fuse/inode.c           |  17 ++-
>  fs/fuse/iomode.c          | 108 +++++++-------
>  fs/fuse/notify.c          |  75 ++++++++++
>  fs/fuse/passthrough.c     |  63 ++++----
>  include/linux/dax.h       |   6 +-
>  include/uapi/linux/fuse.h |  53 ++++++-
>  15 files changed, 904 insertions(+), 164 deletions(-)
>  create mode 100644 fs/fuse/ext_map.c
> 
> -- 
> 2.54.0
> 

John

Re: [PATCH v2 0/8] fuse: DAX device based extent maps (famfs)

[Miklos Szeredi]

On Tue, 6 Oct 2026 at 01:37, John Groves <john@groves.net> wrote:

> I see the kernel-side rejects these s/size/chunk_size/ strip extents if they
> are not all the same size. Why not put the chunk size in the header and let
> the extents honestly describe the offset and range that they cover?
> The current design can't test for sufficient strip extent sizes, although
> it replaces code that did verify this.

Not adding chunk size to the header is intentional to keep the API as
simple as possible.

> However, the smoke tests passing isn't fully legit, because the patch to the
> famfs fuse server passes the entire strip size on CYCLIC mappings, meaning
> it just produces a concatenation of ranges - which does not achieve the
> interleaving goal at all. That case degenerates to simple extents.

Are you talking about the case where there are multiple simple extents
each with a different striped mapping?

That case is supported by the API, but not the implementation (since
there was not test case).  If you do a test case, I'll add the
implementation.

> To be specific: each chase/dereference that results in an L3 cache miss adds
> a stall of (1 * MEMORY_LATENCY) to the fault time. This is the good reason
> why I didn't do any chasing in the fault path of the famfs code that you're
> replacing.

Please benchmark against the in-kernel implementation.  If there's
significant (meaning it may have a real effect on a real-life
workload) performance hit from using the rb-tree, then we can add an
optimization.  I don't want to add complexity without actually being
able to measure the gain.

> OK, one more. You're using the rbtree for indexing by file offset, which is
> a zero-based non-sparse range in this case. If this could not be avoided
> (which I think it can) you should use an xarray or radix tree for that...
> right?

Unlike xarray, the rb_tree API is one I'm familiar with. But nothing
prevents switching to xarray if that's a better choice.

Thanks,
Miklos


>
> >
> > Thanks,
> > Miklos
> >
> > ---
> > Changes since v1:
> >
> >  - moved 3 prep patches to fuse.git#for-next
> >  - introduce FUSE_PASSTHROUGH_V2 (Amir)
> >  - lots of small fixes and cleanups (Amir, Sashiko)
> >
> > ---
> > John Groves (1):
> >   dax: replace exported dax_dev_get() with non-allocating dax_dev_find()
>
> Golly, it seems like there's more than 1 patch worth of content from me here,
> but the chipper/shredder might meet the letter of copyright law in some
> jurisdictions...
>
> >
> > Miklos Szeredi (7):
> >   fuse: add helpers for EIO return value with kernel message
> >   fuse: support 64 bit, server allocated backing ID
> >   fuse: support opening 64 bit backing ID
> >   fuse: add support for opening dax device as backing
> >   fuse: add extent map data structure
> >   fuse: add extent map I/O support
> >   fuse: add support for striped backing
> >
> >  drivers/dax/super.c       |  38 ++++-
> >  fs/fuse/Makefile          |   2 +-
> >  fs/fuse/backing.c         | 292 ++++++++++++++++++++++++++++++-------
> >  fs/fuse/dev.c             |  21 +++
> >  fs/fuse/dev.h             |   3 +
> >  fs/fuse/dir.c             |   3 +-
> >  fs/fuse/ext_map.c         | 299 ++++++++++++++++++++++++++++++++++++++
> >  fs/fuse/file.c            |  16 +-
> >  fs/fuse/fuse_i.h          |  72 +++++++--
> >  fs/fuse/inode.c           |  17 ++-
> >  fs/fuse/iomode.c          | 108 +++++++-------
> >  fs/fuse/notify.c          |  75 ++++++++++
> >  fs/fuse/passthrough.c     |  63 ++++----
> >  include/linux/dax.h       |   6 +-
> >  include/uapi/linux/fuse.h |  53 ++++++-
> >  15 files changed, 904 insertions(+), 164 deletions(-)
> >  create mode 100644 fs/fuse/ext_map.c
> >
> > --
> > 2.54.0
> >
>
> John
>
>

Re: [PATCH v2 0/8] fuse: DAX device based extent maps (famfs)

[John Groves]

On 26/10/06 11:48AM, Miklos Szeredi wrote:
> On Tue, 6 Oct 2026 at 01:37, John Groves <john@groves.net> wrote:
> 
> > I see the kernel-side rejects these s/size/chunk_size/ strip extents if they
> > are not all the same size. Why not put the chunk size in the header and let
> > the extents honestly describe the offset and range that they cover?
> > The current design can't test for sufficient strip extent sizes, although
> > it replaces code that did verify this.
> 
> Not adding chunk size to the header is intentional to keep the API as
> simple as possible.

Fewer fields is nice, but it's on the honor system that the server allocated
large enough strips - since strips carry the chunk_size rather than their
actual size when CYCLIC is set. IMO it would be an improvement to add the
chunk_size to the header; then the strip sizes could be validated.

But it's your call, and I won't argue further about this unless something
changes.

> 
> > However, the smoke tests passing isn't fully legit, because the patch to the
> > famfs fuse server passes the entire strip size on CYCLIC mappings, meaning
> > it just produces a concatenation of ranges - which does not achieve the
> > interleaving goal at all. That case degenerates to simple extents.
> 
> Are you talking about the case where there are multiple simple extents
> each with a different striped mapping?

No

I'm saying your patch to the famfs user space, when famfs sends interleaved
file maps, sets CYCLIC but sets strip extent sizes to the actual strip size,
not the chunk_size. So chunking is at the size of the entire strip, which is
to say: the net is a multi-extent file that is not interleaved.

So that's a bug. I've fixed it locally and will share that in the next day
or two. I also needed to refactor your patch the famfs user space because
it disabled ABI compatibility with some older versions of the famfs kernel
that I'm still supporting users with. If you can wait to patch that further
until I share an update, it will be cleaner on my end.

> 
> That case is supported by the API, but not the implementation (since
> there was not test case).  If you do a test case, I'll add the
> implementation.
> 
> > To be specific: each chase/dereference that results in an L3 cache miss adds
> > a stall of (1 * MEMORY_LATENCY) to the fault time. This is the good reason
> > why I didn't do any chasing in the fault path of the famfs code that you're
> > replacing.
> 
> Please benchmark against the in-kernel implementation.  If there's
> significant (meaning it may have a real effect on a real-life
> workload) performance hit from using the rb-tree, then we can add an
> optimization.  I don't want to add complexity without actually being
> able to measure the gain.

I will do that, but it's not a small undertaking. Back to that in a moment.

I have an idea for how to make fault handling order-1 in the normal famfs
cases but fall back to order log-n if necessary (e.g. if not all extents are 
the same size). I'm testing that now and will share it soon if I don't run 
into any significant problems.

I think we can probably agree than things otherwise being equal, order-1
fault handling (or order-1 falling back to order-n if necessary) is 
preferable to order-log-n. If you or anybody doesn't agree with that, I 
hope we can discuss it.

As for benchmarking to "prove" that the rbtree is a problem, I'm sure there
are use cases where it is not a problem (like small virtual daxdevs on
workstations or laptops). What Micron worries about is huge working sets on 
many terabyte disaggregated memory. If the memory is 45TB (the biggest 
system I have "access" to), the data sets exceed the processor cache by a 
substantially higher factor than even in a maxed out server. That means
more L3 cache misses. I'm trying to avoid an approach with built-in chasing
on performance paths, because Micron's experience is that pointer chasing 
becomes L3-cache-miss bound.

Anyway, one workload (and working-set size) that doesn't exhibit significant 
L3 cache miss performance hit does not prove that others won't be negatively
impacted. Proving a negative, etc...

Having said all that, this topic seems to recur and be contentious - we are
working on tests that can demonstrate bottlenecks - but my current ask is
that if I can offer an alternative approach to rbtree that is better on 
paper, can we please use it?

> 
> > OK, one more. You're using the rbtree for indexing by file offset, which is
> > a zero-based non-sparse range in this case. If this could not be avoided
> > (which I think it can) you should use an xarray or radix tree for that...
> > right?
> 
> Unlike xarray, the rb_tree API is one I'm familiar with. But nothing
> prevents switching to xarray if that's a better choice.

That's fair. And I withdraw the xarray suggestion because I have a better
idea - will share that in a day or two, before Monday almost certainly.
Need to run it through all of my CI...

Thanks,
John

<snip>

Re: [PATCH v2 0/8] fuse: DAX device based extent maps (famfs)

[Miklos Szeredi]

On Fri, 9 Oct 2026 at 01:00, John Groves <john@groves.net> wrote:

> Fewer fields is nice, but it's on the honor system that the server allocated
> large enough strips - since strips carry the chunk_size rather than their
> actual size when CYCLIC is set. IMO it would be an improvement to add the
> chunk_size to the header; then the strip sizes could be validated.

Cyclic maps could easily be generalized to not require a fixed chunk
size.  Giving a different meaning to extent length (strip length)
would break that, it would no longer be a CYCLIC map.  It's an
un-generalization that has dubious value, since the strip length is
implicit in either the file size or the parent extent length.

In other words: make the server validate the strip size, and all will
be good.  The kernel really doesn't need this information.

> So that's a bug. I've fixed it locally and will share that in the next day
> or two. I also needed to refactor your patch the famfs user space because
> it disabled ABI compatibility with some older versions of the famfs kernel
> that I'm still supporting users with. If you can wait to patch that further
> until I share an update, it will be cleaner on my end.

Yeah, thanks.

> Having said all that, this topic seems to recur and be contentious - we are
> working on tests that can demonstrate bottlenecks - but my current ask is
> that if I can offer an alternative approach to rbtree that is better on
> paper, can we please use it?

If it's not adding complexity, then sure.

Thanks,
Miklos