Re: Regression in xfstests on tmpfs-backed NFS exports

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> 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







[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux