About using on-stack fsdata pointer for write_begin() and write_end() callbacks

About using on-stack fsdata pointer for write_begin() and write_end() callbacks

[Qu Wenruo]

Hi,

Recently I'm working on migrating btrfs_buffered_write() to utilize
write_begin() and write_end() callbacks.

Currently only the following filesystems really utilizing that pointer:

- bcachefs
   Which is a structure of 24 bytes without any extra pointer.

- f2fs (for compression)
   Which is holding a pointer to an array of pages.

- ext4
   Only utilize that pointer as a flag for ext4_da_write_begin()

- ocfs2
   This a large structure holding a lot of things

- (Future) btrfs
   Only holds a pointer and a bool.
   (Also needs a way to pass ki_flags to support IOCB_NOWAIT though)

Thus I'm wondering should we make perform_generic_write() to accept a
*fsdata pointer, other than making write_begin() to allocate one.
So that we only need to allocate the memory (or use the on-stack one)
once per write, other than once per folio.

This will cause no change to f2fs/ext4, but should benefit
ocfs2/bcachefs and of-course btrfs.

Or is there some special corner case that relies on the current behavior?

Thanks,
Qu

Re: About using on-stack fsdata pointer for write_begin() and write_end() callbacks

[Christoph Hellwig]

On Mon, Nov 11, 2024 at 02:57:06PM +1030, Qu Wenruo wrote:
> Hi,
> 
> Recently I'm working on migrating btrfs_buffered_write() to utilize
> write_begin() and write_end() callbacks.

Why?  They aren't exactly efficient, and it's just going to create
more Churn for Goldwyn's iomap work.

> Currently only the following filesystems really utilizing that pointer:
> 
> - bcachefs
>   Which is a structure of 24 bytes without any extra pointer.

And as pointed out last time willy and I did go through the users of
write_begin/end this is just dead code that is never called.

> Thus I'm wondering should we make perform_generic_write() to accept a
> *fsdata pointer, other than making write_begin() to allocate one.
> So that we only need to allocate the memory (or use the on-stack one)
> once per write, other than once per folio.

And that scheme was one of my suggestions back then, together with
removing write_begin/end from address_space_operations because they
aren't operations called by MM/pagecache code, but just callbacks
provided by the file system to perform_generic_write.

Re: About using on-stack fsdata pointer for write_begin() and write_end() callbacks

[Qu Wenruo]

在 2024/11/11 16:22, Christoph Hellwig 写道:
> On Mon, Nov 11, 2024 at 02:57:06PM +1030, Qu Wenruo wrote:
>> Hi,
>>
>> Recently I'm working on migrating btrfs_buffered_write() to utilize
>> write_begin() and write_end() callbacks.
> 
> Why?  They aren't exactly efficient, and it's just going to create
> more Churn for Goldwyn's iomap work.

So it is not recommended to go the write_begin() and write_end() 
callbacks at all?
Or just not recommended for btrfs?

I know there are limits like those call backs do not support 
IOCB_NOWAIT, and the memory allocation inefficient problem (it should 
only affect ocfs2), but shouldn't we encourage to use the more common 
paths where all other fses go?

> 
>> Currently only the following filesystems really utilizing that pointer:
>>
>> - bcachefs
>>    Which is a structure of 24 bytes without any extra pointer.
> 
> And as pointed out last time willy and I did go through the users of
> write_begin/end this is just dead code that is never called.
> 
>> Thus I'm wondering should we make perform_generic_write() to accept a
>> *fsdata pointer, other than making write_begin() to allocate one.
>> So that we only need to allocate the memory (or use the on-stack one)
>> once per write, other than once per folio.
> 
> And that scheme was one of my suggestions back then, together with
> removing write_begin/end from address_space_operations because they
> aren't operations called by MM/pagecache code, but just callbacks
> provided by the file system to perform_generic_write.
>

Mind to point me to the old discussion thread? I'd like to know why we 
didn't go that path.

Thanks,
Qu

Re: About using on-stack fsdata pointer for write_begin() and write_end() callbacks

[Christoph Hellwig]

On Mon, Nov 11, 2024 at 06:06:57PM +1030, Qu Wenruo wrote:
> > Why?  They aren't exactly efficient, and it's just going to create
> > more Churn for Goldwyn's iomap work.
> 
> So it is not recommended to go the write_begin() and write_end() callbacks
> at all?
> Or just not recommended for btrfs?

They aren't a very efficient model, so I would not recommend to add
new users.

> I know there are limits like those call backs do not support IOCB_NOWAIT,
> and the memory allocation inefficient problem (it should only affect ocfs2),
> but shouldn't we encourage to use the more common paths where all other fses
> go?

I'd recommend to use iomap.

> > And that scheme was one of my suggestions back then, together with
> > removing write_begin/end from address_space_operations because they
> > aren't operations called by MM/pagecache code, but just callbacks
> > provided by the file system to perform_generic_write.
> > 
> 
> Mind to point me to the old discussion thread? I'd like to know why we
> didn't go that path.

Probably because no one did the work yet.  I don't have a pointer at
hand, but it was a discussion willy kicked up about converting
write_begin/end to folios.

Re: About using on-stack fsdata pointer for write_begin() and write_end() callbacks

[Qu Wenruo]

在 2024/11/12 15:35, Christoph Hellwig 写道:
> On Mon, Nov 11, 2024 at 06:06:57PM +1030, Qu Wenruo wrote:
>>> Why?  They aren't exactly efficient, and it's just going to create
>>> more Churn for Goldwyn's iomap work.
>>
>> So it is not recommended to go the write_begin() and write_end() callbacks
>> at all?
>> Or just not recommended for btrfs?
> 
> They aren't a very efficient model, so I would not recommend to add
> new users.

No wonder no one is adding support for IOCB_NOWAIT.

> 
>> I know there are limits like those call backs do not support IOCB_NOWAIT,
>> and the memory allocation inefficient problem (it should only affect ocfs2),
>> but shouldn't we encourage to use the more common paths where all other fses
>> go?
> 
> I'd recommend to use iomap.

Definitely the way we will go in the long run.

Although I'm still struggling on the out-of-band dirty folio (someone 
marked a folio dirty without notifying the fs) handling.

The iomap writepages implementation will just mark all the folio range 
dirty and start mapping.

So I guess there must be some fs specific handling inside the mapping 
function, let me dig it deeper and check Goldwyn's work for details.

Anyway thanks a lot pointing out that the write_begin() model is already 
kinda deprecated.

Thanks,
Qu

Re: About using on-stack fsdata pointer for write_begin() and write_end() callbacks

[Christoph Hellwig]

On Tue, Nov 12, 2024 at 04:01:42PM +1030, Qu Wenruo wrote:
> Although I'm still struggling on the out-of-band dirty folio (someone marked
> a folio dirty without notifying the fs) handling.

No one is allowed to mark pages dirty without file system involvement.

> The iomap writepages implementation will just mark all the folio range dirty
> and start mapping.

iomap writepages (just like any other writepages) never marks folios
dirty, it clears the dirty bit.

Re: About using on-stack fsdata pointer for write_begin() and write_end() callbacks

[Qu Wenruo]

在 2024/11/12 16:11, Christoph Hellwig 写道:
> On Tue, Nov 12, 2024 at 04:01:42PM +1030, Qu Wenruo wrote:
>> Although I'm still struggling on the out-of-band dirty folio (someone marked
>> a folio dirty without notifying the fs) handling.
>
> No one is allowed to mark pages dirty without file system involvement.

Then iomap should go the ext4 way, warning and error out.
(the ext4_warning_inode() inside mpage_prepare_extent_to_map())

But it's not.

IIRC it's related to the get_user_page() shenanigans but not 100% sure.

>
>> The iomap writepages implementation will just mark all the folio range dirty
>> and start mapping.
>
> iomap writepages (just like any other writepages) never marks folios
> dirty, it clears the dirty bit.
>

I'm talking about the iomap_set_range_dirty() call inside
iomap_writepage_map(), for the "if (i_blocks_per_folio() > 1)" branch.

If every dirty page is going through the fs interfaces, we should not
have a dirty folio without that iomap_folio_state attached.

But iomap just ignores such case and try writeback the whole folio
range. In that case, it can cause problems like the range doesn't have
space properly reserved.

Thanks,
Qu

Re: About using on-stack fsdata pointer for write_begin() and write_end() callbacks

[Christoph Hellwig]

On Tue, Nov 12, 2024 at 04:33:56PM +1030, Qu Wenruo wrote:
> IIRC it's related to the get_user_page() shenanigans but not 100% sure.

While there might be a few stragglers left, everyone should be using
pin_user_pages when manipulating page contents.  The long tolerated
just pin and mark dirty is officially a bug now.

> I'm talking about the iomap_set_range_dirty() call inside
> iomap_writepage_map(), for the "if (i_blocks_per_folio() > 1)" branch.

That does not set the page dirty.  It propagatates the per-folio
dirty state into the per-block dirty bitmap allocated in the line
above.

Re: About using on-stack fsdata pointer for write_begin() and write_end() callbacks

[Qu Wenruo]

在 2024/11/12 16:36, Christoph Hellwig 写道:
> On Tue, Nov 12, 2024 at 04:33:56PM +1030, Qu Wenruo wrote:
>> IIRC it's related to the get_user_page() shenanigans but not 100% sure.
>
> While there might be a few stragglers left, everyone should be using
> pin_user_pages when manipulating page contents.  The long tolerated
> just pin and mark dirty is officially a bug now.

Great to know that.

>
>> I'm talking about the iomap_set_range_dirty() call inside
>> iomap_writepage_map(), for the "if (i_blocks_per_folio() > 1)" branch.
>
> That does not set the page dirty.  It propagatates the per-folio
> dirty state into the per-block dirty bitmap allocated in the line
> above.

I know, I just want to say it's marking the whole folio to be written back.

And in that out-of-band case, shouldn't we error out now instead of
marking the range to be written back?
Especially such out-of-band folio is unstable and can lead to other
problems, like content change halfway and causing csum mismatch.

And if that's the case, I think it will be much easier for btrfs to get
rid of its cow fixup workaround.

Thanks,
Qu