On 10 Dec 2019, at 12:02, Jens Axboe wrote:

On 12/10/19 9:55 AM, Matthew Wilcox wrote:

quoted

On Tue, Dec 10, 2019 at 09:24:52AM -0700, Jens Axboe wrote:

quoted

+/*
+ * 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.

quoted

quoted

+{
+	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.

quoted

quoted

+	__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