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
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
