Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Apr 2023 10:46:24 +0530
Message-Id: <87ttxfhyvr.fsf@doe.com>
From:   Ritesh Harjani (IBM) <ritesh.list@gmail.com>
To:     Matthew Wilcox <willy@infradead.org>
Cc:     linux-ext4@vger.kernel.org, Jan Kara <jack@suse.cz>,
        Ojaswin Mujoo <ojaswin@linux.ibm.com>,
        Disha Goel <disgoel@linux.ibm.com>
Subject: Re: [RFCv1 3/4] ext4: Make mpage_journal_page_buffers use folio
In-Reply-To: <87zg77ici3.fsf@doe.com>
Precedence: bulk

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