> On Apr 8, 2022, at 3:09 PM, Hugh Dickins <hughd@xxxxxxxxxx> wrote:
>
> 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.
100% agree on both counts: not sure (yet) why get_page() was not
used here, and a special case for ZERO_PAGE would be a band-aid.
Anyway, I haven't found a case yet where a duplicate struct page
appears in rq_pages with current kernels.
--
Chuck Lever
