From: Alistair Popple <apopple@nvidia•com>
To: Christoph Hellwig <hch@lst•de>
Cc: dan.j.williams@intel•com, vishal.l.verma@intel•com,
dave.jiang@intel•com, logang@deltatee•com, bhelgaas@google•com,
jack@suse•cz, jgg@ziepe•ca, catalin.marinas@arm•com,
will@kernel•org, mpe@ellerman•id.au, npiggin@gmail•com,
dave.hansen@linux•intel.com, ira.weiny@intel•com,
willy@infradead•org, djwong@kernel•org, tytso@mit•edu,
linmiaohe@huawei•com, david@redhat•com, peterx@redhat•com,
linux-doc@vger•kernel.org, linux-kernel@vger•kernel.org,
linux-arm-kernel@lists•infradead.org,
linuxppc-dev@lists•ozlabs.org, nvdimm@lists•linux.dev,
linux-cxl@vger•kernel.org, linux-fsdevel@vger•kernel.org,
linux-mm@kvack•org, linux-ext4@vger•kernel.org,
linux-xfs@vger•kernel.org, jhubbard@nvidia•com,
david@fromorbit•com
Subject: Re: [PATCH 10/13] fs/dax: Properly refcount fs dax pages
Date: Fri, 06 Sep 2024 16:00:38 +1000 [thread overview]
Message-ID: <87wmjpb9g6.fsf@nvdebian.thelocal> (raw)
In-Reply-To: <20240627054455.GF14837@lst.de>
Christoph Hellwig <hch@lst•de> writes:
>> diff --git a/drivers/dax/device.c b/drivers/dax/device.c
>> index eb61598..b7a31ae 100644
>> --- a/drivers/dax/device.c
>> +++ b/drivers/dax/device.c
>> @@ -126,11 +126,11 @@ static vm_fault_t __dev_dax_pte_fault(struct dev_dax *dev_dax,
>> return VM_FAULT_SIGBUS;
>> }
>>
>> - pfn = phys_to_pfn_t(phys, PFN_DEV|PFN_MAP);
>> + pfn = phys_to_pfn_t(phys, 0);
>>
>> dax_set_mapping(vmf, pfn, fault_size);
>>
>> - return vmf_insert_mixed(vmf->vma, vmf->address, pfn);
>> + return dax_insert_pfn(vmf->vma, vmf->address, pfn, vmf->flags & FAULT_FLAG_WRITE);
>
> Plenty overly long lines here and later.
>
> Q: hould dax_insert_pfn take a vm_fault structure instead of the vma?
> Or are the potential use cases that aren't from the fault path?
Nope, good idea. I will update it to take a vm_fault struct for the next
version.
> similar instead of the bool write passing the fault flags might actually
> make things more readable than the bool.
>
> Also at least currently it seems like there are no modular users despite
> the export, or am I missing something?
It gets used in drivers/dax/device.c which I think is built into
device_dax.ko:
obj-$(CONFIG_DEV_DAX) += device_dax.o
...
device_dax-y := device.o
>> {
>> + /*
>> + * Make sure we flush any cached data to the page now that it's free.
>> + */
>> + if (PageDirty(page))
>> + dax_flush(NULL, page_address(page), page_size(page));
>> +
>
> Adding the magic dax_dev == NULL case to dax_flush and going through it
> vs just calling arch_wb_cache_pmem directly here seems odd.
>
> But I also don't quite understand how it is related to the rest
> of the patch anyway.
Yeah, that should be unnecessary as it gets called elsewhere as needed
so will remove it.
>> if (!pmd_present(*pmd))
>> goto out;
>> diff --git a/mm/mm_init.c b/mm/mm_init.c
>> index b7e1599..f11ee0d 100644
>> --- a/mm/mm_init.c
>> +++ b/mm/mm_init.c
>> @@ -1016,7 +1016,8 @@ static void __ref __init_zone_device_page(struct page *page, unsigned long pfn,
>> */
>> if (pgmap->type == MEMORY_DEVICE_PRIVATE ||
>> pgmap->type == MEMORY_DEVICE_COHERENT ||
>> - pgmap->type == MEMORY_DEVICE_PCI_P2PDMA)
>> + pgmap->type == MEMORY_DEVICE_PCI_P2PDMA ||
>> + pgmap->type == MEMORY_DEVICE_FS_DAX)
>> set_page_count(page, 0);
>> }
>
> So we'll skip this for MEMORY_DEVICE_GENERIC only. Does anyone remember
> if that's actively harmful or just not needed? If the latter it might
> be simpler to just set the page count unconditionally here.
Yeah I'm not sure but the switch statement you suggested at least makes
this much clearer. Once I get this series finished I can chase down the
MEMORY_DEVICE_GENERIC differences. I suspect we can just do it
unconditionally.
next prev parent reply other threads:[~2024-09-06 6:12 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-06-27 0:54 [PATCH 00/13] fs/dax: Fix FS DAX page reference counts Alistair Popple
2024-06-27 0:54 ` [PATCH 01/13] mm/gup.c: Remove redundant check for PCI P2PDMA page Alistair Popple
2024-06-27 6:36 ` Dan Williams
2024-06-27 0:54 ` [PATCH 02/13] pci/p2pdma: Don't initialise page refcount to one Alistair Popple
2024-06-27 5:30 ` Christoph Hellwig
2024-06-29 21:28 ` Bjorn Helgaas
2024-06-27 0:54 ` [PATCH 03/13] fs/dax: Refactor wait for dax idle page Alistair Popple
2024-06-27 5:31 ` Christoph Hellwig
2024-06-27 0:54 ` [PATCH 04/13] fs/dax: Add dax_page_free callback Alistair Popple
2024-06-27 5:33 ` Christoph Hellwig
2024-06-27 23:48 ` Alistair Popple
2024-06-27 0:54 ` [PATCH 05/13] mm: Allow compound zone device pages Alistair Popple
2024-06-27 5:35 ` Christoph Hellwig
2024-06-27 0:54 ` [PATCH 06/13] mm/memory: Add dax_insert_pfn Alistair Popple
2024-06-27 5:22 ` Christoph Hellwig
2024-06-27 11:33 ` Jan Kara
2024-09-06 6:21 ` Alistair Popple
2024-07-02 7:18 ` David Hildenbrand
2024-07-02 10:47 ` Alistair Popple
2024-07-02 11:46 ` Christoph Hellwig
2024-07-02 11:53 ` David Hildenbrand
2024-06-27 0:54 ` [PATCH 07/13] huge_memory: Allow mappings of PUD sized pages Alistair Popple
2024-06-27 22:26 ` kernel test robot
2024-07-02 7:16 ` David Hildenbrand
2024-07-02 10:19 ` Alistair Popple
2024-07-02 11:02 ` David Hildenbrand
2024-07-02 11:30 ` Alistair Popple
2024-07-02 13:01 ` David Hildenbrand
2024-07-02 11:51 ` Christoph Hellwig
2024-06-27 0:54 ` [PATCH 08/13] huge_memory: Allow mappings of PMD " Alistair Popple
2024-06-27 0:54 ` [PATCH 09/13] gup: Don't allow FOLL_LONGTERM pinning of FS DAX pages Alistair Popple
2024-07-01 8:59 ` David Hildenbrand
2024-07-01 23:47 ` Alistair Popple
2024-07-02 10:48 ` David Hildenbrand
2024-06-27 0:54 ` [PATCH 10/13] fs/dax: Properly refcount fs dax pages Alistair Popple
2024-06-27 5:44 ` Christoph Hellwig
2024-09-06 6:00 ` Alistair Popple [this message]
2024-06-27 0:54 ` [PATCH 11/13] huge_memory: Remove dead vmf_insert_pXd code Alistair Popple
2024-07-05 14:24 ` Peter Xu
2024-07-09 4:07 ` Alistair Popple
2024-07-09 15:56 ` Peter Xu
2024-07-12 2:40 ` Alistair Popple
2024-07-12 15:52 ` Peter Xu
2024-06-27 0:54 ` [PATCH 12/13] mm: Remove pXX_devmap callers Alistair Popple
2024-06-27 0:54 ` [PATCH 13/13] mm: Remove devmap related functions and page table bits Alistair Popple
2024-06-27 23:04 ` kernel test robot
2024-06-28 2:12 ` kernel test robot
2024-07-08 11:35 ` Will Deacon
2024-06-27 6:58 ` [PATCH 00/13] fs/dax: Fix FS DAX page reference counts Dan Williams
2024-06-27 7:15 ` Alistair Popple
2024-06-27 20:24 ` Dan Williams
2024-06-28 0:06 ` Alistair Popple
2024-07-01 4:24 ` Dave Chinner
2024-07-01 8:33 ` Alistair Popple
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87wmjpb9g6.fsf@nvdebian.thelocal \
--to=apopple@nvidia$(echo .)com \
--cc=bhelgaas@google$(echo .)com \
--cc=catalin.marinas@arm$(echo .)com \
--cc=dan.j.williams@intel$(echo .)com \
--cc=dave.hansen@linux$(echo .)intel.com \
--cc=dave.jiang@intel$(echo .)com \
--cc=david@fromorbit$(echo .)com \
--cc=david@redhat$(echo .)com \
--cc=djwong@kernel$(echo .)org \
--cc=hch@lst$(echo .)de \
--cc=ira.weiny@intel$(echo .)com \
--cc=jack@suse$(echo .)cz \
--cc=jgg@ziepe$(echo .)ca \
--cc=jhubbard@nvidia$(echo .)com \
--cc=linmiaohe@huawei$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=linux-cxl@vger$(echo .)kernel.org \
--cc=linux-doc@vger$(echo .)kernel.org \
--cc=linux-ext4@vger$(echo .)kernel.org \
--cc=linux-fsdevel@vger$(echo .)kernel.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux-mm@kvack$(echo .)org \
--cc=linux-xfs@vger$(echo .)kernel.org \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=logang@deltatee$(echo .)com \
--cc=mpe@ellerman$(echo .)id.au \
--cc=npiggin@gmail$(echo .)com \
--cc=nvdimm@lists$(echo .)linux.dev \
--cc=peterx@redhat$(echo .)com \
--cc=tytso@mit$(echo .)edu \
--cc=vishal.l.verma@intel$(echo .)com \
--cc=will@kernel$(echo .)org \
--cc=willy@infradead$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox