[PATCH] btrfs: remove the unused @start and @end parameter for btrfs_run_delalloc_range()

[PATCH] btrfs: remove the unused @start and @end parameter for btrfs_run_delalloc_range()

[Qu Wenruo]

Since commit d75855b4518b ("btrfs: Remove
extent_io_ops::writepage_start_hook") removes the writepage_start_hook()
and added btrfs_writepage_cow_fixup() function, there is no need to
follow the old hook parameters.

This patch just remove the @start and @end hook, since currently the
fixup check is full page check, it doesn't need @start and @end hook.

Signed-off-by: Qu Wenruo <wqu@suse.com>

---
Special discussion related to the cow fixup.

Recently I'm exploring the possibility to change how we submit bio for a
delalloc range.

Currently we call writepage_delalloc(), which may add a new delalloc
range, and set all involved pages with Ordered bit.
But all other pages in the delalloc range is not locked.

Then we iterate through each page in the delalloc range, and submit
them.

This window between "delalloc range added" to "submit bio for the range"
allows a page to be invalidated or changed.

I'm not sure if the behavior (with the extra window with page unlocked)
is correct.
But at least we already have compression going through another path.

For compression, we call cow_file_range(), but keeps all the pages in
the range locked, then submit bio for the range, finally unlock the page
range.

This makes sure between "delalloc range added" to "bio submitted" the
page is still locked and won't change.

Not sure if this can eliminate the need for such fixup.

As for the new method, if we hit a dirty page, we always ran delalloc
range for it.

Thus there is no way a dirty page will not be covered by an ordered extent,
thus eliminate the need for fixup.

---
 fs/btrfs/ctree.h     | 2 +-
 fs/btrfs/extent_io.c | 2 +-
 fs/btrfs/inode.c     | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 958fe5d085ea..088f33c01a01 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3190,7 +3190,7 @@ int btrfs_prealloc_file_range_trans(struct inode *inode,
 int btrfs_run_delalloc_range(struct btrfs_inode *inode, struct page *locked_page,
 		u64 start, u64 end, int *page_started, unsigned long *nr_written,
 		struct writeback_control *wbc, bool unlock_pages);
-int btrfs_writepage_cow_fixup(struct page *page, u64 start, u64 end);
+int btrfs_writepage_cow_fixup(struct page *page);
 void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode,
 					  struct page *page, u64 start,
 					  u64 end, int uptodate);
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index b26fd39abd39..b0751920f5ee 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3942,7 +3942,7 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
 	const unsigned int write_flags = wbc_to_write_flags(wbc);
 	bool compressed;
 
-	ret = btrfs_writepage_cow_fixup(page, start, end);
+	ret = btrfs_writepage_cow_fixup(page);
 	if (ret) {
 		/* Fixup worker will requeue */
 		redirty_page_for_writepage(wbc, page);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index baa3c4556a66..684f1ec85351 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2819,7 +2819,7 @@ static void btrfs_writepage_fixup_worker(struct btrfs_work *work)
  * to fix it up.  The async helper will wait for ordered extents, set
  * the delalloc bit and make it safe to write the page.
  */
-int btrfs_writepage_cow_fixup(struct page *page, u64 start, u64 end)
+int btrfs_writepage_cow_fixup(struct page *page)
 {
 	struct inode *inode = page->mapping->host;
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
-- 
2.32.0

Re: [PATCH] btrfs: remove the unused @start and @end parameter for btrfs_run_delalloc_range()

[David Sterba]

On Tue, Jul 27, 2021 at 01:41:32PM +0800, Qu Wenruo wrote:
> Since commit d75855b4518b ("btrfs: Remove
> extent_io_ops::writepage_start_hook") removes the writepage_start_hook()
> and added btrfs_writepage_cow_fixup() function, there is no need to
> follow the old hook parameters.
> 
> This patch just remove the @start and @end hook, since currently the
> fixup check is full page check, it doesn't need @start and @end hook.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Then at least start can be removed from __extent_writepage_io too with
the following

--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3828,9 +3828,8 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
                                 int *nr_ret)
 {
        struct btrfs_fs_info *fs_info = inode->root->fs_info;
-       u64 start = page_offset(page);
-       u64 end = start + PAGE_SIZE - 1;
-       u64 cur = start;
+       u64 cur = page_offset(page);
+       u64 end = cur + PAGE_SIZE - 1;
        u64 extent_offset;
        u64 block_start;
        struct extent_map *em;
---

There's no warning because start is set and used.

Re: [PATCH] btrfs: remove the unused @start and @end parameter for btrfs_run_delalloc_range()

[David Sterba]

On Tue, Jul 27, 2021 at 06:25:07PM +0800, Qu Wenruo wrote:
> 
> 
> On 2021/7/27 下午6:15, David Sterba wrote:
> > On Tue, Jul 27, 2021 at 01:41:32PM +0800, Qu Wenruo wrote:
> >> Since commit d75855b4518b ("btrfs: Remove
> >> extent_io_ops::writepage_start_hook") removes the writepage_start_hook()
> >> and added btrfs_writepage_cow_fixup() function, there is no need to
> >> follow the old hook parameters.
> >>
> >> This patch just remove the @start and @end hook, since currently the
> >> fixup check is full page check, it doesn't need @start and @end hook.
> >>
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >
> > Then at least start can be removed from __extent_writepage_io too with
> > the following
> >
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -3828,9 +3828,8 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
> >                                   int *nr_ret)
> >   {
> >          struct btrfs_fs_info *fs_info = inode->root->fs_info;
> > -       u64 start = page_offset(page);
> > -       u64 end = start + PAGE_SIZE - 1;
> > -       u64 cur = start;
> > +       u64 cur = page_offset(page);
> > +       u64 end = cur + PAGE_SIZE - 1;
> >          u64 extent_offset;
> >          u64 block_start;
> >          struct extent_map *em;
> > ---
> >
> > There's no warning because start is set and used.
> >
> Awesome finding!
> 
> Will update the patch to also include this one.

Hold on, that's just a small fixup but I'm not yet sure about the
nrwritten removal correctness.

Re: [PATCH] btrfs: remove the unused @start and @end parameter for btrfs_run_delalloc_range()

[Qu Wenruo]

On 2021/7/27 下午6:15, David Sterba wrote:
> On Tue, Jul 27, 2021 at 01:41:32PM +0800, Qu Wenruo wrote:
>> Since commit d75855b4518b ("btrfs: Remove
>> extent_io_ops::writepage_start_hook") removes the writepage_start_hook()
>> and added btrfs_writepage_cow_fixup() function, there is no need to
>> follow the old hook parameters.
>>
>> This patch just remove the @start and @end hook, since currently the
>> fixup check is full page check, it doesn't need @start and @end hook.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> Then at least start can be removed from __extent_writepage_io too with
> the following
>
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3828,9 +3828,8 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
>                                   int *nr_ret)
>   {
>          struct btrfs_fs_info *fs_info = inode->root->fs_info;
> -       u64 start = page_offset(page);
> -       u64 end = start + PAGE_SIZE - 1;
> -       u64 cur = start;
> +       u64 cur = page_offset(page);
> +       u64 end = cur + PAGE_SIZE - 1;
>          u64 extent_offset;
>          u64 block_start;
>          struct extent_map *em;
> ---
>
> There's no warning because start is set and used.
>
Awesome finding!

Will update the patch to also include this one.

Thanks,
Qu

Re: [PATCH] btrfs: remove the unused @start and @end parameter for btrfs_run_delalloc_range()

[Qu Wenruo]

On 2021/7/27 下午6:24, David Sterba wrote:
> On Tue, Jul 27, 2021 at 06:25:07PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2021/7/27 下午6:15, David Sterba wrote:
>>> On Tue, Jul 27, 2021 at 01:41:32PM +0800, Qu Wenruo wrote:
>>>> Since commit d75855b4518b ("btrfs: Remove
>>>> extent_io_ops::writepage_start_hook") removes the writepage_start_hook()
>>>> and added btrfs_writepage_cow_fixup() function, there is no need to
>>>> follow the old hook parameters.
>>>>
>>>> This patch just remove the @start and @end hook, since currently the
>>>> fixup check is full page check, it doesn't need @start and @end hook.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>
>>> Then at least start can be removed from __extent_writepage_io too with
>>> the following
>>>
>>> --- a/fs/btrfs/extent_io.c
>>> +++ b/fs/btrfs/extent_io.c
>>> @@ -3828,9 +3828,8 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
>>>                                    int *nr_ret)
>>>    {
>>>           struct btrfs_fs_info *fs_info = inode->root->fs_info;
>>> -       u64 start = page_offset(page);
>>> -       u64 end = start + PAGE_SIZE - 1;
>>> -       u64 cur = start;
>>> +       u64 cur = page_offset(page);
>>> +       u64 end = cur + PAGE_SIZE - 1;
>>>           u64 extent_offset;
>>>           u64 block_start;
>>>           struct extent_map *em;
>>> ---
>>>
>>> There's no warning because start is set and used.
>>>
>> Awesome finding!
>>
>> Will update the patch to also include this one.
>
> Hold on, that's just a small fixup but I'm not yet sure about the
> nrwritten removal correctness.
>
Yeah, after looking into the code, it looks like this is unrelated to
the removal, thus it's better to be an independent fix.

For the correctness, it's indeed a little complex, so take your time.

Thanks,
Qu