[patch] hugetlb: fix i_blocks accounting

[patch] hugetlb: fix i_blocks accounting

[Ken Chen]

resent as I mistakenly bcc'ed linux-mm mailing list, where I really meant to cc.

---------- Forwarded message ----------
From: Ken Chen <kenchen@google.com>
Date: Nov 8, 2007 11:43 PM
Subject: [patch] hugetlb: fix i_blocks accounting
To: Andrew Morton <akpm@linux-foundation.org>, Adam Litke <agl@us.ibm.com>


rebase i_blocks bug fix on top of Adam's recent fs quota work.

------
For administrative purpose, we want to query actual block usage for
hugetlbfs file via fstat.  Currently, hugetlbfs always return 0.  Fix
that up since kernel already has all the information to track it
properly.


Signed-off-by: Ken Chen <kenchen@google.com>

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 770dbed..65371bd 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -168,6 +168,8 @@ struct file *hugetlb_file_setup(const char *name, size_t);
 int hugetlb_get_quota(struct address_space *mapping, long delta);
 void hugetlb_put_quota(struct address_space *mapping, long delta);

+#define BLOCKS_PER_HUGEPAGE	(HPAGE_SIZE / 512)
+
 static inline int is_file_hugepages(struct file *file)
 {
 	if (file->f_op == &hugetlbfs_file_operations)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index b58edd0..0d54370 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -801,6 +801,7 @@ retry:

 		if (vma->vm_flags & VM_SHARED) {
 			int err;
+			struct inode *inode = mapping->host;

 			err = add_to_page_cache(page, mapping, idx, GFP_KERNEL);
 			if (err) {
@@ -809,6 +810,10 @@ retry:
 					goto retry;
 				goto out;
 			}
+
+			spin_lock(&inode->i_lock);
+			inode->i_blocks += BLOCKS_PER_HUGEPAGE;
+			spin_unlock(&inode->i_lock);
 		} else
 			lock_page(page);
 	}
@@ -1159,6 +1164,11 @@ int hugetlb_reserve_pages(struct inode *inode,
long from, long to)
 void hugetlb_unreserve_pages(struct inode *inode, long offset, long freed)
 {
 	long chg = region_truncate(&inode->i_mapping->private_list, offset);
+
+	spin_lock(&inode->i_lock);
+	inode->i_blocks -= BLOCKS_PER_HUGEPAGE * freed;
+	spin_unlock(&inode->i_lock);
+
 	hugetlb_put_quota(inode->i_mapping, (chg - freed));
 	hugetlb_acct_memory(-(chg - freed));
 }

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] hugetlb: fix i_blocks accounting

[Ken Chen]

On Nov 9, 2007 6:17 AM, aglitke <agl@us.ibm.com> wrote:
> > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> > index 770dbed..65371bd 100644
> > --- a/include/linux/hugetlb.h
> > +++ b/include/linux/hugetlb.h
> > @@ -168,6 +168,8 @@ struct file *hugetlb_file_setup(const char *name, size_t);
> >  int hugetlb_get_quota(struct address_space *mapping, long delta);
> >  void hugetlb_put_quota(struct address_space *mapping, long delta);
> >
> > +#define BLOCKS_PER_HUGEPAGE  (HPAGE_SIZE / 512)
>
> Sorry if this is an obvious question, but where does 512 above come
> from?

out of stat(2) man page:

The st_blocks field indicates the number of blocks allocated to the
file,  512-byte
units.   (This  may  be  smaller  than  st_size/512, for example, when
the file has
holes.)

I looked at what other fs do with the i_blocks field (ext2, tmpfs),
they all follow the above convention, regardless what the underlying
fs block size is or arch page size.

> Is this just establishing a new convention that a block is equal
> to 1/512th of whatever size a huge page happens to be?

I'm trying to be consistent with other fs.

> What about on
> ia64 where the hugepage size is set at boot?  Wouldn't that be confusing
> to have the block size change between boots?  What if we just make the
> block size equal to PAGE_SIZE (which is a more stable quantity)?

It shouldn't matter, as there is another field st_blksize which
indicate block size for the filesystem.  i_blocks is just an
accounting on number of blocks allocated and it appears to me that it
was intentionally set to 512 byte unit in the man page (to cut down
confusion?  I have no idea).

- Ken

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] hugetlb: fix i_blocks accounting

[aglitke]

Thanks for that explanation.  It makes complete sense to me now.

On Fri, 2007-11-09 at 09:42 -0800, Ken Chen wrote:
> On Nov 9, 2007 6:17 AM, aglitke <agl@us.ibm.com> wrote:
> > > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> > > index 770dbed..65371bd 100644
> > > --- a/include/linux/hugetlb.h
> > > +++ b/include/linux/hugetlb.h
> > > @@ -168,6 +168,8 @@ struct file *hugetlb_file_setup(const char *name, size_t);
> > >  int hugetlb_get_quota(struct address_space *mapping, long delta);
> > >  void hugetlb_put_quota(struct address_space *mapping, long delta);
> > >
> > > +#define BLOCKS_PER_HUGEPAGE  (HPAGE_SIZE / 512)
> >
> > Sorry if this is an obvious question, but where does 512 above come
> > from?
> 
> out of stat(2) man page:
> 
> The st_blocks field indicates the number of blocks allocated to the
> file,  512-byte
> units.   (This  may  be  smaller  than  st_size/512, for example, when
> the file has
> holes.)
> 
> I looked at what other fs do with the i_blocks field (ext2, tmpfs),
> they all follow the above convention, regardless what the underlying
> fs block size is or arch page size.
> 
> > Is this just establishing a new convention that a block is equal
> > to 1/512th of whatever size a huge page happens to be?
> 
> I'm trying to be consistent with other fs.
> 
> > What about on
> > ia64 where the hugepage size is set at boot?  Wouldn't that be confusing
> > to have the block size change between boots?  What if we just make the
> > block size equal to PAGE_SIZE (which is a more stable quantity)?
> 
> It shouldn't matter, as there is another field st_blksize which
> indicate block size for the filesystem.  i_blocks is just an
> accounting on number of blocks allocated and it appears to me that it
> was intentionally set to 512 byte unit in the man page (to cut down
> confusion?  I have no idea).
> 
> - Ken
> 
-- 
Adam Litke - (agl at us.ibm.com)
IBM Linux Technology Center

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] hugetlb: fix i_blocks accounting

[Andrew Morton]

On Fri, 09 Nov 2007 12:09:57 -0600
aglitke <agl@us.ibm.com> wrote:

> Thanks for that explanation.  It makes complete sense to me now.

We have a distressing number of hugetlb patches here:

hugetlb-follow_hugetlb_page-for-write-access.patch
hugetlb-allow-sticky-directory-mount-option.patch
hugetlb-split-alloc_huge_page-into-private-and-shared-components.patch
hugetlb-split-alloc_huge_page-into-private-and-shared-components-checkpatch-fixes.patch
hugetlb-fix-quota-management-for-private-mappings.patch
hugetlb-debit-quota-in-alloc_huge_page.patch
hugetlb-allow-bulk-updating-in-hugetlb__quota.patch
hugetlb-enforce-quotas-during-reservation-for-shared-mappings.patch
mm-hugetlbc-make-a-function-static.patch
hugetlb-fix-i_blocks-accounting.patch

(all available at http://userweb.kernel.org/~akpm/mmotm/)

Could we please put heads together and work out which of these need to go
into 2.6.24?  And 2.6.23, come to that...

Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] hugetlb: fix i_blocks accounting

[Ken Chen]

On Nov 9, 2007 5:16 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri, 09 Nov 2007 12:09:57 -0600
> aglitke <agl@us.ibm.com> wrote:
>
> > Thanks for that explanation.  It makes complete sense to me now.
>
> We have a distressing number of hugetlb patches here:
>
> hugetlb-follow_hugetlb_page-for-write-access.patch
> hugetlb-allow-sticky-directory-mount-option.patch
> hugetlb-split-alloc_huge_page-into-private-and-shared-components.patch
> hugetlb-split-alloc_huge_page-into-private-and-shared-components-checkpatch-fixes.patch
> hugetlb-fix-quota-management-for-private-mappings.patch
> hugetlb-debit-quota-in-alloc_huge_page.patch
> hugetlb-allow-bulk-updating-in-hugetlb__quota.patch
> hugetlb-enforce-quotas-during-reservation-for-shared-mappings.patch
> mm-hugetlbc-make-a-function-static.patch
> hugetlb-fix-i_blocks-accounting.patch
>
> (all available at http://userweb.kernel.org/~akpm/mmotm/)
>
> Could we please put heads together and work out which of these need to go
> into 2.6.24?  And 2.6.23, come to that...

I would vote all of it.  If we really need to prioritize them, I would
list them in the following order:

1. fs quota fix:
   hugetlb-split-alloc_huge_page-into-private-and-shared-components.patch
   hugetlb-split-alloc_huge_page-into-private-and-shared-components-checkpatch-fixes.patch
   hugetlb-fix-quota-management-for-private-mappings.patch
   hugetlb-debit-quota-in-alloc_huge_page.patch
   hugetlb-allow-bulk-updating-in-hugetlb__quota.patch
   hugetlb-enforce-quotas-during-reservation-for-shared-mappings.patch

2. i_blocks accounting
   hugetlb-fix-i_blocks-accounting.patch

3. follow_hugetlb_page (this is a rather nasty bug, I'm glad we
haven't hit it in real world.  Or maybe Adam did, and hence the
patch?).
   hugetlb-follow_hugetlb_page-for-write-access.patch

4. others (these are really simple single line low risk patches, why not?)
   hugetlb-allow-sticky-directory-mount-option.patch
   mm-hugetlbc-make-a-function-static.patch

oh, there are more bugs in hugetlb: sys_mincore isn't working on
hugetlb range.  I guess that can wait for 2.6.25.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] hugetlb: fix i_blocks accounting

[aglitke]

On Fri, 2007-11-09 at 17:34 -0800, Ken Chen wrote:
> On Nov 9, 2007 5:16 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Fri, 09 Nov 2007 12:09:57 -0600
> > aglitke <agl@us.ibm.com> wrote:
> >
> > > Thanks for that explanation.  It makes complete sense to me now.
> >
> > We have a distressing number of hugetlb patches here:
> >
> > hugetlb-follow_hugetlb_page-for-write-access.patch
> > hugetlb-allow-sticky-directory-mount-option.patch
> > hugetlb-split-alloc_huge_page-into-private-and-shared-components.patch
> > hugetlb-split-alloc_huge_page-into-private-and-shared-components-checkpatch-fixes.patch
> > hugetlb-fix-quota-management-for-private-mappings.patch
> > hugetlb-debit-quota-in-alloc_huge_page.patch
> > hugetlb-allow-bulk-updating-in-hugetlb__quota.patch
> > hugetlb-enforce-quotas-during-reservation-for-shared-mappings.patch
> > mm-hugetlbc-make-a-function-static.patch
> > hugetlb-fix-i_blocks-accounting.patch
> >
> > (all available at http://userweb.kernel.org/~akpm/mmotm/)
> >
> > Could we please put heads together and work out which of these need to go
> > into 2.6.24?  And 2.6.23, come to that...
> 
> I would vote all of it.  If we really need to prioritize them, I would
> list them in the following order:

I agree that all of them are ready to go for 2.6.24.  I don't think #3
(follow_hugetlb_page fix) meets the criteria for a -stable patch
otherwise I'd have suggested it go forth into 2.6.23.  

> 1. fs quota fix:
>    hugetlb-split-alloc_huge_page-into-private-and-shared-components.patch
>    hugetlb-split-alloc_huge_page-into-private-and-shared-components-checkpatch-fixes.patch
>    hugetlb-fix-quota-management-for-private-mappings.patch
>    hugetlb-debit-quota-in-alloc_huge_page.patch
>    hugetlb-allow-bulk-updating-in-hugetlb__quota.patch
>    hugetlb-enforce-quotas-during-reservation-for-shared-mappings.patch
> 
> 2. i_blocks accounting
>    hugetlb-fix-i_blocks-accounting.patch
> 
> 3. follow_hugetlb_page (this is a rather nasty bug, I'm glad we
> haven't hit it in real world.  Or maybe Adam did, and hence the
> patch?).

We're starting to see this with the infiniband driver.  They have a
workaround (to touch each hugepage in userspace before handing it to the
driver), but the sooner it gets fixed upstream, the better obviously.

>    hugetlb-follow_hugetlb_page-for-write-access.patch
> 
> 4. others (these are really simple single line low risk patches, why not?)
>    hugetlb-allow-sticky-directory-mount-option.patch
>    mm-hugetlbc-make-a-function-static.patch
> 
> oh, there are more bugs in hugetlb: sys_mincore isn't working on
> hugetlb range.  I guess that can wait for 2.6.25.
> 
-- 
Adam Litke - (agl at us.ibm.com)
IBM Linux Technology Center

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[patch] hugetlb: fix i_blocks accounting

[Ken Chen]

For administrative purpose, we want to query actual block usage for
hugetlbfs file via fstat.  Currently, hugetlbfs always return 0.  Fix
that up since kernel already has all the information to track it
properly.


Signed-off-by: Ken Chen <kenchen@google.com>


diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 12aca8e..ed6def0 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -862,7 +862,8 @@ out_free:
 int hugetlb_get_quota(struct address_space *mapping)
 {
 	int ret = 0;
-	struct hugetlbfs_sb_info *sbinfo = HUGETLBFS_SB(mapping->host->i_sb);
+	struct inode *inode = mapping->host;
+	struct hugetlbfs_sb_info *sbinfo = HUGETLBFS_SB(inode->i_sb);

 	if (sbinfo->free_blocks > -1) {
 		spin_lock(&sbinfo->stat_lock);
@@ -873,13 +874,17 @@ int hugetlb_get_quota(struct address_space *mapping)
 		spin_unlock(&sbinfo->stat_lock);
 	}

+	if (!ret)
+		inode->i_blocks += BLOCKS_PER_HUGEPAGE;
 	return ret;
 }

 void hugetlb_put_quota(struct address_space *mapping)
 {
-	struct hugetlbfs_sb_info *sbinfo = HUGETLBFS_SB(mapping->host->i_sb);
+	struct inode *inode = mapping->host;
+	struct hugetlbfs_sb_info *sbinfo = HUGETLBFS_SB(inode->i_sb);

+	inode->i_blocks -= BLOCKS_PER_HUGEPAGE;
 	if (sbinfo->free_blocks > -1) {
 		spin_lock(&sbinfo->stat_lock);
 		sbinfo->free_blocks++;
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index ea0f50b..694cf8b 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -168,6 +168,8 @@ struct file *hugetlb_file_setup(const char *name, size_t);
 int hugetlb_get_quota(struct address_space *mapping);
 void hugetlb_put_quota(struct address_space *mapping);

+#define BLOCKS_PER_HUGEPAGE	(HPAGE_SIZE / 512)
+
 static inline int is_file_hugepages(struct file *file)
 {
 	if (file->f_op == &hugetlbfs_file_operations)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] hugetlb: fix i_blocks accounting

[Adam Litke]

On Sat, 2007-10-20 at 11:18 -0700, Ken Chen wrote:
> For administrative purpose, we want to query actual block usage for
> hugetlbfs file via fstat.  Currently, hugetlbfs always return 0.  Fix
> that up since kernel already has all the information to track it
> properly.

Hey Ken.  You might want to wait on this for another minute or two.  I
will be sending out patches later today to fix up hugetlbfs quotas.
Right now the code does not handle private mappings correctly (ie.  it
does not call get_quota() for COW pages and it never calls put_quota()
for any private page).  Because of this, your i_blocks number will be
wrong most of the time.


-- 
Adam Litke - (agl at us.ibm.com)
IBM Linux Technology Center

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] hugetlb: fix i_blocks accounting

[Ken Chen]

On 10/23/07, Adam Litke <agl@us.ibm.com> wrote:
> On Sat, 2007-10-20 at 11:18 -0700, Ken Chen wrote:
> > For administrative purpose, we want to query actual block usage for
> > hugetlbfs file via fstat.  Currently, hugetlbfs always return 0.  Fix
> > that up since kernel already has all the information to track it
> > properly.
>
> Hey Ken.  You might want to wait on this for another minute or two.  I
> will be sending out patches later today to fix up hugetlbfs quotas.
> Right now the code does not handle private mappings correctly (ie.  it
> does not call get_quota() for COW pages and it never calls put_quota()
> for any private page).  Because of this, your i_blocks number will be
> wrong most of the time.

Adam, speaking of hugetlb file system quota, there is another bug in
there for shared mapping as well.

At the time of mmap (MMAP_SHARED), kernel only check page reservation
against available hugetlb page pool.  FS quota is not checked at all.
Now we over commit the fs quota for shared mapping, but still let the
mmap to succeed.  At later point in the page fault path, app will
eventually die with SIGBUS due to lack of fs quota.  This behavior
broke a few apps for us. The bad part is there is no easy recovery
path once a SIGBUS is raised.

I tried with MAP_POPULATE, but unfortunately it doesn't propagate
error code back up to user space on mmap; same thing with mlockall
that also ignores error code returned from make_pages_present().
Using mlock is at best half baked because VM_LOCKED maybe already set
via other means.

So this fs quota thing really needs some love and attention.

- Ken

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] hugetlb: fix i_blocks accounting

[Adam Litke]

On Tue, 2007-10-23 at 17:34 -0700, Ken Chen wrote:
> On 10/23/07, Adam Litke <agl@us.ibm.com> wrote:
> > On Sat, 2007-10-20 at 11:18 -0700, Ken Chen wrote:
> > > For administrative purpose, we want to query actual block usage for
> > > hugetlbfs file via fstat.  Currently, hugetlbfs always return 0.  Fix
> > > that up since kernel already has all the information to track it
> > > properly.
> >
> > Hey Ken.  You might want to wait on this for another minute or two.  I
> > will be sending out patches later today to fix up hugetlbfs quotas.
> > Right now the code does not handle private mappings correctly (ie.  it
> > does not call get_quota() for COW pages and it never calls put_quota()
> > for any private page).  Because of this, your i_blocks number will be
> > wrong most of the time.
> 
> Adam, speaking of hugetlb file system quota, there is another bug in
> there for shared mapping as well.

Yep ;)  I already have a fix for that too in this series.  Coming right
up.

> At the time of mmap (MMAP_SHARED), kernel only check page reservation
> against available hugetlb page pool.  FS quota is not checked at all.
> Now we over commit the fs quota for shared mapping, but still let the
> mmap to succeed.  At later point in the page fault path, app will
> eventually die with SIGBUS due to lack of fs quota.  This behavior
> broke a few apps for us. The bad part is there is no easy recovery
> path once a SIGBUS is raised.
> 
> I tried with MAP_POPULATE, but unfortunately it doesn't propagate
> error code back up to user space on mmap; same thing with mlockall
> that also ignores error code returned from make_pages_present().
> Using mlock is at best half baked because VM_LOCKED maybe already set
> via other means.
> 
> So this fs quota thing really needs some love and attention.

Yep, now that we are actually starting to use it...

-- 
Adam Litke - (agl at us.ibm.com)
IBM Linux Technology Center

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>