[PATCH 1/2] xfs: allow inode allocations in post-growfs disk space

[PATCH 1/2] xfs: allow inode allocations in post-growfs disk space

[Eric Sandeen]

Today, if we perform an xfs_growfs which adds allocation groups,
mp->m_maxagi is not properly updated when the growfs is complete.

Therefore inodes will continue to be allocated only in the
AGs which existed prior to the growfs, and the new space
won't be utilized.

This is because of this path in xfs_growfs_data_private():

xfs_growfs_data_private
	xfs_initialize_perag(mp, nagcount, &nagimax);
	 	if (mp->m_flags & XFS_MOUNT_32BITINODES)
			index = xfs_set_inode32(mp);
 		else
			index = xfs_set_inode64(mp);

	 	if (maxagi)
 			*maxagi = index;

where xfs_set_inode* iterates over the (old) agcount in
mp->m_sb.sb_agblocks, which has not yet been updated
in the growfs path.  So "index" will be returned based on
the old agcount, not the new one, and new AGs are not available
for inode allocation.

Fix this by explicitly passing the proper AG count (which 
xfs_initialize_perag() already has) down another level,
so that xfs_set_inode* can make the proper decision about
acceptable AGs for inode allocation in the potentially
newly-added AGs.

This has been broken since 3.7, when these two
xfs_set_inode* functions were added in commit 2d2194f.
Prior to that, we looped over "agcount" not sb_agblocks
in these calculations.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

tested for regression with the -g growfs group, but this
shows that we need another testcase for growfs.


diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 993cb19..b291ada 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -250,9 +250,9 @@ xfs_initialize_perag(
 		mp->m_flags &= ~XFS_MOUNT_32BITINODES;
 
 	if (mp->m_flags & XFS_MOUNT_32BITINODES)
-		index = xfs_set_inode32(mp);
+		index = xfs_set_inode32(mp, agcount);
 	else
-		index = xfs_set_inode64(mp);
+		index = xfs_set_inode64(mp, agcount);
 
 	if (maxagi)
 		*maxagi = index;
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 87e8b01..ccc564d 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -597,8 +597,13 @@ xfs_max_file_offset(
 	return (((__uint64_t)pagefactor) << bitshift) - 1;
 }
 
+/*
+ * xfs_set_inode32() and xfs_set_inode64() are passed an agcount
+ * because in the growfs case, mp->m_sb.sb_agcount is not updated
+ * yet to the potentially higher ag count.
+ */
 xfs_agnumber_t
-xfs_set_inode32(struct xfs_mount *mp)
+xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount)
 {
 	xfs_agnumber_t	index = 0;
 	xfs_agnumber_t	maxagi = 0;
@@ -620,10 +625,10 @@ xfs_set_inode32(struct xfs_mount *mp)
 		do_div(icount, sbp->sb_agblocks);
 		max_metadata = icount;
 	} else {
-		max_metadata = sbp->sb_agcount;
+		max_metadata = agcount;
 	}
 
-	for (index = 0; index < sbp->sb_agcount; index++) {
+	for (index = 0; index < agcount; index++) {
 		ino = XFS_AGINO_TO_INO(mp, index, agino);
 
 		if (ino > XFS_MAXINUMBER_32) {
@@ -648,11 +653,11 @@ xfs_set_inode32(struct xfs_mount *mp)
 }
 
 xfs_agnumber_t
-xfs_set_inode64(struct xfs_mount *mp)
+xfs_set_inode64(struct xfs_mount *mp, xfs_agnumber_t agcount)
 {
 	xfs_agnumber_t index = 0;
 
-	for (index = 0; index < mp->m_sb.sb_agcount; index++) {
+	for (index = 0; index < agcount; index++) {
 		struct xfs_perag	*pag;
 
 		pag = xfs_perag_get(mp, index);
@@ -1193,6 +1198,7 @@ xfs_fs_remount(
 	char			*options)
 {
 	struct xfs_mount	*mp = XFS_M(sb);
+	xfs_sb_t		*sbp = &mp->m_sb;
 	substring_t		args[MAX_OPT_ARGS];
 	char			*p;
 	int			error;
@@ -1212,10 +1218,10 @@ xfs_fs_remount(
 			mp->m_flags &= ~XFS_MOUNT_BARRIER;
 			break;
 		case Opt_inode64:
-			mp->m_maxagi = xfs_set_inode64(mp);
+			mp->m_maxagi = xfs_set_inode64(mp, sbp->sb_agcount);
 			break;
 		case Opt_inode32:
-			mp->m_maxagi = xfs_set_inode32(mp);
+			mp->m_maxagi = xfs_set_inode32(mp, sbp->sb_agcount);
 			break;
 		default:
 			/*
diff --git a/fs/xfs/xfs_super.h b/fs/xfs/xfs_super.h
index bbe3d15..b4cfe21 100644
--- a/fs/xfs/xfs_super.h
+++ b/fs/xfs/xfs_super.h
@@ -76,8 +76,8 @@ extern __uint64_t xfs_max_file_offset(unsigned int);
 
 extern void xfs_flush_inodes(struct xfs_mount *mp);
 extern void xfs_blkdev_issue_flush(struct xfs_buftarg *);
-extern xfs_agnumber_t xfs_set_inode32(struct xfs_mount *);
-extern xfs_agnumber_t xfs_set_inode64(struct xfs_mount *);
+extern xfs_agnumber_t xfs_set_inode32(struct xfs_mount *, xfs_agnumber_t agcount);
+extern xfs_agnumber_t xfs_set_inode64(struct xfs_mount *, xfs_agnumber_t agcount);
 
 extern const struct export_operations xfs_export_operations;
 extern const struct xattr_handler *xfs_xattr_handlers[];


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/2] xfs: tidy up xfs_set_inode32

[Eric Sandeen]

xfs_set_inode32() caught my eye because it had
weird spacing around the "-1's".  In cleaning that
up, I realized that the assignment in the declaration
of "ino" is never used; it's rewritten before it gets
read.

Drop the ino initializer from its declaration since it's
not used, and move the agino initialization into the body
of the function, mostly so that we can have pretty
whitespace and not exceed 80 columns.  :)

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index e71c0f8..39c9315 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -609,8 +609,8 @@ xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount)
 	xfs_agnumber_t	maxagi = 0;
 	xfs_sb_t	*sbp = &mp->m_sb;
 	xfs_agnumber_t	max_metadata;
-	xfs_agino_t	agino =	XFS_OFFBNO_TO_AGINO(mp, sbp->sb_agblocks -1, 0);
-	xfs_ino_t	ino = XFS_AGINO_TO_INO(mp, sbp->sb_agcount -1, agino);
+	xfs_agino_t	agino;
+	xfs_ino_t	ino;
 	xfs_perag_t	*pag;
 
 	/* Calculate how much should be reserved for inodes to meet
@@ -628,6 +628,8 @@ xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount)
 		max_metadata = agcount;
 	}
 
+	agino =	XFS_OFFBNO_TO_AGINO(mp, sbp->sb_agblocks - 1, 0);
+
 	for (index = 0; index < agcount; index++) {
 		ino = XFS_AGINO_TO_INO(mp, index, agino);
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2] xfs: allow inode allocations in post-growfs disk space

[Brian Foster]

On Wed, Jul 02, 2014 at 11:54:06PM -0500, Eric Sandeen wrote:
> Today, if we perform an xfs_growfs which adds allocation groups,
> mp->m_maxagi is not properly updated when the growfs is complete.
> 
> Therefore inodes will continue to be allocated only in the
> AGs which existed prior to the growfs, and the new space
> won't be utilized.
> 
> This is because of this path in xfs_growfs_data_private():
> 
> xfs_growfs_data_private
> 	xfs_initialize_perag(mp, nagcount, &nagimax);
> 	 	if (mp->m_flags & XFS_MOUNT_32BITINODES)
> 			index = xfs_set_inode32(mp);
>  		else
> 			index = xfs_set_inode64(mp);
> 
> 	 	if (maxagi)
>  			*maxagi = index;
> 
> where xfs_set_inode* iterates over the (old) agcount in
> mp->m_sb.sb_agblocks, which has not yet been updated
> in the growfs path.  So "index" will be returned based on
> the old agcount, not the new one, and new AGs are not available
> for inode allocation.
> 
> Fix this by explicitly passing the proper AG count (which 
> xfs_initialize_perag() already has) down another level,
> so that xfs_set_inode* can make the proper decision about
> acceptable AGs for inode allocation in the potentially
> newly-added AGs.
> 
> This has been broken since 3.7, when these two
> xfs_set_inode* functions were added in commit 2d2194f.
> Prior to that, we looped over "agcount" not sb_agblocks
> in these calculations.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> tested for regression with the -g growfs group, but this
> shows that we need another testcase for growfs.
> 
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 993cb19..b291ada 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -250,9 +250,9 @@ xfs_initialize_perag(
>  		mp->m_flags &= ~XFS_MOUNT_32BITINODES;
>  
>  	if (mp->m_flags & XFS_MOUNT_32BITINODES)
> -		index = xfs_set_inode32(mp);
> +		index = xfs_set_inode32(mp, agcount);
>  	else
> -		index = xfs_set_inode64(mp);
> +		index = xfs_set_inode64(mp, agcount);
>  
>  	if (maxagi)
>  		*maxagi = index;
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 87e8b01..ccc564d 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -597,8 +597,13 @@ xfs_max_file_offset(
>  	return (((__uint64_t)pagefactor) << bitshift) - 1;
>  }
>  
> +/*
> + * xfs_set_inode32() and xfs_set_inode64() are passed an agcount
> + * because in the growfs case, mp->m_sb.sb_agcount is not updated
> + * yet to the potentially higher ag count.
> + */
>  xfs_agnumber_t
> -xfs_set_inode32(struct xfs_mount *mp)
> +xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount)
>  {
>  	xfs_agnumber_t	index = 0;
>  	xfs_agnumber_t	maxagi = 0;
> @@ -620,10 +625,10 @@ xfs_set_inode32(struct xfs_mount *mp)
>  		do_div(icount, sbp->sb_agblocks);
>  		max_metadata = icount;
>  	} else {
> -		max_metadata = sbp->sb_agcount;
> +		max_metadata = agcount;

The fix looks pretty good to me, but what about the 'if' branch of this
logic here? We calculate max_metadata based on sb_dblocks, which also
isn't updated until the growfs tp commit. That appears to be a similar
bug in that we wouldn't set pagf_metadata on the new AGs where
appropriate, which I think means data allocation could steal the new
inode space sooner than anticipated.

I wonder if this is better moved after the superblock is updated?

Brian

>  	}
>  
> -	for (index = 0; index < sbp->sb_agcount; index++) {
> +	for (index = 0; index < agcount; index++) {
>  		ino = XFS_AGINO_TO_INO(mp, index, agino);
>  
>  		if (ino > XFS_MAXINUMBER_32) {
> @@ -648,11 +653,11 @@ xfs_set_inode32(struct xfs_mount *mp)
>  }
>  
>  xfs_agnumber_t
> -xfs_set_inode64(struct xfs_mount *mp)
> +xfs_set_inode64(struct xfs_mount *mp, xfs_agnumber_t agcount)
>  {
>  	xfs_agnumber_t index = 0;
>  
> -	for (index = 0; index < mp->m_sb.sb_agcount; index++) {
> +	for (index = 0; index < agcount; index++) {
>  		struct xfs_perag	*pag;
>  
>  		pag = xfs_perag_get(mp, index);
> @@ -1193,6 +1198,7 @@ xfs_fs_remount(
>  	char			*options)
>  {
>  	struct xfs_mount	*mp = XFS_M(sb);
> +	xfs_sb_t		*sbp = &mp->m_sb;
>  	substring_t		args[MAX_OPT_ARGS];
>  	char			*p;
>  	int			error;
> @@ -1212,10 +1218,10 @@ xfs_fs_remount(
>  			mp->m_flags &= ~XFS_MOUNT_BARRIER;
>  			break;
>  		case Opt_inode64:
> -			mp->m_maxagi = xfs_set_inode64(mp);
> +			mp->m_maxagi = xfs_set_inode64(mp, sbp->sb_agcount);
>  			break;
>  		case Opt_inode32:
> -			mp->m_maxagi = xfs_set_inode32(mp);
> +			mp->m_maxagi = xfs_set_inode32(mp, sbp->sb_agcount);
>  			break;
>  		default:
>  			/*
> diff --git a/fs/xfs/xfs_super.h b/fs/xfs/xfs_super.h
> index bbe3d15..b4cfe21 100644
> --- a/fs/xfs/xfs_super.h
> +++ b/fs/xfs/xfs_super.h
> @@ -76,8 +76,8 @@ extern __uint64_t xfs_max_file_offset(unsigned int);
>  
>  extern void xfs_flush_inodes(struct xfs_mount *mp);
>  extern void xfs_blkdev_issue_flush(struct xfs_buftarg *);
> -extern xfs_agnumber_t xfs_set_inode32(struct xfs_mount *);
> -extern xfs_agnumber_t xfs_set_inode64(struct xfs_mount *);
> +extern xfs_agnumber_t xfs_set_inode32(struct xfs_mount *, xfs_agnumber_t agcount);
> +extern xfs_agnumber_t xfs_set_inode64(struct xfs_mount *, xfs_agnumber_t agcount);
>  
>  extern const struct export_operations xfs_export_operations;
>  extern const struct xattr_handler *xfs_xattr_handlers[];
> 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs: tidy up xfs_set_inode32

[Brian Foster]

On Wed, Jul 02, 2014 at 11:57:41PM -0500, Eric Sandeen wrote:
> xfs_set_inode32() caught my eye because it had
> weird spacing around the "-1's".  In cleaning that
> up, I realized that the assignment in the declaration
> of "ino" is never used; it's rewritten before it gets
> read.
> 
> Drop the ino initializer from its declaration since it's
> not used, and move the agino initialization into the body
> of the function, mostly so that we can have pretty
> whitespace and not exceed 80 columns.  :)
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index e71c0f8..39c9315 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -609,8 +609,8 @@ xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount)
>  	xfs_agnumber_t	maxagi = 0;
>  	xfs_sb_t	*sbp = &mp->m_sb;
>  	xfs_agnumber_t	max_metadata;
> -	xfs_agino_t	agino =	XFS_OFFBNO_TO_AGINO(mp, sbp->sb_agblocks -1, 0);
> -	xfs_ino_t	ino = XFS_AGINO_TO_INO(mp, sbp->sb_agcount -1, agino);
> +	xfs_agino_t	agino;
> +	xfs_ino_t	ino;
>  	xfs_perag_t	*pag;
>  
>  	/* Calculate how much should be reserved for inodes to meet
> @@ -628,6 +628,8 @@ xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount)
>  		max_metadata = agcount;
>  	}
>  
> +	agino =	XFS_OFFBNO_TO_AGINO(mp, sbp->sb_agblocks - 1, 0);
> +
>  	for (index = 0; index < agcount; index++) {
>  		ino = XFS_AGINO_TO_INO(mp, index, agino);
>  
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2] xfs: allow inode allocations in post-growfs disk space

[Eric Sandeen]

On 7/7/14, 1:21 PM, Brian Foster wrote:
> On Wed, Jul 02, 2014 at 11:54:06PM -0500, Eric Sandeen wrote:


<context added>:

        /* Calculate how much should be reserved for inodes to meet
         * the max inode percentage.
         */
        if (mp->m_maxicount) {
                __uint64_t      icount;

                icount = sbp->sb_dblocks * sbp->sb_imax_pct;
                do_div(icount, 100);
                icount += sbp->sb_agblocks - 1;
>> @@ -620,10 +625,10 @@ xfs_set_inode32(struct xfs_mount *mp)
>>  		do_div(icount, sbp->sb_agblocks);
>>  		max_metadata = icount;
>>  	} else {
>> -		max_metadata = sbp->sb_agcount;
>> +		max_metadata = agcount;
> 
> The fix looks pretty good to me, but what about the 'if' branch of this
> logic here? We calculate max_metadata based on sb_dblocks, which also
> isn't updated until the growfs tp commit. That appears to be a similar
> bug in that we wouldn't set pagf_metadata on the new AGs where
> appropriate, which I think means data allocation could steal the new
> inode space sooner than anticipated.

Yeah, good catch...

Hm, well - not that this is an answer, but this code has been this way
since 2005.  So I'd like to fix the *regression* w/ my patch as-is,
and then worry about this.

So, on to worrying about this ... ;)

"max_metadata" seems a little misnamed; inodes can be allocated in higher
AGs, but "max_metadata" and lower are the 'preferred' AGs for inode
allocation.

We only carve out enough via pag->pagf_metadata to reserve m_maxicount,
which (here) is based on the (old) sb_dblocks & sb_imax_pct.

So yeah, it seems that in the growfs case, we don't mark any *new* AGs as
"preferred" for inodes, even though with a fixed sb_imax_pct and a larger
sb_dblocks, we'd need more space to accommodate the imaxpct.

But reserving higher AGs would be a half-measure at best; they weren't
preferred before the growfs, so are very likely not wholly available
after the growfs.

To really nail this down we'd probably need to see how many inode clusters
could be created in each AG above the old threshold, and keep advancing AGs
until we've "preferred" enough.  Bleah.  I hate inode32...

-Eric



> I wonder if this is better moved after the superblock is updated?
> 
> Brian
> 
>>  	}
>>  
>> -	for (index = 0; index < sbp->sb_agcount; index++) {
>> +	for (index = 0; index < agcount; index++) {
>>  		ino = XFS_AGINO_TO_INO(mp, index, agino);
>>  
>>  		if (ino > XFS_MAXINUMBER_32) {
>> @@ -648,11 +653,11 @@ xfs_set_inode32(struct xfs_mount *mp)
>>  }
>>  
>>  xfs_agnumber_t
>> -xfs_set_inode64(struct xfs_mount *mp)
>> +xfs_set_inode64(struct xfs_mount *mp, xfs_agnumber_t agcount)
>>  {
>>  	xfs_agnumber_t index = 0;
>>  
>> -	for (index = 0; index < mp->m_sb.sb_agcount; index++) {
>> +	for (index = 0; index < agcount; index++) {
>>  		struct xfs_perag	*pag;
>>  
>>  		pag = xfs_perag_get(mp, index);
>> @@ -1193,6 +1198,7 @@ xfs_fs_remount(
>>  	char			*options)
>>  {
>>  	struct xfs_mount	*mp = XFS_M(sb);
>> +	xfs_sb_t		*sbp = &mp->m_sb;
>>  	substring_t		args[MAX_OPT_ARGS];
>>  	char			*p;
>>  	int			error;
>> @@ -1212,10 +1218,10 @@ xfs_fs_remount(
>>  			mp->m_flags &= ~XFS_MOUNT_BARRIER;
>>  			break;
>>  		case Opt_inode64:
>> -			mp->m_maxagi = xfs_set_inode64(mp);
>> +			mp->m_maxagi = xfs_set_inode64(mp, sbp->sb_agcount);
>>  			break;
>>  		case Opt_inode32:
>> -			mp->m_maxagi = xfs_set_inode32(mp);
>> +			mp->m_maxagi = xfs_set_inode32(mp, sbp->sb_agcount);
>>  			break;
>>  		default:
>>  			/*
>> diff --git a/fs/xfs/xfs_super.h b/fs/xfs/xfs_super.h
>> index bbe3d15..b4cfe21 100644
>> --- a/fs/xfs/xfs_super.h
>> +++ b/fs/xfs/xfs_super.h
>> @@ -76,8 +76,8 @@ extern __uint64_t xfs_max_file_offset(unsigned int);
>>  
>>  extern void xfs_flush_inodes(struct xfs_mount *mp);
>>  extern void xfs_blkdev_issue_flush(struct xfs_buftarg *);
>> -extern xfs_agnumber_t xfs_set_inode32(struct xfs_mount *);
>> -extern xfs_agnumber_t xfs_set_inode64(struct xfs_mount *);
>> +extern xfs_agnumber_t xfs_set_inode32(struct xfs_mount *, xfs_agnumber_t agcount);
>> +extern xfs_agnumber_t xfs_set_inode64(struct xfs_mount *, xfs_agnumber_t agcount);
>>  
>>  extern const struct export_operations xfs_export_operations;
>>  extern const struct xattr_handler *xfs_xattr_handlers[];
>>
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2] xfs: allow inode allocations in post-growfs disk space

[Brian Foster]

On Mon, Jul 07, 2014 at 02:01:09PM -0500, Eric Sandeen wrote:
> On 7/7/14, 1:21 PM, Brian Foster wrote:
> > On Wed, Jul 02, 2014 at 11:54:06PM -0500, Eric Sandeen wrote:
> 
> 
> <context added>:
> 
>         /* Calculate how much should be reserved for inodes to meet
>          * the max inode percentage.
>          */
>         if (mp->m_maxicount) {
>                 __uint64_t      icount;
> 
>                 icount = sbp->sb_dblocks * sbp->sb_imax_pct;
>                 do_div(icount, 100);
>                 icount += sbp->sb_agblocks - 1;
> >> @@ -620,10 +625,10 @@ xfs_set_inode32(struct xfs_mount *mp)
> >>  		do_div(icount, sbp->sb_agblocks);
> >>  		max_metadata = icount;
> >>  	} else {
> >> -		max_metadata = sbp->sb_agcount;
> >> +		max_metadata = agcount;
> > 
> > The fix looks pretty good to me, but what about the 'if' branch of this
> > logic here? We calculate max_metadata based on sb_dblocks, which also
> > isn't updated until the growfs tp commit. That appears to be a similar
> > bug in that we wouldn't set pagf_metadata on the new AGs where
> > appropriate, which I think means data allocation could steal the new
> > inode space sooner than anticipated.
> 
> Yeah, good catch...
> 
> Hm, well - not that this is an answer, but this code has been this way
> since 2005.  So I'd like to fix the *regression* w/ my patch as-is,
> and then worry about this.
> 

That's fine by me... for this patch:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> So, on to worrying about this ... ;)
> 
> "max_metadata" seems a little misnamed; inodes can be allocated in higher
> AGs, but "max_metadata" and lower are the 'preferred' AGs for inode
> allocation.
> 

My impression is that it's named after its very specific/local use: the
max ag to set the metadata flag. I don't really like the name either. ;)

> We only carve out enough via pag->pagf_metadata to reserve m_maxicount,
> which (here) is based on the (old) sb_dblocks & sb_imax_pct.
> 
> So yeah, it seems that in the growfs case, we don't mark any *new* AGs as
> "preferred" for inodes, even though with a fixed sb_imax_pct and a larger
> sb_dblocks, we'd need more space to accommodate the imaxpct.
> 
> But reserving higher AGs would be a half-measure at best; they weren't
> preferred before the growfs, so are very likely not wholly available
> after the growfs.
> 
> To really nail this down we'd probably need to see how many inode clusters
> could be created in each AG above the old threshold, and keep advancing AGs
> until we've "preferred" enough.  Bleah.  I hate inode32...
> 

That's true, but then you have to worry about if that space is freed up
and all that...

I was just looking at it from the perspective of using old metadata to
update some of our heuristics while thinking it might be easier to move
this hunk of code and fix both problems at once. I doubt this is
something that reproduces a tangible problem out in the wild much, if
ever, given the circumstances.

My sense is that this heuristic is not really a guarantee. E.g., setting
imaxpct doesn't guarantee one that much space. Somebody could tune that
after the fact even after all "preferred" space has been consumed just
the same, so it's probably not worth doing any kind of fancy inode
allocation tracking to try and make this retroactive or dynamic unless
it proves to be a problem. I suspect this primarily exists for the case
of larger inode32 fs' where we don't want data allocations to eat up
limited inode space right off the bat.

Brian

> -Eric
> 
> 
> 
> > I wonder if this is better moved after the superblock is updated?
> > 
> > Brian
> > 
> >>  	}
> >>  
> >> -	for (index = 0; index < sbp->sb_agcount; index++) {
> >> +	for (index = 0; index < agcount; index++) {
> >>  		ino = XFS_AGINO_TO_INO(mp, index, agino);
> >>  
> >>  		if (ino > XFS_MAXINUMBER_32) {
> >> @@ -648,11 +653,11 @@ xfs_set_inode32(struct xfs_mount *mp)
> >>  }
> >>  
> >>  xfs_agnumber_t
> >> -xfs_set_inode64(struct xfs_mount *mp)
> >> +xfs_set_inode64(struct xfs_mount *mp, xfs_agnumber_t agcount)
> >>  {
> >>  	xfs_agnumber_t index = 0;
> >>  
> >> -	for (index = 0; index < mp->m_sb.sb_agcount; index++) {
> >> +	for (index = 0; index < agcount; index++) {
> >>  		struct xfs_perag	*pag;
> >>  
> >>  		pag = xfs_perag_get(mp, index);
> >> @@ -1193,6 +1198,7 @@ xfs_fs_remount(
> >>  	char			*options)
> >>  {
> >>  	struct xfs_mount	*mp = XFS_M(sb);
> >> +	xfs_sb_t		*sbp = &mp->m_sb;
> >>  	substring_t		args[MAX_OPT_ARGS];
> >>  	char			*p;
> >>  	int			error;
> >> @@ -1212,10 +1218,10 @@ xfs_fs_remount(
> >>  			mp->m_flags &= ~XFS_MOUNT_BARRIER;
> >>  			break;
> >>  		case Opt_inode64:
> >> -			mp->m_maxagi = xfs_set_inode64(mp);
> >> +			mp->m_maxagi = xfs_set_inode64(mp, sbp->sb_agcount);
> >>  			break;
> >>  		case Opt_inode32:
> >> -			mp->m_maxagi = xfs_set_inode32(mp);
> >> +			mp->m_maxagi = xfs_set_inode32(mp, sbp->sb_agcount);
> >>  			break;
> >>  		default:
> >>  			/*
> >> diff --git a/fs/xfs/xfs_super.h b/fs/xfs/xfs_super.h
> >> index bbe3d15..b4cfe21 100644
> >> --- a/fs/xfs/xfs_super.h
> >> +++ b/fs/xfs/xfs_super.h
> >> @@ -76,8 +76,8 @@ extern __uint64_t xfs_max_file_offset(unsigned int);
> >>  
> >>  extern void xfs_flush_inodes(struct xfs_mount *mp);
> >>  extern void xfs_blkdev_issue_flush(struct xfs_buftarg *);
> >> -extern xfs_agnumber_t xfs_set_inode32(struct xfs_mount *);
> >> -extern xfs_agnumber_t xfs_set_inode64(struct xfs_mount *);
> >> +extern xfs_agnumber_t xfs_set_inode32(struct xfs_mount *, xfs_agnumber_t agcount);
> >> +extern xfs_agnumber_t xfs_set_inode64(struct xfs_mount *, xfs_agnumber_t agcount);
> >>  
> >>  extern const struct export_operations xfs_export_operations;
> >>  extern const struct xattr_handler *xfs_xattr_handlers[];
> >>
> >>
> >> _______________________________________________
> >> xfs mailing list
> >> xfs@oss.sgi.com
> >> http://oss.sgi.com/mailman/listinfo/xfs
> > 
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
> > 
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs