Re: [PATCH v2 1/5] dax: add dax_get_unmapped_area for pmd mappings

[Toshi Kani]

On Tue, 2016-04-12 at 23:18 -0400, Matthew Wilcox wrote:
> On Tue, Apr 12, 2016 at 02:39:15PM -0600, Toshi Kani wrote:
> > 
> > + * When the target file is not a DAX file, @addr is specified, the
> > + * request is not suitable for pmd mappings, or mm-
> > >get_unmapped_area()
> > + * failed with extended @len, it simply calls the default handler,
> > + * mm->get_unmapped_area(), with the original arguments.
>
> I think you can lose this paragraph.ÂÂIt describes what the function
> does, not why it does it ... and we can see what the function does from
> reading the code.

Agreed. ÂI will remove this paragraph.

> I think this is one of those functions which is actually improved with
> gotos, purely to reduce the indentation level.
> 
> unsigned long dax_get_unmapped_area(struct file *filp, unsigned long
> addr,
> 		unsigned long len, unsigned long pgoff, unsigned long
> flags)
> {
> 	unsigned long off, off_end, off_pmd, len_pmd, addr_pmd;
> 
> 	if (!IS_ENABLED(CONFIG_FS_DAX_PMD) ||
> 	ÂÂÂÂ!filp || addr || !IS_DAX(filp->f_mapping->host))
> 		goto out;
> 
> 	off = pgoff << PAGE_SHIFT;
> 	off_end = off + len;
> 	off_pmd = round_up(off, PMD_SIZE);
> 	if ((off_end <= off_pmd) || ((off_end - off_pmd) < PMD_SIZE))
> 		goto out;
> 
> 	len_pmd = len + PMD_SIZE;
> 	addr_pmd = current->mm->get_unmapped_area(filp, addr, len_pmd,
> 							pgoff, flags);
> 
> 	if (!IS_ERR_VALUE(addr_pmd)) {
> 		addr_pmd += (off - addr_pmd) & (PMD_SIZE - 1);
> 		return addr_pmd;
> 	}
> 
> Âout:
> 	return current->mm->get_unmapped_area(filp, addr, len, pgoff,
> flags);
> }

Sounds good.

> Now ... back to the original version, some questions:
> 
> > 
> > +unsigned long dax_get_unmapped_area(struct file *filp, unsigned long
> > addr,
> > +		unsigned long len, unsigned long pgoff, unsigned long
> > flags)
> > +{
> > +	unsigned long off, off_end, off_pmd, len_pmd, addr_pmd;
> > +
> > +	if (IS_ENABLED(CONFIG_FS_DAX_PMD) &&
> > +	ÂÂÂÂfilp && !addr && IS_DAX(filp->f_mapping->host)) {
> > +		off = pgoff << PAGE_SHIFT;
> > +		off_end = off + len;
>
> Can off + len wrap here, or did that get checked earlier?

Yes, do_mmap() has checked this condition earlier. ÂBut, I think I need to
check it for (off + len_pmd).

> > 
> > +		off_pmd = round_up(off, PMD_SIZE);
> > +
> > +		if ((off_end > off_pmd) && ((off_end - off_pmd) >=
> > PMD_SIZE)) {
>
> We're only going to look for a PMD-aligned mapping if the mapping is at
> least double PMD_SIZE?ÂÂI don't understand that decision.ÂÂSeems to me
> that if I ask to map offset 4MB, length 2MB, I should get a PMD-aligned
> mapping.

It checks if this request can be covered by a PMD page.ÂÂ'off_pmd' is the
first PMD-aligned offset.ÂÂThere needs to be at least 2MB from this offset
to the end, 'off_end', in order to cover with a PMD page.

In your example, 'off_pmd' is still 4MB, which has 2MB to the end.ÂÂSo, so
this request can be covered by a PMD page.

Another example, say, offset 4KB and length 2MB. Â'off_pmd' is 2MB, which
has only 4KB to the end.ÂÂSo, this request cannot be covered by a PMD page.

> Speaking of offset, we don't have any checks that offset is a multiple
> of PMD_SIZE.ÂÂI know that theoretically we could map offset 1.5MB, length
> 3MB and see the first 0.5MB filled with small pages, then the next 2MB
> filled with one large page, and the tail filled with small pages, but I
> think we're better off only looking for PMD-alignment if the user asked
> for an aligned offset in the file.

Yes, that's what it checks. ÂIn this case, 'off_pmd' is set to 2MB, which
has 2.5MB to the end. ÂSo, it can be covered by a PMD page. ÂThe offset
itself does not have to be aligned by 2MB.


> > +			len_pmd = len + PMD_SIZE;
> > +
> > +			addr_pmd = current->mm->get_unmapped_area(
> > +					filp, addr, len_pmd, pgoff,
> > flags);
> > +
> > +			if (!IS_ERR_VALUE(addr_pmd)) {
> > +				addr_pmd += (off - addr_pmd) &
> > (PMD_SIZE - 1);
>
> ... then you wouldn't need this calculation ;-)

Applications should not be required to provide a 2MB-aligned offset.

Thanks,
-Toshi