On 12/4/20 3:12 PM, Darrick J. Wong wrote:
> On Fri, Dec 04, 2020 at 02:35:45PM -0600, Eric Sandeen wrote:
>> On 11/30/20 9:37 PM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>>>
>>> A couple of the superblock validation checks apply only to the kernel,
>>> so move them to xfs_mount.c before we start changing sb_inprogress.
oh also, you're not changing sb_inprogress anymore, right? ;)
>>> This also reduces the diff between kernel and userspace libxfs.
>>
>> My only complaint is that "xfs_sb_validate_mount" isn't really descriptive
>> at all, and nobody reading the code or comments will know why we've chosen
>> to move just these two checks out of the common validator...
>>
>> What does "compatible with this mount" mean?
>
> Compatible with this implementation?
Hm.
So most of xfs_validate_sb_common is doing internal consistency checking
that has nothing at all to do with the host's core capabilities or filesystem
"state" (other than version/features I guess).
You've moved out the PAGE_SIZE check, which depends on the host.
You've also moved the inprogress check, which depends on state.
(and that's not really "kernel-specific" is it?)
You'll later move the NEEDSREPAIR check, which I guess is state.
But you haven't moved the fsb_count-vs-host check, which depends on the host.
(and ... I think that one may actually be kernel-specific,
because it depends on pagecache limitations in the kernel, so maybe it
should be moved out as well?)
So maybe the distinction is internal consistency checks, vs
host-compatibility-and-filesystem-state checks.
How about ultimately:
/*
* Do host compatibility and filesystem state checks here; these are unique
* to the kernel, and may differ in userspace.
*/
xfs_validate_sb_host(
struct xfs_mount *mp,
struct xfs_buf *bp,
struct xfs_sb *sbp)
{
/*
* Don't touch the filesystem if a user tool thinks it owns the primary
* superblock. mkfs doesn't clear the flag from secondary supers, so
* we don't check them at all.
*/
if (XFS_BUF_ADDR(bp) == XFS_SB_DADDR && sbp->sb_inprogress) {
xfs_warn(mp, "Offline file system operation in progress!");
return -EFSCORRUPTED;
}
/* Filesystem claims it needs repair, so refuse the mount. */
if (xfs_sb_version_needsrepair(&mp->m_sb)) {
xfs_warn(mp, "Filesystem needs repair. Please run xfs_repair.");
return -EFSCORRUPTED;
}
/*
* Until this is fixed only page-sized or smaller data blocks work.
*/
if (unlikely(sbp->sb_blocksize > PAGE_SIZE)) {
xfs_warn(mp,
"File system with blocksize %d bytes. "
"Only pagesize (%ld) or less will currently work.",
sbp->sb_blocksize, PAGE_SIZE);
return -ENOSYS;
}
/* Ensure this filesystem fits in the page cache limits */
if (xfs_sb_validate_fsb_count(sbp, sbp->sb_dblocks) ||
xfs_sb_validate_fsb_count(sbp, sbp->sb_rblocks)) {
xfs_warn(mp,
"file system too large to be mounted on this system.");
return -EFBIG;
}
return 0;
}
>> Maybe just fess up in the comment, and say "these checks are different
>> for kernel vs. userspace so we keep them over here" - and as for the
>> function name, *shrug* not sure I have anything better...
>
> _validate_implementation? I don't have a better suggestion either.
>
> --D
>
>> -Eric
>>
>
