* [RFC PATCH 1/3] fs: dax.c: move fs hole signifier from DAX_ZERO_PAGE to XA_ZERO_ENTRY
@ 2020-11-29 4:36 ` Amy Parker
0 siblings, 0 replies; 13+ messages in thread
From: Amy Parker @ 2020-11-29 4:36 UTC (permalink / raw)
To: linux-fsdevel, linux-nvdimm; +Cc: Jan Kara, Matthew Wilcox
DAX uses the DAX_ZERO_PAGE bit to represent holes in files. It could also use
a single entry, such as XArray's XA_ZERO_ENTRY. This distinguishes zero pages
and allows us to shift DAX_EMPTY down (see patch 2/3).
Signed-off-by: Amy Parker <enbyamy@gmail.com>
---
fs/dax.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/fs/dax.c b/fs/dax.c
index 5b47834f2e1b..fa8ca1a71bbd 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -77,9 +77,14 @@ fs_initcall(init_dax_wait_table);
#define DAX_SHIFT (4)
#define DAX_LOCKED (1UL << 0)
#define DAX_PMD (1UL << 1)
-#define DAX_ZERO_PAGE (1UL << 2)
#define DAX_EMPTY (1UL << 3)
+/*
+ * A zero entry, XA_ZERO_ENTRY, is used to represent a zero page. This
+ * definition helps with checking if an entry is a PMD size.
+ */
+#define XA_ZERO_PMD_ENTRY DAX_PMD | (unsigned long)XA_ZERO_ENTRY
+
static unsigned long dax_to_pfn(void *entry)
{
return xa_to_value(entry) >> DAX_SHIFT;
@@ -114,7 +119,7 @@ static bool dax_is_pte_entry(void *entry)
static int dax_is_zero_entry(void *entry)
{
- return xa_to_value(entry) & DAX_ZERO_PAGE;
+ return xa_to_value(entry) & (unsigned long)XA_ZERO_ENTRY;
}
static int dax_is_empty_entry(void *entry)
@@ -738,7 +743,7 @@ static void *dax_insert_entry(struct xa_state *xas,
if (dirty)
__mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
- if (dax_is_zero_entry(entry) && !(flags & DAX_ZERO_PAGE)) {
+ if (dax_is_zero_entry(entry) && !(flags & (unsigned long)XA_ZERO_ENTRY)) {
unsigned long index = xas->xa_index;
/* we are replacing a zero page with block mapping */
if (dax_is_pmd_entry(entry))
@@ -1047,7 +1052,7 @@ static vm_fault_t dax_load_hole(struct xa_state *xas,
vm_fault_t ret;
*entry = dax_insert_entry(xas, mapping, vmf, *entry, pfn,
- DAX_ZERO_PAGE, false);
+ XA_ZERO_ENTRY, false);
ret = vmf_insert_mixed(vmf->vma, vaddr, pfn);
trace_dax_load_hole(inode, vmf, ret);
@@ -1434,7 +1439,7 @@ static vm_fault_t dax_pmd_load_hole(struct
xa_state *xas, struct vm_fault *vmf,
pfn = page_to_pfn_t(zero_page);
*entry = dax_insert_entry(xas, mapping, vmf, *entry, pfn,
- DAX_PMD | DAX_ZERO_PAGE, false);
+ XA_ZERO_PMD_ENTRY, false);
if (arch_needs_pgtable_deposit()) {
pgtable = pte_alloc_one(vma->vm_mm);
--
2.29.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply related [flat|nested] 13+ messages in thread* [RFC PATCH 1/3] fs: dax.c: move fs hole signifier from DAX_ZERO_PAGE to XA_ZERO_ENTRY
@ 2020-11-29 4:36 ` Amy Parker
0 siblings, 0 replies; 13+ messages in thread
From: Amy Parker @ 2020-11-29 4:36 UTC (permalink / raw)
To: linux-fsdevel, linux-nvdimm; +Cc: dan.j.williams, Jan Kara, Matthew Wilcox
DAX uses the DAX_ZERO_PAGE bit to represent holes in files. It could also use
a single entry, such as XArray's XA_ZERO_ENTRY. This distinguishes zero pages
and allows us to shift DAX_EMPTY down (see patch 2/3).
Signed-off-by: Amy Parker <enbyamy@gmail.com>
---
fs/dax.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/fs/dax.c b/fs/dax.c
index 5b47834f2e1b..fa8ca1a71bbd 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -77,9 +77,14 @@ fs_initcall(init_dax_wait_table);
#define DAX_SHIFT (4)
#define DAX_LOCKED (1UL << 0)
#define DAX_PMD (1UL << 1)
-#define DAX_ZERO_PAGE (1UL << 2)
#define DAX_EMPTY (1UL << 3)
+/*
+ * A zero entry, XA_ZERO_ENTRY, is used to represent a zero page. This
+ * definition helps with checking if an entry is a PMD size.
+ */
+#define XA_ZERO_PMD_ENTRY DAX_PMD | (unsigned long)XA_ZERO_ENTRY
+
static unsigned long dax_to_pfn(void *entry)
{
return xa_to_value(entry) >> DAX_SHIFT;
@@ -114,7 +119,7 @@ static bool dax_is_pte_entry(void *entry)
static int dax_is_zero_entry(void *entry)
{
- return xa_to_value(entry) & DAX_ZERO_PAGE;
+ return xa_to_value(entry) & (unsigned long)XA_ZERO_ENTRY;
}
static int dax_is_empty_entry(void *entry)
@@ -738,7 +743,7 @@ static void *dax_insert_entry(struct xa_state *xas,
if (dirty)
__mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
- if (dax_is_zero_entry(entry) && !(flags & DAX_ZERO_PAGE)) {
+ if (dax_is_zero_entry(entry) && !(flags & (unsigned long)XA_ZERO_ENTRY)) {
unsigned long index = xas->xa_index;
/* we are replacing a zero page with block mapping */
if (dax_is_pmd_entry(entry))
@@ -1047,7 +1052,7 @@ static vm_fault_t dax_load_hole(struct xa_state *xas,
vm_fault_t ret;
*entry = dax_insert_entry(xas, mapping, vmf, *entry, pfn,
- DAX_ZERO_PAGE, false);
+ XA_ZERO_ENTRY, false);
ret = vmf_insert_mixed(vmf->vma, vaddr, pfn);
trace_dax_load_hole(inode, vmf, ret);
@@ -1434,7 +1439,7 @@ static vm_fault_t dax_pmd_load_hole(struct
xa_state *xas, struct vm_fault *vmf,
pfn = page_to_pfn_t(zero_page);
*entry = dax_insert_entry(xas, mapping, vmf, *entry, pfn,
- DAX_PMD | DAX_ZERO_PAGE, false);
+ XA_ZERO_PMD_ENTRY, false);
if (arch_needs_pgtable_deposit()) {
pgtable = pte_alloc_one(vma->vm_mm);
--
2.29.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [RFC PATCH 1/3] fs: dax.c: move fs hole signifier from DAX_ZERO_PAGE to XA_ZERO_ENTRY
2020-11-29 4:36 ` Amy Parker
@ 2020-11-30 13:36 ` Jan Kara
-1 siblings, 0 replies; 13+ messages in thread
From: Jan Kara @ 2020-11-30 13:36 UTC (permalink / raw)
To: Amy Parker; +Cc: linux-fsdevel, linux-nvdimm, Jan Kara, Matthew Wilcox
On Sat 28-11-20 20:36:29, Amy Parker wrote:
> DAX uses the DAX_ZERO_PAGE bit to represent holes in files. It could also use
> a single entry, such as XArray's XA_ZERO_ENTRY. This distinguishes zero pages
> and allows us to shift DAX_EMPTY down (see patch 2/3).
>
> Signed-off-by: Amy Parker <enbyamy@gmail.com>
Thanks for the patch. The idea looks nice however I think technically there
are some problems with the patch. See below.
> +/*
> + * A zero entry, XA_ZERO_ENTRY, is used to represent a zero page. This
> + * definition helps with checking if an entry is a PMD size.
> + */
> +#define XA_ZERO_PMD_ENTRY DAX_PMD | (unsigned long)XA_ZERO_ENTRY
> +
Firstly, if you define a macro, we usually wrap it inside braces like:
#define XA_ZERO_PMD_ENTRY (DAX_PMD | (unsigned long)XA_ZERO_ENTRY)
to avoid unexpected issues when macro expands and surrounding operators
have higher priority.
Secondly, I don't think you can combine XA_ZERO_ENTRY with DAX_PMD (or any
other bits for that matter). XA_ZERO_ENTRY is defined as
xa_mk_internal(257) which is ((257 << 2) | 2) - DAX bits will overlap with
the bits xarray internal entries are using and things will break.
Honestly, I find it somewhat cumbersome to use xarray internal entries for
DAX purposes since all the locking (using DAX_LOCKED) and size checking
(using DAX_PMD) functions will have to special-case internal entries to
operate on different set of bits. It could be done, sure, but I'm not sure
it is worth the trouble for saving two bits (we could get rid of
DAX_ZERO_PAGE and DAX_EMPTY bits in this way) in DAX entries. But maybe
Matthew had some idea how to do this in an elegant way...
Honza
> static unsigned long dax_to_pfn(void *entry)
> {
> return xa_to_value(entry) >> DAX_SHIFT;
> @@ -114,7 +119,7 @@ static bool dax_is_pte_entry(void *entry)
>
> static int dax_is_zero_entry(void *entry)
> {
> - return xa_to_value(entry) & DAX_ZERO_PAGE;
> + return xa_to_value(entry) & (unsigned long)XA_ZERO_ENTRY;
> }
>
> static int dax_is_empty_entry(void *entry)
> @@ -738,7 +743,7 @@ static void *dax_insert_entry(struct xa_state *xas,
> if (dirty)
> __mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
>
> - if (dax_is_zero_entry(entry) && !(flags & DAX_ZERO_PAGE)) {
> + if (dax_is_zero_entry(entry) && !(flags & (unsigned long)XA_ZERO_ENTRY)) {
> unsigned long index = xas->xa_index;
> /* we are replacing a zero page with block mapping */
> if (dax_is_pmd_entry(entry))
> @@ -1047,7 +1052,7 @@ static vm_fault_t dax_load_hole(struct xa_state *xas,
> vm_fault_t ret;
>
> *entry = dax_insert_entry(xas, mapping, vmf, *entry, pfn,
> - DAX_ZERO_PAGE, false);
> + XA_ZERO_ENTRY, false);
>
> ret = vmf_insert_mixed(vmf->vma, vaddr, pfn);
> trace_dax_load_hole(inode, vmf, ret);
> @@ -1434,7 +1439,7 @@ static vm_fault_t dax_pmd_load_hole(struct
> xa_state *xas, struct vm_fault *vmf,
>
> pfn = page_to_pfn_t(zero_page);
> *entry = dax_insert_entry(xas, mapping, vmf, *entry, pfn,
> - DAX_PMD | DAX_ZERO_PAGE, false);
> + XA_ZERO_PMD_ENTRY, false);
>
> if (arch_needs_pgtable_deposit()) {
> pgtable = pte_alloc_one(vma->vm_mm);
> --
> 2.29.2
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [RFC PATCH 1/3] fs: dax.c: move fs hole signifier from DAX_ZERO_PAGE to XA_ZERO_ENTRY
@ 2020-11-30 13:36 ` Jan Kara
0 siblings, 0 replies; 13+ messages in thread
From: Jan Kara @ 2020-11-30 13:36 UTC (permalink / raw)
To: Amy Parker
Cc: linux-fsdevel, linux-nvdimm, dan.j.williams, Jan Kara, Matthew Wilcox
On Sat 28-11-20 20:36:29, Amy Parker wrote:
> DAX uses the DAX_ZERO_PAGE bit to represent holes in files. It could also use
> a single entry, such as XArray's XA_ZERO_ENTRY. This distinguishes zero pages
> and allows us to shift DAX_EMPTY down (see patch 2/3).
>
> Signed-off-by: Amy Parker <enbyamy@gmail.com>
Thanks for the patch. The idea looks nice however I think technically there
are some problems with the patch. See below.
> +/*
> + * A zero entry, XA_ZERO_ENTRY, is used to represent a zero page. This
> + * definition helps with checking if an entry is a PMD size.
> + */
> +#define XA_ZERO_PMD_ENTRY DAX_PMD | (unsigned long)XA_ZERO_ENTRY
> +
Firstly, if you define a macro, we usually wrap it inside braces like:
#define XA_ZERO_PMD_ENTRY (DAX_PMD | (unsigned long)XA_ZERO_ENTRY)
to avoid unexpected issues when macro expands and surrounding operators
have higher priority.
Secondly, I don't think you can combine XA_ZERO_ENTRY with DAX_PMD (or any
other bits for that matter). XA_ZERO_ENTRY is defined as
xa_mk_internal(257) which is ((257 << 2) | 2) - DAX bits will overlap with
the bits xarray internal entries are using and things will break.
Honestly, I find it somewhat cumbersome to use xarray internal entries for
DAX purposes since all the locking (using DAX_LOCKED) and size checking
(using DAX_PMD) functions will have to special-case internal entries to
operate on different set of bits. It could be done, sure, but I'm not sure
it is worth the trouble for saving two bits (we could get rid of
DAX_ZERO_PAGE and DAX_EMPTY bits in this way) in DAX entries. But maybe
Matthew had some idea how to do this in an elegant way...
Honza
> static unsigned long dax_to_pfn(void *entry)
> {
> return xa_to_value(entry) >> DAX_SHIFT;
> @@ -114,7 +119,7 @@ static bool dax_is_pte_entry(void *entry)
>
> static int dax_is_zero_entry(void *entry)
> {
> - return xa_to_value(entry) & DAX_ZERO_PAGE;
> + return xa_to_value(entry) & (unsigned long)XA_ZERO_ENTRY;
> }
>
> static int dax_is_empty_entry(void *entry)
> @@ -738,7 +743,7 @@ static void *dax_insert_entry(struct xa_state *xas,
> if (dirty)
> __mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
>
> - if (dax_is_zero_entry(entry) && !(flags & DAX_ZERO_PAGE)) {
> + if (dax_is_zero_entry(entry) && !(flags & (unsigned long)XA_ZERO_ENTRY)) {
> unsigned long index = xas->xa_index;
> /* we are replacing a zero page with block mapping */
> if (dax_is_pmd_entry(entry))
> @@ -1047,7 +1052,7 @@ static vm_fault_t dax_load_hole(struct xa_state *xas,
> vm_fault_t ret;
>
> *entry = dax_insert_entry(xas, mapping, vmf, *entry, pfn,
> - DAX_ZERO_PAGE, false);
> + XA_ZERO_ENTRY, false);
>
> ret = vmf_insert_mixed(vmf->vma, vaddr, pfn);
> trace_dax_load_hole(inode, vmf, ret);
> @@ -1434,7 +1439,7 @@ static vm_fault_t dax_pmd_load_hole(struct
> xa_state *xas, struct vm_fault *vmf,
>
> pfn = page_to_pfn_t(zero_page);
> *entry = dax_insert_entry(xas, mapping, vmf, *entry, pfn,
> - DAX_PMD | DAX_ZERO_PAGE, false);
> + XA_ZERO_PMD_ENTRY, false);
>
> if (arch_needs_pgtable_deposit()) {
> pgtable = pte_alloc_one(vma->vm_mm);
> --
> 2.29.2
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [RFC PATCH 1/3] fs: dax.c: move fs hole signifier from DAX_ZERO_PAGE to XA_ZERO_ENTRY
2020-11-30 13:36 ` Jan Kara
@ 2020-11-30 14:22 ` Amy Parker
-1 siblings, 0 replies; 13+ messages in thread
From: Amy Parker @ 2020-11-30 14:22 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-fsdevel, linux-nvdimm, Matthew Wilcox
> > +/*
> > + * A zero entry, XA_ZERO_ENTRY, is used to represent a zero page. This
> > + * definition helps with checking if an entry is a PMD size.
> > + */
> > +#define XA_ZERO_PMD_ENTRY DAX_PMD | (unsigned long)XA_ZERO_ENTRY
> > +
>
> Firstly, if you define a macro, we usually wrap it inside braces like:
>
> #define XA_ZERO_PMD_ENTRY (DAX_PMD | (unsigned long)XA_ZERO_ENTRY)
>
> to avoid unexpected issues when macro expands and surrounding operators
> have higher priority.
Oops! Must've missed that - I'll make sure to get on that when
revising this patch.
> Secondly, I don't think you can combine XA_ZERO_ENTRY with DAX_PMD (or any
> other bits for that matter). XA_ZERO_ENTRY is defined as
> xa_mk_internal(257) which is ((257 << 2) | 2) - DAX bits will overlap with
> the bits xarray internal entries are using and things will break.
Could you provide an example of this overlap? I can't seem to find any.
> Honestly, I find it somewhat cumbersome to use xarray internal entries for
> DAX purposes since all the locking (using DAX_LOCKED) and size checking
> (using DAX_PMD) functions will have to special-case internal entries to
> operate on different set of bits. It could be done, sure, but I'm not sure
> it is worth the trouble for saving two bits (we could get rid of
> DAX_ZERO_PAGE and DAX_EMPTY bits in this way) in DAX entries. But maybe
> Matthew had some idea how to do this in an elegant way...
Yeah, that's likely the best we've got for now. Hopefully Matthew can
provide some
insight into this.
Best regards,
Amy Parker
(she/her)
>
> Honza
>
> > static unsigned long dax_to_pfn(void *entry)
> > {
> > return xa_to_value(entry) >> DAX_SHIFT;
> > @@ -114,7 +119,7 @@ static bool dax_is_pte_entry(void *entry)
> >
> > static int dax_is_zero_entry(void *entry)
> > {
> > - return xa_to_value(entry) & DAX_ZERO_PAGE;
> > + return xa_to_value(entry) & (unsigned long)XA_ZERO_ENTRY;
> > }
> >
> > static int dax_is_empty_entry(void *entry)
> > @@ -738,7 +743,7 @@ static void *dax_insert_entry(struct xa_state *xas,
> > if (dirty)
> > __mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
> >
> > - if (dax_is_zero_entry(entry) && !(flags & DAX_ZERO_PAGE)) {
> > + if (dax_is_zero_entry(entry) && !(flags & (unsigned long)XA_ZERO_ENTRY)) {
> > unsigned long index = xas->xa_index;
> > /* we are replacing a zero page with block mapping */
> > if (dax_is_pmd_entry(entry))
> > @@ -1047,7 +1052,7 @@ static vm_fault_t dax_load_hole(struct xa_state *xas,
> > vm_fault_t ret;
> >
> > *entry = dax_insert_entry(xas, mapping, vmf, *entry, pfn,
> > - DAX_ZERO_PAGE, false);
> > + XA_ZERO_ENTRY, false);
> >
> > ret = vmf_insert_mixed(vmf->vma, vaddr, pfn);
> > trace_dax_load_hole(inode, vmf, ret);
> > @@ -1434,7 +1439,7 @@ static vm_fault_t dax_pmd_load_hole(struct
> > xa_state *xas, struct vm_fault *vmf,
> >
> > pfn = page_to_pfn_t(zero_page);
> > *entry = dax_insert_entry(xas, mapping, vmf, *entry, pfn,
> > - DAX_PMD | DAX_ZERO_PAGE, false);
> > + XA_ZERO_PMD_ENTRY, false);
> >
> > if (arch_needs_pgtable_deposit()) {
> > pgtable = pte_alloc_one(vma->vm_mm);
> > --
> > 2.29.2
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [RFC PATCH 1/3] fs: dax.c: move fs hole signifier from DAX_ZERO_PAGE to XA_ZERO_ENTRY
@ 2020-11-30 14:22 ` Amy Parker
0 siblings, 0 replies; 13+ messages in thread
From: Amy Parker @ 2020-11-30 14:22 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-fsdevel, linux-nvdimm, dan.j.williams, Matthew Wilcox
> > +/*
> > + * A zero entry, XA_ZERO_ENTRY, is used to represent a zero page. This
> > + * definition helps with checking if an entry is a PMD size.
> > + */
> > +#define XA_ZERO_PMD_ENTRY DAX_PMD | (unsigned long)XA_ZERO_ENTRY
> > +
>
> Firstly, if you define a macro, we usually wrap it inside braces like:
>
> #define XA_ZERO_PMD_ENTRY (DAX_PMD | (unsigned long)XA_ZERO_ENTRY)
>
> to avoid unexpected issues when macro expands and surrounding operators
> have higher priority.
Oops! Must've missed that - I'll make sure to get on that when
revising this patch.
> Secondly, I don't think you can combine XA_ZERO_ENTRY with DAX_PMD (or any
> other bits for that matter). XA_ZERO_ENTRY is defined as
> xa_mk_internal(257) which is ((257 << 2) | 2) - DAX bits will overlap with
> the bits xarray internal entries are using and things will break.
Could you provide an example of this overlap? I can't seem to find any.
> Honestly, I find it somewhat cumbersome to use xarray internal entries for
> DAX purposes since all the locking (using DAX_LOCKED) and size checking
> (using DAX_PMD) functions will have to special-case internal entries to
> operate on different set of bits. It could be done, sure, but I'm not sure
> it is worth the trouble for saving two bits (we could get rid of
> DAX_ZERO_PAGE and DAX_EMPTY bits in this way) in DAX entries. But maybe
> Matthew had some idea how to do this in an elegant way...
Yeah, that's likely the best we've got for now. Hopefully Matthew can
provide some
insight into this.
Best regards,
Amy Parker
(she/her)
>
> Honza
>
> > static unsigned long dax_to_pfn(void *entry)
> > {
> > return xa_to_value(entry) >> DAX_SHIFT;
> > @@ -114,7 +119,7 @@ static bool dax_is_pte_entry(void *entry)
> >
> > static int dax_is_zero_entry(void *entry)
> > {
> > - return xa_to_value(entry) & DAX_ZERO_PAGE;
> > + return xa_to_value(entry) & (unsigned long)XA_ZERO_ENTRY;
> > }
> >
> > static int dax_is_empty_entry(void *entry)
> > @@ -738,7 +743,7 @@ static void *dax_insert_entry(struct xa_state *xas,
> > if (dirty)
> > __mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
> >
> > - if (dax_is_zero_entry(entry) && !(flags & DAX_ZERO_PAGE)) {
> > + if (dax_is_zero_entry(entry) && !(flags & (unsigned long)XA_ZERO_ENTRY)) {
> > unsigned long index = xas->xa_index;
> > /* we are replacing a zero page with block mapping */
> > if (dax_is_pmd_entry(entry))
> > @@ -1047,7 +1052,7 @@ static vm_fault_t dax_load_hole(struct xa_state *xas,
> > vm_fault_t ret;
> >
> > *entry = dax_insert_entry(xas, mapping, vmf, *entry, pfn,
> > - DAX_ZERO_PAGE, false);
> > + XA_ZERO_ENTRY, false);
> >
> > ret = vmf_insert_mixed(vmf->vma, vaddr, pfn);
> > trace_dax_load_hole(inode, vmf, ret);
> > @@ -1434,7 +1439,7 @@ static vm_fault_t dax_pmd_load_hole(struct
> > xa_state *xas, struct vm_fault *vmf,
> >
> > pfn = page_to_pfn_t(zero_page);
> > *entry = dax_insert_entry(xas, mapping, vmf, *entry, pfn,
> > - DAX_PMD | DAX_ZERO_PAGE, false);
> > + XA_ZERO_PMD_ENTRY, false);
> >
> > if (arch_needs_pgtable_deposit()) {
> > pgtable = pte_alloc_one(vma->vm_mm);
> > --
> > 2.29.2
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [RFC PATCH 1/3] fs: dax.c: move fs hole signifier from DAX_ZERO_PAGE to XA_ZERO_ENTRY
2020-11-30 14:22 ` Amy Parker
@ 2020-11-30 15:09 ` Jan Kara
-1 siblings, 0 replies; 13+ messages in thread
From: Jan Kara @ 2020-11-30 15:09 UTC (permalink / raw)
To: Amy Parker; +Cc: Jan Kara, linux-fsdevel, linux-nvdimm, Matthew Wilcox
On Mon 30-11-20 06:22:42, Amy Parker wrote:
> > > +/*
> > > + * A zero entry, XA_ZERO_ENTRY, is used to represent a zero page. This
> > > + * definition helps with checking if an entry is a PMD size.
> > > + */
> > > +#define XA_ZERO_PMD_ENTRY DAX_PMD | (unsigned long)XA_ZERO_ENTRY
> > > +
> >
> > Firstly, if you define a macro, we usually wrap it inside braces like:
> >
> > #define XA_ZERO_PMD_ENTRY (DAX_PMD | (unsigned long)XA_ZERO_ENTRY)
> >
> > to avoid unexpected issues when macro expands and surrounding operators
> > have higher priority.
>
> Oops! Must've missed that - I'll make sure to get on that when
> revising this patch.
>
> > Secondly, I don't think you can combine XA_ZERO_ENTRY with DAX_PMD (or any
> > other bits for that matter). XA_ZERO_ENTRY is defined as
> > xa_mk_internal(257) which is ((257 << 2) | 2) - DAX bits will overlap with
> > the bits xarray internal entries are using and things will break.
>
> Could you provide an example of this overlap? I can't seem to find any.
Well XA_ZERO_ENTRY | DAX_PMD == ((257 << 2) | 2) | (1 << 1). So the way
you've defined XA_ZERO_PMD_ENTRY the DAX_PMD will just get lost. AFAIU (but
Matthew might correct me here), for internal entries (and XA_ZERO_ENTRY is
one instance of such entry) low 10-bits of the of the entry values are
reserved for internal xarray usage so DAX could use only higher bits. For
classical value entries, only the lowest bit is reserved for xarray usage,
all the rest is available for the user (and so DAX uses it).
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/3] fs: dax.c: move fs hole signifier from DAX_ZERO_PAGE to XA_ZERO_ENTRY
@ 2020-11-30 15:09 ` Jan Kara
0 siblings, 0 replies; 13+ messages in thread
From: Jan Kara @ 2020-11-30 15:09 UTC (permalink / raw)
To: Amy Parker
Cc: Jan Kara, linux-fsdevel, linux-nvdimm, dan.j.williams, Matthew Wilcox
On Mon 30-11-20 06:22:42, Amy Parker wrote:
> > > +/*
> > > + * A zero entry, XA_ZERO_ENTRY, is used to represent a zero page. This
> > > + * definition helps with checking if an entry is a PMD size.
> > > + */
> > > +#define XA_ZERO_PMD_ENTRY DAX_PMD | (unsigned long)XA_ZERO_ENTRY
> > > +
> >
> > Firstly, if you define a macro, we usually wrap it inside braces like:
> >
> > #define XA_ZERO_PMD_ENTRY (DAX_PMD | (unsigned long)XA_ZERO_ENTRY)
> >
> > to avoid unexpected issues when macro expands and surrounding operators
> > have higher priority.
>
> Oops! Must've missed that - I'll make sure to get on that when
> revising this patch.
>
> > Secondly, I don't think you can combine XA_ZERO_ENTRY with DAX_PMD (or any
> > other bits for that matter). XA_ZERO_ENTRY is defined as
> > xa_mk_internal(257) which is ((257 << 2) | 2) - DAX bits will overlap with
> > the bits xarray internal entries are using and things will break.
>
> Could you provide an example of this overlap? I can't seem to find any.
Well XA_ZERO_ENTRY | DAX_PMD == ((257 << 2) | 2) | (1 << 1). So the way
you've defined XA_ZERO_PMD_ENTRY the DAX_PMD will just get lost. AFAIU (but
Matthew might correct me here), for internal entries (and XA_ZERO_ENTRY is
one instance of such entry) low 10-bits of the of the entry values are
reserved for internal xarray usage so DAX could use only higher bits. For
classical value entries, only the lowest bit is reserved for xarray usage,
all the rest is available for the user (and so DAX uses it).
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/3] fs: dax.c: move fs hole signifier from DAX_ZERO_PAGE to XA_ZERO_ENTRY
2020-11-30 15:09 ` Jan Kara
@ 2020-11-30 15:15 ` Amy Parker
-1 siblings, 0 replies; 13+ messages in thread
From: Amy Parker @ 2020-11-30 15:15 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-fsdevel, linux-nvdimm, Matthew Wilcox
On Mon, Nov 30, 2020 at 7:09 AM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 30-11-20 06:22:42, Amy Parker wrote:
> > > > +/*
> > > > + * A zero entry, XA_ZERO_ENTRY, is used to represent a zero page. This
> > > > + * definition helps with checking if an entry is a PMD size.
> > > > + */
> > > > +#define XA_ZERO_PMD_ENTRY DAX_PMD | (unsigned long)XA_ZERO_ENTRY
> > > > +
> > >
> > > Firstly, if you define a macro, we usually wrap it inside braces like:
> > >
> > > #define XA_ZERO_PMD_ENTRY (DAX_PMD | (unsigned long)XA_ZERO_ENTRY)
> > >
> > > to avoid unexpected issues when macro expands and surrounding operators
> > > have higher priority.
> >
> > Oops! Must've missed that - I'll make sure to get on that when
> > revising this patch.
> >
> > > Secondly, I don't think you can combine XA_ZERO_ENTRY with DAX_PMD (or any
> > > other bits for that matter). XA_ZERO_ENTRY is defined as
> > > xa_mk_internal(257) which is ((257 << 2) | 2) - DAX bits will overlap with
> > > the bits xarray internal entries are using and things will break.
> >
> > Could you provide an example of this overlap? I can't seem to find any.
>
> Well XA_ZERO_ENTRY | DAX_PMD == ((257 << 2) | 2) | (1 << 1). So the way
> you've defined XA_ZERO_PMD_ENTRY the DAX_PMD will just get lost. AFAIU (but
> Matthew might correct me here), for internal entries (and XA_ZERO_ENTRY is
> one instance of such entry) low 10-bits of the of the entry values are
> reserved for internal xarray usage so DAX could use only higher bits. For
> classical value entries, only the lowest bit is reserved for xarray usage,
> all the rest is available for the user (and so DAX uses it).
Ah, thank you. I'm not as familiar with DAX as I'd like. So what should we do?
Best regards,
Amy Parker
(she/her)
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/3] fs: dax.c: move fs hole signifier from DAX_ZERO_PAGE to XA_ZERO_ENTRY
@ 2020-11-30 15:15 ` Amy Parker
0 siblings, 0 replies; 13+ messages in thread
From: Amy Parker @ 2020-11-30 15:15 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-fsdevel, linux-nvdimm, dan.j.williams, Matthew Wilcox
On Mon, Nov 30, 2020 at 7:09 AM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 30-11-20 06:22:42, Amy Parker wrote:
> > > > +/*
> > > > + * A zero entry, XA_ZERO_ENTRY, is used to represent a zero page. This
> > > > + * definition helps with checking if an entry is a PMD size.
> > > > + */
> > > > +#define XA_ZERO_PMD_ENTRY DAX_PMD | (unsigned long)XA_ZERO_ENTRY
> > > > +
> > >
> > > Firstly, if you define a macro, we usually wrap it inside braces like:
> > >
> > > #define XA_ZERO_PMD_ENTRY (DAX_PMD | (unsigned long)XA_ZERO_ENTRY)
> > >
> > > to avoid unexpected issues when macro expands and surrounding operators
> > > have higher priority.
> >
> > Oops! Must've missed that - I'll make sure to get on that when
> > revising this patch.
> >
> > > Secondly, I don't think you can combine XA_ZERO_ENTRY with DAX_PMD (or any
> > > other bits for that matter). XA_ZERO_ENTRY is defined as
> > > xa_mk_internal(257) which is ((257 << 2) | 2) - DAX bits will overlap with
> > > the bits xarray internal entries are using and things will break.
> >
> > Could you provide an example of this overlap? I can't seem to find any.
>
> Well XA_ZERO_ENTRY | DAX_PMD == ((257 << 2) | 2) | (1 << 1). So the way
> you've defined XA_ZERO_PMD_ENTRY the DAX_PMD will just get lost. AFAIU (but
> Matthew might correct me here), for internal entries (and XA_ZERO_ENTRY is
> one instance of such entry) low 10-bits of the of the entry values are
> reserved for internal xarray usage so DAX could use only higher bits. For
> classical value entries, only the lowest bit is reserved for xarray usage,
> all the rest is available for the user (and so DAX uses it).
Ah, thank you. I'm not as familiar with DAX as I'd like. So what should we do?
Best regards,
Amy Parker
(she/her)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/3] fs: dax.c: move fs hole signifier from DAX_ZERO_PAGE to XA_ZERO_ENTRY
2020-11-30 15:09 ` Jan Kara
@ 2020-11-30 16:36 ` Matthew Wilcox
-1 siblings, 0 replies; 13+ messages in thread
From: Matthew Wilcox @ 2020-11-30 16:36 UTC (permalink / raw)
To: Jan Kara; +Cc: Amy Parker, linux-fsdevel, linux-nvdimm
On Mon, Nov 30, 2020 at 04:09:23PM +0100, Jan Kara wrote:
> On Mon 30-11-20 06:22:42, Amy Parker wrote:
> > > > +/*
> > > > + * A zero entry, XA_ZERO_ENTRY, is used to represent a zero page. This
> > > > + * definition helps with checking if an entry is a PMD size.
> > > > + */
> > > > +#define XA_ZERO_PMD_ENTRY DAX_PMD | (unsigned long)XA_ZERO_ENTRY
> > > > +
> > >
> > > Firstly, if you define a macro, we usually wrap it inside braces like:
> > >
> > > #define XA_ZERO_PMD_ENTRY (DAX_PMD | (unsigned long)XA_ZERO_ENTRY)
> > >
> > > to avoid unexpected issues when macro expands and surrounding operators
> > > have higher priority.
> >
> > Oops! Must've missed that - I'll make sure to get on that when
> > revising this patch.
> >
> > > Secondly, I don't think you can combine XA_ZERO_ENTRY with DAX_PMD (or any
> > > other bits for that matter). XA_ZERO_ENTRY is defined as
> > > xa_mk_internal(257) which is ((257 << 2) | 2) - DAX bits will overlap with
> > > the bits xarray internal entries are using and things will break.
> >
> > Could you provide an example of this overlap? I can't seem to find any.
>
> Well XA_ZERO_ENTRY | DAX_PMD == ((257 << 2) | 2) | (1 << 1). So the way
> you've defined XA_ZERO_PMD_ENTRY the DAX_PMD will just get lost. AFAIU (but
> Matthew might correct me here), for internal entries (and XA_ZERO_ENTRY is
> one instance of such entry) low 10-bits of the of the entry values are
> reserved for internal xarray usage so DAX could use only higher bits. For
> classical value entries, only the lowest bit is reserved for xarray usage,
> all the rest is available for the user (and so DAX uses it).
The XArray entries are pretty inventive in how they are used ...
1. If bit 0 is set, it's a value entry. That is, it encodes an integer
between 0 and LONG_MAX.
2. If bits 0 & 1 are clear, it's a pointer.
3. If bit 0 is clear and bit 1 is set, it's _either_ an internal entry,
_or_ it's a pointer that's only 2-byte aligned. These can exist on m68k,
alas.
Internal entries above -MAX_ERRNO are used for returning errors.
Internal entries below 1024 (256 << 2) are used for sibling entries.
Internal entry 256 is the retry entry.
Internal entry 257 is the zero entry.
Internal entries 258-1023 are not currently used.
Internal entries between 4096 and MAX_ERRNO are pointers to the next
level of the tree.
The m68k pointer problem is "solved" by only allowing them to be in a
node which is the bottom of the tree. This means that the optimisation
of placing a single pointer at index 0 in the root of the tree has to be
disabled for these pointers. That's unfortunate, but there's no other
way to solve it, given the need for RCU readers. You also can't use
an m68k pointer for a multi-index entry.
There's also support for pointers tagged in their lower bits. Those are
incompatible with value entries. And you can't use pointer tag 2 ...
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/3] fs: dax.c: move fs hole signifier from DAX_ZERO_PAGE to XA_ZERO_ENTRY
@ 2020-11-30 16:36 ` Matthew Wilcox
0 siblings, 0 replies; 13+ messages in thread
From: Matthew Wilcox @ 2020-11-30 16:36 UTC (permalink / raw)
To: Jan Kara; +Cc: Amy Parker, linux-fsdevel, linux-nvdimm, dan.j.williams
On Mon, Nov 30, 2020 at 04:09:23PM +0100, Jan Kara wrote:
> On Mon 30-11-20 06:22:42, Amy Parker wrote:
> > > > +/*
> > > > + * A zero entry, XA_ZERO_ENTRY, is used to represent a zero page. This
> > > > + * definition helps with checking if an entry is a PMD size.
> > > > + */
> > > > +#define XA_ZERO_PMD_ENTRY DAX_PMD | (unsigned long)XA_ZERO_ENTRY
> > > > +
> > >
> > > Firstly, if you define a macro, we usually wrap it inside braces like:
> > >
> > > #define XA_ZERO_PMD_ENTRY (DAX_PMD | (unsigned long)XA_ZERO_ENTRY)
> > >
> > > to avoid unexpected issues when macro expands and surrounding operators
> > > have higher priority.
> >
> > Oops! Must've missed that - I'll make sure to get on that when
> > revising this patch.
> >
> > > Secondly, I don't think you can combine XA_ZERO_ENTRY with DAX_PMD (or any
> > > other bits for that matter). XA_ZERO_ENTRY is defined as
> > > xa_mk_internal(257) which is ((257 << 2) | 2) - DAX bits will overlap with
> > > the bits xarray internal entries are using and things will break.
> >
> > Could you provide an example of this overlap? I can't seem to find any.
>
> Well XA_ZERO_ENTRY | DAX_PMD == ((257 << 2) | 2) | (1 << 1). So the way
> you've defined XA_ZERO_PMD_ENTRY the DAX_PMD will just get lost. AFAIU (but
> Matthew might correct me here), for internal entries (and XA_ZERO_ENTRY is
> one instance of such entry) low 10-bits of the of the entry values are
> reserved for internal xarray usage so DAX could use only higher bits. For
> classical value entries, only the lowest bit is reserved for xarray usage,
> all the rest is available for the user (and so DAX uses it).
The XArray entries are pretty inventive in how they are used ...
1. If bit 0 is set, it's a value entry. That is, it encodes an integer
between 0 and LONG_MAX.
2. If bits 0 & 1 are clear, it's a pointer.
3. If bit 0 is clear and bit 1 is set, it's _either_ an internal entry,
_or_ it's a pointer that's only 2-byte aligned. These can exist on m68k,
alas.
Internal entries above -MAX_ERRNO are used for returning errors.
Internal entries below 1024 (256 << 2) are used for sibling entries.
Internal entry 256 is the retry entry.
Internal entry 257 is the zero entry.
Internal entries 258-1023 are not currently used.
Internal entries between 4096 and MAX_ERRNO are pointers to the next
level of the tree.
The m68k pointer problem is "solved" by only allowing them to be in a
node which is the bottom of the tree. This means that the optimisation
of placing a single pointer at index 0 in the root of the tree has to be
disabled for these pointers. That's unfortunate, but there's no other
way to solve it, given the need for RCU readers. You also can't use
an m68k pointer for a multi-index entry.
There's also support for pointers tagged in their lower bits. Those are
incompatible with value entries. And you can't use pointer tag 2 ...
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/3] fs: dax.c: move fs hole signifier from DAX_ZERO_PAGE to XA_ZERO_ENTRY
2020-11-30 16:36 ` Matthew Wilcox
(?)
@ 2021-02-02 18:42 ` Amy Parker
-1 siblings, 0 replies; 13+ messages in thread
From: Amy Parker @ 2021-02-02 18:42 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Jan Kara, linux-fsdevel, linux-nvdimm, dan.j.williams
Apologize for taking a long time - this got swamped in a sea of other
kernel bits.
There doesn't seem to be much of a point now to switch to the XArray
zero entry for what is currently DAX_ZERO_ENTRY. However, if you want
to explore it further, a potential thought could be adapting the bits
around the XArray 257 entry. There's a large swath of available bits
that we could use between bit 3 and bit 9. If Matthew really wants to
use the XA zero entry for it, perhaps we could shift the other DAX
bits to those?
Best regards,
Amy Parker
(she/her/hers)
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-02-02 18:44 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-29 4:36 [RFC PATCH 1/3] fs: dax.c: move fs hole signifier from DAX_ZERO_PAGE to XA_ZERO_ENTRY Amy Parker
2020-11-29 4:36 ` Amy Parker
2020-11-30 13:36 ` Jan Kara
2020-11-30 13:36 ` Jan Kara
2020-11-30 14:22 ` Amy Parker
2020-11-30 14:22 ` Amy Parker
2020-11-30 15:09 ` Jan Kara
2020-11-30 15:09 ` Jan Kara
2020-11-30 15:15 ` Amy Parker
2020-11-30 15:15 ` Amy Parker
2020-11-30 16:36 ` Matthew Wilcox
2020-11-30 16:36 ` Matthew Wilcox
2021-02-02 18:42 ` Amy Parker
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.