[PATCH] hugetlbfs: zero partial pages during fallocate hole punch

[PATCH] hugetlbfs: zero partial pages during fallocate hole punch

[Mike Kravetz]

hugetlbfs fallocate support was originally added with commit 70c3547e36f5
("hugetlbfs: add hugetlbfs_fallocate()").  Initial support only operated
on whole hugetlb pages.  This makes sense for populating files as other
interfaces such as mmap and truncate require hugetlb page size alignment.
Only operating on whole hugetlb pages for the hole punch case was a
simplification and there was no compelling use case to zero partial pages.

In a recent discussion[1] it was assumed that hugetlbfs hole punch would
zero partial hugetlb pages as that is in line with the man page
description saying 'partial filesystem  blocks  are  zeroed'.  However,
the hugetlbfs hole punch code actually does this:

        hole_start = round_up(offset, hpage_size);
        hole_end = round_down(offset + len, hpage_size);

Modify code to zero partial hugetlb pages in hole punch range.  It is
possible that application code could note a change in behavior.  However,
that would imply the code is passing in an unaligned range and expecting
only whole pages be removed.  This is unlikely as the fallocate
documentation states the opposite.

The current hugetlbfs fallocate hole punch behavior is tested with the
libhugetlbfs test fallocate_align[2].  This test will be updated to
validate partial page zeroing.

[1] https://lore.kernel.org/linux-mm/20571829-9d3d-0b48-817c-b6b15565f651@redhat.com/
[2] https://github.com/libhugetlbfs/libhugetlbfs/blob/master/tests/fallocate_align.c

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 fs/hugetlbfs/inode.c | 68 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 53 insertions(+), 15 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index eca1d0fabd7e..b0d0947a7e9b 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -584,41 +584,79 @@ static void hugetlb_vmtruncate(struct inode *inode, loff_t offset)
 	remove_inode_hugepages(inode, offset, LLONG_MAX);
 }
 
+static void hugetlbfs_zero_partial_page(struct hstate *h,
+					struct address_space *mapping,
+					unsigned long start,
+					unsigned long end)
+{
+	pgoff_t idx = start >> huge_page_shift(h);
+	struct page *page;
+
+	page = find_lock_page(mapping, idx);
+	if (!page)
+		return;
+
+	start = start & ~huge_page_mask(h);
+	end = end & ~huge_page_mask(h);
+	if (!end)
+		end = huge_page_size(h);
+
+	zero_user_segment(page, (unsigned int)start, (unsigned int)end);
+
+	unlock_page(page);
+	put_page(page);
+}
+
 static long hugetlbfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
 {
+	struct hugetlbfs_inode_info *info = HUGETLBFS_I(inode);
+	struct address_space *mapping = inode->i_mapping;
 	struct hstate *h = hstate_inode(inode);
 	loff_t hpage_size = huge_page_size(h);
 	loff_t hole_start, hole_end;
 
 	/*
-	 * For hole punch round up the beginning offset of the hole and
-	 * round down the end.
+	 * hole_start and hole_end indicate the full pages within the hole.
 	 */
 	hole_start = round_up(offset, hpage_size);
 	hole_end = round_down(offset + len, hpage_size);
 
-	if (hole_end > hole_start) {
-		struct address_space *mapping = inode->i_mapping;
-		struct hugetlbfs_inode_info *info = HUGETLBFS_I(inode);
+	inode_lock(inode);
 
-		inode_lock(inode);
+	/* protected by i_rwsem */
+	if (info->seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE)) {
+		inode_unlock(inode);
+		return -EPERM;
+	}
 
-		/* protected by i_rwsem */
-		if (info->seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE)) {
-			inode_unlock(inode);
-			return -EPERM;
-		}
+	i_mmap_lock_write(mapping);
+
+	/* If range starts before first full page, zero partial page. */
+	if (offset < hole_start)
+		hugetlbfs_zero_partial_page(h, mapping,
+				offset, min(offset + len, hole_start));
 
-		i_mmap_lock_write(mapping);
+	/* Unmap users of full pages in the hole. */
+	if (hole_end > hole_start) {
 		if (!RB_EMPTY_ROOT(&mapping->i_mmap.rb_root))
 			hugetlb_vmdelete_list(&mapping->i_mmap,
 					      hole_start >> PAGE_SHIFT,
 					      hole_end >> PAGE_SHIFT, 0);
-		i_mmap_unlock_write(mapping);
-		remove_inode_hugepages(inode, hole_start, hole_end);
-		inode_unlock(inode);
 	}
 
+	/* If range extends beyond last full page, zero partial page. */
+	if ((offset + len) > hole_end && (offset + len) > hole_start)
+		hugetlbfs_zero_partial_page(h, mapping,
+				hole_end, offset + len);
+
+	i_mmap_unlock_write(mapping);
+
+	/* Remove full pages from the file. */
+	if (hole_end > hole_start)
+		remove_inode_hugepages(inode, hole_start, hole_end);
+
+	inode_unlock(inode);
+
 	return 0;
 }
 
-- 
2.35.3

Re: [PATCH] hugetlbfs: zero partial pages during fallocate hole punch

[Matthew Wilcox]

On Mon, Jun 13, 2022 at 11:08:58AM -0700, Mike Kravetz wrote:
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index eca1d0fabd7e..b0d0947a7e9b 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -584,41 +584,79 @@ static void hugetlb_vmtruncate(struct inode *inode, loff_t offset)
>  	remove_inode_hugepages(inode, offset, LLONG_MAX);
>  }
>  
> +static void hugetlbfs_zero_partial_page(struct hstate *h,
> +					struct address_space *mapping,
> +					unsigned long start,

This should be loff_t.  Otherwise we can truncate on 32-bit machines.

> +					unsigned long end)
> +{
> +	pgoff_t idx = start >> huge_page_shift(h);
> +	struct page *page;
> +
> +	page = find_lock_page(mapping, idx);
> +	if (!page)
> +		return;
> +
> +	start = start & ~huge_page_mask(h);
> +	end = end & ~huge_page_mask(h);
> +	if (!end)
> +		end = huge_page_size(h);
> +
> +	zero_user_segment(page, (unsigned int)start, (unsigned int)end);
> +
> +	unlock_page(page);
> +	put_page(page);

We haven't started converting hugetlbfs to folios yet, but here's how
that would look (which will save us converting it later):

	folio = filemap_lock_folio(mapping, idx);
	if (!folio)
		return;
...
	folio_zero_segment(folio, start, end);
	folio_unlock(folio);
	folio_put(folio);

Pretty much a 1-for-1 replacement.

Re: [PATCH] hugetlbfs: zero partial pages during fallocate hole punch

[Mike kravetz]

On Mon, Jun 13, 2022 at 07:30:22PM +0100”, Matthew Wilcox wrote:
> On Mon, Jun 13, 2022 at 11:08:58AM -0700, Mike Kravetz wrote:
> > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> > index eca1d0fabd7e..b0d0947a7e9b 100644
> > --- a/fs/hugetlbfs/inode.c
> > +++ b/fs/hugetlbfs/inode.c
> > @@ -584,41 +584,79 @@ static void hugetlb_vmtruncate(struct inode *inode, loff_t offset)
> >  	remove_inode_hugepages(inode, offset, LLONG_MAX);
> >  }
> >  
> > +static void hugetlbfs_zero_partial_page(struct hstate *h,
> > +					struct address_space *mapping,
> > +					unsigned long start,
> 
> This should be loff_t.  Otherwise we can truncate on 32-bit machines.
> 

Thanks!  I missed that.

> > +					unsigned long end)
> > +{
> > +	pgoff_t idx = start >> huge_page_shift(h);
> > +	struct page *page;
> > +
> > +	page = find_lock_page(mapping, idx);
> > +	if (!page)
> > +		return;
> > +
> > +	start = start & ~huge_page_mask(h);
> > +	end = end & ~huge_page_mask(h);
> > +	if (!end)
> > +		end = huge_page_size(h);
> > +
> > +	zero_user_segment(page, (unsigned int)start, (unsigned int)end);
> > +
> > +	unlock_page(page);
> > +	put_page(page);
> 
> We haven't started converting hugetlbfs to folios yet, but here's how
> that would look (which will save us converting it later):
> 
> 	folio = filemap_lock_folio(mapping, idx);
> 	if (!folio)
> 		return;
> ...
> 	folio_zero_segment(folio, start, end);
> 	folio_unlock(folio);
> 	folio_put(folio);
> 
> Pretty much a 1-for-1 replacement.
> 

Great.  The folio conversion is on my todo list, but I would not be
offended if someone beat me to it.
-- 
Mike Kravetz

Re: [PATCH] hugetlbfs: zero partial pages during fallocate hole punch

[Andrew Morton]

On Mon, 13 Jun 2022 11:36:30 -0700 Mike kravetz <mike.kravetz@oracle.com> wrote:

> On Mon, Jun 13, 2022 at 07:30:22PM +0100”, Matthew Wilcox wrote:
> > On Mon, Jun 13, 2022 at 11:08:58AM -0700, Mike Kravetz wrote:
> > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> > > index eca1d0fabd7e..b0d0947a7e9b 100644
> > > --- a/fs/hugetlbfs/inode.c
> > > +++ b/fs/hugetlbfs/inode.c
> > > @@ -584,41 +584,79 @@ static void hugetlb_vmtruncate(struct inode *inode, loff_t offset)
> > >  	remove_inode_hugepages(inode, offset, LLONG_MAX);
> > >  }
> > >  
> > > +static void hugetlbfs_zero_partial_page(struct hstate *h,
> > > +					struct address_space *mapping,
> > > +					unsigned long start,
> > 
> > This should be loff_t.  Otherwise we can truncate on 32-bit machines.
> > 
> 
> Thanks!  I missed that.

I did this:

--- a/fs/hugetlbfs/inode.c~hugetlbfs-zero-partial-pages-during-fallocate-hole-punch-fix
+++ a/fs/hugetlbfs/inode.c
@@ -602,8 +602,7 @@ static void hugetlb_vmtruncate(struct in
 
 static void hugetlbfs_zero_partial_page(struct hstate *h,
 					struct address_space *mapping,
-					unsigned long start,
-					unsigned long end)
+					loff_t start, loff_t end)
 {
 	pgoff_t idx = start >> huge_page_shift(h);
 	struct page *page;
_

Re: [PATCH] hugetlbfs: zero partial pages during fallocate hole punch

[Mike Kravetz]

On Mon, Jun 13, 2022 at 11:57:32AM -0700”, Andrew Morton wrote:
> On Mon, 13 Jun 2022 11:36:30 -0700 Mike kravetz <mike.kravetz@oracle.com> wrote:
> 
> > On Mon, Jun 13, 2022 at 07:30:22PM +0100”, Matthew Wilcox wrote:
> > > On Mon, Jun 13, 2022 at 11:08:58AM -0700, Mike Kravetz wrote:
> > > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> > > > index eca1d0fabd7e..b0d0947a7e9b 100644
> > > > --- a/fs/hugetlbfs/inode.c
> > > > +++ b/fs/hugetlbfs/inode.c
> > > > @@ -584,41 +584,79 @@ static void hugetlb_vmtruncate(struct inode *inode, loff_t offset)
> > > >  	remove_inode_hugepages(inode, offset, LLONG_MAX);
> > > >  }
> > > >  
> > > > +static void hugetlbfs_zero_partial_page(struct hstate *h,
> > > > +					struct address_space *mapping,
> > > > +					unsigned long start,
> > > 
> > > This should be loff_t.  Otherwise we can truncate on 32-bit machines.
> > > 
> > 
> > Thanks!  I missed that.
> 
> I did this:

Thank you.

I will send an official v2 with this change, just in case there are more
comments.

-- 
Mike Kravetz

> 
> --- a/fs/hugetlbfs/inode.c~hugetlbfs-zero-partial-pages-during-fallocate-hole-punch-fix
> +++ a/fs/hugetlbfs/inode.c
> @@ -602,8 +602,7 @@ static void hugetlb_vmtruncate(struct in
>  
>  static void hugetlbfs_zero_partial_page(struct hstate *h,
>  					struct address_space *mapping,
> -					unsigned long start,
> -					unsigned long end)
> +					loff_t start, loff_t end)
>  {
>  	pgoff_t idx = start >> huge_page_shift(h);
>  	struct page *page;
> _
> 

Re: [PATCH] hugetlbfs: zero partial pages during fallocate hole punch

[Mike Kravetz]

Below is v2 of the patch.  I was just going to change the type of start/end
hugetlbfs_zero_partial_page arguments.  However, Matthew also convinced me
to do the simple folio conversion.  Tested with new libhugetlbfs code.


From 36a18e0b07c2e189092cc2d516e8cfedcb57d191 Mon Sep 17 00:00:00 2001
From: Mike Kravetz <mike.kravetz@oracle.com>
Date: Mon, 13 Jun 2022 13:36:48 -0700
Subject: [Patch v2] hugetlbfs: zero partial pages during fallocate hole punch

hugetlbfs fallocate support was originally added with commit 70c3547e36f5
("hugetlbfs: add hugetlbfs_fallocate()").  Initial support only operated
on whole hugetlb pages.  This makes sense for populating files as other
interfaces such as mmap and truncate require hugetlb page size alignment.
Only operating on whole hugetlb pages for the hole punch case was a
simplification and there was no compelling use case to zero partial pages.

In a recent discussion[1] it was assumed that hugetlbfs hole punch would
zero partial hugetlb pages as that is in line with the man page
description saying 'partial filesystem  blocks  are  zeroed'.  However,
the hugetlbfs hole punch code actually does this:

        hole_start = round_up(offset, hpage_size);
        hole_end = round_down(offset + len, hpage_size);

Modify code to zero partial hugetlb pages in hole punch range.  It is
possible that application code could note a change in behavior.  However,
that would imply the code is passing in an unaligned range and expecting
only whole pages be removed.  This is unlikely as the fallocate
documentation states the opposite.

The current hugetlbfs fallocate hole punch behavior is tested with the
libhugetlbfs test fallocate_align[2].  This test will be updated to
validate partial page zeroing.

[1] https://lore.kernel.org/linux-mm/20571829-9d3d-0b48-817c-b6b15565f651@redhat.com/
[2] https://github.com/libhugetlbfs/libhugetlbfs/blob/master/tests/fallocate_align.c

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
v1->v2:	Change type of hugetlbfs_zero_partial_page start/end args to
	loff_t and convert function to use folio.  (Matthew)
 fs/hugetlbfs/inode.c | 68 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 53 insertions(+), 15 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index eca1d0fabd7e..20336cb3c040 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -584,41 +584,79 @@ static void hugetlb_vmtruncate(struct inode *inode, loff_t offset)
 	remove_inode_hugepages(inode, offset, LLONG_MAX);
 }
 
+static void hugetlbfs_zero_partial_page(struct hstate *h,
+					struct address_space *mapping,
+					loff_t start,
+					loff_t end)
+{
+	pgoff_t idx = start >> huge_page_shift(h);
+	struct folio *folio;
+
+	folio = filemap_lock_folio(mapping, idx);
+	if (!folio)
+		return;
+
+	start = start & ~huge_page_mask(h);
+	end = end & ~huge_page_mask(h);
+	if (!end)
+		end = huge_page_size(h);
+
+	folio_zero_segment(folio, (size_t)start, (size_t)end);
+
+	folio_unlock(folio);
+	folio_put(folio);
+}
+
 static long hugetlbfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
 {
+	struct hugetlbfs_inode_info *info = HUGETLBFS_I(inode);
+	struct address_space *mapping = inode->i_mapping;
 	struct hstate *h = hstate_inode(inode);
 	loff_t hpage_size = huge_page_size(h);
 	loff_t hole_start, hole_end;
 
 	/*
-	 * For hole punch round up the beginning offset of the hole and
-	 * round down the end.
+	 * hole_start and hole_end indicate the full pages within the hole.
 	 */
 	hole_start = round_up(offset, hpage_size);
 	hole_end = round_down(offset + len, hpage_size);
 
-	if (hole_end > hole_start) {
-		struct address_space *mapping = inode->i_mapping;
-		struct hugetlbfs_inode_info *info = HUGETLBFS_I(inode);
+	inode_lock(inode);
 
-		inode_lock(inode);
+	/* protected by i_rwsem */
+	if (info->seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE)) {
+		inode_unlock(inode);
+		return -EPERM;
+	}
 
-		/* protected by i_rwsem */
-		if (info->seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE)) {
-			inode_unlock(inode);
-			return -EPERM;
-		}
+	i_mmap_lock_write(mapping);
+
+	/* If range starts before first full page, zero partial page. */
+	if (offset < hole_start)
+		hugetlbfs_zero_partial_page(h, mapping,
+				offset, min(offset + len, hole_start));
 
-		i_mmap_lock_write(mapping);
+	/* Unmap users of full pages in the hole. */
+	if (hole_end > hole_start) {
 		if (!RB_EMPTY_ROOT(&mapping->i_mmap.rb_root))
 			hugetlb_vmdelete_list(&mapping->i_mmap,
 					      hole_start >> PAGE_SHIFT,
 					      hole_end >> PAGE_SHIFT, 0);
-		i_mmap_unlock_write(mapping);
-		remove_inode_hugepages(inode, hole_start, hole_end);
-		inode_unlock(inode);
 	}
 
+	/* If range extends beyond last full page, zero partial page. */
+	if ((offset + len) > hole_end && (offset + len) > hole_start)
+		hugetlbfs_zero_partial_page(h, mapping,
+				hole_end, offset + len);
+
+	i_mmap_unlock_write(mapping);
+
+	/* Remove full pages from the file. */
+	if (hole_end > hole_start)
+		remove_inode_hugepages(inode, hole_start, hole_end);
+
+	inode_unlock(inode);
+
 	return 0;
 }
 
-- 
2.35.3

Re: [PATCH] hugetlbfs: zero partial pages during fallocate hole punch

[Muchun Song]

On Tue, Jun 14, 2022 at 4:46 AM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> Below is v2 of the patch.  I was just going to change the type of start/end
> hugetlbfs_zero_partial_page arguments.  However, Matthew also convinced me
> to do the simple folio conversion.  Tested with new libhugetlbfs code.
>
>
> From 36a18e0b07c2e189092cc2d516e8cfedcb57d191 Mon Sep 17 00:00:00 2001
> From: Mike Kravetz <mike.kravetz@oracle.com>
> Date: Mon, 13 Jun 2022 13:36:48 -0700
> Subject: [Patch v2] hugetlbfs: zero partial pages during fallocate hole punch
>
> hugetlbfs fallocate support was originally added with commit 70c3547e36f5
> ("hugetlbfs: add hugetlbfs_fallocate()").  Initial support only operated
> on whole hugetlb pages.  This makes sense for populating files as other
> interfaces such as mmap and truncate require hugetlb page size alignment.
> Only operating on whole hugetlb pages for the hole punch case was a
> simplification and there was no compelling use case to zero partial pages.
>
> In a recent discussion[1] it was assumed that hugetlbfs hole punch would
> zero partial hugetlb pages as that is in line with the man page
> description saying 'partial filesystem  blocks  are  zeroed'.  However,
> the hugetlbfs hole punch code actually does this:
>
>         hole_start = round_up(offset, hpage_size);
>         hole_end = round_down(offset + len, hpage_size);
>
> Modify code to zero partial hugetlb pages in hole punch range.  It is
> possible that application code could note a change in behavior.  However,
> that would imply the code is passing in an unaligned range and expecting
> only whole pages be removed.  This is unlikely as the fallocate
> documentation states the opposite.
>
> The current hugetlbfs fallocate hole punch behavior is tested with the
> libhugetlbfs test fallocate_align[2].  This test will be updated to
> validate partial page zeroing.
>
> [1] https://lore.kernel.org/linux-mm/20571829-9d3d-0b48-817c-b6b15565f651@redhat.com/
> [2] https://github.com/libhugetlbfs/libhugetlbfs/blob/master/tests/fallocate_align.c
>
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>

LGTM.

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.