Re: [PATCH 01/12] iomap: check if folio size is equal to FS block size

[Darrick J. Wong]

On Sat, Oct 05, 2024 at 03:17:47AM +0100, Matthew Wilcox wrote:
> On Fri, Oct 04, 2024 at 04:04:28PM -0400, Goldwyn Rodrigues wrote:
> > Filesystems such as BTRFS use folio->private so that they receive a
> > callback while releasing folios. Add check if folio size is same as
> > filesystem block size while evaluating iomap_folio_state from
> > folio->private.
> > 
> > I am hoping this will be removed when all of btrfs code has moved to
> > iomap and BTRFS uses iomap's subpage.
> 
> This seems like a terrible explanation for why you need this patch.
> 
> As I understand it, what you're really doing is saying that iomap only
> uses folio->private for block size < folio size.  So if you add this
> hack, iomap won't look at folio->private for block size == folio size
> and that means that btrfs can continue to use it.
> 
> I don't think this is a good way to start the conversion.  I appreciate
> that it's a long, complex procedure, and you can't do the whole
> conversion in a single patchset.
> 
> Also, please stop calling this "subpage".  That's btrfs terminology,
> it's confusing as hell, and it should be deleted from your brain.

I've long wondered if 'subpage' is shorthand for 'subpage blocksize'?
If so then the term makes sense to me as a fs developer, but I can also
see how it might not make sense to anyone from the mm side of things.

Wait, is a btrfs sector the same as what ext4/xfs call a fs block?

> But I don't understand why you need it at all.  btrfs doesn't attach
> private data to folios unless block size < page size.  Which is precisely
> the case that you're not using.  So it seems like you could just drop
> this patch and everything would still work.

I was also wondering this.  Given that the end of struct btrfs_subpage
is an uptodate/dirty/ordered bitmap, maybe iomap_folio_ops should grow a
method to allocate a struct iomap_folio_state object, and then you could
embed one in the btrfs subpage object and provide that custom allocation
function?

(and yes that makes for an ugly mess of pointer math crud to have two
VLAs inside struct btrfs_subpage, so this might be too ugly to live in
practice)

--D