| Message ID | 9d0fe99d45e88fd0446df745c31b561fefab898e.1681669004.git.ritesh.list@gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | ext4: misc left over folio changes | expand |
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?
Matthew Wilcox <willy@infradead.org> 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/20230324180129.1220691-7-willy@infradead.org/ Thanks for the quick review! -ritesh
Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes: > Matthew Wilcox <willy@infradead.org> 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/20230324180129.1220691-7-willy@infradead.org/ 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
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);
This patch converts mpage_journal_page_buffers() to use folio and also removes the PAGE_SIZE assumption. Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> --- fs/ext4/inode.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-)