[0/2] btrfs: convert btrfs_buffered_write() to folio interface

Message

Qu Wenruo Oct. 10, 2024, 4:46 a.m. UTC

Inspired by generic_perform_write(), convert btrfs_buffered_write() to
go a page-by-page iteration, then convert it to use folio interface.

This should provide the basis for use to go
address_operations->write_begin() and write_end() callbacks.

There is a tiny timing change.
Before this patchset we wait for the page writeback after we get an
uptodate or no need to read the page.

But now we follow the regular FGP_WRITEBEGIN, which implies FGP_STABLE
and will wait for writeback before reading the page.

Qu Wenruo (2):
  btrfs: make buffered write to copy one page a time
  btrfs: convert btrfs_buffered_write() to use folio interface

 fs/btrfs/file.c             | 259 +++++++++++++-----------------------
 fs/btrfs/file.h             |   2 +-
 fs/btrfs/free-space-cache.c |  15 ++-
 3 files changed, 102 insertions(+), 174 deletions(-)

Comments

David Sterba Oct. 22, 2024, 1:17 a.m. UTC | #1

On Thu, Oct 10, 2024 at 03:16:11PM +1030, Qu Wenruo wrote:
> Inspired by generic_perform_write(), convert btrfs_buffered_write() to
> go a page-by-page iteration, then convert it to use folio interface.
> 
> This should provide the basis for use to go
> address_operations->write_begin() and write_end() callbacks.
> 
> There is a tiny timing change.
> Before this patchset we wait for the page writeback after we get an
> uptodate or no need to read the page.
> 
> But now we follow the regular FGP_WRITEBEGIN, which implies FGP_STABLE
> and will wait for writeback before reading the page.
> 
> Qu Wenruo (2):
>   btrfs: make buffered write to copy one page a time
>   btrfs: convert btrfs_buffered_write() to use folio interface

So this is another step towards folios, ok. The first patch got a LKP
bot report about performance degradataion due to the change from
multiple pages to per page loop, but you mention that. The drop is 16%,
that's not something that we should ignore. It was for one workload so
this is mabye acceptable.

Another thing is that how it's changed to the write begin/end, I don't
understand this enough to give it a yes/no. Also this is the buffered
write so quite fundamental file operation. I've had the patches in
misc-next (and in linux-next) for some time, so we have some testing. I
hope we get more reviews too. We're now in rc4, so it's about the time
to get it in or delay until the next cycle if there are doubts.

Regarding the patches, I have only style comments, you touch a lot of
code that is not changed often so it would be good to fix the style of
code or comments but correctness comes first so this can be fixed later,
I'd really like to be sure this is safe.

My suggestion is to add the patches to for-next, we'll have enough time
to catch problems and expose the new code to more testing environments.

Qu Wenruo Oct. 22, 2024, 2:35 a.m. UTC | #2

在 2024/10/22 11:47, David Sterba 写道:
> On Thu, Oct 10, 2024 at 03:16:11PM +1030, Qu Wenruo wrote:
>> Inspired by generic_perform_write(), convert btrfs_buffered_write() to
>> go a page-by-page iteration, then convert it to use folio interface.
>>
>> This should provide the basis for use to go
>> address_operations->write_begin() and write_end() callbacks.
>>
>> There is a tiny timing change.
>> Before this patchset we wait for the page writeback after we get an
>> uptodate or no need to read the page.
>>
>> But now we follow the regular FGP_WRITEBEGIN, which implies FGP_STABLE
>> and will wait for writeback before reading the page.
>>
>> Qu Wenruo (2):
>>    btrfs: make buffered write to copy one page a time
>>    btrfs: convert btrfs_buffered_write() to use folio interface
> 
> So this is another step towards folios, ok. The first patch got a LKP
> bot report about performance degradataion due to the change from
> multiple pages to per page loop, but you mention that. The drop is 16%,
> that's not something that we should ignore. It was for one workload so
> this is mabye acceptable.

The performance drop in fact is larger than my expectation, and affects 
quite some benchmarks, thus I'm not in a rush to push it for upstream 
anytime soon.

> 
> Another thing is that how it's changed to the write begin/end, I don't
> understand this enough to give it a yes/no.

It's only one step towards that interface, and there are still quite 
some work left.

And the change to write_begin/end interface is not high on my to-do list.

> Also this is the buffered
> write so quite fundamental file operation. I've had the patches in
> misc-next (and in linux-next) for some time, so we have some testing. I
> hope we get more reviews too. We're now in rc4, so it's about the time
> to get it in or delay until the next cycle if there are doubts.
> 
> Regarding the patches, I have only style comments, you touch a lot of
> code that is not changed often so it would be good to fix the style of
> code or comments but correctness comes first so this can be fixed later,
> I'd really like to be sure this is safe.
> 
> My suggestion is to add the patches to for-next, we'll have enough time
> to catch problems and expose the new code to more testing environments.

OK, I can do the push, but if anyone hits any functional problems I'll 
be more than happier to revert/remove them immediately.

Thanks,
Qu