On 10 Dec 2019, at 12:02, Jens Axboe wrote:
> On 12/10/19 9:55 AM, Matthew Wilcox wrote:
>> On Tue, Dec 10, 2019 at 09:24:52AM -0700, Jens Axboe wrote:
>>> +/*
>>> + * Start writeback on the pages in pgs[], and then try and remove
>>> those pages
>>> + * from the page cached. Used with RWF_UNCACHED.
>>> + */
>>> +void write_drop_cached_pages(struct page **pgs, struct
>>> address_space *mapping,
>>> + unsigned *nr)
>>
>> It would seem more natural to use a pagevec instead of pgs/nr.
>
> I did look into that, but they are intertwined with LRU etc. I
> deliberately avoided the LRU on the read side, as it adds noticeable
> overhead and gains us nothing since the pages will be dropped agian.
>
>>> +{
>>> + loff_t start, end;
>>> + int i;
>>> +
>>> + end = 0;
>>> + start = LLONG_MAX;
>>> + for (i = 0; i < *nr; i++) {
>>> + struct page *page = pgs[i];
>>> + loff_t off;
>>> +
>>> + off = (loff_t) page_to_index(page) << PAGE_SHIFT;
>>
>> Isn't that page_offset()?
>
> I guess it is! I'll make that change.
>
>>> + __filemap_fdatawrite_range(mapping, start, end, WB_SYNC_NONE);
>>> +
>>> + for (i = 0; i < *nr; i++) {
>>> + struct page *page = pgs[i];
>>> +
>>> + lock_page(page);
>>> + if (page->mapping == mapping) {
>>
>> So you're protecting against the page being freed and reallocated to
>> a
>> different file, but not against the page being freed and reallocated
>> to a location in the same file which is outside (start, end)?
>
> I guess so, we can add that too, probably just check if the index is
> still the same. More of a behavioral thing, shouldn't be any
> correctness issues there.
Since we have a reference on the page, the mapping can go to NULL but
otherwise it should stay in the same mapping at the same offset.
But, Jens and I both just realized he needs to take the reference on the
page before write_end is called.
-chris
