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

[Goldwyn Rodrigues]

On  9:57 07/10, Darrick J. Wong wrote:
> 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.

Yes, it is subpage blocksize.

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

Yup, fs_info->sectorsize.

> 
> > 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)
> 

btrfs does use iomap->private  and writes out EXTENT_FOLIO_PRIVATE. This
is not ideal, but it requires it to get a callback from mm before folios
are released. Refer set_folio_extent_mapped(). BTRFS does it for every
folio (for filesystems which is not a subpage blocksize). Perhaps there
is a better way to do this?

Ideally, after the move to iomap, we should not require btrfs_subpage
structures, and most (if not all) folio "handlings" will be done by
iomap but that is still far way off.

-- 
Goldwyn