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@xxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
