public inbox for [email protected]
 help / color / mirror / Atom feed
From: Johannes Weiner <[email protected]>
To: Pavel Begunkov <[email protected]>
Cc: Christoph Hellwig <[email protected]>,
	Jens Axboe <[email protected]>,
	Alexander Viro <[email protected]>,
	[email protected], [email protected],
	[email protected], [email protected],
	Matthew Wilcox <[email protected]>
Subject: Re: [PATCH 2/2] block: no-copy bvec for direct IO
Date: Fri, 11 Dec 2020 17:13:52 +0100	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>

On Fri, Dec 11, 2020 at 03:47:23PM +0000, Pavel Begunkov wrote:
> On 11/12/2020 15:38, Johannes Weiner wrote:
> > On Fri, Dec 11, 2020 at 02:20:11PM +0000, Pavel Begunkov wrote:
> >> On 11/12/2020 14:06, Johannes Weiner wrote:
> >>> On Wed, Dec 09, 2020 at 08:40:05AM +0000, Christoph Hellwig wrote:
> >>>>> +	/*
> >>>>> +	 * In practice groups of pages tend to be accessed/reclaimed/refaulted
> >>>>> +	 * together. To not go over bvec for those who didn't set BIO_WORKINGSET
> >>>>> +	 * approximate it by looking at the first page and inducing it to the
> >>>>> +	 * whole bio
> >>>>> +	 */
> >>>>> +	if (unlikely(PageWorkingset(iter->bvec->bv_page)))
> >>>>> +		bio_set_flag(bio, BIO_WORKINGSET);
> >>>>
> >>>> IIRC the feedback was that we do not need to deal with BIO_WORKINGSET
> >>>> at all for direct I/O.
> >>>
> >>> Yes, this hunk is incorrect. We must not use this flag for direct IO.
> >>> It's only for paging IO, when you bring in the data at page->mapping +
> >>> page->index. Otherwise you tell the pressure accounting code that you
> >>> are paging in a thrashing page, when really you're just reading new
> >>> data into a page frame that happens to be hot.
> >>>
> >>> (As per the other thread, bio_add_page() currently makes that same
> >>> mistake for direct IO. I'm fixing that.)
> >>
> >> I have that stuff fixed, it just didn't go into the RFC. That's basically
> >> removing replacing add_page() with its version without BIO_WORKINGSET
> 
> I wrote something strange... Should have been "replacing add_page() in
> those functions with a version without BIO_WORKINGSET".

No worries, I understood.

> >> in bio_iov_iter_get_pages() and all __bio_iov_*_{add,get}_pages() +
> >> fix up ./fs/direct-io.c. Should cover all direct cases if I didn't miss
> >> some.
> > 
> > Ah, that's fantastic! Thanks for clarifying.
> 
> To keep it clear, do we go with what I have stashed (I'm planning to
> reiterate this weekend)? or you're going to write it up yourself?
> Just in case there is some cooler way you have in mind :)

Honestly, I only wrote all my ideas down and asked for feedback
because I wasn't super excited about any of them ;-)

If your changes happen to separate the direct io path from the
buffered io path naturally, I'm okay with it.

I'd say let's go with what you already have and see whether Jens and
Christoph like it. We can always do follow-on cleanups.

  reply	other threads:[~2020-12-11 17:23 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-09  2:19 [RFC 0/2] nocopy bvec for direct IO Pavel Begunkov
2020-12-09  2:19 ` [PATCH 1/2] iov: introduce ITER_BVEC_FLAG_FIXED Pavel Begunkov
2020-12-09  8:36   ` Christoph Hellwig
2020-12-09  9:06     ` Christoph Hellwig
2020-12-09 11:54       ` Pavel Begunkov
2020-12-09 13:07     ` Al Viro
2020-12-09 13:37       ` Pavel Begunkov
2020-12-09 17:55         ` Christoph Hellwig
2020-12-09 18:24           ` Matthew Wilcox
2020-12-13 22:09             ` Pavel Begunkov
2020-12-09  2:19 ` [PATCH 2/2] block: no-copy bvec for direct IO Pavel Begunkov
2020-12-09  8:40   ` Christoph Hellwig
2020-12-09 12:01     ` Pavel Begunkov
2020-12-09 12:05       ` Christoph Hellwig
2020-12-09 12:03         ` Pavel Begunkov
2020-12-11 14:06     ` Johannes Weiner
2020-12-11 14:20       ` Pavel Begunkov
2020-12-11 15:38         ` Johannes Weiner
2020-12-11 15:47           ` Pavel Begunkov
2020-12-11 16:13             ` Johannes Weiner [this message]
2020-12-09 21:13   ` David Laight
2020-12-09  6:50 ` [RFC 0/2] nocopy " Christoph Hellwig
2020-12-09 11:54   ` Pavel Begunkov
2020-12-09 16:53 ` Jens Axboe
2020-12-13 22:03   ` Pavel Begunkov
2020-12-09 17:06 ` Al Viro

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox