On Fri, 8 Apr 2022, Chuck Lever III wrote:
> > On Apr 7, 2022, at 6:26 PM, Hugh Dickins <hughd@xxxxxxxxxx> wrote:
> > On Thu, 7 Apr 2022, Chuck Lever III wrote:
> >>
> >> 847 static int
> >> 848 nfsd_splice_actor(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
> >> 849 struct splice_desc *sd)
> >> 850 {
> >> 851 struct svc_rqst *rqstp = sd->u.data;
> >> 852 struct page **pp = rqstp->rq_next_page;
> >> 853 struct page *page = buf->page;
> >> 854
> >> 855 if (rqstp->rq_res.page_len == 0) {
> >> 856 svc_rqst_replace_page(rqstp, page);
> >> 857 rqstp->rq_res.page_base = buf->offset;
> >> 858 } else if (page != pp[-1]) {
> >> 859 svc_rqst_replace_page(rqstp, page);
> >> 860 }
> >> 861 rqstp->rq_res.page_len += sd->len;
> >> 862
> >> 863 return sd->len;
> >> 864 }
> >>
> >> rq_next_page should point to the first unused element of
> >> rqstp->rq_pages, so IIUC that check is looking for the
> >> final page that is part of the READ payload.
> >>
> >> But that does suggest that if page -> ZERO_PAGE and so does
> >> pp[-1], then svc_rqst_replace_page() would not be invoked.
>
> To put a little more color on this, I think the idea here
> is to prevent releasing the same page twice. It might be
> possible that NFSD can add the same page to the rq_pages
> array more than once, and we don't want to do a double
> put_page().
>
> The only time I can think this might happen is if the
> READ payload is partially contained in the page that
> contains the NFS header. I'm not sure that can ever
> happen these days.
I'd have thought that if a page were repeated, then its refcount would
have been raised twice, and so require a double put_page(). But it's
no concern of mine. The only thing I'd say is, if you do find a good
way to robustify that code for duplicates, please don't make it
conditional on ZERO_PAGE - that's just a special case which I
mis-introduced and is now about to go away.
> >
> > We might be able to avoid that revert, and go the whole way to using
> > iov_iter_zero() instead. But the significant slowness of clear_user()
> > relative to copy to user, on x86 at least, does ask for a hybrid.
> >
> > Suggested patch below, on top of 5.18-rc1, passes my own testing:
> > but will it pass yours? It seems to me safe, and as fast as before,
> > but we don't know yet if this iov_iter_zero() works right for you.
> > Chuck, please give it a go and let us know.
>
> Applied to stock v5.18-rc1. The tests pass as expected.
Great, thanks a lot, I'll move ahead with sending akpm the patch
with a proper commit message.
Hugh
