2023-04-16 18:38:06

by Ritesh Harjani

[permalink] [raw]
Subject: [RFCv1 3/4] ext4: Make mpage_journal_page_buffers use folio

This patch converts mpage_journal_page_buffers() to use folio and also
removes the PAGE_SIZE assumption.

Signed-off-by: Ritesh Harjani (IBM) <[email protected]>
---
fs/ext4/inode.c | 30 ++++++++++++++----------------
1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 5bb141288b1b..3f76b06e9aa4 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2349,11 +2349,11 @@ static bool ext4_folio_nomap_can_writeout(struct folio *folio)
return false;
}

-static int ext4_journal_page_buffers(handle_t *handle, struct page *page,
- int len)
+static int ext4_journal_page_buffers(handle_t *handle, struct folio *folio,
+ size_t len)
{
- struct buffer_head *page_bufs = page_buffers(page);
- struct inode *inode = page->mapping->host;
+ struct buffer_head *page_bufs = folio_buffers(folio);
+ struct inode *inode = folio->mapping->host;
int ret, err;

ret = ext4_walk_page_buffers(handle, inode, page_bufs, 0, len,
@@ -2362,7 +2362,7 @@ static int ext4_journal_page_buffers(handle_t *handle, struct page *page,
NULL, write_end_fn);
if (ret == 0)
ret = err;
- err = ext4_jbd2_inode_add_write(handle, inode, page_offset(page), len);
+ err = ext4_jbd2_inode_add_write(handle, inode, folio_pos(folio), len);
if (ret == 0)
ret = err;
EXT4_I(inode)->i_datasync_tid = handle->h_transaction->t_tid;
@@ -2374,23 +2374,21 @@ static int ext4_journal_page_buffers(handle_t *handle, struct page *page,

static int mpage_journal_page_buffers(handle_t *handle,
struct mpage_da_data *mpd,
- struct page *page)
+ struct folio *folio)
{
struct inode *inode = mpd->inode;
loff_t size = i_size_read(inode);
- int len;
+ size_t len = folio_size(folio);

- ClearPageChecked(page);
- clear_page_dirty_for_io(page);
+ folio_clear_checked(folio);
+ folio_clear_dirty_for_io(folio);
mpd->wbc->nr_to_write--;

- if (page->index == size >> PAGE_SHIFT &&
+ if (folio_pos(folio) + len > size &&
!ext4_verity_in_progress(inode))
- len = size & ~PAGE_MASK;
- else
- len = PAGE_SIZE;
+ len = size - folio_pos(folio);

- return ext4_journal_page_buffers(handle, page, len);
+ return ext4_journal_page_buffers(handle, folio, len);
}

/*
@@ -2546,7 +2544,7 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
WARN_ON_ONCE(sb->s_writers.frozen >=
SB_FREEZE_FS);
err = mpage_journal_page_buffers(handle,
- mpd, &folio->page);
+ mpd, folio);
if (err < 0)
goto out;
}
@@ -6184,7 +6182,7 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
err = __block_write_begin(&folio->page, 0, len, ext4_get_block);
if (!err) {
ret = VM_FAULT_SIGBUS;
- if (ext4_journal_page_buffers(handle, &folio->page, len))
+ if (ext4_journal_page_buffers(handle, folio, len))
goto out_error;
} else {
folio_unlock(folio);
--
2.39.2


2023-04-16 19:48:40

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [RFCv1 3/4] ext4: Make mpage_journal_page_buffers use folio

On Mon, Apr 17, 2023 at 12:01:52AM +0530, Ritesh Harjani (IBM) wrote:
> This patch converts mpage_journal_page_buffers() to use folio and also
> removes the PAGE_SIZE assumption.

Bit of an oversight on my part. I neglected to do this after Jan added
it. Perils of parallel development ...

> -static int ext4_journal_page_buffers(handle_t *handle, struct page *page,
> - int len)
> +static int ext4_journal_page_buffers(handle_t *handle, struct folio *folio,
> + size_t len)

Should this be called ext4_journal_folio_buffers?

2023-04-17 00:42:48

by Ritesh Harjani

[permalink] [raw]
Subject: Re: [RFCv1 3/4] ext4: Make mpage_journal_page_buffers use folio

Matthew Wilcox <[email protected]> writes:

> On Mon, Apr 17, 2023 at 12:01:52AM +0530, Ritesh Harjani (IBM) wrote:
>> This patch converts mpage_journal_page_buffers() to use folio and also
>> removes the PAGE_SIZE assumption.
>
> Bit of an oversight on my part. I neglected to do this after Jan added
> it. Perils of parallel development ...
>

Yes, these got left overs because of the parallel series.

>> -static int ext4_journal_page_buffers(handle_t *handle, struct page *page,
>> - int len)
>> +static int ext4_journal_page_buffers(handle_t *handle, struct folio *folio,
>> + size_t len)
>
> Should this be called ext4_journal_folio_buffers?

Sure. Will make the change. Otherwise this patch looks good to you?
I also had a query regarding setting "len = size - folio_pos(folio)" in this patch.
Details of which I had pasted in the cover letter. Let me copy-paste
it here from the cover letter. Could you please take a look at it?


<copy-paste>
Also had a query w.r.t your change [1]. I couldn't understand this change diff
from [1]. Given if we are making the conversion to folio, then shouldn't we do
len = size - folio_pos(pos), instead of len = size & ~PAGE_MASK
Could you please tell if the current change in [1] is kept deliberately?
At other places you did make len as size - folio_pos(pos) which removes the
PAGE_SIZE assumption.

-static int mpage_submit_page(struct mpage_da_data *mpd, struct page *page)
+static int mpage_submit_folio(struct mpage_da_data *mpd, struct folio *folio)
{
- int len;
+ size_t len;

<...>

size = i_size_read(mpd->inode);
- if (page->index == size >> PAGE_SHIFT &&
+ len = folio_size(folio);
+ if (folio_pos(folio) + len > size &&
!ext4_verity_in_progress(mpd->inode))
len = size & ~PAGE_MASK;
- else
- len = PAGE_SIZE;
- err = ext4_bio_write_page(&mpd->io_submit, page, len);
+ err = ext4_bio_write_page(&mpd->io_submit, &folio->page, len);
if (!err)
mpd->wbc->nr_to_write--;

[1]: https://lore.kernel.org/linux-ext4/[email protected]/


Thanks for the quick review!

-ritesh

2023-04-17 05:21:28

by Ritesh Harjani

[permalink] [raw]
Subject: Re: [RFCv1 3/4] ext4: Make mpage_journal_page_buffers use folio

Ritesh Harjani (IBM) <[email protected]> writes:

> Matthew Wilcox <[email protected]> writes:
>
>> On Mon, Apr 17, 2023 at 12:01:52AM +0530, Ritesh Harjani (IBM) wrote:
>>> This patch converts mpage_journal_page_buffers() to use folio and also
>>> removes the PAGE_SIZE assumption.
>>
>> Bit of an oversight on my part. I neglected to do this after Jan added
>> it. Perils of parallel development ...
>>
>
> Yes, these got left overs because of the parallel series.
>
>>> -static int ext4_journal_page_buffers(handle_t *handle, struct page *page,
>>> - int len)
>>> +static int ext4_journal_page_buffers(handle_t *handle, struct folio *folio,
>>> + size_t len)
>>
>> Should this be called ext4_journal_folio_buffers?
>
> Sure. Will make the change. Otherwise this patch looks good to you?
> I also had a query regarding setting "len = size - folio_pos(folio)" in this patch.
> Details of which I had pasted in the cover letter. Let me copy-paste
> it here from the cover letter. Could you please take a look at it?
>
>
> <copy-paste>
> Also had a query w.r.t your change [1]. I couldn't understand this change diff
> from [1]. Given if we are making the conversion to folio, then shouldn't we do
> len = size - folio_pos(pos), instead of len = size & ~PAGE_MASK
> Could you please tell if the current change in [1] is kept deliberately?
> At other places you did make len as size - folio_pos(pos) which removes the
> PAGE_SIZE assumption.
>
> -static int mpage_submit_page(struct mpage_da_data *mpd, struct page *page)
> +static int mpage_submit_folio(struct mpage_da_data *mpd, struct folio *folio)
> {
> - int len;
> + size_t len;
>
> <...>
>
> size = i_size_read(mpd->inode);
> - if (page->index == size >> PAGE_SHIFT &&
> + len = folio_size(folio);
> + if (folio_pos(folio) + len > size &&
> !ext4_verity_in_progress(mpd->inode))
> len = size & ~PAGE_MASK;
> - else
> - len = PAGE_SIZE;
> - err = ext4_bio_write_page(&mpd->io_submit, page, len);
> + err = ext4_bio_write_page(&mpd->io_submit, &folio->page, len);
> if (!err)
> mpd->wbc->nr_to_write--;
>
> [1]: https://lore.kernel.org/linux-ext4/[email protected]/

Here is the complete function. Looking at it again, I think we should make
len = size - folio_pos(folio) (at linenumber 26, like how it is done at
other places in ext4-folio patches), because we now call
ext4_bio_write_folio() instead of ext4_bio_write_page().

Although I know it doesn't make a difference in the functionality today
since folio_size(folio) today in case of ext4 is still PAGE_SIZE.

Please let me know if this understanding is correct. If yes, then I can
write a patch to make len = size - folio_pos(folio) at line 26.

If not I will be happy to know more about what am I missing.

1 static int mpage_submit_folio(struct mpage_da_data *mpd, struct folio *folio)
2 {
3 size_t len;
4 loff_t size;
5 int err;
6
7 BUG_ON(folio->index != mpd->first_page);
8 folio_clear_dirty_for_io(folio);
9 /*
10 * We have to be very careful here! Nothing protects writeback path
11 * against i_size changes and the page can be writeably mapped into
12 * page tables. So an application can be growing i_size and writing
13 * data through mmap while writeback runs. folio_clear_dirty_for_io()
14 * write-protects our page in page tables and the page cannot get
15 * written to again until we release folio lock. So only after
16 * folio_clear_dirty_for_io() we are safe to sample i_size for
17 * ext4_bio_write_folio() to zero-out tail of the written page. We rely
18 * on the barrier provided by folio_test_clear_dirty() in
19 * folio_clear_dirty_for_io() to make sure i_size is really sampled only
20 * after page tables are updated.
21 */
22 size = i_size_read(mpd->inode);
23 len = folio_size(folio);
24 if (folio_pos(folio) + len > size &&
25 !ext4_verity_in_progress(mpd->inode))
26 len = size & ~PAGE_MASK;
27 err = ext4_bio_write_folio(&mpd->io_submit, folio, len);
28 if (!err)
29 mpd->wbc->nr_to_write--;
30
31 return err;
32 }

Thanks a lot!!
-ritesh