On Fri, Sep 22, 2023 at 9:54 AM Yang Shi <shy828301@xxxxxxxxx> wrote: > > On Tue, Sep 19, 2023 at 10:44 PM Saurabh Singh Sengar > <ssengar@xxxxxxxxxxxxxxxxxxx> wrote: > > > > On Tue, Sep 05, 2023 at 11:58:17PM -0700, Saurabh Singh Sengar wrote: > > > On Fri, Aug 25, 2023 at 08:09:07AM -0700, Zach O'Keefe wrote: > > > > On Fri, Aug 25, 2023 at 5:58 AM David Hildenbrand <david@xxxxxxxxxx> wrote: > > > > > > > > > > On 25.08.23 14:49, Matthew Wilcox wrote: > > > > > > On Fri, Aug 25, 2023 at 09:59:23AM +0200, David Hildenbrand wrote: > > > > > >> Especially, we do have bigger ->huge_fault changes coming up: > > > > > >> > > > > > >> https://lkml.kernel.org/r/20230818202335.2739663-1-willy@xxxxxxxxxxxxx > > > > > > > > FWIW, one of those patches updates the docs to read, > > > > > > > > "->huge_fault() is called when there is no PUD or PMD entry present. This > > > > gives the filesystem the opportunity to install a PUD or PMD sized page. > > > > Filesystems can also use the ->fault method to return a PMD sized page, > > > > so implementing this function may not be necessary. In particular, > > > > filesystems should not call filemap_fault() from ->huge_fault(). [..]" > > > > > > > > Which won't work (in the general case) without this patch (well, at > > > > least the ->huge_fault() check part). > > > > > > > > So, if we're advertising this is the way it works, maybe that gives a > > > > stronger argument for addressing it sooner vs when the first in-tree > > > > user depends on it? > > > > > > > > > >> If the driver is not in the tree, people don't care. > > > > > >> > > > > > >> You really should try upstreaming that driver. > > > > > >> > > > > > >> > > > > > >> So this patch here adds complexity (which I don't like) in order to keep an > > > > > >> OOT driver working -- possibly for a short time. I'm tempted to say "please > > > > > >> fix your driver to not use huge faults in that scenario, it is no longer > > > > > >> supported". > > > > > >> > > > > > >> But I'm just about to vanish for 1.5 week into vacation :) > > > > > >> > > > > > >> @Willy, what are your thoughts? > > > > > > > > > > > > Fundamentally there was a bad assumption with the original patch -- > > > > > > it assumed that the only reason to support ->huge_fault was for DAX, > > > > > > and that's not true. It's just that the only drivers in-tree which > > > > > > support ->huge_fault do so in order to support DAX. > > > > > > > > > > Okay, and we are willing to continue supporting that then and it's > > > > > nothing we want to stop OOT drivers from doing. > > > > > > > > > > Fine with me; we should probably reflect that in the patch description. > > > > > > > > I can change these paragraphs, > > > > > > > > "During the review of the above commits, it was determined that in-tree > > > > users weren't affected by the change; most notably, since the only relevant > > > > user (in terms of THP) of VM_MIXEDMAP or ->huge_fault is DAX, which is > > > > explicitly approved early in approval logic. However, there is at least > > > > one occurrence where an out-of-tree driver that used > > > > VM_HUGEPAGE|VM_MIXEDMAP with a vm_ops->huge_fault handler, was broken. > > > > > > > > Remove the VM_NO_KHUGEPAGED check when not in collapse path and give > > > > any ->huge_fault handler a chance to handle the fault. Note that we > > > > don't validate the file mode or mapping alignment, which is consistent > > > > with the behavior before the aforementioned commits." > > > > > > > > To read, > > > > > > > > "The above commits, however, overfit the existing in-tree use cases, > > > > and assume that > > > > the only reason to support ->huge_fault was for DAX (which is > > > > explicitly approved early in the approval logic). > > > > This is a bad assumption to make and unnecessarily prevents general > > > > support of ->huge_fault by filesystems. Allow returning "true" if such > > > > a handler exists, giving the fault path an opportunity to exercise it. > > > > > > > > Similarly, the rationale for including the VM_NO_KHUGEPAGED check > > > > along the fault path was that it didn't alter any in-tree users, but > > > > was likewise similarly unnecessarily restrictive (and reads odd). > > > > Remove the check from the fault path." > > > > > > > > > > > > > Any chance this can make it to 6.6 kernel ? > > > > ping > > I think we tend to merge this patch, but anyway it is Andrew's call. > Included Andrew in this loop. Sorry for delay -- just back from (another) OOO, >From this back/forth with David/Matthew, seems like we're OK saying, "this was a mistake", and that we can take the patch (need some form of Ack or Reviewed-by from them first, to confirm) > > Fundamentally there was a bad assumption with the original patch -- > > it assumed that the only reason to support ->huge_fault was for DAX, > > and that's not true. It's just that the only drivers in-tree which > > support ->huge_fault do so in order to support DAX. > > Okay, and we are willing to continue supporting that then and it's > nothing we want to stop OOT drivers from doing. > > Fine with me; we should probably reflect that in the patch description. But, I don't know about timing. We are in 6.6-rc2, and this hasn't been exposed in Andrew's trees yet. 6.6 is looking like it could be a LTS candidate, in which case this patch could flow backwards from -stable (which would also land in 6.1-y) .. but I don't know if that path is suitable for this. Otherwise, perhaps you could include this fix when you're ready to upstream your driver? > > > > > > > > - Saurabh
