Re: Best solution for shifting DAX_ZERO_PAGE to XA_ZERO_ENTRY
Sorry for the long reply time - personal issues came up. On Sun, Nov 8, 2020 at 7:36 PM Matthew Wilcox 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 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-le...@lists.01.org
Re: Best solution for shifting DAX_ZERO_PAGE to XA_ZERO_ENTRY
On Sun, Nov 08, 2020 at 05:54:14PM -0800, Amy Parker wrote: > On Sun, Nov 8, 2020 at 5:50 PM Matthew Wilcox 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-le...@lists.01.org
Re: Best solution for shifting DAX_ZERO_PAGE to XA_ZERO_ENTRY
On Sun, Nov 8, 2020 at 5:50 PM Matthew Wilcox 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-le...@lists.01.org
Re: Best solution for shifting DAX_ZERO_PAGE to XA_ZERO_ENTRY
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-le...@lists.01.org
Re: Best solution for shifting DAX_ZERO_PAGE to XA_ZERO_ENTRY
On Sun, Nov 8, 2020 at 5:35 PM 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"). > > --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-le...@lists.01.org
Re: Best solution for shifting DAX_ZERO_PAGE to XA_ZERO_ENTRY
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-le...@lists.01.org
Best solution for shifting DAX_ZERO_PAGE to XA_ZERO_ENTRY
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-le...@lists.01.org