* [PATCH] xfs: fix up inode32/64 (re)mount handling
@ 2016-02-17 4:47 Eric Sandeen
2016-02-17 18:30 ` Brian Foster
0 siblings, 1 reply; 5+ messages in thread
From: Eric Sandeen @ 2016-02-17 4:47 UTC (permalink / raw)
To: xfs
inode32/inode64 allocator behavior with respect to mount,
remount and growfs is a little tricky.
The inode32 mount option should only enable the inode32
allocator heuristics if the filesystem is large enough
for 64-bit inodes to exist. Today, it has this behavior
on the initial mount, but a remount with inode32
unconditionally changes the allocation heuristics, even
for a small fs.
Also, an inode32 mounted small filesystem should transition
to the inode32 allocator if the filesystem is subsequently
grown to a sufficient size. Today that does not happen.
This patch consolidates xfs_set_inode32 and xfs_set_inode64
into a single new function, and moves the "is the maximum inode
number big enough to matter" test into that function, so
it doesn't rely on the caller to get it right - which
remount did not do, previously.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
Note, this goes after my token-parsing patch for mount.
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index f8c4a50..83c2f92 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -185,9 +185,6 @@ xfs_initialize_perag(
xfs_agnumber_t index;
xfs_agnumber_t first_initialised = 0;
xfs_perag_t *pag;
- xfs_agino_t agino;
- xfs_ino_t ino;
- xfs_sb_t *sbp = &mp->m_sb;
int error = -ENOMEM;
/*
@@ -230,22 +227,7 @@ xfs_initialize_perag(
radix_tree_preload_end();
}
- /*
- * If we mount with the inode64 option, or no inode overflows
- * the legacy 32-bit address space clear the inode32 option.
- */
- agino = XFS_OFFBNO_TO_AGINO(mp, sbp->sb_agblocks - 1, 0);
- ino = XFS_AGINO_TO_INO(mp, agcount - 1, agino);
-
- if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) && ino > XFS_MAXINUMBER_32)
- mp->m_flags |= XFS_MOUNT_32BITINODES;
- else
- mp->m_flags &= ~XFS_MOUNT_32BITINODES;
-
- if (mp->m_flags & XFS_MOUNT_32BITINODES)
- index = xfs_set_inode32(mp, agcount);
- else
- index = xfs_set_inode64(mp, agcount);
+ index = xfs_set_inode_alloc(mp, agcount);
if (maxagi)
*maxagi = index;
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 2c38bef..40acf8f 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -205,9 +205,8 @@ typedef struct xfs_mount {
#define XFS_MOUNT_GRPID (1ULL << 9) /* group-ID assigned from directory */
#define XFS_MOUNT_NORECOVERY (1ULL << 10) /* no recovery - dirty fs */
#define XFS_MOUNT_DFLT_IOSIZE (1ULL << 12) /* set default i/o size */
-#define XFS_MOUNT_32BITINODES (1ULL << 14) /* do not create inodes above
- * 32 bits in size */
-#define XFS_MOUNT_SMALL_INUMS (1ULL << 15) /* users wants 32bit inodes */
+#define XFS_MOUNT_SMALL_INUMS (1ULL << 14) /* user wants 32bit inodes */
+#define XFS_MOUNT_32BITINODES (1ULL << 15) /* inode32 allocator active */
#define XFS_MOUNT_NOUUID (1ULL << 16) /* ignore uuid during mount */
#define XFS_MOUNT_BARRIER (1ULL << 17)
#define XFS_MOUNT_IKEEP (1ULL << 18) /* keep empty inode clusters*/
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index fe4c14e..044b416 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -577,23 +577,35 @@ xfs_max_file_offset(
}
/*
- * 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.
+ * Set parameters for inode allocation heuristics, taking into account
+ * filesystem size and inode32/inode64 mount options; i.e. specifically
+ * whether or not XFS_MOUNT_SMALL_INUMS is set.
+ *
+ * Inode allocation patterns are altered only if inode32 is requested
+ * (XFS_MOUNT_SMALL_INUMS), and the filesystem is sufficiently large.
+ * If altered, XFS_MOUNT_32BITINODES is set as well.
+ *
+ * An agcount independent of that in the mount structure is provided
+ * because in the growfs case, mp->m_sb.sb_agcount is not yet updated
+ * to the potentially higher ag count.
+ *
+ * Returns the maximum AG index which may contain inodes.
*/
xfs_agnumber_t
-xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount)
+xfs_set_inode_alloc(
+ struct xfs_mount *mp,
+ xfs_agnumber_t agcount)
{
- xfs_agnumber_t index = 0;
+ xfs_agnumber_t index;
xfs_agnumber_t maxagi = 0;
xfs_sb_t *sbp = &mp->m_sb;
xfs_agnumber_t max_metadata;
xfs_agino_t agino;
xfs_ino_t ino;
- xfs_perag_t *pag;
- /* Calculate how much should be reserved for inodes to meet
- * the max inode percentage.
+ /*
+ * Calculate how much should be reserved for inodes to meet
+ * the max inode percentage. Used only for inode32.
*/
if (mp->m_maxicount) {
__uint64_t icount;
@@ -607,54 +619,48 @@ xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount)
max_metadata = agcount;
}
+ /* Get the last possible inode in the filesystem */
agino = XFS_OFFBNO_TO_AGINO(mp, sbp->sb_agblocks - 1, 0);
+ ino = XFS_AGINO_TO_INO(mp, agcount - 1, agino);
+
+ /*
+ * If user asked for no more than 32-bit inodes, and the fs is
+ * sufficiently large, set XFS_MOUNT_32BITINODES if we must alter
+ * the allocator to accommodate the request.
+ */
+ if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) && ino > XFS_MAXINUMBER_32)
+ mp->m_flags |= XFS_MOUNT_32BITINODES;
+ else
+ mp->m_flags &= ~XFS_MOUNT_32BITINODES;
for (index = 0; index < agcount; index++) {
- ino = XFS_AGINO_TO_INO(mp, index, agino);
+ struct xfs_perag *pag;
- if (ino > XFS_MAXINUMBER_32) {
- pag = xfs_perag_get(mp, index);
- pag->pagi_inodeok = 0;
- pag->pagf_metadata = 0;
- xfs_perag_put(pag);
- continue;
- }
+ ino = XFS_AGINO_TO_INO(mp, index, agino);
pag = xfs_perag_get(mp, index);
- pag->pagi_inodeok = 1;
- maxagi++;
- if (index < max_metadata)
- pag->pagf_metadata = 1;
- xfs_perag_put(pag);
- }
- mp->m_flags |= (XFS_MOUNT_32BITINODES |
- XFS_MOUNT_SMALL_INUMS);
- return maxagi;
-}
-
-xfs_agnumber_t
-xfs_set_inode64(struct xfs_mount *mp, xfs_agnumber_t agcount)
-{
- xfs_agnumber_t index = 0;
-
- for (index = 0; index < agcount; index++) {
- struct xfs_perag *pag;
+ if (mp->m_flags & XFS_MOUNT_32BITINODES) {
+ if (ino > XFS_MAXINUMBER_32) {
+ pag->pagi_inodeok = 0;
+ pag->pagf_metadata = 0;
+ } else {
+ pag->pagi_inodeok = 1;
+ maxagi++;
+ if (index < max_metadata)
+ pag->pagf_metadata = 1;
+ else
+ pag->pagf_metadata = 0;
+ }
+ } else {
+ pag->pagi_inodeok = 1;
+ pag->pagf_metadata = 0;
+ }
- pag = xfs_perag_get(mp, index);
- pag->pagi_inodeok = 1;
- pag->pagf_metadata = 0;
xfs_perag_put(pag);
}
- /* There is no need for lock protection on m_flags,
- * the rw_semaphore of the VFS superblock is locked
- * during mount/umount/remount operations, so this is
- * enough to avoid concurency on the m_flags field
- */
- mp->m_flags &= ~(XFS_MOUNT_32BITINODES |
- XFS_MOUNT_SMALL_INUMS);
- return index;
+ return (mp->m_flags & XFS_MOUNT_32BITINODES) ? maxagi : agcount;
}
STATIC int
@@ -1224,10 +1230,12 @@ xfs_fs_remount(
mp->m_flags &= ~XFS_MOUNT_BARRIER;
break;
case Opt_inode64:
- mp->m_maxagi = xfs_set_inode64(mp, sbp->sb_agcount);
+ mp->m_flags &= ~XFS_MOUNT_SMALL_INUMS;
+ mp->m_maxagi = xfs_set_inode_alloc(mp, sbp->sb_agcount);
break;
case Opt_inode32:
- mp->m_maxagi = xfs_set_inode32(mp, sbp->sb_agcount);
+ mp->m_flags |= XFS_MOUNT_SMALL_INUMS;
+ mp->m_maxagi = xfs_set_inode_alloc(mp, sbp->sb_agcount);
break;
default:
/*
diff --git a/fs/xfs/xfs_super.h b/fs/xfs/xfs_super.h
index 499058f..2dfb1ce 100644
--- a/fs/xfs/xfs_super.h
+++ b/fs/xfs/xfs_super.h
@@ -65,8 +65,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 *, xfs_agnumber_t agcount);
-extern xfs_agnumber_t xfs_set_inode64(struct xfs_mount *, xfs_agnumber_t agcount);
+extern xfs_agnumber_t xfs_set_inode_alloc(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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] xfs: fix up inode32/64 (re)mount handling
2016-02-17 4:47 [PATCH] xfs: fix up inode32/64 (re)mount handling Eric Sandeen
@ 2016-02-17 18:30 ` Brian Foster
2016-02-18 5:46 ` Eric Sandeen
0 siblings, 1 reply; 5+ messages in thread
From: Brian Foster @ 2016-02-17 18:30 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
On Tue, Feb 16, 2016 at 10:47:49PM -0600, Eric Sandeen wrote:
> inode32/inode64 allocator behavior with respect to mount,
> remount and growfs is a little tricky.
>
> The inode32 mount option should only enable the inode32
> allocator heuristics if the filesystem is large enough
> for 64-bit inodes to exist. Today, it has this behavior
> on the initial mount, but a remount with inode32
> unconditionally changes the allocation heuristics, even
> for a small fs.
>
> Also, an inode32 mounted small filesystem should transition
> to the inode32 allocator if the filesystem is subsequently
> grown to a sufficient size. Today that does not happen.
>
> This patch consolidates xfs_set_inode32 and xfs_set_inode64
> into a single new function, and moves the "is the maximum inode
> number big enough to matter" test into that function, so
> it doesn't rely on the caller to get it right - which
> remount did not do, previously.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> Note, this goes after my token-parsing patch for mount.
>
...
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index fe4c14e..044b416 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -577,23 +577,35 @@ xfs_max_file_offset(
> }
>
> /*
> - * 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.
> + * Set parameters for inode allocation heuristics, taking into account
> + * filesystem size and inode32/inode64 mount options; i.e. specifically
> + * whether or not XFS_MOUNT_SMALL_INUMS is set.
> + *
> + * Inode allocation patterns are altered only if inode32 is requested
> + * (XFS_MOUNT_SMALL_INUMS), and the filesystem is sufficiently large.
> + * If altered, XFS_MOUNT_32BITINODES is set as well.
> + *
> + * An agcount independent of that in the mount structure is provided
> + * because in the growfs case, mp->m_sb.sb_agcount is not yet updated
> + * to the potentially higher ag count.
> + *
> + * Returns the maximum AG index which may contain inodes.
> */
> xfs_agnumber_t
> -xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount)
> +xfs_set_inode_alloc(
> + struct xfs_mount *mp,
> + xfs_agnumber_t agcount)
> {
> - xfs_agnumber_t index = 0;
> + xfs_agnumber_t index;
> xfs_agnumber_t maxagi = 0;
> xfs_sb_t *sbp = &mp->m_sb;
> xfs_agnumber_t max_metadata;
> xfs_agino_t agino;
> xfs_ino_t ino;
> - xfs_perag_t *pag;
>
> - /* Calculate how much should be reserved for inodes to meet
> - * the max inode percentage.
> + /*
> + * Calculate how much should be reserved for inodes to meet
> + * the max inode percentage. Used only for inode32.
> */
> if (mp->m_maxicount) {
> __uint64_t icount;
> @@ -607,54 +619,48 @@ xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount)
> max_metadata = agcount;
> }
>
> + /* Get the last possible inode in the filesystem */
> agino = XFS_OFFBNO_TO_AGINO(mp, sbp->sb_agblocks - 1, 0);
> + ino = XFS_AGINO_TO_INO(mp, agcount - 1, agino);
> +
> + /*
> + * If user asked for no more than 32-bit inodes, and the fs is
> + * sufficiently large, set XFS_MOUNT_32BITINODES if we must alter
> + * the allocator to accommodate the request.
> + */
> + if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) && ino > XFS_MAXINUMBER_32)
> + mp->m_flags |= XFS_MOUNT_32BITINODES;
> + else
> + mp->m_flags &= ~XFS_MOUNT_32BITINODES;
In the current code, we call into xfs_set_inode64() if
XFS_MOUNT_SMALL_INUMS is not set or it is, but the largest inode is
within XFS_MAXINUMBER_32. In that latter case, xfs_set_inode64() does:
mp->m_flags &= ~(XFS_MOUNT_32BITINODES |
XFS_MOUNT_SMALL_INUMS);
... which I think means we want to clear XFS_MOUNT_SMALL_INUMS along
with XFS_MOUNT_32BITINODES here, yes? The rest looks fine to me:
Reviewed-by: Brian Foster <bfoster@redhat.com>
>
> for (index = 0; index < agcount; index++) {
> - ino = XFS_AGINO_TO_INO(mp, index, agino);
> + struct xfs_perag *pag;
>
> - if (ino > XFS_MAXINUMBER_32) {
> - pag = xfs_perag_get(mp, index);
> - pag->pagi_inodeok = 0;
> - pag->pagf_metadata = 0;
> - xfs_perag_put(pag);
> - continue;
> - }
> + ino = XFS_AGINO_TO_INO(mp, index, agino);
>
> pag = xfs_perag_get(mp, index);
> - pag->pagi_inodeok = 1;
> - maxagi++;
> - if (index < max_metadata)
> - pag->pagf_metadata = 1;
> - xfs_perag_put(pag);
> - }
> - mp->m_flags |= (XFS_MOUNT_32BITINODES |
> - XFS_MOUNT_SMALL_INUMS);
>
> - return maxagi;
> -}
> -
> -xfs_agnumber_t
> -xfs_set_inode64(struct xfs_mount *mp, xfs_agnumber_t agcount)
> -{
> - xfs_agnumber_t index = 0;
> -
> - for (index = 0; index < agcount; index++) {
> - struct xfs_perag *pag;
> + if (mp->m_flags & XFS_MOUNT_32BITINODES) {
> + if (ino > XFS_MAXINUMBER_32) {
> + pag->pagi_inodeok = 0;
> + pag->pagf_metadata = 0;
> + } else {
> + pag->pagi_inodeok = 1;
> + maxagi++;
> + if (index < max_metadata)
> + pag->pagf_metadata = 1;
> + else
> + pag->pagf_metadata = 0;
> + }
> + } else {
> + pag->pagi_inodeok = 1;
> + pag->pagf_metadata = 0;
> + }
>
> - pag = xfs_perag_get(mp, index);
> - pag->pagi_inodeok = 1;
> - pag->pagf_metadata = 0;
> xfs_perag_put(pag);
> }
>
> - /* There is no need for lock protection on m_flags,
> - * the rw_semaphore of the VFS superblock is locked
> - * during mount/umount/remount operations, so this is
> - * enough to avoid concurency on the m_flags field
> - */
> - mp->m_flags &= ~(XFS_MOUNT_32BITINODES |
> - XFS_MOUNT_SMALL_INUMS);
> - return index;
> + return (mp->m_flags & XFS_MOUNT_32BITINODES) ? maxagi : agcount;
> }
>
> STATIC int
> @@ -1224,10 +1230,12 @@ xfs_fs_remount(
> mp->m_flags &= ~XFS_MOUNT_BARRIER;
> break;
> case Opt_inode64:
> - mp->m_maxagi = xfs_set_inode64(mp, sbp->sb_agcount);
> + mp->m_flags &= ~XFS_MOUNT_SMALL_INUMS;
> + mp->m_maxagi = xfs_set_inode_alloc(mp, sbp->sb_agcount);
> break;
> case Opt_inode32:
> - mp->m_maxagi = xfs_set_inode32(mp, sbp->sb_agcount);
> + mp->m_flags |= XFS_MOUNT_SMALL_INUMS;
> + mp->m_maxagi = xfs_set_inode_alloc(mp, sbp->sb_agcount);
> break;
> default:
> /*
> diff --git a/fs/xfs/xfs_super.h b/fs/xfs/xfs_super.h
> index 499058f..2dfb1ce 100644
> --- a/fs/xfs/xfs_super.h
> +++ b/fs/xfs/xfs_super.h
> @@ -65,8 +65,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 *, xfs_agnumber_t agcount);
> -extern xfs_agnumber_t xfs_set_inode64(struct xfs_mount *, xfs_agnumber_t agcount);
> +extern xfs_agnumber_t xfs_set_inode_alloc(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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] xfs: fix up inode32/64 (re)mount handling
2016-02-17 18:30 ` Brian Foster
@ 2016-02-18 5:46 ` Eric Sandeen
2016-02-18 5:51 ` Eric Sandeen
0 siblings, 1 reply; 5+ messages in thread
From: Eric Sandeen @ 2016-02-18 5:46 UTC (permalink / raw)
To: xfs
On 2/17/16 12:30 PM, Brian Foster wrote:
> On Tue, Feb 16, 2016 at 10:47:49PM -0600, Eric Sandeen wrote:
>> inode32/inode64 allocator behavior with respect to mount,
>> remount and growfs is a little tricky.
>>
>> The inode32 mount option should only enable the inode32
>> allocator heuristics if the filesystem is large enough
>> for 64-bit inodes to exist. Today, it has this behavior
>> on the initial mount, but a remount with inode32
>> unconditionally changes the allocation heuristics, even
>> for a small fs.
>>
>> Also, an inode32 mounted small filesystem should transition
>> to the inode32 allocator if the filesystem is subsequently
>> grown to a sufficient size. Today that does not happen.
>>
>> This patch consolidates xfs_set_inode32 and xfs_set_inode64
>> into a single new function, and moves the "is the maximum inode
>> number big enough to matter" test into that function, so
>> it doesn't rely on the caller to get it right - which
>> remount did not do, previously.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> Note, this goes after my token-parsing patch for mount.
...
>> @@ -607,54 +619,48 @@ xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount)
>> max_metadata = agcount;
>> }
>>
>> + /* Get the last possible inode in the filesystem */
>> agino = XFS_OFFBNO_TO_AGINO(mp, sbp->sb_agblocks - 1, 0);
>> + ino = XFS_AGINO_TO_INO(mp, agcount - 1, agino);
>> +
>> + /*
>> + * If user asked for no more than 32-bit inodes, and the fs is
>> + * sufficiently large, set XFS_MOUNT_32BITINODES if we must alter
>> + * the allocator to accommodate the request.
>> + */
>> + if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) && ino > XFS_MAXINUMBER_32)
>> + mp->m_flags |= XFS_MOUNT_32BITINODES;
>> + else
>> + mp->m_flags &= ~XFS_MOUNT_32BITINODES;
>
> In the current code, we call into xfs_set_inode64() if
> XFS_MOUNT_SMALL_INUMS is not set or it is, but the largest inode is
> within XFS_MAXINUMBER_32. In that latter case, xfs_set_inode64() does:
>
> mp->m_flags &= ~(XFS_MOUNT_32BITINODES |
> XFS_MOUNT_SMALL_INUMS);
>
> ... which I think means we want to clear XFS_MOUNT_SMALL_INUMS along
> with XFS_MOUNT_32BITINODES here, yes? The rest looks fine to me:
I don't think so; that was a bug, AFAICT.
XFS_MOUNT_32BITINODES means that inode32 was specified at mount
time, i.e. the user wants no more than 32-bit inodes for the
duration of this mount.
So this is actually a bugfix for the 2nd item mentioned above:
>> Also, an inode32 mounted small filesystem should transition
>> to the inode32 allocator if the filesystem is subsequently
>> grown to a sufficient size. Today that does not happen.
> Reviewed-by: Brian Foster <bfoster@redhat.com>
Thanks,
-Eric
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: fix up inode32/64 (re)mount handling
2016-02-18 5:46 ` Eric Sandeen
@ 2016-02-18 5:51 ` Eric Sandeen
2016-02-18 12:08 ` Brian Foster
0 siblings, 1 reply; 5+ messages in thread
From: Eric Sandeen @ 2016-02-18 5:51 UTC (permalink / raw)
To: xfs
On 2/17/16 11:46 PM, Eric Sandeen wrote:
> On 2/17/16 12:30 PM, Brian Foster wrote:
>> On Tue, Feb 16, 2016 at 10:47:49PM -0600, Eric Sandeen wrote:
>>> inode32/inode64 allocator behavior with respect to mount,
>>> remount and growfs is a little tricky.
>>>
>>> The inode32 mount option should only enable the inode32
>>> allocator heuristics if the filesystem is large enough
>>> for 64-bit inodes to exist. Today, it has this behavior
>>> on the initial mount, but a remount with inode32
>>> unconditionally changes the allocation heuristics, even
>>> for a small fs.
>>>
>>> Also, an inode32 mounted small filesystem should transition
>>> to the inode32 allocator if the filesystem is subsequently
>>> grown to a sufficient size. Today that does not happen.
>>>
>>> This patch consolidates xfs_set_inode32 and xfs_set_inode64
>>> into a single new function, and moves the "is the maximum inode
>>> number big enough to matter" test into that function, so
>>> it doesn't rely on the caller to get it right - which
>>> remount did not do, previously.
>>>
>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>> ---
>>>
>>> Note, this goes after my token-parsing patch for mount.
>
> ...
>
>>> @@ -607,54 +619,48 @@ xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount)
>>> max_metadata = agcount;
>>> }
>>>
>>> + /* Get the last possible inode in the filesystem */
>>> agino = XFS_OFFBNO_TO_AGINO(mp, sbp->sb_agblocks - 1, 0);
>>> + ino = XFS_AGINO_TO_INO(mp, agcount - 1, agino);
>>> +
>>> + /*
>>> + * If user asked for no more than 32-bit inodes, and the fs is
>>> + * sufficiently large, set XFS_MOUNT_32BITINODES if we must alter
>>> + * the allocator to accommodate the request.
>>> + */
>>> + if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) && ino > XFS_MAXINUMBER_32)
>>> + mp->m_flags |= XFS_MOUNT_32BITINODES;
>>> + else
>>> + mp->m_flags &= ~XFS_MOUNT_32BITINODES;
>>
>> In the current code, we call into xfs_set_inode64() if
>> XFS_MOUNT_SMALL_INUMS is not set or it is, but the largest inode is
>> within XFS_MAXINUMBER_32. In that latter case, xfs_set_inode64() does:
>>
>> mp->m_flags &= ~(XFS_MOUNT_32BITINODES |
>> XFS_MOUNT_SMALL_INUMS);
>>
>> ... which I think means we want to clear XFS_MOUNT_SMALL_INUMS along
>> with XFS_MOUNT_32BITINODES here, yes? The rest looks fine to me:
>
> I don't think so; that was a bug, AFAICT.
>
> XFS_MOUNT_32BITINODES means that inode32 was specified at mount
Ugh; I had that backwards.
*XFS_MOUNT_SMALL_INUMS* means that inode32 was specified at mount time.
For the reasons I stated, *that* flag should never be cleared. It
signifies a specified mount option, which does not go away just because
the filesystem is currently small.
Maybe we need clearer flag names :/
-Eric
> time, i.e. the user wants no more than 32-bit inodes for the
> duration of this mount.
>
> So this is actually a bugfix for the 2nd item mentioned above:
>
>>> Also, an inode32 mounted small filesystem should transition
>>> to the inode32 allocator if the filesystem is subsequently
>>> grown to a sufficient size. Today that does not happen.
>
>> Reviewed-by: Brian Foster <bfoster@redhat.com>
>
> Thanks,
> -Eric
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: fix up inode32/64 (re)mount handling
2016-02-18 5:51 ` Eric Sandeen
@ 2016-02-18 12:08 ` Brian Foster
0 siblings, 0 replies; 5+ messages in thread
From: Brian Foster @ 2016-02-18 12:08 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
On Wed, Feb 17, 2016 at 11:51:47PM -0600, Eric Sandeen wrote:
>
>
> On 2/17/16 11:46 PM, Eric Sandeen wrote:
> > On 2/17/16 12:30 PM, Brian Foster wrote:
> >> On Tue, Feb 16, 2016 at 10:47:49PM -0600, Eric Sandeen wrote:
> >>> inode32/inode64 allocator behavior with respect to mount,
> >>> remount and growfs is a little tricky.
> >>>
> >>> The inode32 mount option should only enable the inode32
> >>> allocator heuristics if the filesystem is large enough
> >>> for 64-bit inodes to exist. Today, it has this behavior
> >>> on the initial mount, but a remount with inode32
> >>> unconditionally changes the allocation heuristics, even
> >>> for a small fs.
> >>>
> >>> Also, an inode32 mounted small filesystem should transition
> >>> to the inode32 allocator if the filesystem is subsequently
> >>> grown to a sufficient size. Today that does not happen.
> >>>
> >>> This patch consolidates xfs_set_inode32 and xfs_set_inode64
> >>> into a single new function, and moves the "is the maximum inode
> >>> number big enough to matter" test into that function, so
> >>> it doesn't rely on the caller to get it right - which
> >>> remount did not do, previously.
> >>>
> >>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >>> ---
> >>>
> >>> Note, this goes after my token-parsing patch for mount.
> >
> > ...
> >
> >>> @@ -607,54 +619,48 @@ xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount)
> >>> max_metadata = agcount;
> >>> }
> >>>
> >>> + /* Get the last possible inode in the filesystem */
> >>> agino = XFS_OFFBNO_TO_AGINO(mp, sbp->sb_agblocks - 1, 0);
> >>> + ino = XFS_AGINO_TO_INO(mp, agcount - 1, agino);
> >>> +
> >>> + /*
> >>> + * If user asked for no more than 32-bit inodes, and the fs is
> >>> + * sufficiently large, set XFS_MOUNT_32BITINODES if we must alter
> >>> + * the allocator to accommodate the request.
> >>> + */
> >>> + if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) && ino > XFS_MAXINUMBER_32)
> >>> + mp->m_flags |= XFS_MOUNT_32BITINODES;
> >>> + else
> >>> + mp->m_flags &= ~XFS_MOUNT_32BITINODES;
> >>
> >> In the current code, we call into xfs_set_inode64() if
> >> XFS_MOUNT_SMALL_INUMS is not set or it is, but the largest inode is
> >> within XFS_MAXINUMBER_32. In that latter case, xfs_set_inode64() does:
> >>
> >> mp->m_flags &= ~(XFS_MOUNT_32BITINODES |
> >> XFS_MOUNT_SMALL_INUMS);
> >>
> >> ... which I think means we want to clear XFS_MOUNT_SMALL_INUMS along
> >> with XFS_MOUNT_32BITINODES here, yes? The rest looks fine to me:
> >
> > I don't think so; that was a bug, AFAICT.
> >
> > XFS_MOUNT_32BITINODES means that inode32 was specified at mount
>
> Ugh; I had that backwards.
>
> *XFS_MOUNT_SMALL_INUMS* means that inode32 was specified at mount time.
> For the reasons I stated, *that* flag should never be cleared. It
> signifies a specified mount option, which does not go away just because
> the filesystem is currently small.
>
> Maybe we need clearer flag names :/
>
Ah, I missed that part. Sounds good, thanks for the explanation! (And
yes, the flag names are not clear.. ;P)
Brian
> -Eric
>
> > time, i.e. the user wants no more than 32-bit inodes for the
> > duration of this mount.
> >
> > So this is actually a bugfix for the 2nd item mentioned above:
> >
> >>> Also, an inode32 mounted small filesystem should transition
> >>> to the inode32 allocator if the filesystem is subsequently
> >>> grown to a sufficient size. Today that does not happen.
> >
> >> Reviewed-by: Brian Foster <bfoster@redhat.com>
> >
> > Thanks,
> > -Eric
> >
> > _______________________________________________
> > 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-02-18 12:08 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-17 4:47 [PATCH] xfs: fix up inode32/64 (re)mount handling Eric Sandeen
2016-02-17 18:30 ` Brian Foster
2016-02-18 5:46 ` Eric Sandeen
2016-02-18 5:51 ` Eric Sandeen
2016-02-18 12:08 ` Brian Foster
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox