On 2022-09-23 17:07, Jason Gunthorpe wrote: > On Fri, Sep 23, 2022 at 05:01:26PM -0600, Logan Gunthorpe wrote: >> >> >> >> On 2022-09-23 16:58, Jason Gunthorpe wrote: >>> On Fri, Sep 23, 2022 at 02:11:03PM -0600, Logan Gunthorpe wrote: >>>> >>>> >>>> On 2022-09-23 13:53, Jason Gunthorpe wrote: >>>>> On Fri, Sep 23, 2022 at 01:08:31PM -0600, Logan Gunthorpe wrote: >>>>> I'm encouraging Dan to work on better infrastructure in pgmap core >>>>> because every pgmap implementation has this issue currently. >>>>> >>>>> For that reason it is probably not so relavent to this series. >>>>> >>>>> Perhaps just clarify in the commit message that the FOLL_LONGTERM >>>>> restriction is to copy DAX until the pgmap page refcounts are fixed. >>>> >>>> Ok, I'll add that note. >>>> >>>> Per the fix for the try_grab_page(), to me it doesn't fit well in >>>> try_grab_page() without doing a bunch of cleanup to change the >>>> error handling, and the same would have to be added to try_grab_folio(). >>>> So I think it's better to leave it where it was, but move it below the >>>> respective grab calls. Does the incremental patch below look correct? >>> >>> Oh? I was thinking of just a very simple thing: >> >> Really would like it to return -EREMOTEIO instead of -ENOMEM as that's the >> error used for bad P2PDMA page everywhere. > > I'd rather not see GUP made more fragile just for that.. Not sure how that's more fragile... You're way seems more dangerous given the large number of call sites we are adding it to when it might not apply. > >> Plus the concern that some of the callsites of try_grab_page() might not have >> a get or a pin and thus it's not safe which was the whole point of the change >> anyway. > > try_grab_page() calls folio_ref_inc(), that is only legal if it knows > the page is already a valid pointer under the PTLs, so it is safe to > check the pgmap as well. My point is it doesn't get a reference or a pin unless FOLL_PIN or FOLL_GET is set and the documentation states that neither might be set, in which case folio_ref_inc() will not be called... Logan
