Re: [PATCH v1 1/1] fs/splice: add missing callback for inaccessible pages

[John Hubbard]

On 2020-04-30 12:54, Christian Borntraeger wrote:
> On 30.04.20 21:02, Christian Borntraeger wrote:
> > On 30.04.20 20:12, Christian Borntraeger wrote:
> > > On 29.04.20 18:07, Dave Hansen wrote:
> > > > On 4/28/20 3:50 PM, Claudio Imbrenda wrote:
> > > > > If a page is inaccesible and it is used for things like sendfile, then
> > > > > the content of the page is not always touched, and can be passed
> > > > > directly to a driver, causing issues.
> > > > >
> > > > > This patch fixes the issue by adding a call to arch_make_page_accessible
> > > > > in page_cache_pipe_buf_confirm; this fixes the issue.
> > > >
> > > > I spent about 5 minutes putting together a patch:
> > > >
> > > > 	https://sr71.net/~dave/intel/accessible.patch
> > >
> > > You only set the page flag for compound pages. that of course leaves a big pile
> > > of pages marked a not accessible, thus explaining the sendto trace and all kind
> > > of other random traces.
> > >
> > >
> > > What do you see when you also do the  SetPageAccessible(page);
> > > in the else page of prep_new_page (order == 0).
> > > (I do get > 10000 of these non compound page allocs just during boot).
> >
> > And yes, I think you are right that we should call the callback also for !FOLL_PIN.


Disclaimer: I haven't dug into the details of the latest points above,
so answers below will be narrowly focused.


> Thinking again about this I am no longer sure. Adding John Hubbard.
>
> Documentation/core-api/pin_user_pages.rst says:
> -------snip----------
> Another way of thinking about these flags is as a progression of restrictions:
> FOLL_GET is for struct page manipulation, without affecting the data that the
> struct page refers to. FOLL_PIN is a *replacement* for FOLL_GET, and is for
> short term pins on pages whose data *will* get accessed. As such, FOLL_PIN is
> a "more severe" form of pinning. And finally, FOLL_LONGTERM is an even more
> restrictive case that has FOLL_PIN as a prerequisite: this is for pages that
> will be pinned longterm, and whose data will be accessed.
> -------snip----------
>
> So John,is it ok to give a page to an I/O device where the code has used gup
> with FOLL_GET (or gup fast without pup) or would you consider this a bug?

Well, it's a bug (or a bug-in-waiting): even though gup/FOLL_GET works
just as well (and as badly) as ever, pup/FOLL_PIN is required in order
to safely and correctly allow a non-CPU device to operate on a page's
data. Core mm and fs code is going to key off of page_maybe_dma_pinned()
in order to make critical decisions about writeback and umount, and
FOLL_PIN opts into that; FOLL_GET does not.

Basically, you'd be creating another set of call sites that someone
would have to convert to pup/FOLL_PIN.

btw, on the FOLL_LONGTERM documentation above: that's more of an
aspiration than a description of current behavior, in some ways.
The current FOLL_LONGTERM is a little more quirky than is implied
there.

Also on a related note, I've been slow in posting patches to implement
the remaining call site conversions, and am trying to get back to that
asap. There have been some distractions. :) Once every call site is
correctly using gup or pup, it will be easier for everyone.


thanks,
--
John Hubbard
NVIDIA