* Best solution for shifting DAX_ZERO_PAGE to XA_ZERO_ENTRY
@ 2020-11-09 1:15 Amy Parker
2020-11-09 1:33 ` Darrick J. Wong
0 siblings, 1 reply; 7+ messages in thread
From: Amy Parker @ 2020-11-09 1:15 UTC (permalink / raw)
To: linux-fsdevel, linux-nvdimm, Matthew Wilcox, dan.j.williams, Jan Kara
I've been writing a patch to migrate the defined DAX_ZERO_PAGE
to XA_ZERO_ENTRY for representing holes in files. XA_ZERO_ENTRY
is defined in include/linux/xarray.h, where it's defined using
xa_mk_internal(257). This function returns a void pointer, which
is incompatible with the bitwise arithmetic it is performed on with.
Currently, DAX_ZERO_PAGE is defined as an unsigned long,
so I considered typecasting it. Typecasting every time would be
repetitive and inefficient. I thought about making a new definition
for it which has the typecast, but this breaks the original point of
using already defined terms.
Should we go the route of adding a new definition, we might as
well just change the definition of DAX_ZERO_PAGE. This would
break the simplicity of the current DAX bit definitions:
#define DAX_LOCKED (1UL << 0)
#define DAX_PMD (1UL << 1)
#define DAX_ZERO_PAGE (1UL << 2)
#define DAX_EMPTY (1UL << 3)
Any thoughts on this, and what could be the best solution here?
Best regards,
Amy Parker
(they/them)
_______________________________________________
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] 7+ messages in thread
* Re: Best solution for shifting DAX_ZERO_PAGE to XA_ZERO_ENTRY
2020-11-09 1:15 Best solution for shifting DAX_ZERO_PAGE to XA_ZERO_ENTRY Amy Parker
@ 2020-11-09 1:33 ` Darrick J. Wong
2020-11-09 1:44 ` Amy Parker
2020-11-09 1:50 ` Matthew Wilcox
0 siblings, 2 replies; 7+ messages in thread
From: Darrick J. Wong @ 2020-11-09 1:33 UTC (permalink / raw)
To: Amy Parker; +Cc: linux-fsdevel, linux-nvdimm, Matthew Wilcox, Jan Kara
On Sun, Nov 08, 2020 at 05:15:55PM -0800, Amy Parker wrote:
> I've been writing a patch to migrate the defined DAX_ZERO_PAGE
> to XA_ZERO_ENTRY for representing holes in files.
Why? IIRC XA_ZERO_ENTRY ("no mapping in the address space") isn't the
same as DAX_ZERO_PAGE ("the zero page is mapped into the address space
because we took a read fault on a sparse file hole").
--D
> XA_ZERO_ENTRY
> is defined in include/linux/xarray.h, where it's defined using
> xa_mk_internal(257). This function returns a void pointer, which
> is incompatible with the bitwise arithmetic it is performed on with.
>
> Currently, DAX_ZERO_PAGE is defined as an unsigned long,
> so I considered typecasting it. Typecasting every time would be
> repetitive and inefficient. I thought about making a new definition
> for it which has the typecast, but this breaks the original point of
> using already defined terms.
>
> Should we go the route of adding a new definition, we might as
> well just change the definition of DAX_ZERO_PAGE. This would
> break the simplicity of the current DAX bit definitions:
>
> #define DAX_LOCKED (1UL << 0)
> #define DAX_PMD (1UL << 1)
> #define DAX_ZERO_PAGE (1UL << 2)
> #define DAX_EMPTY (1UL << 3)
>
> Any thoughts on this, and what could be the best solution here?
>
> Best regards,
> Amy Parker
> (they/them)
_______________________________________________
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] 7+ messages in thread
* Re: Best solution for shifting DAX_ZERO_PAGE to XA_ZERO_ENTRY
2020-11-09 1:33 ` Darrick J. Wong
@ 2020-11-09 1:44 ` Amy Parker
2020-11-09 1:50 ` Matthew Wilcox
1 sibling, 0 replies; 7+ messages in thread
From: Amy Parker @ 2020-11-09 1:44 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-fsdevel, linux-nvdimm, Matthew Wilcox, Jan Kara
On Sun, Nov 8, 2020 at 5:35 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> On Sun, Nov 08, 2020 at 05:15:55PM -0800, Amy Parker wrote:
> > I've been writing a patch to migrate the defined DAX_ZERO_PAGE
> > to XA_ZERO_ENTRY for representing holes in files.
>
> Why? IIRC XA_ZERO_ENTRY ("no mapping in the address space") isn't the
> same as DAX_ZERO_PAGE ("the zero page is mapped into the address space
> because we took a read fault on a sparse file hole").
>
> --D
Matthew recommended that we use a single entry here instead of adding
extra unnecessary definitions:
> Right now, fs/dax.c uses a bit -- DAX_ZERO_PAGE --
> to represent a hole in the file. It could equally well use a single entry,
> and we already have one defined, XA_ZERO_ENTRY. I think a patch to
> convert it over would be a great idea.
In practice, it works perfectly fine - ran a qemu instance with a 20 GiB
NVDIMM, set up a standard namespace and ran XFS with DAX enabled
on it, and then passed over it with xfstests. Nothing changed versus
the current effect.
_______________________________________________
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] 7+ messages in thread
* Re: Best solution for shifting DAX_ZERO_PAGE to XA_ZERO_ENTRY
2020-11-09 1:33 ` Darrick J. Wong
2020-11-09 1:44 ` Amy Parker
@ 2020-11-09 1:50 ` Matthew Wilcox
2020-11-09 1:54 ` Amy Parker
1 sibling, 1 reply; 7+ messages in thread
From: Matthew Wilcox @ 2020-11-09 1:50 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Amy Parker, linux-fsdevel, linux-nvdimm, Jan Kara
On Sun, Nov 08, 2020 at 05:33:22PM -0800, Darrick J. Wong wrote:
> On Sun, Nov 08, 2020 at 05:15:55PM -0800, Amy Parker wrote:
> > I've been writing a patch to migrate the defined DAX_ZERO_PAGE
> > to XA_ZERO_ENTRY for representing holes in files.
>
> Why? IIRC XA_ZERO_ENTRY ("no mapping in the address space") isn't the
> same as DAX_ZERO_PAGE ("the zero page is mapped into the address space
> because we took a read fault on a sparse file hole").
There's no current user of XA_ZERO_ENTRY in i_pages, whether it be
DAX or non-DAX.
> > XA_ZERO_ENTRY
> > is defined in include/linux/xarray.h, where it's defined using
> > xa_mk_internal(257). This function returns a void pointer, which
> > is incompatible with the bitwise arithmetic it is performed on with.
We don't really perform bitwise arithmetic on it, outside of:
static int dax_is_zero_entry(void *entry)
{
return xa_to_value(entry) & DAX_ZERO_PAGE;
}
> > Currently, DAX_ZERO_PAGE is defined as an unsigned long,
> > so I considered typecasting it. Typecasting every time would be
> > repetitive and inefficient. I thought about making a new definition
> > for it which has the typecast, but this breaks the original point of
> > using already defined terms.
> >
> > Should we go the route of adding a new definition, we might as
> > well just change the definition of DAX_ZERO_PAGE. This would
> > break the simplicity of the current DAX bit definitions:
> >
> > #define DAX_LOCKED (1UL << 0)
> > #define DAX_PMD (1UL << 1)
> > #define DAX_ZERO_PAGE (1UL << 2)
> > #define DAX_EMPTY (1UL << 3)
I was proposing deleting the entire bit and shifting DAX_EMPTY down.
_______________________________________________
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] 7+ messages in thread
* Re: Best solution for shifting DAX_ZERO_PAGE to XA_ZERO_ENTRY
2020-11-09 1:50 ` Matthew Wilcox
@ 2020-11-09 1:54 ` Amy Parker
2020-11-09 3:35 ` Matthew Wilcox
0 siblings, 1 reply; 7+ messages in thread
From: Amy Parker @ 2020-11-09 1:54 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Darrick J. Wong, linux-fsdevel, linux-nvdimm, Jan Kara
On Sun, Nov 8, 2020 at 5:50 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Sun, Nov 08, 2020 at 05:33:22PM -0800, Darrick J. Wong wrote:
> > On Sun, Nov 08, 2020 at 05:15:55PM -0800, Amy Parker wrote:
> > > I've been writing a patch to migrate the defined DAX_ZERO_PAGE
> > > to XA_ZERO_ENTRY for representing holes in files.
> >
> > Why? IIRC XA_ZERO_ENTRY ("no mapping in the address space") isn't the
> > same as DAX_ZERO_PAGE ("the zero page is mapped into the address space
> > because we took a read fault on a sparse file hole").
>
> There's no current user of XA_ZERO_ENTRY in i_pages, whether it be
> DAX or non-DAX.
>
> > > XA_ZERO_ENTRY
> > > is defined in include/linux/xarray.h, where it's defined using
> > > xa_mk_internal(257). This function returns a void pointer, which
> > > is incompatible with the bitwise arithmetic it is performed on with.
>
> We don't really perform bitwise arithmetic on it, outside of:
>
> static int dax_is_zero_entry(void *entry)
> {
> return xa_to_value(entry) & DAX_ZERO_PAGE;
> }
We also have:
if (dax_is_zero_entry(entry) && !(flags & DAX_ZERO_PAGE)) {
unsigned long index = xas->xa_index;
/* we are replacing a zero page with block mapping */
if (dax_is_pmd_entry(entry))
unmap_mapping_pages(mapping, index & ~PG_PMD_COLOUR,
PG_PMD_NR, false);
else /* pte entry */
unmap_mapping_pages(mapping, index, 1, false);
}
and:
*entry = dax_insert_entry(xas, mapping, vmf, *entry, pfn,
DAX_PMD | DAX_ZERO_PAGE, false);
>
> > > Currently, DAX_ZERO_PAGE is defined as an unsigned long,
> > > so I considered typecasting it. Typecasting every time would be
> > > repetitive and inefficient. I thought about making a new definition
> > > for it which has the typecast, but this breaks the original point of
> > > using already defined terms.
> > >
> > > Should we go the route of adding a new definition, we might as
> > > well just change the definition of DAX_ZERO_PAGE. This would
> > > break the simplicity of the current DAX bit definitions:
> > >
> > > #define DAX_LOCKED (1UL << 0)
> > > #define DAX_PMD (1UL << 1)
> > > #define DAX_ZERO_PAGE (1UL << 2)
> > > #define DAX_EMPTY (1UL << 3)
>
> I was proposing deleting the entire bit and shifting DAX_EMPTY down.
That'd probably be a better idea - so what should we do about the type
issue? Not typecasting it causes it not to compile.
_______________________________________________
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] 7+ messages in thread
* Re: Best solution for shifting DAX_ZERO_PAGE to XA_ZERO_ENTRY
2020-11-09 1:54 ` Amy Parker
@ 2020-11-09 3:35 ` Matthew Wilcox
2020-11-27 16:41 ` Amy Parker
0 siblings, 1 reply; 7+ messages in thread
From: Matthew Wilcox @ 2020-11-09 3:35 UTC (permalink / raw)
To: Amy Parker; +Cc: Darrick J. Wong, linux-fsdevel, linux-nvdimm, Jan Kara
On Sun, Nov 08, 2020 at 05:54:14PM -0800, Amy Parker wrote:
> On Sun, Nov 8, 2020 at 5:50 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Sun, Nov 08, 2020 at 05:33:22PM -0800, Darrick J. Wong wrote:
> > > On Sun, Nov 08, 2020 at 05:15:55PM -0800, Amy Parker wrote:
> > > > XA_ZERO_ENTRY
> > > > is defined in include/linux/xarray.h, where it's defined using
> > > > xa_mk_internal(257). This function returns a void pointer, which
> > > > is incompatible with the bitwise arithmetic it is performed on with.
> >
> > We don't really perform bitwise arithmetic on it, outside of:
> >
> > static int dax_is_zero_entry(void *entry)
> > {
> > return xa_to_value(entry) & DAX_ZERO_PAGE;
> > }
>
> We also have:
>
> if (dax_is_zero_entry(entry) && !(flags & DAX_ZERO_PAGE)) {
> unsigned long index = xas->xa_index;
> /* we are replacing a zero page with block mapping */
> if (dax_is_pmd_entry(entry))
> unmap_mapping_pages(mapping, index & ~PG_PMD_COLOUR,
> PG_PMD_NR, false);
> else /* pte entry */
> unmap_mapping_pages(mapping, index, 1, false);
> }
>
> and:
>
> *entry = dax_insert_entry(xas, mapping, vmf, *entry, pfn,
> DAX_PMD | DAX_ZERO_PAGE, false);
Right. We need to be able to distinguish whether an entry represents
a PMD size. So maybe we need XA_ZERO_PMD_ENTRY ... ? Or we could use
the recently-added xa_get_order().
> > > > Should we go the route of adding a new definition, we might as
> > > > well just change the definition of DAX_ZERO_PAGE. This would
> > > > break the simplicity of the current DAX bit definitions:
> > > >
> > > > #define DAX_LOCKED (1UL << 0)
> > > > #define DAX_PMD (1UL << 1)
> > > > #define DAX_ZERO_PAGE (1UL << 2)
> > > > #define DAX_EMPTY (1UL << 3)
> >
> > I was proposing deleting the entire bit and shifting DAX_EMPTY down.
>
> That'd probably be a better idea - so what should we do about the type
> issue? Not typecasting it causes it not to compile.
I don't think you'll need to do any casting once the bit operations go
away ...
_______________________________________________
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] 7+ messages in thread
* Re: Best solution for shifting DAX_ZERO_PAGE to XA_ZERO_ENTRY
2020-11-09 3:35 ` Matthew Wilcox
@ 2020-11-27 16:41 ` Amy Parker
0 siblings, 0 replies; 7+ messages in thread
From: Amy Parker @ 2020-11-27 16:41 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Darrick J. Wong, linux-fsdevel, linux-nvdimm, Jan Kara
Sorry for the long reply time - personal issues came up.
On Sun, Nov 8, 2020 at 7:36 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Sun, Nov 08, 2020 at 05:54:14PM -0800, Amy Parker wrote:
> > On Sun, Nov 8, 2020 at 5:50 PM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Sun, Nov 08, 2020 at 05:33:22PM -0800, Darrick J. Wong wrote:
> > > > On Sun, Nov 08, 2020 at 05:15:55PM -0800, Amy Parker wrote:
> > > > > XA_ZERO_ENTRY
> > > > > is defined in include/linux/xarray.h, where it's defined using
> > > > > xa_mk_internal(257). This function returns a void pointer, which
> > > > > is incompatible with the bitwise arithmetic it is performed on with.
> > >
> > > We don't really perform bitwise arithmetic on it, outside of:
> > >
> > > static int dax_is_zero_entry(void *entry)
> > > {
> > > return xa_to_value(entry) & DAX_ZERO_PAGE;
> > > }
> >
> > We also have:
> >
> > if (dax_is_zero_entry(entry) && !(flags & DAX_ZERO_PAGE)) {
> > unsigned long index = xas->xa_index;
> > /* we are replacing a zero page with block mapping */
> > if (dax_is_pmd_entry(entry))
> > unmap_mapping_pages(mapping, index & ~PG_PMD_COLOUR,
> > PG_PMD_NR, false);
> > else /* pte entry */
> > unmap_mapping_pages(mapping, index, 1, false);
> > }
> >
> > and:
> >
> > *entry = dax_insert_entry(xas, mapping, vmf, *entry, pfn,
> > DAX_PMD | DAX_ZERO_PAGE, false);
>
> Right. We need to be able to distinguish whether an entry represents
> a PMD size. So maybe we need XA_ZERO_PMD_ENTRY ... ? Or we could use
> the recently-added xa_get_order().
I could add an additional dependent patch for this. Where would we
want XA_ZERO_PMD_ENTRY declared? Considering we're dependent
on DAX_PMD, I'd say in fs/dax.c, but if there's a better solution I'm missing...
> >
> > That'd probably be a better idea - so what should we do about the type
> > issue? Not typecasting it causes it not to compile.
>
> I don't think you'll need to do any casting once the bit operations go
> away ...
True, but what're we going to do about dax_is_zero_entry? We haven't
figured out what to do about that yet... a typecast back to void* of
xa_to_value locally could work, as it itself is just shifting an entry right
by 1 bit and then typecasting it to unsigned long. Thoughts?
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] 7+ messages in thread
end of thread, other threads:[~2020-11-27 16:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-09 1:15 Best solution for shifting DAX_ZERO_PAGE to XA_ZERO_ENTRY Amy Parker
2020-11-09 1:33 ` Darrick J. Wong
2020-11-09 1:44 ` Amy Parker
2020-11-09 1:50 ` Matthew Wilcox
2020-11-09 1:54 ` Amy Parker
2020-11-09 3:35 ` Matthew Wilcox
2020-11-27 16:41 ` Amy Parker
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).