xfs: do not unconditionally enable hasalign feature on V5 filesystems

On a ppc64 system, executing generic/256 test with 32k block size gives the following call trace,

XFS: Assertion failed: args->maxlen > 0, file: /root/repos/linux/fs/xfs/libxfs/xfs_alloc.c, line: 2026

kernel BUG at /root/repos/linux/fs/xfs/xfs_message.c:113!

Oops: Exception in kernel mode, sig: 5 [#1]

SMP NR_CPUS=2048
DEBUG_PAGEALLOC
NUMA
pSeries

Modules linked in:
CPU: 2 PID: 19361 Comm: mkdir Not tainted 4.10.0-rc5 #58

task: c000000102606d80 task.stack: c0000001026b8000
NIP: c0000000004ef798 LR: c0000000004ef798 CTR: c00000000082b290
REGS: c0000001026bb090 TRAP: 0700 Not tainted (4.10.0-rc5)
MSR: 8000000000029032 <SF,EE,ME,IR,DR,RI>
CR: 28004428 XER: 00000000
CFAR: c0000000004ef180 SOFTE: 1
GPR00: c0000000004ef798 c0000001026bb310 c000000001157300 ffffffffffffffea
GPR04: 000000000000000a c0000001026bb130 0000000000000000 ffffffffffffffc0
GPR08: 00000000000000d1 0000000000000021 00000000ffffffd1 c000000000dd4990
GPR12: 0000000022004444 c00000000fe00800 0000000020000000 0000000000000000
GPR16: 0000000000000000 0000000043a606fc 0000000043a76c08 0000000043a1b3d0
GPR20: 000001002a35cd60 c0000001026bbb80 0000000000000000 0000000000000001
GPR24: 0000000000000240 0000000000000004 c00000062dc55000 0000000000000000
GPR28: 0000000000000004 c00000062ecd9200 0000000000000000 c0000001026bb6c0 NIP [c0000000004ef798] .assfail+0x28/0x30
LR [c0000000004ef798] .assfail+0x28/0x30

Call Trace:
[c0000001026bb310] [c0000000004ef798] .assfail+0x28/0x30 (unreliable)
[c0000001026bb380] [c000000000455d74] .xfs_alloc_space_available+0x194/0x1b0
[c0000001026bb410] [c00000000045b914] .xfs_alloc_fix_freelist+0x144/0x480
[c0000001026bb580] [c00000000045c368] .xfs_alloc_vextent+0x698/0xa90
[c0000001026bb650] [c0000000004a6200] .xfs_ialloc_ag_alloc+0x170/0x820
[c0000001026bb7c0] [c0000000004a9098] .xfs_dialloc+0x158/0x320
[c0000001026bb8a0] [c0000000004e628c] .xfs_ialloc+0x7c/0x610
[c0000001026bb990] [c0000000004e8138] .xfs_dir_ialloc+0xa8/0x2f0
[c0000001026bbaa0] [c0000000004e8814] .xfs_create+0x494/0x790
[c0000001026bbbf0] [c0000000004e5ebc] .xfs_generic_create+0x2bc/0x410
[c0000001026bbce0] [c0000000002b4a34] .vfs_mkdir+0x154/0x230
[c0000001026bbd70] [c0000000002bc444] .SyS_mkdirat+0x94/0x120
[c0000001026bbe30] [c00000000000b760] system_call+0x38/0xfc

Instruction dump:
4e800020 60000000 7c0802a6 7c862378 3c82ffca 7ca72b78 38841c18 7c651b78 38600000 f8010010 f821ff91 4bfff94d <0fe00000> 60000000 7c0802a6 7c892378

The root cause of the problem is due to the fact that xfs_sb_version_hasalign() returns true when we are working on a V5 filesystem. Due to this args.minalignslop (in xfs_ialloc_ag_alloc()) gets the unsigned equivalent of -1 assigned to it. This later causes alloc_len in xfs_alloc_space_available() to have a value of 0. In such a scenario when args.total is also 0, the assert statement "ASSERT(args->maxlen > 0);" fails.

This commit fixes the bug by checking for the existance of XFS_SB_VERSION_ALIGNBIT bit in xfs_sb->sb_versionnum field even on V5 filesystems.

Signed-off-by Chandan Rajendra <[email protected]>

---
PS: From what I could understand from other code paths, xfs_alloc_arg->total must be set to the sum of space required by the current code path and the space already reserved in the current transaction. Please let me know if my understanding is incorrect. Also, In the code path listed in the above call trace we have xfs_alloc_arg->total set to 0 which probably isn't correct either.

 fs/xfs/libxfs/xfs_format.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

fs/xfs/libxfs/xfs_format.h +1 -2

index 6b7579e..57a5264 100644
@@ -346,8 +346,7 @@ static inline void xfs_sb_version_addquota(struct xfs_sb *sbp)
 
 static inline bool xfs_sb_version_hasalign(struct xfs_sb *sbp)
 {
-	return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 ||
-		(sbp->sb_versionnum & XFS_SB_VERSION_ALIGNBIT));
+	return (sbp->sb_versionnum & XFS_SB_VERSION_ALIGNBIT);
 }
 
 static inline bool xfs_sb_version_hasdalign(struct xfs_sb *sbp)

Quoted 7 lines

The root cause of the problem is due to the fact that xfs_sb_version_hasalign() returns true when we are working on a V5 filesystem. Due to this args.minalignslop (in xfs_ialloc_ag_alloc()) gets the unsigned equivalent of -1 assigned to it. This later causes alloc_len in xfs_alloc_space_available() to have a value of 0. In such a scenario when args.total is also 0, the assert statement "ASSERT(args->maxlen > 0);" fails.

Hm, the intent of the _haslign() function is to say that V5 must always imply the "alignbit" - i.e. we don't want to grow an infinite feature matrix, and by the time you get to V5 supers, there are many things which cannot be turned on or off, such as this feature.

So what happens here... xfs_ialloc_ag_alloc does:

args.minalignslop = xfs_ialloc_cluster_alignment(args.mp) - 1;

so you're saying that cluster_alignment comes out as 0?

That function is checking _hasalign:

static inline int
xfs_ialloc_cluster_alignment(
struct xfs_mount *mp)

{
        if (xfs_sb_version_hasalign(&mp->m_sb) &&
            mp->m_sb.sb_inoalignmt >=
                        XFS_B_TO_FSBT(mp, mp->m_inode_cluster_size))
                return mp->m_sb.sb_inoalignmt;
        return 1;
}

So are you saying that this function returns 0? That would imply that sb_inoalignmt and m_inode_cluster_size are both zero, yes? Is this what you see?

Quoted 31 lines · Eric Sandeen

Quoted 7 lines

The root cause of the problem is due to the fact that xfs_sb_version_hasalign() returns true when we are working on a V5 filesystem. Due to this args.minalignslop (in xfs_ialloc_ag_alloc()) gets the unsigned equivalent of -1 assigned to it. This later causes alloc_len in xfs_alloc_space_available() to have a value of 0. In such a scenario when args.total is also 0, the assert statement "ASSERT(args->maxlen > 0);" fails.

Hm, the intent of the _haslign() function is to say that V5 must always imply the "alignbit" - i.e. we don't want to grow an infinite feature matrix, and by the time you get to V5 supers, there are many things which cannot be turned on or off, such as this feature.

So what happens here... xfs_ialloc_ag_alloc does:

args.minalignslop = xfs_ialloc_cluster_alignment(args.mp) - 1;

so you're saying that cluster_alignment comes out as 0?

That function is checking _hasalign:

static inline int
xfs_ialloc_cluster_alignment(
struct xfs_mount *mp)

{
        if (xfs_sb_version_hasalign(&mp->m_sb) &&
            mp->m_sb.sb_inoalignmt >=
                        XFS_B_TO_FSBT(mp, mp->m_inode_cluster_size))
                return mp->m_sb.sb_inoalignmt;
        return 1;
}

Quoted 3 lines

So are you saying that this function returns 0? That would imply that sb_inoalignmt and m_inode_cluster_size are both zero, yes? Is this what you see?

Sorry, I guess that means XFS_B_TO_FSBT(mp, mp->m_inode_cluster_size)) is zero; inode cluster size is 8192 in this case I think, and that is in fact 0 filesystem blocks when computed with this macro.

I need to think about this a little bit to convince myself that the inode alignment bit really /should/ be off for a filesystem of this geometry, vs changing the macro to recognize the case.

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 40 lines · Eric Sandeen

Quoted 31 lines · Eric Sandeen

Quoted 7 lines

The root cause of the problem is due to the fact that xfs_sb_version_hasalign() returns true when we are working on a V5 filesystem. Due to this args.minalignslop (in xfs_ialloc_ag_alloc()) gets the unsigned equivalent of -1 assigned to it. This later causes alloc_len in xfs_alloc_space_available() to have a value of 0. In such a scenario when args.total is also 0, the assert statement "ASSERT(args->maxlen > 0);" fails.

Hm, the intent of the _haslign() function is to say that V5 must always imply the "alignbit" - i.e. we don't want to grow an infinite feature matrix, and by the time you get to V5 supers, there are many things which cannot be turned on or off, such as this feature.

So what happens here... xfs_ialloc_ag_alloc does:

args.minalignslop = xfs_ialloc_cluster_alignment(args.mp) - 1;

so you're saying that cluster_alignment comes out as 0?

That function is checking _hasalign:

static inline int
xfs_ialloc_cluster_alignment(
struct xfs_mount *mp)

{
        if (xfs_sb_version_hasalign(&mp->m_sb) &&
            mp->m_sb.sb_inoalignmt >=
                        XFS_B_TO_FSBT(mp, mp->m_inode_cluster_size))
                return mp->m_sb.sb_inoalignmt;
        return 1;
}

Quoted 3 lines

So are you saying that this function returns 0? That would imply that sb_inoalignmt and m_inode_cluster_size are both zero, yes? Is this what you see?

Sorry, I guess that means XFS_B_TO_FSBT(mp, mp->m_inode_cluster_size)) is zero; inode cluster size is 8192 in this case I think, and that is in fact 0 filesystem blocks when computed with this macro.

Quoted 3 lines

I need to think about this a little bit to convince myself that the inode alignment bit really /should/ be off for a filesystem of this geometry, vs changing the macro to recognize the case.

Why isn't that XFS_B_TO_FSBT instead a call to xfs_icluster_size_fsb()? That function is used elsewhere to compute the number of fsblocks backing an inode cluster, which seems like what we need here to figure out whether inoalignmt makes sense w.r.t. the size of an inode cluster.

Quoted 1 line · Eric Sandeen

On 2/15/17 10:13 AM, Eric Sandeen wrote:

Quoted 4 lines

Sorry, I guess that means XFS_B_TO_FSBT(mp, mp->m_inode_cluster_size)) is zero; inode cluster size is 8192 in this case I think, and that is in fact 0 filesystem blocks when computed with this macro.

Quoted 3 lines

I need to think about this a little bit to convince myself that the inode alignment bit really /should/ be off for a filesystem of this geometry, vs changing the macro to recognize the case.

Why isn't that XFS_B_TO_FSBT instead a call to xfs_icluster_size_fsb()? That function is used elsewhere to compute the number of fsblocks backing an inode cluster, which seems like what we need here to figure out whether inoalignmt makes sense w.r.t. the size of an inode cluster.

--D

Quoted 6 lines

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 31 lines · Eric Sandeen

Quoted 7 lines

The root cause of the problem is due to the fact that xfs_sb_version_hasalign() returns true when we are working on a V5 filesystem. Due to this args.minalignslop (in xfs_ialloc_ag_alloc()) gets the unsigned equivalent of -1 assigned to it. This later causes alloc_len in xfs_alloc_space_available() to have a value of 0. In such a scenario when args.total is also 0, the assert statement "ASSERT(args->maxlen > 0);" fails.

Hm, the intent of the _haslign() function is to say that V5 must always imply the "alignbit" - i.e. we don't want to grow an infinite feature matrix, and by the time you get to V5 supers, there are many things which cannot be turned on or off, such as this feature.

So what happens here... xfs_ialloc_ag_alloc does:

args.minalignslop = xfs_ialloc_cluster_alignment(args.mp) - 1;

so you're saying that cluster_alignment comes out as 0?

That function is checking _hasalign:

static inline int
xfs_ialloc_cluster_alignment(
struct xfs_mount *mp)

{
        if (xfs_sb_version_hasalign(&mp->m_sb) &&
            mp->m_sb.sb_inoalignmt >=
                        XFS_B_TO_FSBT(mp, mp->m_inode_cluster_size))
                return mp->m_sb.sb_inoalignmt;
        return 1;
}

Quoted 3 lines

So are you saying that this function returns 0? That would imply that sb_inoalignmt and m_inode_cluster_size are both zero, yes? Is this what you see?

Sorry, I guess that means XFS_B_TO_FSBT(mp, mp->m_inode_cluster_size)) is zero; inode cluster size is 8192 in this case I think, and that is in fact 0 filesystem blocks when computed with this macro.

I need to think about this a little bit to convince myself that the inode alignment bit really /should/ be off for a filesystem of this geometry, vs changing the macro to recognize the case.

-Eric

Hm, the intent of the _haslign() function is to say that V5 must always imply the "alignbit" - i.e. we don't want to grow an infinite feature matrix, and by the time you get to V5 supers, there are many things which cannot be turned on or off, such as this feature.

Quoted 4 lines · Eric Sandeen

Hm, the intent of the _haslign() function is to say that V5 must always imply the "alignbit" - i.e. we don't want to grow an infinite feature matrix, and by the time you get to V5 supers, there are many things which cannot be turned on or off, such as this feature.

I'm rethinking this a bit; while my question may have uncovered another real bug, I think maybe your patch /is/ ok.

While the intent /was/ to ensure that some features are not optional on V5 filesystems, the mkfs code itself might end up turning off inode alignment for sufficiently large filesystem blocks and sufficiently small inode sizes. The reality is, for sufficiently large fs blocks, every new inode chunk allocation is aligned, because it is sub-block sized. And so there is nothing uniquely "aligned" about any allocation.

i.e. on v4 superblocks, the right combination of inode size & fs block size will turn off the alignment feature, even if the user did not request it.

So for v5 supers, it does seem odd to report the presence of the alignment feature even when the flag is not set in the superblock due to the geometry details.

This goes way back to:

04a1e6c5b xfs: add CRC checks to the superblock

which doesn't really explain why the xfs_sb_version_hasalign got changed.

Anyway; it probably makes sense to do a targeted fix to resolve the issue you've run into, but we might want to take a broader look at when the alignment feature (and feature flag) gets set and reported, to make sure that it is all consistent and as expected.