Message-ID: <1460559303.24985.52.camel@hpe.com>
Subject: Re: [PATCH v2 1/5] dax: add dax_get_unmapped_area for pmd mappings
From: Toshi Kani <toshi.kani@hpe.com>
To: Matthew Wilcox <willy@linux.intel.com>
Cc: akpm@linux-foundation.org, dan.j.williams@intel.com,
        viro@zeniv.linux.org.uk, ross.zwisler@linux.intel.com,
        kirill.shutemov@linux.intel.com, david@fromorbit.com, jack@suse.cz,
        tytso@mit.edu, adilger.kernel@dilger.ca, linux-nvdimm@ml01.01.org,
        linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org,
        xfs@oss.sgi.com, linux-kernel@vger.kernel.org
Date: Wed, 13 Apr 2016 08:55:03 -0600
In-Reply-To: <20160413031801.GO2781@linux.intel.com>
References: <1460493555-31611-1-git-send-email-toshi.kani@hpe.com>
	 <1460493555-31611-2-git-send-email-toshi.kani@hpe.com>
	 <20160413031801.GO2781@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4067
Lines: 120

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